Support startup validation of schema query/mutation/types having mapping

[nort3x]

i just started exploring this project, i read the documents and couldn't figure this out (so in that case i'm emphasizing on importance of configuration being much more straight forward and possibly default enabled )

for schema files we are supposed to write valid mappings by signature and return type but if i make any mistake (not annotating Arguments, or methods) it remains silent until a request hit it

i was lucky i had defined not-null return type in schem otherwise i suppose it would remain completely silent

{
  "errors": [
    {
      "message": "The field at path '/uploadTestFile' was declared as a non null type, but the code involved in retrieving data has wrongly returned a null value.  The graphql specification requires that the parent field be set to null, or if that is non nullable that it bubble up null to its parent and so on. The non-nullable type is 'File' within parent type 'Mutation'",
      "path": [
        "uploadTestFile"
      ],
      "extensions": {
        "classification": "NullValueInNonNullableField"
      }
    }
  ],
  "data": null
}

my point is this is not an issue at all! but when i'm writing schema file i'm going with schema-first approach and it would be much more better experience if these mistakes would bubble up at startup rather than request resolving

[rstoyanchev]

I would expect this to be uncovered by tests, but I see your point, there is an opportunity to validate data fetcher registrations against schema fields.

As far as I can see currently GraphQL Java iterates fields and looks up DataFetcher registrations. That means a registration in RuntimeWiring could remain unused.

We could use a GraphQLTypeVisitor to go the other way, i.e. iterate DataFetcher registrations and cross check against field coordinates. Or is there a better way? It feels like it could be a GraphQL Java feature.

[danielshiplett]

I agree that this is an important feature that is currently lacking the in the Spring for GraphQL implementation. We are looking at switching to using Spring for GraphQL from the GraphQL Kickstart project. We rely heavily on the startup introspection to catch any failures to implement or map resolvers correctly.

Additionally, we use the com.kobylynskyi.graphql plugin to generate the API interfaces for our schema. This allows us to perform some amount of compile time checking of types against our implementation before we even get to the final startup introspection.

It is very important that our implementation 100% matches the GraphQL schema so that we don't respond to clients with GraphQL specific errors that they cannot do anything about (or are unlikely to understand).

[JamesPeters98]

Is there any update on this? I also agree that this is an important feature, especially during initial development it is very easy to miss a resolver and not notice it.

[rstoyanchev]

This is now scheduled for 1.2 M1. We'll see if there is a way to backport it to 1.1.x in the mean time such that it can be enabled if desired.

[nurkiewicz]

I confirm this is a major issue for me. I tried plain Java kickstart and it beautifully shows missing resolvers on startup, e.g.:

Caused by: [...]FieldResolverError: No method or field found as defined in schema [...] 
with any of the following signatures [...], in priority order:

  com.nurkiewicz.graphql.Player.trustworthiness()
  com.nurkiewicz.graphql.Player.getTrustworthiness()
  com.nurkiewicz.graphql.Player.trustworthiness

I'd assume Spring could inherit this feature. I don't want to write tests for things that can be detected on startup automatically.

[bclozel]

We should make this arrangement flexible enough to support sliced tests in Spring Boot.

Right now @GraphQlTest allows you to test a subset of controllers so that you don't have to start the entire application. By definition, there will be missing resolvers with sliced tests and we should not prevent this use case.

[rstoyanchev]

Slice tests should be okay. My understanding of this issue is that GraphQL Java iterates over schema types/fields and then crosschecks against RuntimeWiring registrations, so in effect, a controller method could remain unregistered silently. So it's fine to test one controller at a time, as long as all controller methods correspond to valid schema coordinates.

[rstoyanchev]

It appears my understanding is incorrect. @nort3x, or anyone else here, could you comment on your expectations for this. Do you expect us to very that:

A) Every controller method is mapped to a valid schema field B) Every schema field is mapped to a controller method, or is otherwise covered by a corresponding object property?

I think B) could be a challenge to do in some cases. For example:

• if an application registers a DataFetcher directly with GraphQL Java, instead of declaring a controller method, we wouldn't know its return type, and wouldn't be able to verify if its properties and structure match the schema without any gaps.
• if a controller method does not specify the exact return type, we would have the same issue, and a controller method may not always be able to be precise about its return type, e.g. with interfaces and unions.

[nort3x]

@rstoyanchev despite the serious facts about DataFetecher and union interface return type, it's still option B.

the need comes from the idea "i have a schema which I'm implementing based on it, let's have some sanity checks at the startup" it also worth mentioning i once had to re-implement a microservice while using Kickstarter, the schema was complex and this good feature was a nightmare in development process because it expected that i have it all at the beginning which was too much to ask for (wasn't flexible enough, just staright shutdown)

[JamesPeters98]

I agree B is what would be needed.

I ended up using graphql-java-codegen (https://github.com/kobylynskyi/graphql-java-codegen) to solve this. I generate an interface for each query file and then wrote a unit test to make sure every interface was implemented. And then you get compile time checks on any missing queries from each interface.

So I'm basically requiring a QueryMapping/MutationMapping for every query/mutation

[nurkiewicz]

Yes, I'm also referring to option B. Just like @nort3x I need a sanity check that whatever I run conforms to schema. In other words it's impossible to run application that can't handle certain schema fields. If I advertise a certain API, I should be 100% sure I can handle it.

That being said, at least in these situations it makes sense to disable such validation:

• I start with a huge schema and don't want to implement everything at once
• I run slice tests and don't have all controllers in context

[rstoyanchev]

Thanks for the feedback. Verifying root fields, i.e. those under Query, Mutation, Subscription, is straight. Verifying fields on all other GraphQL types is not as straight forward since looking at controller methods and their return types may not provide enough information.

@JamesPeters98 I don't fully understand the approach you took, but are you only verifying Query and Mutation mappings, or are you also checking all other fields?

Does anyone have union / interface types and are you able to validate those too, and if so how since a verification tool can't statically determine what types would be returned at runtime in order to match those to the schema types?

[JamesPeters98]

@rstoyanchev Sorry yes I missed that bit out. It also generates TypeResolver interfaces with @SchemaMapping methods.

I don't use any union/interface types though

[bclozel]

I have a draft implementation for this and it covers quite a few cases already. I'm also documenting known limitations, like Unions. For a start, we'd like to introduce this feature with very little public API as we'd like to get feedback on the feature itself before possibly opening it up in the future.

Initially, this feature would be included in the org.springframework.graphql.data.method.annotation.support as package private and used directly by the GraphQlSource.Builder. This could be enabled at all times, since only log statements would be involved: an inspection failure would not fail the startup phase.

From a developer experience point of view, the important part is the information you get in the logs. I'm asking for opinions here about the logging level and the actual messages. Here's an example of what we could get:

INFO: GraphQL schema inspection found missing mappings for [Query], missing data fetchers for types [Book, Author].
DEBUG: Missing fields on types:
               - Book [subTitle,pageCount]
               - Author [address,nickName]
DEBUG: Missing operations for:
               - Query [greeting,findByIsbn]
               - Mutation [createBook]

We could add more information there, telling developers what could be missing. In case of a type field, we could point to adding a bean property or a @SchemaMapping annotation. In case of an operation, we could point to a @QueryMapping, @MutationMapping annotation. But of course this approach could be more verbose.

[bclozel]

I've just pushed a first version for this feature. This will be available in 1.2.0-SNAPSHOT in a few minutes and in the upcoming 1.2.0-M1 release. I've mostly kept the logging information described in the previous commit. Because the information is minimal, the entire report is printed at the INFO level.

The schema inspection visits the schema operations and types discovered along the way when those are exposed by a TypedDataFetcher. This implementation has known limitations, such as Union types: currently we don't see a way to effectively resolve the Java types backing the Union and derived schema types. Interfaces and Type extensions are supported.

As a first step, we would like the community (including @nurkiewicz, @danielshiplett, @JamesPeters98 ) to give it a try and send us feedback about things we possibly missed. Once we've gathered enough feedback on it, we might open this up a bit more: configuring the behavior (just logging, failing the app), exposing the inspection report for consumption by user code, etc.

[bclozel]

After discussing with @rstoyanchev, it seems that performing the schema mapping inspection on the TypeDefinitionRegistry could be too early, as other mechanisms could contribute to the schema. We've decided to use instead the GraphQLSchema instance directly. As for querying for DataFetcher instances, we'll keep using the RuntimeWiring instance right after Spring for GraphQL contributed to it. After that, data fetchers could wrapped by other extensions, preventing us from getting the type information we need.

The TypedDataFetcher interface has been moved to a different package and renamed as SelfDescribingDataFetcher to enable further extension and to be more descriptive.

I've also slightly changed the report format to remain on a single line:

INFO 91221 --- [  restartedMain] efaultSchemaResourceGraphQlSourceBuilder : GraphQL schema inspection found missing mappings for: Query[authorById], Book[missing].

[danielshiplett]

So far, I think this looks promising. Here's some log output from my sample application that reports the missing schema resolvers:

2023-03-12T15:42:14.023-04:00  INFO 258948 --- [  restartedMain] efaultSchemaResourceGraphQlSourceBuilder : GraphQL schema inspection found missing mappings for: Mutation[createPost], Subscription[commentsByUser], User[posts], Comment[user, post]

Also, until there is another option for failure in case of missing schema, could I request that the logging of the report be done at the WARN level if there is anything missing? This might make it more visible.

I do like the idea of being able to access the report via code. Doing this in an ApplicationReadyEvent listener would allow me to choose the behavior in case of missing schema resolvers. This could be a good extension if people want more than just an optional failure.

[eduanb]

My 2c here:

With this new feature, Spring knows exactly how the schema should look and log mismatches on startup. A great feature and a perfect fit for an integration test. Since this schema is known, it is not such a far stretch to expand this feature to make Spring GrapQL code-first and have it generate the schema completely. This would put Spring GrapQL in the unique and powerful position of having the user choose between schema-first or code-first.

Many users would prefer code-first as it is a lot less "duplicate" code and faster to get going. Other users would prefer the explicit schema in schema-first. Having the choice would be awesome!

Any opinions @bclozel or @rstoyanchev ?

[bclozel]

@eduanb

With this new feature, Spring knows exactly how the schema should look and log mismatches on startup. A great feature and a perfect fit for an integration test.

This feature is not about integration tests, but more about development time. We don't expect those checks to be even complete and accurate as there are many cases we can't cover. Here it's the opposite: the schema is defined by the developer and we inspect the data fetchers available (controllers, "manually" registered ones, data repositories and more) and check whether we think something is missing.

Since this schema is known, it is not such a far stretch to expand this feature to make Spring GrapQL code-first and have it generate the schema completely

I don't follow this line of reasoning - we have been able to implement this only because we assume the schema is mostly complete. We're adding new features to take the boilerplate out of the schema, like in #619.

[eduanb]

I understand it better now. I thought that for this introspection to work, the expected schema would be generated from the code and compared to the provided schema by the developer. As you explained, this searches for data fetchers matching the schema and thus my reasoning was wrong.

I was just being optimistic and hoping this would enable a code-first option for Spring GraphQL, which would be a nice feature to have.

[bclozel]

@eduanb I understand better now - indeed, if we had taken that approach for inspection the code generation approach would have been very close. We're not investing in this space right now and it's not clear whether our current annotation model would allow to deterministically generate a full, compliant schema. Our general thinking is that the schema should be manually crafted as the core contract with all clients and that it shouldn't evolve "by accident" just because you refactored something in your application.

[rstoyanchev]

Quick note to add a link to the reference documentation for those looking to understand and experiment with the feature.

There have also been a few improvements. Most notably, basic support for schema interface types, and also including in the report any types that are skipped by the inspection (such as unions). This helps to provide a more complete picture. There is also one further improvement planned in #671.

[danielshiplett]

Quick note to add a link to the reference documentation for those looking to understand and experiment with the feature.

There have also been a few improvements. Most notably, basic support for schema interface types, and also including in the report any types that are skipped by the inspection (such as unions). This helps to provide a more complete picture. There is also one further improvement planned in #671.

Thanks for the update on the progress on this @rstoyanchev. Do you know, are there plans to add a hook into this inspection? Ideally, I'd like some way to listen for the inspection report event so that I can decide if I want to take further action based on the report result. Like an application failure.

[rstoyanchev]

@danielshiplett, I've created #672 for this.

[bclozel]

@danielshiplett we didn't get much feedback so far on the actual feature: is it applicable to existing schemas, is the provided information actionable, are there false positives/negatives... we will probably refine that in the 1.2.x generation.

We could expose a contract to help with custom behavior (such as failing at startup), but I guess we should consider this for 1.3.0.

[rstoyanchev]

@bclozel looks like we commented around the same time. I've proposed something concrete under #672. Let's continue the discussion there. I think there are probably not too many ways to do this. If we settle on something like that, it's quite feasible for 1.2 still.

[JamesPeters98]

Hey, I just got around to testing this when upgrading to Spring Boot 3.1. I seem to be getting false positives for unmapped fields that have a java field.

I think it happens when the controller method is a CompletableFuture of a List of the object:

@Controller
public class ExampleQueryController {
  @QueryMapping
  public CompletableFuture<List<ExampleObject>> exampleObject(DataFetchingEnvironment env) {
    return CompletableFuture.completedFuture(List.of(new ExampleObject(1, "name")));
  }

}

GraphQL schema:

type ExampleObject {
  name: String,
  id: Int
}

type Query {
  exampleObject: [ExampleObject]
}

Object class:

public class ExampleObject {

  public String name;
  public int id;

  public ExampleObject(int id, String name) {
    this.id = id;
    this.name = name;
  }

  public Integer getId() {
    return id;
  }

  public String getName() {
    return name;
  }

}

[bclozel]

@JamesPeters98 thanks for trying the RC version. Can you raise a new issue for this problem?

[JamesPeters98]

@bclozel Raised as a new issue in #674