The Wayback Machine - https://web.archive.org/web/20220423121503/https://github.com/diesel-rs/diesel/issues/3032
Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for currently unsupported JSON/JSONB operators #3032

Closed
6 of 18 tasks
weiznich opened this issue Jan 28, 2022 · 26 comments · Fixed by #3092
Closed
6 of 18 tasks

Add support for currently unsupported JSON/JSONB operators #3032

weiznich opened this issue Jan 28, 2022 · 26 comments · Fixed by #3092

Comments

@weiznich
Copy link
Member

@weiznich weiznich commented Jan 28, 2022

Diesel currently supports the postgres json and jsonb types. We do not provide built-in support for various operators available for these types. This is a tracking issue for adding support for these operators.

The general strategy for adding support for new operators is as following:

  1. Define the operator via infix_operator!(). The contains operator would be defined as following: infix_operator!(KeyExists, " ? ", backend: Pg); These operators can be defined here. If there is already an existing definition, this step could be skipped.
  2. Define a new helper type here. Helper types are a tool for diesel users to simplify type definitions. Again if there is already a matching type definition this could be skipped.
  3. Add a new ExpressionMethods extension trait for the corresponding types. This only needs to happen once for jsonb and once for jsonb + json. See the PgRangeExpressionMethods trait here as example.
  4. Add a new method to the corresponding ExpressionMethods trait. See here for an example for a different operator/type. This definition should:
    • Use the previously defined helper type
    • Contain a documentation with an example
  5. Check if compile test output is up to date by running TRYBUILD=overwrite cargo test inside of diesel_compile_tests. (This is required if you've added a new operator based on infix_operator!
  6. Submit a PR with the change

Operator list:

  • json/jsonb -> integer (indexing by index, returns json/jsonb) **
  • json/jsonb -> text (indexing by key name, returns json/jsonb) **
  • json/jsonb ->> integer (indexing by index, returns field as text) **
  • json/jsonb ->> text (indexing by key name, returns field as text) **
  • json/jsonb #> text[] (indexing by path, returns field as json/jsonb) *
  • json/jsonb #>> text[] (indexing by path, returns field as text) *
  • jsonb @> jsonb (checks if the second value is contained by the first value, returns a boolean) *
  • jsonb <@ jsonb (checks if the first value is contained by the second value, returns a boolean) *
  • jsonb ? text (checks if a string exists as top level key in the given json, returns a boolean) *
  • jsonb ?| text[] (checks if any of the strings exists as top level key in the given json, returns a boolean) *
  • jsonb ?& text[] (checks if all of the strings exist as top level key in the given json, returns a boolean) *
  • jsonb || jsonb (concats two json values, returns jsonb) *
  • jsonb - text (remove the key given as string from the json, returns jsonb) **
  • jsonb - text[] (removes all keys given as string from the json, returns jsonb) **
  • jsonb - integer (removes the field with the given index from the json, returns jsonb) **
  • jsonb #- text[] (removes the given path from the json, returns jsonb) *
  • jsonb @? jsonpath (Does JSON path return any item for the specified JSON value?, returns a boolean) ***
  • jsonb @@ jsonpath (Returns the result of a JSON path predicate check for the specified JSON value. Only the first item of the result is taken into account. If the result is not Boolean, then NULL is returned., returns a boolean`) ***

Items marked with one * can follow the instructions directly, items marked with ** require additional work to unify the different input types behind a common operator/function call (See here for a general example on how to abstract over expressions of different types), items marked with *** likely require additional design work.

Please add a comment to this issue if you plan to work on a specific operator.

If there is anything unclear about how to add a support for a specific operator just ask and we will try to answer your questions.

@czotomo
Copy link
Contributor

@czotomo czotomo commented Jan 31, 2022

Believe it or not, I was considering suggesting this some time last week!
I will gladly take this task up for the one asterisk operators and maybe this work will evolve into discussion on other operators.

Edit:
on the second thought, this probably shouldn't all be done by one person.
First operator I will work on:
jsonb || jsonb

@weiznich what version do we add this in, current or master?

@weiznich
Copy link
Member Author

@weiznich weiznich commented Feb 1, 2022

@czotomo Any new feature should target the master branch.

@czotomo
Copy link
Contributor

@czotomo czotomo commented Feb 1, 2022

Jsonb concat PR: #3035

@czotomo
Copy link
Contributor

@czotomo czotomo commented Feb 3, 2022

Alright, let's continue with the trivial ones.
I'm going to implement
jsonb ? text

Edit:
PR #3036

@czotomo
Copy link
Contributor

@czotomo czotomo commented Feb 4, 2022

Alright, I'll implement the array and object indexing operators by ints and strings, I don't see a reason to split this work.
json/jsonb -> integer => json/jsonb
json/jsonb -> text => json/jsonb
json/jsonb ->> integer => text
json/jsonb ->> text => text

@mrbuzz
Copy link
Contributor

@mrbuzz mrbuzz commented Feb 4, 2022

Hey, I'd like to get my hands dirty with these two operators if that's ok..
jsonb ?| text[]
jsonb ?& text[]

@weiznich
Copy link
Member Author

@weiznich weiznich commented Feb 4, 2022

@czotomo You can likely combine these two operations

json/jsonb -> integer => json/jsonb
json/jsonb -> text => json/jsonb

in one function by using a similar trick as for implementing the PgJsonbExxpressionMethods trait for jsonb + nullable jsonb. To do that you can use a generic type for the sqltype passed into AsExpression and restrict that type to Integer or Text by using am auxiliary trait.

(Also these operators need there own ExpressionMethods trait as these need to be implemented for the Json type as well. Again thats possible by using a corresponding auxiliary trait implemented for jsonb and json + their nullable variants.)

@mrbuzz 👍 go for it

@zaneduffield
Copy link
Contributor

@zaneduffield zaneduffield commented Feb 6, 2022

Hi, I'd like to try adding support for these two operators.
jsonb @> jsonb
jsonb <@ jsonb

@gamesbrainiac
Copy link
Contributor

@gamesbrainiac gamesbrainiac commented Feb 6, 2022

@czotomo Would it be possible to team up on this? Quite new to Rust, but have plenty of development experience. I'd be happy with just being a fly on the wall, and just see how the problem is solved.

@czotomo
Copy link
Contributor

@czotomo czotomo commented Feb 6, 2022

@gamesbrainiac sure, I did not do too much work on these four yet. Please reach out to me on gitter (same handle as on Github) so we can sync.

@mrbuzz
Copy link
Contributor

@mrbuzz mrbuzz commented Feb 8, 2022

I'd like to take these three operators as my next task:
jsonb - text
jsonb - text[]
jsonb - integer
I might need some guidance as I'm relatively new to Rust, I'll start by looking at the example code

@gamesbrainiac
Copy link
Contributor

@gamesbrainiac gamesbrainiac commented Feb 8, 2022

Me and @czotomo have made some progress, but we're a little stuck. If @mrbuzz wants to join in, we plan of having another crack at it tomorrow.

@weiznich
Copy link
Member Author

@weiznich weiznich commented Feb 8, 2022

@gamesbrainiac @czotomo Is there any information that I could provide to help you? As additional side note: Opening a WIP PR could also be a good idea, as this makes it easier for others to comment on specific code issues, as they have access to the whole change.

@weiznich
Copy link
Member Author

@weiznich weiznich commented Feb 8, 2022

@mrbuzz I'm happy to answer any question that comes up. Just ask .

@zaneduffield
Copy link
Contributor

@zaneduffield zaneduffield commented Feb 9, 2022

I'll try these operators next.
json/jsonb #> text[]
json/jsonb #>> text[]

Edit: they aren't marked as ** like the others above them which also operate on json/jsonb. Won't they also require a marker trait for json/jsonb ?

@mrbuzz
Copy link
Contributor

@mrbuzz mrbuzz commented Feb 9, 2022

Me and @czotomo have made some progress, but we're a little stuck. If @mrbuzz wants to join in, we plan of having another crack at it tomorrow.

That'd be cool! Thanks for the invite. At what time are you meeting? I am a little short on time today but I'll see what I can do

@weiznich
Copy link
Member Author

@weiznich weiznich commented Feb 9, 2022

@zaneduffield Yes those would need marker traits for json/jsonb. So basically this thing here:

impl<T> PgJsonbExpressionMethods for T
where
T: Expression,
T::SqlType: JsonbOrNullableJsonb,
{
}
#[doc(hidden)]
/// Marker trait used to implement `PgJsonbExpressionMethods` on the appropriate types.
pub trait JsonbOrNullableJsonb {}
impl JsonbOrNullableJsonb for Jsonb {}
impl JsonbOrNullableJsonb for Nullable<Jsonb> {}

but so that the helper trait (JsonbOrNullableJsonb) is implemented for Json and Nullable<Json> as well.

@gamesbrainiac
Copy link
Contributor

@gamesbrainiac gamesbrainiac commented Feb 9, 2022

This is the progress we've made so far. gamesbrainiac@3e857bb

We tried the tests, but it keeps failing. It should work, but it's hard to figure out what's wrong.

@mrbuzz Do you have gitter? We usually communicate using that.

@czotomo
Copy link
Contributor

@czotomo czotomo commented Feb 9, 2022

@weiznich is there a diesel development / maintenance Gitter room where people who know ins and outs hang out, or are the conversations strictly performed in issues' and PRs' comments?

@weiznich
Copy link
Member Author

@weiznich weiznich commented Feb 10, 2022

@gamesbrainiac
The following change fixes the test:

@@ -1363,7 +1363,7 @@ pub trait PgAnyJsonExpressionMethods: Expression + Sized {
     ///     .values((name.eq("Claus"), address.eq(&santas_address)))
     ///     .execute(conn)?;
     ///
-    /// let santas_postcode = contacts.select(address.retrieve_as_object("postcode")).get_result::<serde_json::Value>(conn)?;
+    /// let santas_postcode = contacts.select(address.retrieve_as_object::<diesel::sql_types::Text, _>("postcode")).get_result::<serde_json::Value>(conn)?;
     /// assert_eq!(santas_postcode, serde_json::json!("99705"));
     ///
     ///
@@ -1388,7 +1388,8 @@ pub trait PgAnyJsonExpressionMethods: Expression + Sized {
     ///
     /// let roberts_second_address_in_db = contacts
     ///                             .filter(name.eq("Robert Downey Jr."))
-    ///                             .select(address.retrieve_as_object(1))
+    ///                             .select(address.retrieve_as_object::<
+    ///                             diesel::sql_types::Integer, _>(1))
     ///                             .get_result::<serde_json::Value>(conn)?;
     ///
     /// let roberts_second_address = serde_json::json!({
@@ -1405,9 +1406,9 @@ pub trait PgAnyJsonExpressionMethods: Expression + Sized {
     /// #     Ok(())
     /// # }
     /// ```
-    fn retrieve_as_object<T, ST>(self, other: T) -> dsl::RetrieveAsObjectJsonb<Self, T, ST>
+    fn retrieve_as_object<ST, T>(self, other: T) -> dsl::RetrieveAsObjectJsonb<Self, T, ST>
     where
-        ST: SqlType + TypedExpressionType,
+        ST: SqlType + TypedExpressionType + TextOrInteger,
         T: AsExpression<ST>,
     {
         Grouped(RetrieveAsObjectJsonb::new(self, other.as_expression()))
@@ -1432,6 +1433,5 @@ impl<T> PgAnyJsonExpressionMethods for T
 where
     Self::SqlType: JsonOrNullableJsonOrJsonbOrNullableJsonb,
     T: Expression,
-    T::SqlType: TextOrInteger,
 {
 }

To explain that a bit:

The change in line 1432 (removing T::SqlType: TextOrInteger) fixes the problem that the trait is not implemented for any jsonb column. That happens because that condition reads like: If the column is (a json or jsonb or nullable json or nullable jsonb) and (text or integer). That something that cannot be true at any point of time, as a column has just one sql type. What we cant here is to assert that the left side of the expression is JsonOrNullableJsonOrJsonbOrNullableJsonb while the right side is TextOrInteger. So we remove the trait bound here and add it in line 1405, where other represents the right side of the expression.

The other changes work around the fact that rustc cannot infer the correct sql type for &str and integer, so that they must be specified directly. That's a bit unfortunate, as this makes the current interface hard to use. I would like to hear opinions here so that we hopefully can find a better solution.

@czotomo It's fine to ask these question in the normal gitter room, but if there is need for that we can just create a new room for that.

@gamesbrainiac
Copy link
Contributor

@gamesbrainiac gamesbrainiac commented Feb 10, 2022

Perhaps we have two methods? Or perhaps we always get text, and cast it later to Integer? You're right, this looks very ugly.

@mrbuzz
Copy link
Contributor

@mrbuzz mrbuzz commented Feb 12, 2022

@mrbuzz I'm happy to answer any question that comes up. Just ask .

Just opened a draft PR #3044 for the remove key operators. I used an extra generic parameter to carry the input type as @gamesbrainiac and @czotomo did in their implementation.

This is the progress we've made so far. gamesbrainiac@3e857bb

We tried the tests, but it keeps failing. It should work, but it's hard to figure out what's wrong.

@mrbuzz Do you have gitter? We usually communicate using that.

Yep! Same name as on Github. Feel free to ping me any time

@weiznich
Copy link
Member Author

@weiznich weiznich commented Feb 14, 2022

I just want to leave a comment here that I'm away for this week. So don't expect prompt answers from me. I will try yo answer the outstanding questions next week.

@weiznich
Copy link
Member Author

@weiznich weiznich commented Mar 19, 2022

@mrbuzz @gamesbrainiac @czotomo Sorry for being away so long. I've pushed #3092 which contains all of your work + some fixes to the operators. Additionally it adds the missing operators. Feel free to leave comments about potential improvements on that PR.
Thanks again for your work ❤️

@czotomo
Copy link
Contributor

@czotomo czotomo commented Mar 21, 2022

@weiznich should these operators be mentioned in the changelog for version 2?

@weiznich
Copy link
Member Author

@weiznich weiznich commented Mar 22, 2022

@czotomo Yes that would be great

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment