Filter out negative line and column error locations in toSpecification#3753
Merged
Filter out negative line and column error locations in toSpecification#3753
Conversation
dondonz
commented
Nov 24, 2024
|
|
||
| List<SourceLocation> mkLocations() { | ||
| return [mkLocation(666, 999), mkLocation(333, 0)] | ||
| return [mkLocation(666, 999), mkLocation(333, 1)] |
Member
Author
There was a problem hiding this comment.
Fix old tests to be compliant
dondonz
commented
Nov 24, 2024
| if (line < 1 || column < 1) { | ||
| return null; | ||
| } | ||
| return Map.of("line", line, "column", column); |
Member
Author
There was a problem hiding this comment.
Tiny change, this becomes an immutable map
Contributor
Test Results 304 files ±0 304 suites ±0 45s ⏱️ +5s 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. |
bbakerman
reviewed
Nov 25, 2024
| .build() | | ||
| [ | ||
| locations: [[line: 666, column: 999], [line: 333, column: 0]], | ||
| locations: [[line: 666, column: 999], [line: 333, column: 1]], |
Member
There was a problem hiding this comment.
Why do these numbers chance - what caused that?
Member
Author
There was a problem hiding this comment.
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
Member
Author
There was a problem hiding this comment.
The source locations for these old tests are generated by a method lower down in this file
List<SourceLocation> mkLocations()
bbakerman
approved these changes
Nov 25, 2024
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #3661
Internally graphql-java represents an "empty"
SourceLocationas line -1 and column -1.graphql-java/src/main/java/graphql/parser/ExtendedBailStrategy.java
Line 72 in ee32161
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
toSpecificationand 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.