Repository navigation
Warn at build when a resolver method is missing a parameter for an argument - #890
Merged
Merged
Conversation
|
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 #418
Checklist
Description
When a resolver method is missing the parameter for an argument but has a
DataFetchingEnvironment,GraphQLContextorcontextClassparameter instead, the parameter count still matches. SoFieldResolverScanner#verifyMethodArgumentsaccepts it and maps the arguments by position, and in the issue's examplepersistgoes into theCustomContextparameter. The issue says this should throw like it does forDataFetchingEnvironment, butDataFetchingEnvironmentisn't checked either:This builds, and every query of
usersfails:Parameter specified as non-null is null: ... parameter envwithoutfirst,Cannot construct instance of graphql.schema.DataFetchingEnvironmentwith it. Same for a type resolver, e.g.thumbnail(image: Image, env: DataFetchingEnvironment)forthumbnail(size: Int).Now
SchemaClassScanner#warnIfParameterMissingchecks the parameter of every argument, not just the last one, and logs:It uses the same three types
verifyMethodArgumentsallows after the arguments, soallowedTrailingParameterTypesisn't private anymore. For thecontextClassit only warns when the argument is a spec scalar, since the context class can also be a real argument type, e.g.contextClass(Map)with an input object mapped to aMap. The check runs after the field's types are scanned, so a resolver that already fails the build (like a generic methodfun <T> gen(value: T)) gets the same error as before.Nothing else changes, and the field still fails at query time like before. Throwing instead has to wait for 15.0.0: #889.
The new test in
MethodFieldResolverTestcovers aMapcontext class, aGraphQLContextin the first slot and thethumbnailcase on aGraphQLResolver, plus two fields that shouldn't warn (aMapinput object and a correct trailing context).mvn clean verifypasses (346 tests), also with-Dgraphql-java.version=25.1. Didn't run JDK 11 locally, CI covers it.I checked it through the installed jar in a separate consumer project with
contextClass(AppContext::class):users(first: Int)->users(env),contextArgument(name: String)->contextArgument(context: AppContext)andUser.thumbnail(size: Int)->thumbnail(user, env)each log the warning and fail at query time like beforegreeting(name: String!)->greeting(name, env)doesn't warn and returns"greeting: hi ada"edealer-graphql's schema (351 resolvers) didn't log any of these on the first version of this change. The later changes can only make it warn less.
Release note: Resolver methods that are missing the parameter for an argument, where a
DataFetchingEnvironment,GraphQLContextor context class parameter takes its place, now log a warning at build.