Add sqlite functions json and jsonb
#4388
Conversation
`bundled-bindings` to `bundled` to support jsonb.
xuehaonan27
force-pushed
the
add_sqlite_json_functions
branch
from
bc47130 to
287c9f6
Compare
|
Branch updated by force pushing: |
| /// # Ok(()) | ||
| /// # } | ||
| /// ``` | ||
| fn json<E: JsonOrNullableJsonOrJsonbOrNullableJsonb + SingleValue + MaybeNullableValue<Text>>(e: E) -> E::Out; |
There was a problem hiding this comment.
I think the correct signature here would be fn json<E: TextOrNullableText + MaybeNullableValue<Json>>(e: E) -> E::Out; as this function essentially turns text into json, not the other way around.
| /// # Ok(()) | ||
| /// # } | ||
| /// ``` | ||
| fn jsonb<E: JsonOrNullableJsonOrJsonbOrNullableJsonb + SingleValue + MaybeNullableValue<Jsonb>>(e: E) -> E::Out; |
There was a problem hiding this comment.
I think the correct signature here would be fn json<E: BinaryOrNullableBinary + MaybeNullableValue<Json>>(e: E) -> E::Out; as this function essentially turns a blob into json, not the other way around.
There was a problem hiding this comment.
Maybe you want to say fn **jsonb**<E: BinaryOrNullableBinary + MaybeNullableValue<**Jsonb**>>(e: E) -> E::Out; ? I tried out and found MaybeNullableValue<Jsonb> works but MaybeNullableValue<Json> not.
There was a problem hiding this comment.
Yes, that should be jsonb and not json. I just copied the other commend and failed to adjust that part, sorry for this.
diesel/Cargo.toml
Outdated
| @@ -24,7 +24,7 @@ include = [ | |||
| byteorder = { version = "1.0", optional = true } | |||
| chrono = { version = "0.4.20", optional = true, default-features = false, features = ["clock", "std"] } | |||
| libc = { version = "0.2.0", optional = true } | |||
| libsqlite3-sys = { version = ">=0.17.2, <0.31.0", optional = true, features = ["bundled_bindings"] } | |||
| libsqlite3-sys = { version = ">=0.17.2, <0.31.0", optional = true, features = ["bundled"] } | |||
There was a problem hiding this comment.
I would prefer not to enable the bundled feature here as that breaks anyone that requires dynamically linking libsqlite3.
I would rather prefer to have a SELECT sqlite_version(); query in the doc tests and just skip the test if the local version is too old.
There was a problem hiding this comment.
In fact, this has nothing to do with the version of sqlite3, but with the feature switch of the libsqlite3-sys crate that diesel depends on. If we continue to use the bundled_bindings feature instead of bundled, even if the sqlite3 version is higher than 3.45.0 (mine is 3.47.2), it will still report an error of no such function: jsonb. This means that we cannot test jsonb at all, and (possibly) all JSONB-related functions mentioned in that issue.
There was a problem hiding this comment.
I have raised an issue under rusqlite (the repository where libsqlite3-sys is located) and received a relevant answer. rusqlite/rusqlite#1614
There was a problem hiding this comment.
I've tested that locally and I believe it really just depends on which libsqlite3 (system library, not rust crate) version is linked. If that version is newer than 3.45 it works, if it's older it doesn't work. At least that's the case with my local 3.46 version.
There was a problem hiding this comment.
Oh you are right, I rechecked my libsqlite and found that it was linked to the system library provided by MacOS instead of the library provided by homebrew. 😭
|
Change made |
Add SQLite functions
jsonandjsonbaccording to issue #4366 .Following the implementation of PostgreSQL bindings, module declaration statements were added and modified in files such as
diesel/src/sqlite/mod.rsanddiesel/src/sqlite/expression/mod.rsto facilitate easier coding and testing for future contributors.NOTE: Testing on SQLite 3.47.2 with the
libsqlite-sys0.30.1 dependency failed due to the error:no such function: jsonb. Since SQLite 3.47.2 already supports JSONB, the issue lies within the SQLite binding crate. To resolve this, I updated thelibsqlite3-sysdependency ofdieselby switching the feature frombundled_bindingstobundled.