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

Add descriptive value validation errors #811

Merged

Conversation

zaksabeast
Copy link
Contributor

Fixes: #693

This pull request adds descriptive errors when validating argument and default values.

For example, an input object missing a field would show Error for argument "arg": "ExampleInputObject" is missing fields: "b".

If there's a different approach you'd like to take for this, I'd be happy to implement that as well.

@LegNeato
Copy link
Member

Thanks for the PR! Are there perf implications of this change?

@zaksabeast
Copy link
Contributor Author

Sorry for the late response, and thank you for updating this branch!

The only performance implications I'm aware of would be for errors that are nested fairly deep within a graph. For example, an error on a GraphQL object nested 10 levels deep would have the error bubble up to each of the 10 levels, and create a new error string each time to help pinpoint the error's location.

If this is undesirable (since it could create larger errors for larger graphs), an alternate solution could be to have an enum for the different types of validations, and not build upon errors for objects or lists. This would ensure everything else would bubble up to an object or list, so the relative location of the error would be known, but the error wouldn't grow in size if it was deeply nested.

The second solution would be more performant since it wouldn't need to create as many strings on larger graphs, but at the cost of less information to locate an error.

Are there any performance implications you're wondering about specifically?

@toxeus
Copy link
Contributor

toxeus commented Feb 25, 2021

@zaksabeast do you plan to continue work on this PR?

@zaksabeast
Copy link
Contributor Author

@toxeus, sorry for the late response. I'm up for continuing this PR, although I'm not quite sure what changes are needed.

@haarts
Copy link

haarts commented Aug 12, 2021

This looks like a very valuable addition to Juniper. @LegNeato is there a particular reason why this isn't moving forward? ('I'm too busy' is a totally acceptable answer, I use it frequently)

@haarts
Copy link

haarts commented Aug 16, 2021

Perhaps @tyranron you can shine your light on this?

@tyranron
Copy link
Member

@haarts definitely not the first thing on my agenda, but I'll try to include this into new 0.16 major release.

@toxeus
Copy link
Contributor

toxeus commented Aug 23, 2021

@haarts @tyranron I think this would be a great addition for the next release. It feels inapt that GraphQLScalar::from_input_value() returns an Option instead of a Result which could carry detailed information about de-serialization errors that could (or rather should) be passed back to the client. Currently, I often have to reach out to the backend logs to find out why a certain GraphQL request failed. This doesn't feel right, especially if we consider that juniper can also be used to implement services for third parties that probably will not have access to backend logs.

@tyranron tyranron added this to the 0.16.0 milestone Nov 24, 2023
@tyranron tyranron added enhancement Improvement of existing features or bugfix k::ui/ux Related to UI/UX experience labels Nov 24, 2023
# Conflicts:
#	juniper/src/types/utilities.rs
#	juniper/src/validation/rules/arguments_of_correct_type.rs
#	juniper/src/validation/rules/default_values_of_correct_type.rs
@tyranron
Copy link
Member

It feels inapt that GraphQLScalar::from_input_value() returns an Option instead of a Result which could carry detailed information about de-serialization errors that could (or rather should) be passed back to the client.

This was done in #987.

@tyranron tyranron enabled auto-merge (squash) November 30, 2023 20:19
@tyranron tyranron merged commit ace6935 into graphql-rust:master Nov 30, 2023
174 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvement of existing features or bugfix k::ui/ux Related to UI/UX experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants