Add SQLite support for serde_json::Value using the Json/Jsonb #4284

JMLX42 · 2024-09-24T12:52:06Z

Implements #3918

…onbtypes

diesel/src/sqlite/types/json.rs

weiznich

Thanks for opening this PR. This is already a good starting point, but we need to adjust the (de)serialization of the jsonb type to match the actual underlying SQLite data format. Additionally tests + documentation need some tweaks.

Finally it would be great to have a changelog entry for this change.

diesel/src/sql_types/mod.rs

diesel/src/sqlite/types/json.rs

weiznich

Thanks for the update. It looks now much closer to what I had in mind here. I think we still should improve the test situation by adding more tests for jsonb roundtrips. We also might want to have a few tests that load data from the database and check that the expected result comes back. Finally it seems like that the handling of escaped string sequences is still missing from the parser/writer implementation. This should also be covered by tests.

diesel/src/sqlite/types/json.rs

weiznich

Thanks for the update. I feel we are mostly there now. I've added a few minor remarks otherwise I would like to see an entry into the Changelog.md file in the repository root for this feature + at least a handful tests that are essentilally:

let res = diesel::select(sql::<Jsonb>("jsonb('{\"whatever\": 42}')")).get_result::<serde_json::Value>(conn).unwrap();
assert_eq!(res, serde_json::json!(…));

to verify that loading values also works

weiznich · 2024-09-27T13:03:24Z

diesel/src/sql_types/mod.rs

+/// let inserted_address = insert_into(contacts)
+///     .values((name.eq("Claus"), address.eq(&santas_address)))
+///     .returning(address)
+///     .get_result::<serde_json::Value>(connection)?;


I think that fails on CI because support for get_result() for insert statements for sqlite is behind a feature flag.

weiznich · 2024-09-27T13:04:46Z

diesel/src/sql_types/mod.rs

+/// }
+///
+/// # #[cfg(all(feature = "sqlite", feature = "serde_json"))]
+/// # fn main() -> Result<(), Box<dyn std::error::Error>> {


I would prefer to not duplicate the whole example. I suggest to just put the create table statement behind #[cfg(feature = "postgres")] and #[cfg(feature = "sqlite")], as everything else is the same.

@weiznich like this def9cee ?

weiznich · 2024-09-27T13:06:53Z

diesel/src/sqlite/types/json.rs

+}
+
+// Read a JSON integer in canonical format (INT)
+fn read_jsonb_int(bytes: &[u8], payload_size: usize) -> deserialize::Result<serde_json::Value> {


bytes should already be payload_size here, right? In that case we don't need to take another subslice below. (The same likely applies to the other read functions as well)

weiznich · 2024-09-27T13:12:32Z

diesel/src/sqlite/types/json.rs

+        return Err("Payload size exceeds the maximum allowed size of 2GB".into());
+    }
+
+    let mut header = Vec::new();


We likely should use Vec::with_capacity() here as we know the payload size beforehand?

diesel/src/sqlite/types/json.rs

JMLX42 · 2024-09-27T21:40:28Z

Thanks for the update. I feel we are mostly there now. I've added a few minor remarks otherwise I would like to see an entry into the Changelog.md file in the repository root for this feature + at least a handful tests that are essentilally:
let res = diesel::select(sql::<Jsonb>("jsonb('{\"whatever\": 42}')")).get_result::<serde_json::Value>(conn).unwrap();
assert_eq!(res, serde_json::json!(…));
to verify that loading values also works

@weiznich done in 7fe0c67

JMLX42 · 2024-09-27T21:42:37Z

Finally it would be great to have a changelog entry for this change.

@weiznich Done in 76ac650

JMLX42 · 2024-09-27T21:46:33Z

@weiznich I just updated the description with the check list. Please update it if necessary.

weiznich · 2024-09-30T16:04:34Z

The added changes look fine so far, the only thing missing here is making the CI pass. That likely requires small adjustments to when the tests are run. For postgres they should be gated with #[cfg(all(feature = "postgres", feature = "serde_json"))] for sqlite they likely require #[cfg(all(feature = "sqlite", feature = "serde_json", feature = "returning_clauses_for_sqlite_3_35"))].

JMLX42 · 2024-09-30T18:00:45Z

for sqlite they likely require #[cfg(all(feature = "sqlite", feature = "serde_json", feature = "returning_clauses_for_sqlite_3_35"))].

@weiznich it gets deeper: since the doc for the Jsonb type also needs the returning_clauses_for_sqlite_3_35, either:

8657492 We required the returning_clauses_for_sqlite_3_35 feature for the Jsonb type as a whole. Which means we also require it for:
- the whole sqlite::types::json mod use
- the type_impl::json use of crate::sql_types::Jsonb and the SerdeJsonValueProxy type.
Or we de-factorize the doc example to completely separate the SQLite from the Pg code (but you asked me not to here).

JMLX42 · 2024-09-30T18:27:21Z

for sqlite they likely require #[cfg(all(feature = "sqlite", feature = "serde_json", feature = "returning_clauses_for_sqlite_3_35"))].

@weiznich it gets deeper: since the doc for the Jsonb type also needs the returning_clauses_for_sqlite_3_35, either:

1. [8657492](https://github.com/diesel-rs/diesel/commit/8657492b88ca05d5fcd8cb0f1cb474fca40d26bb) We required the `returning_clauses_for_sqlite_3_35` feature for the `Jsonb` type as a whole. Which means we also require it for:
   
   * the whole `sqlite::types::json` mod use
   * the `type_impl::json` use of `crate::sql_types::Jsonb` and the `SerdeJsonValueProxy` type.

2. Or we de-factorize the doc example to completely separate the SQLite from the Pg code (but you asked me not to [here](https://github.com/diesel-rs/diesel/pull/4284#discussion_r1778595059)).

@weiznich Actually I think I found a good middle-ground. I'll let you know when the CI is green.

JMLX42 · 2024-09-30T20:51:05Z

@weiznich the CI is green except for this:

https://github.com/diesel-rs/diesel/actions/runs/11113502349/job/30877989999

serde_json::Number does not exist before version 0.9.0. Can I upgrade the dependency version requirement or should I find a workaround?

JMLX42 · 2024-10-01T08:14:48Z

serde_json::Number does not exist before version 0.9.0. Can I upgrade the dependency version requirement or should I find a workaround?

@weiznich since:

0.9.0 dates from 2017
diesel relies on Rust edition 2021

I chose to upgrade the lower bound req to 0.9.0: e3b1f58

JMLX42 · 2024-10-01T09:02:21Z

@weiznich CI is all green now. If you are OK with the current state of the PR, I will squash my changes, then rebase. Then you can merge them.

weiznich · 2024-10-01T09:24:03Z

diesel/Cargo.toml

@@ -30,7 +30,7 @@ mysqlclient-src = { version = "0.1.0", optional = true }
 pq-sys = { version = ">=0.4.0, <0.7.0", optional = true }
 pq-src = { version = "0.3", optional = true }
 quickcheck = { version = "1.0.3", optional = true }
-serde_json = { version = ">=0.8.0, <2.0", optional = true }
+serde_json = { version = ">=0.9.0, <2.0", optional = true }


I fear its not possible to change the minimal supported version here, as that's a breaking change. We would need to find a different solution for this. The question is how…

I likely need to have some time thinking about possible solutions if you don't have any suggestions.

I'm sorry that this turns up only in the end, but it's important to keep stability here.

weiznich

Thanks for the update. I'm sorry that I still have found something that needs to be changed, but the Jsonb type shouldn't be gated behind the serde_json feature. Only the actual ToSql/FromSql impls for serde_json::Value should be gated, as you can use columns/values of those type without actually loading them from the database, e.g by converting them to string or just comparing single fields.

diesel/src/pg/expression/expression_methods.rs

weiznich · 2024-10-01T18:46:27Z

diesel/src/sql_types/mod.rs

+/// # fn main() {}
+/// ```
+#[cfg(all(
+    feature = "serde_json",


This types should be independent from the serde_json feature. It's fine to gate the doc-test to that feature, but not the type.

weiznich

Thanks for fixing and thanks again for working on this ❤️

I think the next step for json support in the SQLite backend is to add definitions for the built-in SQL functions to diesel. Especially adding the json and jsonb functions should make it much easier to actually use this work. If you are interested in working on that as well, just reach out and I will provide more details on how this should be done. That part should be much more straight forward than adding these impls, as we don't need to care about parsing complicated stuff.

sofiaritz · 2024-10-13T19:28:06Z

diesel/src/sqlite/types/json.rs

@@ -0,0 +1,1073 @@
+//! Support for JSON and JSONB values under PostgreSQL.


nit - Should this comment be: //! Support for JSON and JSONB values under SQLite.?

Great work! Hoping this lands on the next diesel version :)

nit - Should this comment be: //! Support for JSON and JSONB values under SQLite.?

Can you submit a PR to fix that?

Great work! Hoping this lands on the next diesel version :)

It will be part of the next diesel feature release, but as always: We don't give any other ETA than when it's done.

Can you submit a PR to fix that?

Done: #4307

feat(sqlite): add support for serde_json::Value using the Json and Js…

7572da1

…onbtypes

JMLX42 commented Sep 24, 2024

View reviewed changes

diesel/src/sqlite/types/json.rs Outdated Show resolved Hide resolved

weiznich reviewed Sep 24, 2024

View reviewed changes

diesel/src/sql_types/mod.rs Show resolved Hide resolved

diesel/src/sql_types/mod.rs Show resolved Hide resolved

diesel/src/sqlite/types/json.rs Outdated Show resolved Hide resolved

diesel/src/sqlite/types/json.rs Outdated Show resolved Hide resolved

first implementation of a SQLite JSONB format reader

eea59fd

JMLX42 force-pushed the feature/sqlite-json branch from 18e3a95 to eea59fd Compare September 24, 2024 20:05

Jean-Marc Le Roux added 7 commits September 24, 2024 22:32

first implementation of a SQLite JSONB format writer

c3a6d70

add unit tests for the SQLite JSONB read/write functions

d917a89

fix clippy warning

cd7018d

update the Jsonb rustdoc

a557574

working json_to_sql and jsonb_to_sql tests for sqlite

c7b121e

add more json/jsonb tests for sqlite

1e1c31b

fix broken ref to doctest_setup.rs

0f0d9a6

weiznich reviewed Sep 25, 2024

View reviewed changes

diesel/src/sqlite/types/json.rs Outdated Show resolved Hide resolved

diesel/src/sqlite/types/json.rs Outdated Show resolved Hide resolved

diesel/src/sqlite/types/json.rs Outdated Show resolved Hide resolved

Jean-Marc Le Roux added 4 commits September 26, 2024 15:48

add Jsonb to SQLite tests

209c862

add support for SQLite JSONB TEXTJ fields

fd7f565

fix clippy errors

1b95507

add a separate SQLite + Jsonb example

3a1a612

JMLX42 changed the title ~~feat(sqlite): add support for serde_json::Value using the Json/Jsonb~~ Add support for serde_json::Value using the Json/Jsonb Sep 27, 2024

JMLX42 changed the title ~~Add support for serde_json::Value using the Json/Jsonb~~ Add SQLite support for serde_json::Value using the Json/Jsonb Sep 27, 2024

Jean-Marc Le Roux added 2 commits September 27, 2024 12:54

fix clippy errors

187ea47

fix Jsonb SQLite example

268a3df

weiznich requested changes Sep 27, 2024

View reviewed changes

Jean-Marc Le Roux added 4 commits September 27, 2024 23:19

factorize the Jsonb SQLite/Pg example

def9cee

add Jsonb scalar fields payload size consistency checks

ae02494

avoid Vec re-allocations in create_jsonb_header

2af1253

add tests for FromSql<Jsonb, Sqlite>

7fe0c67

update CHANGELOG with SQLite+Json/Jsonb support

76ac650

JMLX42 force-pushed the feature/sqlite-json branch 2 times, most recently from 04a9a11 to 9d5bf4b Compare September 30, 2024 18:25

JMLX42 force-pushed the feature/sqlite-json branch 3 times, most recently from c8ac82f to acaefe9 Compare September 30, 2024 20:18

JMLX42 requested a review from weiznich October 1, 2024 09:26

weiznich reviewed Oct 1, 2024

View reviewed changes

JMLX42 force-pushed the feature/sqlite-json branch 3 times, most recently from 059e51d to d7ec7d7 Compare October 1, 2024 15:58

JMLX42 requested a review from weiznich October 1, 2024 16:05

weiznich reviewed Oct 1, 2024

View reviewed changes

JMLX42 requested a review from weiznich October 1, 2024 19:43

JMLX42 force-pushed the feature/sqlite-json branch 2 times, most recently from 7558709 to e8179fe Compare October 1, 2024 20:00

Jean-Marc Le Roux added 2 commits October 1, 2024 22:04

add a feature gate for SQLite's Json and Jsonb FromSql/ToSql impls

adc5ebf

remove the serde_json feature gate on the Json and Jsonb types

de6b847

JMLX42 force-pushed the feature/sqlite-json branch from e8179fe to de6b847 Compare October 1, 2024 20:05

weiznich approved these changes Oct 2, 2024

View reviewed changes

weiznich added this pull request to the merge queue Oct 2, 2024

Merged via the queue into diesel-rs:master with commit 381be19 Oct 2, 2024
48 checks passed

sofiaritz reviewed Oct 13, 2024

View reviewed changes

sofiaritz mentioned this pull request Oct 14, 2024

Fix comment related to JSON and JSONB support under SQLite #4307

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add SQLite support for serde_json::Value using the Json/Jsonb #4284

Add SQLite support for serde_json::Value using the Json/Jsonb #4284

JMLX42 commented Sep 24, 2024 •

edited

Loading

weiznich left a comment

weiznich left a comment

weiznich left a comment

weiznich Sep 27, 2024

weiznich Sep 27, 2024

JMLX42 Sep 27, 2024

weiznich Sep 27, 2024

weiznich Sep 27, 2024

JMLX42 commented Sep 27, 2024

JMLX42 commented Sep 27, 2024

JMLX42 commented Sep 27, 2024

weiznich commented Sep 30, 2024

JMLX42 commented Sep 30, 2024

JMLX42 commented Sep 30, 2024

JMLX42 commented Sep 30, 2024

JMLX42 commented Oct 1, 2024

JMLX42 commented Oct 1, 2024

weiznich Oct 1, 2024

weiznich left a comment

weiznich Oct 1, 2024

weiznich left a comment

sofiaritz Oct 13, 2024

weiznich Oct 14, 2024

sofiaritz Oct 14, 2024

		@@ -0,0 +1,1073 @@
		//! Support for JSON and JSONB values under PostgreSQL.

Add SQLite support for serde_json::Value using the Json/Jsonb #4284

Add SQLite support for serde_json::Value using the Json/Jsonb #4284

Conversation

JMLX42 commented Sep 24, 2024 • edited Loading

weiznich left a comment

Choose a reason for hiding this comment

weiznich left a comment

Choose a reason for hiding this comment

weiznich left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JMLX42 commented Sep 27, 2024

JMLX42 commented Sep 27, 2024

JMLX42 commented Sep 27, 2024

weiznich commented Sep 30, 2024

JMLX42 commented Sep 30, 2024

JMLX42 commented Sep 30, 2024

JMLX42 commented Sep 30, 2024

JMLX42 commented Oct 1, 2024

JMLX42 commented Oct 1, 2024

Choose a reason for hiding this comment

weiznich left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weiznich left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JMLX42 commented Sep 24, 2024 •

edited

Loading