Skip to content
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.

Sign up for GitHub

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

Improved messages for json errors #1893

Merged
merged 6 commits into from
Oct 9, 2024
Merged

Improved messages for json errors #1893

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0164a5e
Improved messages for json errors
YohDeadfall Sep 30, 2024
43f3a7d
Improved messages
YohDeadfall Oct 8, 2024
1edb439
Fixed a typo
YohDeadfall Oct 8, 2024
55183f5
Made messages lower-case
YohDeadfall Oct 8, 2024
3aa05e5
Fixed typos
YohDeadfall Oct 9, 2024
2a727f7
Unwrap instead of useless messages
YohDeadfall Oct 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 8 additions & 8 deletions pgrx/src/datum/json.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ impl FromDatum for Json {
let len = varsize_any_exhdr(varlena);
let data = vardata_any(varlena);
let slice = std::slice::from_raw_parts(data as *const u8, len);
let value = serde_json::from_slice(slice).expect("failed to parse Json value");
let value = serde_json::from_slice(slice).expect("failed to parse a json value");
workingjubilee marked this conversation as resolved.
Show resolved Hide resolved
Some(Json(value))
}
}
Expand All @@ -67,12 +67,12 @@ impl FromDatum for JsonB {
pg_sys::jsonb_out,
&[Some(detoasted.into())],
)
.expect("failed to convert jsonb to a cstring");
.expect("failed to convert a jsonb to a cstring");
Copy link
Member

@workingjubilee workingjubilee Sep 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Likewise, "Datum must refer to JSONB for Postgres to serialize it to a cstring" or something similar.


let value = serde_json::from_str(
cstr.to_str().expect("text version of jsonb is not valid UTF8"),
cstr.to_str().expect("a text version of the jsonb is not valid UTF8"),
)
.expect("failed to parse JsonB value");
.expect("failed to parse a jsonb value");
Copy link
Member

Choose a reason for hiding this comment

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

reframe in terms of requirements, etc.


// free the cstring returned from direct_function_call -- we don't need it anymore
pg_sys::pfree(cstr.as_ptr() as void_mut_ptr);
Expand Down Expand Up @@ -122,7 +122,7 @@ impl FromDatum for JsonString {
/// for json
impl IntoDatum for Json {
fn into_datum(self) -> Option<pg_sys::Datum> {
let string = serde_json::to_string(&self.0).expect("failed to serialize Json value");
let string = serde_json::to_string(&self.0).expect("failed to serialize a json value");
Copy link
Member

Choose a reason for hiding this comment

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

reframe in terms of requirements, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

An attempt to find a correct wording has been failed completely. The thing here is that serde_json should be able to serialize it's own types without any issue. Moreover, the Value type says that it's a valid JSON representation.

Copy link
Member

Choose a reason for hiding this comment

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

Ah. Then if we don't have anything useful to say, simply .unwrap().

string.into_datum()
}

Expand All @@ -134,9 +134,9 @@ impl IntoDatum for Json {
/// for jsonb
impl IntoDatum for JsonB {
fn into_datum(self) -> Option<pg_sys::Datum> {
let string = serde_json::to_string(&self.0).expect("failed to serialize JsonB value");
let cstring =
alloc::ffi::CString::new(string).expect("string version of jsonb is not valid UTF8");
let string = serde_json::to_string(&self.0).expect("failed to serialize a jsonb value");
let cstring = alloc::ffi::CString::new(string)
.expect("string version of jsonb contains a non-terminating nul");
workingjubilee marked this conversation as resolved.
Show resolved Hide resolved

unsafe { direct_function_call_as_datum(pg_sys::jsonb_in, &[Some(cstring.as_ptr().into())]) }
}
Expand Down
Loading