Repository navigation
Let input types share a dictionary class - #851
Open
oryan-block wants to merge 4 commits into
Open
oryan-block wants to merge 4 commits into
oryan-block wants to merge 4 commits into
Conversation
When the scanner couldn't find a Java property for an input field, e.g. because the input is bound to a Map or ObjectNode, the getter name doesn't match the schema field (getFoo_Back for foo_back) or the field is private, it only logged "Cannot find definition for field ..." and never registered the field's type. Nested input objects and custom scalars only reachable that way were left out of the scan, and the parser then failed with "Expected type 'X' to be a GraphQLInputType, but it wasn't!", which pointed users at the wrong problem. The scanner now still registers what such a field needs. A nested input object is registered without a class and its fields are scanned the same way; a class found for it later is set and scanned as usual, so it doesn't clash. A custom scalar is registered like any other. An enum can't be built without its Java class, so if none is found for it anywhere, the scan fails with a message naming the enum, the input type, the field and the class, and how to fix it. The garbled warning is replaced by one that's only logged when an input class (not a Map or JsonNode) lacks a matching property. Input fields of a Map or JsonNode are no longer matched against the methods and fields it inherits from the JDK or Jackson, so fields named like values, elements or fields work with ObjectNode. Getters declared by a Map subclass are still used. A class taken from the dictionary for an input field is matched against the field's unwrapped type, so a list field whose element type is in the dictionary no longer fails with "Java class is not a List". A misspelled property on an input class whose nested input type is only reachable through it used to fail the build with the misleading error above. It now builds with the warning and, like any other misspelled property, fails when a client sends the field, or the value is dropped if the ObjectMapper ignores unknown properties. There's still no build-time check that input class properties match the schema (#422). The parser's "Expected type ... to be a GraphQLInputType" error is replaced by one that tells apart a type that isn't defined, a type of the wrong kind (an output type used as an input or vice versa) and a type that was defined but never reached while scanning, with a hint for reaching it that depends on the kind of type. It names the field or argument using the type and its source location. Schemas passed through schemaString() are still parsed as one document without a source name, so their line numbers count from the start of all strings (#392). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
SchemaParserDictionary stored its entries in a BiMap, so mapping two type names to one class, e.g. two input types to ObjectNode or Map, threw "value already present" as soon as the second was added. Input types never go into the scanner's type => class bimap, so nothing after that needs their classes to be unique. Directive input types still need a dictionary entry, and entries for nested inputs under a Map or ObjectNode are a common workaround, so both hit this. The dictionary is now a plain map. Every other kind of type still needs a class of its own, but the scanner checks that when the schema is built and names the class and the types sharing it. getDictionary() keeps its signature, since graphql-spring-boot passes its result to the builder. If several types share a class, the inverse of the returned copy only maps it back to the first of them. Names other than input types that share a class now fail in build() with a SchemaClassScannerError, instead of in dictionary() with an IllegalArgumentException. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The test for an input type and an object type sharing a class bound the input type through a resolver parameter, so its dictionary entry was never read. It now only uses the input type in a directive, which needs the entry. The test for object types sharing a class also maps an input type to that class, to check it is left out of the error. The scanner comment no longer says every other kind of type needs a class of its own. Scalars don't, but they keep the old rule. Co-Authored-By: Claude Opus 5.5 <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
1 participant
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 #465
Checklist
Description
Stacked on #842, merge that first.
Mapping two input types to the same class in the dictionary fails before the schema is even built. The example from #465:
The same happens with two inputs mapped to
Map, or an input type and an object type mapped to one class.SchemaParserDictionarystored its entries in aBiMap, andBiMap#putrejects a value that's already there. That's the right rule for output types. Object, interface, union and enum classes go into the type to class bimap that interface and union type resolution use to find the type of an object, so one class can only stand for one type. Input types never go into that bimap, though, so there's no reason for them to be held to it.Now
SchemaParserDictionarykeeps its entries in an insertion-orderedlinkedMapOf, andadd()accepts several names for one class.SchemaClassScannertakes a plainMap<String, Class<*>>. The one-class-per-type rule moved intoSchemaClassScanner'sinit, and it now skips input object types. If dictionary entries that aren't input types share a class, the build fails with aSchemaClassScannerErrorthat names the class and those types, for example "Class in supplied dictionary 'com.example.Foo' specified type names 'A', 'B', but only input object types can share a class!". That check runs after the existing "type definition not found" check, so a missing name is still reported first.getDictionary()keeps its signature and still returns aBiMap<String, Class<*>>, because graphql-spring-boot callsbuilder.dictionary(dictionary.getDictionary()). It now returns a copy made by the newBiMap#unmodifiableCopyOf, which keeps every key and maps a shared class back to its first key in the inverse. So the forward map has every entry, andinverse()andvalues()can be smaller than the map. The Javadoc says so.I didn't exempt scalars too. A dictionary class on a scalar is never used, since
TypeClassMatcherturns it into aScalarMatchand drops the class, so allowing scalars to share one wouldn't enable anything. Enums can't share a class because they go into the bimap. Directive input objects still need a dictionary entry, as before. Whether they should is a separate question, closer to #843. I didn't deprecategetDictionary()either, since graphql-spring-boot has no other way to read the entries yet. #842 says it'd close #465 by hand with a comment. That's no longer needed, so that sentence should come out of #842's description.The new
SchemaParserDictionaryTestbuilds real schemas and the positive tests run queries against them. It covers the exact code from the issue (two top-level inputs mapped toObjectNode), a nested input and a list of nested inputs under anObjectNodeinput, both mapped toObjectNode, two directive input types mapped toMap, the graphql-spring-boot path throughSchemaParserDictionary().add(...)andgetDictionary(), and an input type and an object type with an interface sharing a class, where the input is only reachable through a directive so its entry is actually read, with__typenamechecked. A negative test checks that two object types sharing a class fail with the exact message, and that an input type mapped to the same class isn't listed in it. On the base branch all six fail: five with "value already present" and the negative one with anIllegalArgumentExceptioninstead of aSchemaClassScannerError.BiMapTesthas two new tests forunmodifiableCopyOf, covering key order, values, the first-key-wins inverse and that the copy can't be changed.clean verifypasses.Behaviour change: several input object types can now share a dictionary class. Other types sharing a class are still rejected, but by
build()orscan()with aSchemaClassScannerErrorinstead of bydictionary()oradd()with anIllegalArgumentException. Dictionary entries the scan never reads now log the existing "Dictionary mapping was provided but never used" WARN, which includes the issue's own code, since those top-level inputs are already bound through resolver parameters.getDictionary()returns a snapshot instead of a live view. Entries keep insertion order instead ofHashMaporder, so if several dictionary names are missing from the schema, the first one added is reported. Probably worth a minor version and a line in the release notes.🤖 Generated with Claude Code