implement json_object #4230

valkrypton · 2024-09-03T12:09:31Z

added support for json_object under #4216

valkrypton · 2024-09-03T12:21:14Z

@weiznich what value should i pass it in auto_type.rs as pg_extras::array is of type Array<Integer>?

weiznich · 2024-09-03T17:08:03Z

@valkrypton I would suggest to just define a new column with the type Array<Text> in the pg_extras table.

guissalustiano

Thanks for open the PR!

I have some comments about the type signature and also request the other variant of this function

guissalustiano · 2024-09-03T17:31:57Z

diesel/src/pg/expression/functions.rs

+    /// assert_eq!(expected,json);
+    ///
+    /// let json = diesel::select(json_object(vec!["hello","world","John","Doe"])).get_result::<Value>(connection)?;
+    /// let expected:Value = serde_json::from_str(r#"{"hello":"world","John":"Doe"}"#).unwrap();


Could we use the serde_json::json! macro instead to construct the expected values?

guissalustiano · 2024-09-03T17:38:40Z

diesel/src/pg/expression/functions.rs

+    /// #     Ok(())
+    /// # }
+    /// ```
+    fn json_object(text_array:Array<Text>)->Json;


Arrays in diesel are always generate arrays with nullable elements, with will make impossible to use this methods with array columns from tables. But this method breaks if we pass a array to them.
I would like @weiznich opinion here

I would just accept both variants here. That might require an additional helper trait that is implemented for both Array<Text> and Array<Nullable<Text>>. Additionally what's about Nullable<Array<_>> here?

guissalustiano · 2024-09-03T17:40:50Z

diesel/src/pg/expression/functions.rs

@@ -1423,3 +1423,38 @@ define_sql_function! {
    /// ```
    fn array_upper<Arr: ArrayOrNullableArray + SingleValue>(array: Arr, dimension: Integer) -> Nullable<Integer>;
 }
+
+define_sql_function! {


This function also support a form jsonb_object(keys text[], values text[]), whitch takes keys and values pairwise from two separate arrays. Could you implement that too?
You can use #[sql_name ="..."] like in array_to_string definition and you also need to add some docs in the base function (exemple)

yes sure ill implement that too

valkrypton · 2024-09-06T12:09:45Z

@guissalustiano can you take a look at the type signature now?

guissalustiano · 2024-09-06T15:48:20Z

Sure!
The signature fn json_object(text_array:Nullable<Array<Text>>)->Json; is broken because if you call SELECT json_object(NULL) it returns NULL, but the function signatures don't tell you that.

A signature like fn json_object(text_array:Nullable<Array<Text>>)->Nullable<Json>; would fix that, but is annoying to have this extra Nullable<_> when you use that with non-nullable inputs. On to_json we use a helper trait to handle this.

We also would like to support an Nullable<Array<Nullable<Text>>> because diesel always generates columns as Array<Nullable<_>>, and so not supporting that would mean that json_object couldn't be used with a table, only literal values.

Could you also please remove the extra files in the commit? You can keep the branch out-of-date, don't worry, or use rebase to avoid showing the news commits in your PR

valkrypton · 2024-09-09T07:26:07Z

@guissalustiano take a look now, also i tried but couldn't figure out how to remove the extra files in the commit

weiznich

I've added a comment explaining how to get the right function signature. Hopefully that helps to cleanup the confusion there.

Also I would like to see some cleanup of the changes in this PR. The diff looks really wired, it seems like there is some half broken merge in there. Let me know if you need help to figure out how to solve that.

weiznich · 2024-09-09T18:08:09Z

diesel/src/pg/expression/functions.rs

+    /// #     Ok(())
+    /// # }
+    /// ```
+    fn json_object<Arr:MaybeNullableValue<Array<Nullable<Text>>>>(text_array:Arr) -> Nullable<Json>;


I'm sorry but this is still not correct. The correct signature would be:

Suggested change

fn json_object<Arr:MaybeNullableValue<Array<Nullable<Text>>>>(text_array:Arr) -> Nullable<Json>;

fn json_object<Arr:ArrayOrNullableArray + MaybeNullableValue<Json>>(text_array:Arr) -> Arr::Out where Arr::Inner: TextOrNullableText;

To explain the bounds:

Arr: ArrayOrNullableArray says the input type is either Array<T> or Nullable<Array<T>>

Arr: MaybeNullableValue<Json> says that the output values has the same nullability as the input value. That means Nullable<Array<T>> evaluates to Nullable<Json> and Array<T> evaluates to Json via the Arr::Out return type.

Arr::Inner: TextOrNullableText says that the inner type of the array is either Text or Nullable<Text>

Now the bad thing is that the declare_sql_function!() macro does not support where clauses, so we need to have a new helper trait for this. For that we need to declare:

trait TextArrayOrNullableTextArray {}

and implement it for the following types:

Array<Text>

Nullable<Array<Text>>

Array<Nullable<Text>>

Nullable<Array<Nullable<Text>>>

(which essentially only lists the supported argument types)

That gives us then the following function signature:

Suggested change

fn json_object<Arr:MaybeNullableValue<Array<Nullable<Text>>>>(text_array:Arr) -> Nullable<Json>;

fn json_object<Arr:TextArrayOrNullableTextArray + MaybeNullableValue<Json>>>(text_array:Arr) -> Arr:Out;

``

valkrypton · 2024-09-09T18:23:41Z

Thanks for the detailed explanation, i appreciate it.
The pr got messed up when i rebased this branch onto the updated master, i had no idea the previous commits would show up here as well. I tried to fix but couldn't figure it out, so if you could help me I'd appreciate it.

* Add `#[diagnostic::on_unimplemented]` for new helper trait * Fix formatting

weiznich

Thanks for the update. The signature looks good now.

I just rebased this PR again to resolve the merge conflicts, but the history already looked right before that. So either it was a github issue before or you already fixed it 🙏

I also added a #[diagnostic::on_unimplemented] attribute to the new helper trait to improve the compiler error message if some user passes in a wrong typed value.

weiznich requested a review from a team September 3, 2024 17:08

guissalustiano suggested changes Sep 3, 2024

View reviewed changes

weiznich requested changes Sep 9, 2024

View reviewed changes

valkrypton force-pushed the feat/add/json_object branch 2 times, most recently from ce549b0 to 3340b6f Compare September 10, 2024 07:24

valkrypton requested a review from weiznich September 10, 2024 07:27

Ali Tariq and others added 4 commits September 10, 2024 18:04

implement json_object

e172494

fixed auto_type.rs

e16542b

fixed type signature

2c9b3c6

Minor cleanup

24612ba

* Add `#[diagnostic::on_unimplemented]` for new helper trait * Fix formatting

weiznich force-pushed the feat/add/json_object branch from 3340b6f to 24612ba Compare September 10, 2024 16:13

weiznich approved these changes Sep 10, 2024

View reviewed changes

weiznich enabled auto-merge September 10, 2024 16:15

weiznich added this pull request to the merge queue Sep 10, 2024

Merged via the queue into diesel-rs:master with commit f78e6b8 Sep 10, 2024
48 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implement json_object #4230

implement json_object #4230

valkrypton commented Sep 3, 2024

valkrypton commented Sep 3, 2024

weiznich commented Sep 3, 2024

guissalustiano left a comment

guissalustiano Sep 3, 2024

guissalustiano Sep 3, 2024 •

edited

Loading

weiznich Sep 6, 2024

guissalustiano Sep 3, 2024 •

edited

Loading

valkrypton Sep 4, 2024

valkrypton commented Sep 6, 2024

guissalustiano commented Sep 6, 2024 •

edited

Loading

valkrypton commented Sep 9, 2024

weiznich left a comment

weiznich Sep 9, 2024

valkrypton commented Sep 9, 2024

weiznich left a comment

	fn json_object<Arr:MaybeNullableValue<Array<Nullable<Text>>>>(text_array:Arr) -> Nullable<Json>;
	fn json_object<Arr:ArrayOrNullableArray + MaybeNullableValue<Json>>(text_array:Arr) -> Arr::Out where Arr::Inner: TextOrNullableText;

	fn json_object<Arr:MaybeNullableValue<Array<Nullable<Text>>>>(text_array:Arr) -> Nullable<Json>;
	fn json_object<Arr:TextArrayOrNullableTextArray + MaybeNullableValue<Json>>>(text_array:Arr) -> Arr:Out;

implement json_object #4230

implement json_object #4230

Conversation

valkrypton commented Sep 3, 2024

valkrypton commented Sep 3, 2024

weiznich commented Sep 3, 2024

guissalustiano left a comment

Choose a reason for hiding this comment

guissalustiano Sep 3, 2024

Choose a reason for hiding this comment

guissalustiano Sep 3, 2024 • edited Loading

Choose a reason for hiding this comment

weiznich Sep 6, 2024

Choose a reason for hiding this comment

guissalustiano Sep 3, 2024 • edited Loading

Choose a reason for hiding this comment

valkrypton Sep 4, 2024

Choose a reason for hiding this comment

valkrypton commented Sep 6, 2024

guissalustiano commented Sep 6, 2024 • edited Loading

valkrypton commented Sep 9, 2024

weiznich left a comment

Choose a reason for hiding this comment

weiznich Sep 9, 2024

Choose a reason for hiding this comment

valkrypton commented Sep 9, 2024

weiznich left a comment

Choose a reason for hiding this comment

guissalustiano Sep 3, 2024 •

edited

Loading

guissalustiano Sep 3, 2024 •

edited

Loading

guissalustiano commented Sep 6, 2024 •

edited

Loading