cjoudrey / graphql-schema-linter

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

TypeDefs in split files error (i.e. merge-graphql-schemas) #147

Open henriklundgren opened 6 years ago

henriklundgren commented 6 years ago
"file1.gql"
type Query {
  items: [Item]
}
"file2.gql"
type Query {
  item: Item
}
cjoudrey commented 6 years ago

Hi @henriklundgren!

Thanks for opening the issue. graphql-schema-linter should provide clearer errors for this case.

Are file1.gql and file2.gql two separate schemas or are you trying to extend the schema?

If these are two separate schemas, you should run graphql-schema-linter twice, once for each file.

If you are trying to extend the schema, you should be using extend type Query in one of the two files.

Let me know if I misunderstood your issue!

henriklundgren commented 6 years ago

Hi,

I use mergeTypes to handle this case.

Thanks

henriklundgren commented 6 years ago

@cjoudrey I am sorry if I was short in previous comment.

I have typeDefs split between several files, I import them and merge the types with mergeTypes(typeDefs). I.e. the above becomes,

type Query {
  item: Item
  items: [Item]
}

Would this be possible to fix in the linter?

cjoudrey commented 6 years ago

@henriklundgren thanks for clarifying.

This is the first time I see this package (and this use case.)

I realize this isn't ideal, but one solution could be to write a simple script that output's the resulting schema and you can run graphql-schema-linter against it.

A more complete solution could be to make graphql-schema-linter aware of these types of schemas and having a command line option to have graphql-schema-linter run mergeTypes on the separate files. I'm a little less fond of adding these kind of specific things to the library, unless it is the norm.

In both cases, one problem will be associating back an error from the "merged schema" back to the original file.

Hope this helps!

mshwery commented 4 years ago

I would say anecdotally that multi-file graphql schemas being merged are quite common.

danbahrami commented 4 years ago

+1 for this issue. I have a schema that is growing steadily and would like to split it into five or six modules. Each module will contain a few top level queries. This is the error I'm getting

$ graphql-schema-linter ./types/*.graphql

It looks like you may have hit a bug in graphql-schema-linter.

It would be super helpful if you could report this here: https://github.com/cjoudrey/graphql-schema-linter/issues/new

Error: Type "Query" was defined more than once.
    at buildASTSchema (/home/vagrant/development/concierge/node_modules/graphql-schema-linter/node_modules/graphql/utilities/buildASTSchema.js:116:17)
    at validateSchemaDefinition (/home/vagrant/development/concierge/node_modules/graphql-schema-linter/lib/validator.js:33:51)
    at run (/home/vagrant/development/concierge/node_modules/graphql-schema-linter/lib/runner.js:66:56)
    at Object.<anonymous> (/home/vagrant/development/concierge/node_modules/graphql-schema-linter/lib/cli.js:15:32)
    at Module._compile (internal/modules/cjs/loader.js:955:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:991:10)
    at Module.load (internal/modules/cjs/loader.js:811:32)
    at Function.Module._load (internal/modules/cjs/loader.js:723:14)
    at Function.Module.runMain (internal/modules/cjs/loader.js:1043:10)
    at internal/main/run_main_module.js:17:11
error Command failed with exit code 3.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

It would be a bit of a pain to have to run the linter 5 or 6 times, it would also miss things like clashing type definitions between files.

shellscape commented 3 years ago

Weighing in here to throw my support behind support of separate files. If ESLint et al were to throw errors because the same variable was declared in separate files, the tool would be unusable. We were planning on using this in a monorepo to lint all schemas at once, but with this limitation we're pretty boned.

cjoudrey commented 3 years ago

👋 @shellscape thanks for the feedback.

Just for clarity, are you saying you'd like to run graphql-schema-linter once and pass it multiple distinct GraphQL schemas or are you saying that you also have a schema that is divided into multiple files and gets merged into one eventually?

If you're saying the latter, I'm curious to hear what tool you use to merge them. To be honest, I don't have much context on this use case so any clarity would help.

shellscape commented 3 years ago

to run graphql-schema-linter once and pass it multiple distinct GraphQL schemas

Exactly. ESLint, for example, evaluates each file it encounters individually. In a monorepo, this is the only way to avoid running graphql-schema-linter multiple times for schemas in different monorepo projects.

cjoudrey commented 3 years ago

In a monorepo, this is the only way to avoid running graphql-schema-linter multiple times for schemas in different monorepo projects.

Gotcha! Ok, I think the problem you are describing is different from what is described in the original issue.

In your case, the root problem is that graphql-schema-linter assumes multiple .graphql files should be treated as one schema and it'll simply concat them.

It makes me wonder if maybe we should be changing this default behaviour such that graphql-schema-linter treats each .graphql as distinct schemas and then provide a --merge-schema option to preserve the existing behaviour. We could also introduce a --merge-schema-strategy <concat | merge-types | ...> option to support different merge strategies to solve the problem described in the original issue.

Edit: The thing I struggle with is graphql-schema-linter is fundamentally meant to lint one schema. If someone needs to lint many, I would just run graphql-schema-linter N times. 🤔

shellscape commented 3 years ago

I would love that solution.

rperryng commented 3 years ago

👋 is there any momentum on this issue still? Would love it if one day there could be some support for graphql-tools's # import syntax (docs).

In our case, we have some common type definitions stored within a common directory, that get imported via the syntax above into other distinct GraphQL schemas, which we lint one-by-one (one schema could include multiple files) as mentioned above.

Our first try at a workaround was to use graphql-schema-linter on the "compiled" schema files, but this of course leads to lint failures referencing locations in the compiled schema files rather than the source files.

A second try at a workaround was to simply supply all the definitions in our common directory whenever a distinct schema is being linted. This works okay (need to suppress the defined-types-are-used rule) but if a schema happens to have a conflicting type definition with one of those in the common directory its a hard stop with the failure message:

There can be only one type named "ExampleType".                              invalid-graphql-schema
rperryng commented 3 years ago

I took a super quick jab at supporting # import statements here

It's quite crude:

But it does unblock me for now. I'd be happy to try and contribute back a proper implementation into the project if you had some vision or guidance on implementation. Thanks!

rperryng commented 2 years ago

@cjoudrey Just wondering if there's anything I can do to move the dial on this one - I have been using the quick workaround I mentioned above but would love to contribute something back upstream if you have any direction.

Is the project still being maintained?

cjoudrey commented 2 years ago

Is the project still being maintained?

👋 Hi @rperryng, unfortunately I haven't been able to give this project the time it deserves over the last year. I've been pretty distant from the GraphQL community during that time as well. This is why issues like this one haven't been resolved yet. I'm currently on parental leave so I don't expect this to change in the near future.

As for your idea of supporting # import ... from ..., from the get go I've been trying to stay away from library / cloud provide specific features (e.g. AWS AppSync directives and graphql-tools features) as I felt the sum of all those features would add complexity to the linter and would be a burden to maintain. I'm happy to be proven wrong on this though. One thing though, rather than re-implement # import ... from ... in the codebase, I would first try to leverage graphql-tools itself in the linter for this. Not sure if that's entirely possible though.

rperryng commented 2 years ago

@cjoudrey Thanks for your response! Understandable that you are keeping your package free from being "library-aware" and sticking to only supporting official GraphQL syntax. I appreciate the work put in, and I definitely got a lot of mileage out of graphql-schema-linter over the last year 🙂 .

It turns out that late last year The Guild / graphql-tools has released an eslint plugin that can accomplish schema linting. Since it's maintained by The Guild, the graphql-tools loaders are all supported out of the box (which includes # import ... syntax). I'm currently in the process of moving my linting solutions over to that package.

I hope you are enjoying your parental leave, all the best!

cjoudrey commented 2 years ago

I appreciate the work put in, and I definitely got a lot of mileage out of graphql-schema-linter over the last year 🙂 .

I'm really happy to hear that. 🎉 😄 graphql-schema-linter started as a passion project, and to my surprise, a lot of other people cared about the same things as me. 😄

It turns out that late last year The Guild / graphql-tools has released an eslint plugin that can accomplish schema linting.

That's amazing! I wasn't aware of this project. I'm happy that there are other options available.

I hope you are enjoying your parental leave, all the best!

Thanks! ❤️ Same to you. 😄