filter out null error location in toSpecification#3886
filter out null error location in toSpecification#3886bbakerman merged 1 commit intographql-java:masterfrom RafeArnold:handle-null-error-locations
Conversation
andimarek
left a comment
There was a problem hiding this comment.
I get the problem and agree that this needs to be improved, but I am not sure just returning null is the right solution. @bbakerman @dondonz thoughts?
So location is an optional field with an array of locations
So this makes sense to me - if we ended up with a array of locations where one of them is null - then just making it null makes sense We have had this code since November 2024 It was fixed by donna in 69569de |
|
@bbakerman ok ... approved |
|
Thanks for this PR - I cant believe we went this long without any reporting this |
|
Thanks for the PR! |
|
Thanks for merging so fast!
I'm guessing it never got caught because it popped up when generating a schema, not when executing a query. In the general case, an application wouldn't serialise an error caused during schema generation. We just have a somewhat niche use case. |
A fix for #3885
I felt that changing
GraphQLErrorHelper.location(), rather thanBaseError.getLocations(), was the least intrusive approach. However, there may be other places, in the present or in the future, where NPEs could show up due to null error locations, so maybe the change toBaseError.getLocations()outlined in #3885 would be a better option. Maybe both changes? It's at the discretion of the reviewer.