juniper icon indicating copy to clipboard operation
juniper copied to clipboard

Add descriptive value validation errors

Open zaksabeast opened this issue 5 years ago • 8 comments

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.

zaksabeast avatar Nov 21 '20 15:11 zaksabeast

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

LegNeato avatar Dec 16 '20 04:12 LegNeato

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?

zaksabeast avatar Dec 23 '20 13:12 zaksabeast

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

toxeus avatar Feb 25 '21 08:02 toxeus

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

zaksabeast avatar Mar 05 '21 02:03 zaksabeast

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 avatar Aug 12 '21 12:08 haarts

Perhaps @tyranron you can shine your light on this?

haarts avatar Aug 16 '21 09:08 haarts

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

tyranron avatar Aug 16 '21 09:08 tyranron

@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.

toxeus avatar Aug 23 '21 11:08 toxeus