Skip to content

Filter out negative line and column error locations in toSpecification #3753

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.

Sign up for GitHub

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

Jump to bottom
Merged
dondonz merged 1 commit into from
Nov 27, 2024
Merged

Filter out negative line and column error locations in toSpecification #3753

dondonz merged 1 commit into from
Nov 27, 2024

Conversation

@dondonz
Copy link
Member

@dondonz dondonz commented Nov 24, 2024 •
edited
Loading

Fixes #3661

Internally graphql-java represents an "empty" SourceLocation as line -1 and column -1.

graphql-java/src/main/java/graphql/parser/ExtendedBailStrategy.java

Line 72 in ee32161

SourceLocation location = sourceLocation == null ? SourceLocation.EMPTY : sourceLocation;

However in the issue raised it turns out the specification requires these numbers to be positive integers, starting at 1. https://spec.graphql.org/draft/#sel-GAPHRPFCCaCGX5zM

This PR updates the toSpecification method to filter out error locations that do not have both a positive line and column number. This only changes toSpecification and not any internal representation of SourceLocation.

This could be considered a breaking change, however it's necessary to bring behaviour in line with the spec.

dondonz
dondonz commented Nov 24, 2024
View reviewed changes
src/test/groovy/graphql/GraphQLErrorTest.groovy

List<SourceLocation> mkLocations() {
return [mkLocation(666, 999), mkLocation(333, 0)]
return [mkLocation(666, 999), mkLocation(333, 1)]
Copy link
Member Author

@dondonz dondonz Nov 24, 2024

Choose a reason for hiding this comment

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

Fix old tests to be compliant

Copy link
Member

@bbakerman bbakerman Nov 25, 2024

Choose a reason for hiding this comment

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

Ahh I see

dondonz
dondonz commented Nov 24, 2024
View reviewed changes
src/main/java/graphql/GraphqlErrorHelper.java
if (line < 1 || column < 1) {
return null;
}
return Map.of("line", line, "column", column);
Copy link
Member Author

@dondonz dondonz Nov 24, 2024

Choose a reason for hiding this comment

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

Tiny change, this becomes an immutable map

@dondonz dondonz added the breaking change requires a new major version to be relased label Nov 24, 2024
@dondonz dondonz added this to the 23.x breaking changes milestone Nov 24, 2024
@github-actions
Copy link
Contributor

github-actions bot commented Nov 24, 2024

Test Results

  304 files  ±0    304 suites  ±0   45s ⏱️ +5s
3 438 tests +1  3 433 ✅ +1  5 💤 ±0  0 ❌ ±0 
3 527 runs  +1  3 522 ✅ +1  5 💤 ±0  0 ❌ ±0 

Results for commit 69569de. ± Comparison against base commit f83ba25.

This pull request removes 129 and adds 108 tests. Note that renamed tests count towards both. 
	?
                __schema { types { fields { args { type { name fields { name }}}}}}
                __schema { types { fields { type { name fields { name }}}}}
                __schema { types { inputFields { type { inputFields { name }}}}}
                __schema { types { interfaces { fields { type { interfaces { name } } } } } }
                __schema { types { name} }
                __type(name : "t") { name }
                a1: __schema { types { name} }
                a1: __type(name : "t") { name }
                a2 :  __type(name : "t1") { name }
…

graphql.GraphQLErrorTest ‑ toSpecification filters out error locations with line and column not starting at 1, as required in spec
graphql.ScalarsBooleanTest ‑ parseValue throws exception for invalid input <java.lang.Object@236861da>
graphql.ScalarsBooleanTest ‑ serialize throws exception for invalid input <java.lang.Object@794c5f5e>
graphql.ScalarsIDTest ‑ parseValue allows any object via String.valueOf <java.lang.Object@368424db>
graphql.ScalarsIDTest ‑ serialize allows any object via String.valueOf <java.lang.Object@1a536164>
graphql.ScalarsIntTest ‑ parseValue throws exception for invalid input <java.lang.Object@15d65fcf>
graphql.ScalarsIntTest ‑ serialize throws exception for invalid input <java.lang.Object@1377b1a0>
graphql.execution.instrumentation.TracingInstrumentationTest ‑ do not trace introspection information [testExecutionStrategy: <graphql.execution.AsyncExecutionStrategy@2cc3b0a7 fieldCollector=graphql.execution.FieldCollector@41628b4f executionStepInfoFactory=graphql.execution.ExecutionStepInfoFactory@4cb641a5 dataFetcherExceptionHandler=graphql.execution.SimpleDataFetcherExceptionHandler@71077d1 resolvedType=graphql.execution.ResolveType@36e7b91c>, #0]
graphql.execution.instrumentation.TracingInstrumentationTest ‑ do not trace introspection information [testExecutionStrategy: <graphql.execution.AsyncSerialExecutionStrategy@1dcfbad1 fieldCollector=graphql.execution.FieldCollector@19e5e846 executionStepInfoFactory=graphql.execution.ExecutionStepInfoFactory@12418d3f dataFetcherExceptionHandler=graphql.execution.SimpleDataFetcherExceptionHandler@2258228f resolvedType=graphql.execution.ResolveType@42210d27>, #1]
graphql.execution.instrumentation.TracingInstrumentationTest ‑ tracing captures timings as expected [testExecutionStrategy: <graphql.execution.AsyncExecutionStrategy@3020f22 fieldCollector=graphql.execution.FieldCollector@6a40e660 executionStepInfoFactory=graphql.execution.ExecutionStepInfoFactory@4a57ad67 dataFetcherExceptionHandler=graphql.execution.SimpleDataFetcherExceptionHandler@1f1b69bb resolvedType=graphql.execution.ResolveType@c730e65>, #0]
…

bbakerman
bbakerman reviewed Nov 25, 2024
View reviewed changes
src/test/groovy/graphql/GraphQLErrorTest.groovy
.build() |
[
locations: [[line: 666, column: 999], [line: 333, column: 0]],
locations: [[line: 666, column: 999], [line: 333, column: 1]],
Copy link
Member

@bbakerman bbakerman Nov 25, 2024

Choose a reason for hiding this comment

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

Why do these numbers chance - what caused that?

Copy link
Member Author

@dondonz dondonz Nov 25, 2024

Choose a reason for hiding this comment

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

In the past, our tests had "0" being used as a valid column location. This PR bans anything less than 1 for line and column number, which made these old tests fail. So I adjusted these old tests to use a valid location number

Then added a new test with invalid numbers to check the new filtering behaviour

Copy link
Member Author

@dondonz dondonz Nov 25, 2024

Choose a reason for hiding this comment

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

The source locations for these old tests are generated by a method lower down in this file

List<SourceLocation> mkLocations()

@dondonz dondonz added this pull request to the merge queue Nov 27, 2024
Merged via the queue into master with commit 1598445 Nov 27, 2024
2 checks passed
@dondonz dondonz deleted the 3661-invalid-error-location branch November 27, 2024 22:54
@jbellenger jbellenger mentioned this pull request May 24, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bbakerman bbakerman bbakerman approved these changes

Assignees

No one assigned

Labels

breaking change requires a new major version to be relased

Projects

None yet

Milestone

23.0 breaking changes

Development

Successfully merging this pull request may close these issues.

Invalid location is used inside GraphQL error

3 participants

@dondonz @bbakerman