Repository navigation
Fix line numbers in schema syntax errors and name the source - #838
Open
oryan-block wants to merge 1 commit into
Open
oryan-block wants to merge 1 commit into
oryan-block wants to merge 1 commit into
Conversation
SchemaParserBuilder appended every schemaString to one StringBuilder and parsed the result as a single unnamed source. A syntax error in the second or later string was reported with a line number counted across all strings, and nothing said which string it came from. This is what Spring Boot starter users hit, since the starter passes each .graphqls file as its own schemaString. Files added with file() were already parsed with their name, but graphql-java's InvalidSyntaxException message only contains the line and column, so the name was only visible on the exception's SourceLocation. Each schemaString is now its own source in one MultiSourceReader, so they are still parsed as a single document, but line numbers in parse errors and AST source locations are relative to the string they came from. graphql-java's MultiSourceReader numbers the last line of the last source from the start of the first source, so every source is ended with a line break, which leaves only the end of input on that line, and the line of an unexpected end of input error is made relative to the last source before it is reported. A new schemaString(string, sourceName) overload names the source, and when a syntax error comes from a named source or file the exception is rethrown as an InvalidSyntaxException subclass whose message ends with "in <sourceName>". Spring Boot starter users get line numbers relative to each file from this change alone, but file names only once graphql-spring-boot passes them to the new overload. Fixes #392 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
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
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 #392
Checklist
Description
When there's a syntax error in the second or a later
schemaString, the error says something likeInvalid syntax with offending token '(' at line 9 column 17while the mistake is on line 2 of some file. The column is right, but the line counts every line of the strings before it, and nothing says which string it came from. That's what Spring Boot starter users hit, since the starter passes each .graphqls resource as its ownschemaString.SchemaParserBuilder.schemaString()appended everything to oneStringBuilderandparseDocuments()parsed it as a single unnamed source. Files added withfile()were already parsed one at a time under their own name, but graphql-java'sInvalidSyntaxExceptionmessage only has the line and column, so the name was only onSourceLocation.sourceName.Schema strings are now kept as a list, and each one is its own source in a single
MultiSourceReader. They're still parsed as one document, so a definition split across calls and comment-only strings keep working, but lines in syntax errors and ASTSourceLocations are now relative to the string they came from. That includes theschema <unknown>:Npart ofFieldResolverScanner's "No method found as defined in schema" message. There's also a newschemaString(string, sourceName)overload to name the source. When a syntax error comes from a named source or a file, it's rethrown asSchemaSyntaxException, an internal subclass ofInvalidSyntaxException, with " in " appended to the message, e.g.Invalid syntax with offending token '!' at line 2 column 15 in InvalidSyntax.graphqls. Location and source preview are kept.graphql-java 26.1's
MultiSourceReader.getSourceAndLineFromOverallLinehas a bug: for anything on the last line of the last source it returns the cumulative line (page) instead ofoverallLineNumber - previousPage. That covers every unexpected end of input error (e.g. a missing closing brace in the last file) and anything in a one-line last string. To work around it, when there's more than one source each one gets a trailing line break if it's missing, so only the end of input is left on that line. The line of an end of input error is then made relative to the last source, in both the location and the message. The number in the message is matched with the sameMessageFormatformatting graphql-java uses, since it groups thousands ("1,103"). A single source (onefile()or oneschemaString) gets no extra line break, so its positions are the same as on master. I haven't reported the bug upstream yet, but once it's fixed there the workaround can go.I didn't parse each schema string as its own document like files are. It would avoid the workaround, but a comment-only string fails to parse on its own with
'<EOF>', so a commented-out .graphqls file would break app startup for starter users, and a definition split across calls would break too. Unnamed strings also don't get a made-up name like "schemaString #2", since that would put fake names in every AST node'sSourceLocation. So an error in an unnamed string shows the line within that string but not which one. Which means starter users get per-file line numbers from this alone, but file names only once graphql-spring-boot (GraphQLJavaToolsAutoConfiguration/ClasspathResourceSchemaStringProvider) passes the resource name throughschemaString(content, name). That's a follow-up in that repo. Also,SchemaSyntaxExceptionusesInvalidSyntaxException's protected constructor, which graphql-java marks@Internal(its own subclasses use it too), so a future graphql-java release could break it.Behaviour change: with several
schemaStringcalls, line numbers in syntax errors and ASTSourceLocations are relative to their own string instead of counted across all of them, and the same goes for the line inFieldResolverScanner's missing method message. Syntax errors from named sources (file()or the new overload) are now thrown asSchemaSyntaxExceptionwith " in " at the end of the message. It's still anInvalidSyntaxException, so existing catches work, but for named sources it replaces graphql-java's own subclasses (MoreTokensSyntaxException,ParseCancelledException). New public API:schemaString(String, String).🤖 Generated with Claude Code