Skip to content

filter out null error location in toSpecification #3886

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

Merged
merged 1 commit into from
Apr 4, 2025
Merged

filter out null error location in toSpecification #3886

merged 1 commit into from
Apr 4, 2025

Conversation

RafeArnold
Copy link
Contributor

@RafeArnold RafeArnold commented Apr 3, 2025

A fix for #3885

I felt that changing GraphQLErrorHelper.location(), rather than BaseError.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 to BaseError.getLocations() outlined in #3885 would be a better option. Maybe both changes? It's at the discretion of the reviewer.

Copy link
Member

@andimarek andimarek left a comment

Choose a reason for hiding this comment

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

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?

@bbakerman bbakerman added this to the 23.0 breaking changes milestone Apr 3, 2025
@bbakerman
Copy link
Member

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

If an error can be associated to a particular point in the requested GraphQL document, it should contain an entry with the key locations with a list of locations, where each location is a map with the keys line and column, both positive numbers starting from 1 which describe the beginning of an associated syntax element.

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

    public static Object locations(List<SourceLocation> locations) {
        return mapAndDropNulls(locations, GraphqlErrorHelper::location);
    }

It was fixed by donna in 69569de

@andimarek
Copy link
Member

@bbakerman ok ... approved

@bbakerman bbakerman merged commit 23012ad into graphql-java:master Apr 4, 2025
1 check passed
@bbakerman
Copy link
Member

Thanks for this PR - I cant believe we went this long without any reporting this

@dondonz
Copy link
Member

dondonz commented Apr 4, 2025

Thanks for the PR!

@RafeArnold RafeArnold deleted the handle-null-error-locations branch April 4, 2025 07:22
@RafeArnold
Copy link
Contributor Author

Thanks for merging so fast!

Thanks for this PR - I cant believe we went this long without any reporting this

I'm guessing it never got caught because it popped up when generating a schema, not when executing a query.

SchemaDefinition schemaDefinition = typeRegistry.schemaDefinition().orElse(SchemaDefinition.newSchemaDefinition().build());

In the general case, an application wouldn't serialise an error caused during schema generation. We just have a somewhat niche use case.

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.

4 participants