Error Bubbling

[modosc]

(this fixes #1020 and #1967) see also #160)

this pr adds a new schema option error_bubbling. with this set true (which is the default) the behavior is exactly the same as the current behavior, ie field errors bubble up to all the InputObject's that contain them.

with error_bubbling false this doesn't happen - we don't log static validation errors if the ast node that's invalid isn't the node we're visiting in ArgumentLiteralsAreCompatible.

update: i merged in my branch that fixes #1967 as well.

[rmosolgo]

Wow, this is great! I'm really impressed by the progress you made here, I've always been intimidated by this issue.

The behavior that you've added here is much better than the previous behavior, what do you think about making it the new default ?

[modosc]

The behavior that you've added here is much better than the previous behavior, what do you think about making it the new default ?

i'm ok with it - seems like it would make sense to do that in 1.9 but 1.8 probably should keep the current behavior? i'm also in the unenviable position of having to support the old behavior on a per-user basis for the time being so maybe that's where my conservatism comes from.

[rmosolgo]

unenviable position

😅

Sure, we can switch the default in 1.9.

[modosc]

also out of curiosity - would you be open to migrating to rspec? test::unit / minitest make it so hard to do basic things like "just run this single spec"

[rmosolgo]

Did you try focus for running a single example? Personally I've been happy with minitest and I'm not excited about the operational shakeup of migrating test frameworks.

[modosc]

Did you try focus for running a single example? Personally I've been happy with minitest and I'm not excited about the operational shakeup of migrating test frameworks.

that solved my problem, thanks.

[modosc]

also, i started on #1967 and the fix is really just an extension of this (including gating off of error_bubbling. might make sense to put that in this branch too? right now i've just forked and i'll push the pr up later today, just a heads up. merged into this branch.

[modosc]

i just tested this out with my company code and it looks like there's something not quite right. will update when i've sorted it out.

[modosc]

just figured it out - bubbling was never tested or fixed for the GraphQL::CoercionError case. this should be a simple fix, working on it now.

[modosc]

ok, looks good on my end now.

[rmosolgo]

👌 😎 Thanks again for your work here! It's such a long-needed improvement and it really required a big effort. Looks great.

[modosc]

👌 😎 Thanks again for your work here! It's such a long-needed improvement and it really required a big effort. Looks great.

thanks! next time i'll squash first, i'm horrified to see my wip commit checked in.