cjoudrey / graphql-schema-linter

Validate GraphQL schema definitions against a set of rules
MIT License
694 stars 62 forks source link

Add support for Apollo Federation directives #210

Open danielferragut opened 4 years ago

danielferragut commented 4 years ago

Hello!

I have taken an interest in using this library in one of my projects which uses Apollo Federation as its architecture for handling micro services. When using simple schemas, and linting from the root where it is possible to reach all services, this library works splendidly.

Problems start to arise, however, when using Federation's custom directives. These directives are vital to build schemas because one service's schema can fully interact with other service's schemas without a problem, as they are isolated from one another. For some directives there are workarounds, but there are some problems with others as they conflict with the way that simple schemas are done.

Before explaining the problem, lets define a example by defining two types, suppose we have a A type and a B type. There is a micro service for handling A , as well as another one for B. The B type is defined in its own micro service, we don't really care about its fields, only the fact that it has a id. Now lets define the A type, while also updating B so that it supports the new aType field.


type A @key(fields: "id") {
  id: ID!
  field: string
}

extend type B @key(fields: "id") {
  id: ID! @external 
  aType: A
}

We are modifying a schema that belongs to another service seamlessly. All of this is possible thanks to the @key and @external directives that Apollo Federation provides. Using the linter on the schema above, however, generates 2 errors:

The former can be solved with some workarounds, creating 'dummy' definitions of these directives on another schema file, but not actually using them on the real code. The latter, however, is a little bit more complex. As we are dealing with Federation, we need to put the ID there again so that we can use it and use the @external directive to indicate that we are using the same ID, not defining a new one.

Suggestion It would be very good if this library had simple support to Apollo/Apollo Federation directives (currently there are only 5 different directives). As of right now, there is no way to completely lint a project with Apollo Federation, as the use of these directives is inevitable. This issue would be considered fixed if some kind of option was created to allow support for Apollo Federation, ideally understanding the directives and linting them accordingly.

How much work do you think this would take? If needed, I could take a crack at it but wouldn't know where to start.

Cheers

cjoudrey commented 4 years ago

Hey Daniel,

First, I'd like to thank you for such a well written issue. I really appreciate the time you took to write it. šŸ˜„

@key and @external directives are not defined. ... The former can be solved with some workarounds, creating 'dummy' definitions of these directives on another schema file

Yeah, that's the strategy I've recommended when it came up in the past. (i.e. https://github.com/cjoudrey/graphql-schema-linter/issues/165#issuecomment-468379498)

Luckily the docs for Apollo Federation make this a tad easier as they provide the definitions here: https://www.apollographql.com/docs/apollo-server/federation/federation-spec/#federation-schema-specification.

Just for completeness, my reasoning for not wanting to include those definitions within graphql-schema-linter is maintainability and opening the door for more of these. For now, I feel like the tool should remain agnostic.

Adding a --use-apollo-federation flag that automatically stitches the 'dummy' definitions to the inputted schema is simple on the surface, but it gets messy if/when those dummy definitions change. You end up needing to support all past versions of the 'dummy' definitions and the flag becomes --use-apollo-federation v1.0.5. I'd prefer not having to maintain that feature and rather have people maintain the dummy definitions in their own repository.

In the B schema: "Field "B.id" can only be defined once.". As B's ID was defined in its own service's schema, the linter thinks this is a double definition of a field.

Yeah, that's a bit trickier. If you forget about Apollo Federation and strictly think about GraphQL, I think the error is correct.

To be honest, this is the first time I hear about Apollo Federation.

My gut reaction is to want to patch the schema in such a way that graphql-schema-linter works. For example, have a separate script that comments out fields that have @external directive and the pass that new schema graphql-schema-linter. It feels a bit dirty though.

A more elaborate fix would be to allow disabling rules for specific parts of the schema. This has been an open feature request for quite some time (#18). I just haven't gotten around implementing it.

Using that feature we could have something like:

extend type B @key(fields: "id") {
  id: ID! @external # lint-disable-line field-already-defined
  aType: A
}

This would require a second fix though. Today, a lot of the base validations of the GraphQL schema are offloaded to graphql-js and aren't reported back at the same level of granularity as the rest of the errors.

https://github.com/cjoudrey/graphql-schema-linter/blob/ccbb3be5b72f144e15d393b230f894c5d1ca3f20/src/validator.js#L33-L44

That would need to be solved in order to leverage the above feature. In other words, the Field "B.id" can only be defined once. error is currently showing up as a invalid-graphql-schema error and it bails out right away. In reality, we should be treating it as just another error and continuing with the rest of the validation instead of the early return.

I realize this doesn't quite solve your problem, but I'm hoping it at least gives you an idea of how I'd address it. I don't know when I'll get around implementing those two features though. I'd be happy to šŸ‘€ any pull requests that attempt to fix this though.

Cheers!

danielferragut commented 4 years ago

Thanks for the quick and detailed answer Christian!

Just for completeness, my reasoning for not wanting to include those definitions within graphql-schema-linter is maintainability and opening the door for more of these. For now, I feel like the tool should remain agnostic.

I completely understand, if this library were to support every GraphQL framework that appears, it would become quite a hassle to maintain the code.

From were I stand, using the script solution might be more viable as it is the quick and dirty way to solve this problem :sweat_smile:. I am going to take a look at the source code for this lib and see how viable it is to solve the disable parts of the schema problem. Or, depending on how easy it is to code it, I might fork this repo and create a version of this library solely for Apollo Federation, as there is a dire need for linting schemas with this type of framework.

That said, the script solution is probably the way I am going to solve this problem. If I 'solve it', I will be sure to post the script here, so other people can use it if them want to.

Again, thanks for all the help!

csilvers commented 4 years ago

FWIW we also need to lint schemas with federation. Our approach is to just lint one schema file at a time, rather than the whole schema together. So far that has worked fine, and gotten around the problem with @external.

The only workarounds we need to do are to add the definitions of the directives, as already mentioned, and to add a fake Query node if a file doesn't have one. So we need a driver in front of the linter to decide whether to add fake_query_node.schema to the linter commandline or not, based on the contents of the actual schema file we're linting.

csilvers commented 4 years ago

(I take it back, it hasn't been working cleanly for us at all. The problem is when a schema file references types defined in some other schema file.)

mshwery commented 4 years ago

Wonder if the closest we could get it by loading the entire schema AST... then only validating the parts of the AST that a given file define?

danielferragut commented 4 years ago

I haven't had chance to take a really good look at it, but GraphQL Code Generator is a tool that generates Typescript types from GraphQL schemas and they have support for Apollo Federation schemas (this line for example shows how they deal with Apollo Federation schemas ). The problem of linting schemas and the one of generating types are very similar, so it should be possible to see how they deal with this and try to bring their solution to this problem. Again, I have got no idea if this is viable or not, but it may be a way to add support for Apollo Federation (though it looks like it would need some kind of flag to indicate Apollo Federation schemas).

mac2000 commented 4 years ago

Another approach might be to have a simple script which will:

But unfortunately all required pieces are not exported from this library it seems that it does not expected to be used programmatically

The only way I were able to wire everything up is by calling runner and passing my own streams to catch and parse output but it is kind of hacky

Wondering if there is a chance that some more classes will be exported in future for such use cases

Or may be even better will be ability to have transformations script(s) applied before validation the same way as custom rules

shellscape commented 3 years ago

I'd luuuuurv to use this but the lack of native support for federation directives is a game ender. Any chance this could get some traction?

shellscape commented 3 years ago

Checking in after 6 months. @cjoudrey any plans to give this some love in the near future?

mac2000 commented 3 years ago

@shellscape at the very end, even if federation will be added, you still end up with further configuration and even your own rules, so you will have a configuration file and all you need is just to add federation stuff to exclusions.

In our case dotnet implementation did not sort fields alphabetically, as a results there were no other way for us to use linter without configuration file, as a result missing federation is not a problem at all

shellscape commented 3 years ago

@mac2000 I'm honestly not sure what you're referencing. Having looked at the source for this package, and the source for buildFederatedSchema, it's clear that to support federation, adding the necessary directives to the validate methods is relatively trivial.