Validate at schema build time that @DgsData and @InputArgument match actual schema elements

[tinnou]

Pull request checklist

• Please read our contributor guide
• Consider creating a discussion on the discussion forum
  first
• Make sure the PR doesn't introduce backward compatibility issues
• Make sure to have sufficient test cases

Pull Request type

• Bugfix
• Feature
• Refactoring (no functional changes, no api changes)
• Build related changes
• Other (please describe):

Changes in this PR

Current issues:

1. When performing a refactor of data fetcher related classes, it's possible there can be data fetchers annotated with @DgsData that become orphan where there is no matching field on the schema that is wired to it. I can't think of any use case where this is not a developer error. Currently the framework will not fail (only log at warn level) when a request is processed. This is too late in the development workflow and can be easily missed by service owners.
2. Similarly, data fetcher parameters annotated with @InputArgument can be missed if the annotation name value or method parameter name doesn't match any argument of the corresponding field on the schema.

Proposed solution:

Add some validation at schema build time in the SchemaProvider to prevent the DGS from starting if there are obvious orphan parameters or data fetchers.

Note: It is currently possible to wire a data fetcher on a field of every possible type of a union through the current wiring logic. However it is not guaranteed that every member type will have the requested field. In case of unions, I chose to not perform any validation as a subset of the types could have the field while others would not.

Alternatives considered

We could keep the status quo, as in, we would not perform any ahead-of-time validation and only log are a higher level. This does not come with the benefit of failing fast when building the schema vs at runtime when executing requests.

[srinivasankavitha]

Changes look great! Thanks for the fixes!

[srinivasankavitha]

Might be good to add a comment on when the fallback resolver is invoked in this scenario for future reference.

[tinnou]

will do, FallbackEnvironmentArgumentResolver covers the case where there is no @InputArgument as a last resort it uses the method parameter name, below named name :

       @DgsComponent
        class Fetcher {
            @DgsData(parentType = "Query", field = "hello")
            fun someFetcher(name: String?): String {
                return "Hello, ${name ?: "no name"}"
            }
        }

type Query {
   hello(name: String): String
}

It's covered by test case named: when no @InputArgument fallback to method param name to match argument on schema

[kilink]

I'm not sure that this method needs to live in MethodDataFetcherFactory. I think some of the checks themselves are basically a no-op in practice, since we're filtering parameters that are annotated with InputArgument, and the InputArgumentResolver matches any method parameter with this annotation. I think the SchemaProvider could simply perform the check directly by just collecting field names from the annotated parameters.

[tinnou]

Let me reply to both of your comments in one go as they are related.

I'm not sure that this method needs to live in MethodDataFetcherFactory. I think some of the checks themselves are basically a no-op in practice, since we're filtering parameters that are annotated with InputArgument, and the InputArgumentResolver matches any method parameter with this annotation. I think the SchemaProvider could simply perform the check directly by just collecting field names from the annotated parameters.

I went back and forth on this, between adding the logic in the Factory or in the SchemaProvider and I opted for the Factory for the following reasons:

• if we were to add this in the schema provider, we would need to inject the resolvers that are also injected in the Factory. The ArgumentResolverComposite would also possibly need to be replicated to match what is being done at runtime. This would make the argument resolution & validation logic more brittle as it would live in multiple places.

• The argument resolvers list is auto-wired and DGS owners could be injecting their own list of ArgumentResolvers. If that's the case our validation should run if and only if our specific InputArgument resolver would be selected by the composite. The way to do that without leaking the implementation elsewhere was to place it in the Factory.

I also contemplated the idea of introducing a new ArgumentResolver interface method so that each argument resolver could optionally perform their own validation, but I felt ArgumentResolver is about resolution not validation and so I abandoned the idea. We could introduce yet another interface only for validation but I thought it would be overkill for now, at least. Happy to revisit.

Kind of related to my other comment. Is there a test that actually exercises this case? I don't know that it's possible, since we are only checking parameters that are annotated with @InputArgument, so basically by definition the resolver would always be InputArgumentResolver. IMO it isn't necessary to check the resolvers, we could simply validate that the field name from the annotation matches something from the schema, and I don't know that it needs to live here.

Great catch, I thought I added one but I think I got confused on the behavior of the FallbackEnvironmentArgumentResolver initially and forgot to go back and update. The idea is, if the FallbackEnvironmentArgumentResolver is selected, the resolved argument name will be the method parameter name and we can check whether there is a matching argument on the schema. I added a couple of tests and fixed the factory logic to look at all the method parameters instead of the ones annotated with @InputArgument in my last commit.

Now the problem is the following test is failing An unknown argument should be null, and not result in an error which leads me to believe that the behavior of having a null value passed in when nothing matches was intended. We can definitely argue about whether it was a good decision, but at this point changing this behavior would be effectively breaking.

To conclude, I'm leaning towards only running validation for fields annotated with @InputArgument, the same way I'm doing it now just we won't look at FallbackEnvironmentArgumentResolver and keep the behavior of a fallback as a best effort, last resort resolver that doesn't get any schema build time validation benefit. Thoughts?

[srinivasankavitha]

@tinnou and I just had a quick discussion and decided on
(1) Removing the logic for checking the fallback resolver altogether, and that would allow anything without an @InputArgument to go through
(2) Introduce a helper that does line 40 and 51 in the MethodDataFetcherFactory. This will allow other classes to determine the resolver used for a given method
(3) Move the rest of the validation logic to the schema provider, which will use this new helper. It would be good I think to have the validation logic use the InputArgumentResolver to avoid leaking/reimplementing details of how @InputArgument annotation works - specifically since you can have something like @InputArgument(name=something) somethingElse: String

Does that seem reasonable?

[kilink]

Kind of related to my other comment. Is there a test that actually exercises this case? I don't know that it's possible, since we are only checking parameters that are annotated with @InputArgument, so basically by definition the resolver would always be InputArgumentResolver. IMO it isn't necessary to check the resolvers, we could simply validate that the field name from the annotation matches something from the schema, and I don't know that it needs to live here.

[srinivasankavitha]

Thanks for the changes, looks good to me! One minor suggestion for addition test case when schemaValidationWiring is disabled.

[srinivasankavitha]

Should we add a test that verifies validation does NOT happen when the schemaValidationWiring flag is set to false?