Skip to content

Conversation

@JakobKlocker
Copy link
Contributor

added json_patch & jsonb_patch with the test cases provided by sqlite.
I'm not completely certain if the jsonb test cases are correct, since jsonb_patch returns a jsonb, yet they pass if I compare it to a json. This was done in the existing jsonb function as well, so I guess it should be correct.

#4366 (comment)

@weiznich weiznich requested a review from a team March 30, 2025 17:14
Copy link
Member

@weiznich weiznich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this and sorry for taking that long to review.

The implementation itself already looks on the right way, but we want to adjust the signatures slightly to better handle Null values as explained in the review comments.

/// ```
#[sql_name = "json_patch"]
#[cfg(feature = "sqlite")]
fn json_patch<J: JsonOrNullableJson + SingleValue>(j: J, patch: J) -> Json;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The return type is not correct. This function (and also jsonb_patch) returns Null if either the first or second argument is nullable.

We might want something like

Suggested change
fn json_patch<J: JsonOrNullableJson + SingleValue>(j: J, patch: J) -> Json;
fn json_patch<J1: JsonOrNullableJson + SingleValue + CombinedNullableValue<J2, Json>, J2: JsonOrNullableJson + SingleValue>(j: J1, patch: J1) -> J1::Out;

with the CombinedNullableValue trait defined here:

pub trait CombinedNullableValue<O, Out>: SingleValue {
type Out: SingleValue;
}
impl<T, O, Out> CombinedNullableValue<O, Out> for T
where
T: SingleValue,
O: SingleValue,
T::IsNull: OneIsNullable<O::IsNull>,
<T::IsNull as OneIsNullable<O::IsNull>>::Out: MaybeNullableType<Out>,
<<T::IsNull as OneIsNullable<O::IsNull>>::Out as MaybeNullableType<Out>>::Out: SingleValue,
{
type Out = <<T::IsNull as OneIsNullable<O::IsNull>>::Out as MaybeNullableType<Out>>::Out;
}

(That needs to be moved to diesel/src/expressions/nullable.rs into a pub(crate) private module.

///
/// assert_eq!(json!({"b":2}), result);
///
/// let result = diesel::select(json_patch::<Json, _, _>(json!({"a":1,"b":2}), json!({"a":9,"b":null,"c":8})))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be great to test for patch_json::<Nullable<Json>, _>(None, json!({something})) and patch_json(json!{something}, None) here. (Also for patch_jsonb)

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

Labels

None yet

3 participants