-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
json_typeof
and jsonb_typeof
function
#4250
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for opening the PR!
The changes look good, I only would like to see some tests about the null behavior, in past cases, Postgres returns NULL
when you pass NULL
to this kind of functions.
If this is case, the function signature is wrong, It should also cover the NULL input return.
To fix that we can use
fn json_typeof<E: JsonOrNullableJson + SingleValue + MaybeNullableValue<Text>>(e: E) -> E::Out;
The E: MaybeNulllableValue<Text>
says that the output values has the same nullability as the input value. That means Nullable<Json<T>>
evaluates to Nullable<Text>
and Json<T>
evaluates to Text
via the E::Out
return type
/// .get_result::<String>(connection)?; | ||
/// | ||
/// assert_eq!("null".to_string(), result); | ||
/// # Ok(()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a test with None input?
It is something like this:
/// let result = diesel::select(json_typeof::<Nullable<Json>, _>(None::<Value>))
/// .get_result::<String>(connection)?;
///
/// assert_eq!(<something>, result);
/// let result = diesel::select(jsonb_typeof::<Jsonb, _>(json!(null))) | ||
/// .get_result::<String>(connection)?; | ||
/// | ||
/// assert_eq!("null".to_string(), result); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you also add a None test here?
Thank you @guissalustiano for the suggestions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Thanks for submitting this PR ❤️ |
Added the
json_typeof
andjsonb_typeof
functions under Issue#4216