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

Jump to bottom

Improved messages for json errors #1893

Merged
merged 6 commits into from
Oct 9, 2024
Merged

Improved messages for json errors #1893

merged 6 commits into from
Oct 9, 2024

Conversation

YohDeadfall
Copy link
Contributor

@YohDeadfall YohDeadfall commented Sep 30, 2024 •
edited by workingjubilee
Loading

workingjubilee
workingjubilee requested changes Sep 30, 2024
View reviewed changes
pgrx/src/datum/json.rs Outdated
@@ -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.

pgrx/src/datum/json.rs Outdated
)
.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.

pgrx/src/datum/json.rs Outdated
@@ -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().

SteveLauC
SteveLauC reviewed Oct 8, 2024
View reviewed changes
workingjubilee
workingjubilee approved these changes Oct 9, 2024
View reviewed changes
Copy link
Member

@workingjubilee workingjubilee left a comment

Choose a reason for hiding this comment

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

thank you for trying. this looks good with the nits addressed.

@YohDeadfall @workingjubilee
Fixed typos
3aa05e5 
Co-authored-by: Jubilee <[email protected]>
@workingjubilee workingjubilee reopened this Oct 9, 2024
@workingjubilee workingjubilee merged commit 3dda1d5 into pgcentralfoundation:develop Oct 9, 2024
14 checks passed
@eeeebbbbrrrr eeeebbbbrrrr mentioned this pull request Oct 14, 2024
Merged
eeeebbbbrrrr added a commit that referenced this pull request Oct 14, 2024
@eeeebbbbrrrr
Update version to 0.12.6 (#1919)
123b2b0 
Welcome to pgrx v0.12.6. This releases fixes a number of bugs, cleans up
various parts of the code, adds more Postgres `#include` headers, and
other little things.

To upgrade, first install cargo-pgrx with `cargo install cargo-pgrx
--version 0.12.6 --locked`. Then you can run `cargo pgrx upgrade` in
your extension's crate to update its dependencies.

## What's Changed
* Removed extra allocation from `IntoDatum for char` by @YohDeadfall in
#1887
* Fixed IntoDatum impl for char introduced by #1887 by @YohDeadfall in
#1889
* Support refs to unsized SqlTranslatable types by @workingjubilee in
#1890
* Nullable is Copy when T is Copy by @workingjubilee in
#1884
* Macro based IntoDatum for string types by @YohDeadfall in
#1886
* `impl FromDatum for CString` by @workingjubilee in
#1896
* Add `BorrowDatum` for unsizing borrows of datums by @workingjubilee in
#1891
* Always panic if len of varlena exceeds the maximum by @YohDeadfall in
#1894
* Use cstr literal syntax to avoid unsafe and ??? by @workingjubilee in
#1898
* Implement UnboxDatum for ItemPointerData by @syvb in
#1900
* fix compilation on some CPUs and Windows by @usamoi in
#1901
* docs: update generic type name of TableIterator by @SteveLauC in
#1905
* Remove Python workaround for Homebrew by @workingjubilee in
#1908
* Improved messages for json errors by @YohDeadfall in
#1893
* Adds more headers by @aykut-bozkurt in
#1910
* docs: align description of GucContext::SuBackend and
GucContext::Backend with original PostgreSQL description by @mrl5 in
#1909
* docs: update outdated README by @SteveLauC in
#1914
* fix compilation on emscripten by @usamoi in
#1917
* feat: include 'storage/indexfsm.h' by @SteveLauC in
#1912
* refactor: more CStr literals by @SteveLauC in
#1911
* fix segfault with empty `numrange` during `from_datum()` by
@eeeebbbbrrrr in #1918

## New Contributors
* @mrl5 made their first contribution in
#1909

**Full Changelog**:
v0.12.5...v0.12.6
usamoi pushed a commit to tensorchord/pgrx that referenced this pull request Mar 6, 2025
@YohDeadfall
Improved messages for json errors (pgcentralfoundation#1893)
24c4600 
Use error messages that better reflect
- https://doc.rust-lang.org/std/option/enum.Option.html#recommended-message-style
- https://rustc-dev-guide.rust-lang.org/diagnostics.html
usamoi pushed a commit to tensorchord/pgrx that referenced this pull request Mar 6, 2025
@eeeebbbbrrrr
Update version to 0.12.6 (pgcentralfoundation#1919)
fae2a79 
Welcome to pgrx v0.12.6. This releases fixes a number of bugs, cleans up
various parts of the code, adds more Postgres `#include` headers, and
other little things.

To upgrade, first install cargo-pgrx with `cargo install cargo-pgrx
--version 0.12.6 --locked`. Then you can run `cargo pgrx upgrade` in
your extension's crate to update its dependencies.

## What's Changed
* Removed extra allocation from `IntoDatum for char` by @YohDeadfall in
pgcentralfoundation#1887
* Fixed IntoDatum impl for char introduced by pgcentralfoundation#1887 by @YohDeadfall in
pgcentralfoundation#1889
* Support refs to unsized SqlTranslatable types by @workingjubilee in
pgcentralfoundation#1890
* Nullable is Copy when T is Copy by @workingjubilee in
pgcentralfoundation#1884
* Macro based IntoDatum for string types by @YohDeadfall in
pgcentralfoundation#1886
* `impl FromDatum for CString` by @workingjubilee in
pgcentralfoundation#1896
* Add `BorrowDatum` for unsizing borrows of datums by @workingjubilee in
pgcentralfoundation#1891
* Always panic if len of varlena exceeds the maximum by @YohDeadfall in
pgcentralfoundation#1894
* Use cstr literal syntax to avoid unsafe and ??? by @workingjubilee in
pgcentralfoundation#1898
* Implement UnboxDatum for ItemPointerData by @syvb in
pgcentralfoundation#1900
* fix compilation on some CPUs and Windows by @usamoi in
pgcentralfoundation#1901
* docs: update generic type name of TableIterator by @SteveLauC in
pgcentralfoundation#1905
* Remove Python workaround for Homebrew by @workingjubilee in
pgcentralfoundation#1908
* Improved messages for json errors by @YohDeadfall in
pgcentralfoundation#1893
* Adds more headers by @aykut-bozkurt in
pgcentralfoundation#1910
* docs: align description of GucContext::SuBackend and
GucContext::Backend with original PostgreSQL description by @mrl5 in
pgcentralfoundation#1909
* docs: update outdated README by @SteveLauC in
pgcentralfoundation#1914
* fix compilation on emscripten by @usamoi in
pgcentralfoundation#1917
* feat: include 'storage/indexfsm.h' by @SteveLauC in
pgcentralfoundation#1912
* refactor: more CStr literals by @SteveLauC in
pgcentralfoundation#1911
* fix segfault with empty `numrange` during `from_datum()` by
@eeeebbbbrrrr in pgcentralfoundation#1918

## New Contributors
* @mrl5 made their first contribution in
pgcentralfoundation#1909

**Full Changelog**:
pgcentralfoundation/pgrx@v0.12.5...v0.12.6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SteveLauC SteveLauC SteveLauC left review comments

@workingjubilee workingjubilee workingjubilee approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@YohDeadfall @workingjubilee @SteveLauC