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.

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

Improve json cast error messages #7312

Merged
merged 8 commits into from
May 9, 2024
Merged

Improve json cast error messages #7312

merged 8 commits into from
May 9, 2024

Conversation

dnwpark
Copy link
Contributor

@dnwpark dnwpark commented May 7, 2024

Provide context to error messages when casting to from json to collections.

Given SELECT <tuple<a: int64>>to_json('{"b": 1}'); the following error is produced:

InvalidValueError: while casting 'std::json' to 'tuple<a: std::int64>', at tuple element 'a', missing value in JSON object

Given SELECT <tuple<a: int64>>to_json('{"a": "b"}'); the following error is produced:

InvalidValueError: while casting 'std::json' to 'tuple<a: std::int64>', at tuple element 'a', expected JSON number or null; got JSON string

@dnwpark
Copy link
Contributor Author

dnwpark commented May 9, 2024

Looks bigger than it is :)

@dnwpark dnwpark marked this pull request as ready for review May 9, 2024 00:42
Copy link
Member

@msullivan msullivan left a comment

Choose a reason for hiding this comment

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

This looks great, and will be a nice improvement.

Remember to bump EDGEDB_CATALOG_VERSION

qlast.Constant.boolean(allow_null),
]
if error_message_context := cast_message_context(subctx):
detail = qlast.Constant.string(
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be cleaner to build the dict and json.dumps it

@@ -85,6 +85,31 @@ std::json_get(
$$;
};


CREATE FUNCTION
Copy link
Member

Choose a reason for hiding this comment

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

We want this because it produces a better error message than leaving it to the cast, or what?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function lets us produce different error messages for when a tuple is missing an element and when the element is there but explicitly set to null.

Comment on lines +456 to +461
error_message_context = ''
if err_details.detail_json:
error_message_context = (
err_details.detail_json.get('error_message_context', '')
)

Copy link
Member

Choose a reason for hiding this comment

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

The big question for me with this PR is:
Is it better to have the context prepended to the actual message, or would it be better to put it in the details field?

My inclination had been details, but this looks pretty nice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess either works, but it seemed to me that details were often more about providing information from beyond the immediate context. (eg. stuff about the inheritance of the types involved in an error)

@dnwpark dnwpark merged commit c1a26b4 into master May 9, 2024
24 checks passed
@dnwpark dnwpark deleted the json-cast-error-messages branch May 9, 2024 22:18
msullivan added a commit that referenced this pull request May 24, 2024
Apparently #7312 some code in a SQL function that only works on
Postgres 16. Fix it.

Fixes #7365.
@msullivan msullivan mentioned this pull request May 24, 2024
msullivan added a commit that referenced this pull request May 24, 2024
Apparently #7312 some code in a SQL function that only works on
Postgres 16. Fix it.

Fixes #7365.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants