Skip to content

Sqlite json/jsonb patch #4554

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

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants