Adding Support for json_populate_record and jsonb_populate_record

[prkbuilds]

Using the traits already defined and have written the test_cases with my understanding of the functions, but open to suggestions and learnings!

[prkbuilds]

just for context the PR is for the issue #4216

[weiznich]

I'm sorry to take that long to review this PR and I'm also sorry to bring up an issue, but it seems like the nullable bounds are not correct yet. I've added a more detailed comment in the specific location on how this can be fixed.

[weiznich]

I've played a bit around on the postgresql command line and it seems like the output type of this function is only null if both inputs are null. The CombineNullableValue helper trait expresses a constraint that says "if one of the inputs is null the output is null", so that's slightly different. We need to have a similar trait, but with slightly different bounds. Essentially we need a new trait with the same structure, but that replace the usage of OneIsNullable in the bounds with AllIsNullable. The trait implementation of CombineNullableValue is here:

diesel/diesel/src/pg/expression/expression_methods.rs

Lines 3698 to 3707 in a5f7548

	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 can be just copied + adjusted for the new trait.

[prkbuilds]

Thanks @weiznich ! This is great to know and really informative, I totally understand what needs to be done, I'll implement the required change are add the changes. and thanks for telling a good way to test the nullability, will definitely keep that in mind next time.

[prkbuilds]

I'm not sure if the naming convention is the best for the new Nullable Trait implementation. I think this will take care of Nullablility, but let me know if there is anything I might have missed.

[weiznich]

Thanks for the update. This looks good beside the name of the new trait.

[weiznich]

Can we name this CombinedAllNullableValue or something like that instead to transport that this only evaluates to Nullable if all inputs are Nullable?

[prkbuilds]

Hi @weiznich , sure I will change the name accordingly. And yes they allarenullable only returns nullable value when both values are null, completing the logic we need for this trait.

[weiznich]

Thanks for the update