Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

buildASTSchema() should throw on encountering extend definition #922

Closed
leebyron opened this issue Jun 20, 2017 · 11 comments · Fixed by #2248
Closed

buildASTSchema() should throw on encountering extend definition #922

leebyron opened this issue Jun 20, 2017 · 11 comments · Fixed by #2248
Assignees

Comments

@leebyron
Copy link
Contributor

buildASTSchema() exists to parse a fully complete schema definition and produce a schema. However currently if it encounters an extend definition, or any other definition it does not use for that matter, it silently skips over it. This can lead to a confusing debugging situation.

Instead, buildASTSchema() should throw when it encounters a definition that it does not use, similar to how execute() throws if a the provided document contains any definition other than operations.

@OlegIlyenko
Copy link
Contributor

Out of curiosity: is there something that prevents supporting extend in buildASTSchema?

For example this definition:

type Queries {
  querySubset: QuerySubset
}

type Query {
  queries: Queries
}

extend type Queries {
  hello: String
}

Look semantically correct to me. When used in this form in may look a bit unusual, but the last definition could be loaded from a different file. From what I can tell, supporting extend does not have any downsides and provides more flexibility.

@leebyron
Copy link
Contributor Author

That would be new behavior, but I'd also be happy to entertain PRs adding that as well. If so, the same change should probably be made in extendSchema if multiple extend statements are found, and care should be put into ensuring that the order in which the extensions are applied result in the same output.

@dyst5422
Copy link

dyst5422 commented Jun 21, 2017

I think this might be a breaking change for graphql-tools. I believe the way it currently does schema extension concatenates a number of schema language strings together and then pulls the extensions out to add them in a second step.

I agree with @OlegIlyenko about looking for a solution that handles the extensions and perhaps warning on this case in the mean time.

@kaushik94
Copy link

Hey I would like to chip in on this one. Can I first take up the issue of adding a warning? Then I will try to understand and help implement handling extend statement?

@viztor
Copy link

viztor commented Aug 5, 2018

Does extend work now? @leebyron
or do we expect them to work maybe in the future?

The upside I'm seeing is as such allowing people building graphql application to modularize their application schema.

@IvanGoncharov
Copy link
Member

Does extend work now?

@viztor Not at the moment but I plan to implement it for 14.1.0 release.
Currently, buildSchema/extendSchema is under heavy refactoring with the goal to implement #1389.

@langpavel
Copy link
Contributor

langpavel commented Sep 20, 2018

For usecase where somebody needs multiple files merge up to one AST I write this helper: https://gist.github.com/langpavel/9653b99afe993167fc4bacafbfcc7909
Feel free to use it as an example of implementation which merge extend types into defined ones. Of course WITHOUT ANY WARRANTY :-)

What it can do:

  • load multiple *.gql files
  • parse them
  • merge definitions into one Document
  • sort definitions by multiple criteria
  • print AST like concatenated SDL
  • merge extend nodes into definition nodes, so AST looks like extended FTW
  • you can print this AST into SDL file of course
  • you can run buildASTSchema on merged AST →
  • you can execute introspection query on it

really not much of code with this possibilities, look on gist :-)
BTW: I care about location and Sources

@aishwar
Copy link

aishwar commented May 16, 2019

Hello, appreciate the work all of you do here. Just curious, when is the fix for this expected to be released?

@IvanGoncharov
Copy link
Member

@aishwar Thanks.
Can't say exact date but I will try to include it in 15.0.0 that I plan to release next month.

@dandv
Copy link

dandv commented May 19, 2019

I've been flipping around 10+ issues on this topic, and I'd like some clarification: does Object extension actually work per the spec in the current version of this reference JS implementation, or is it undocumented because it only partially works (i.e. schema can be defined, but extended fields can't be queried)? (To further muddy the waters, extending types does work with graphql-tools.)

If it does work, what am I doing wrong in this example that I'm getting this error?

GraphQLError: Cannot query field "salary" on type "Person".

@IvanGoncharov
Copy link
Member

Current implementation already fully support extensions using extendSchema function:

const { graphql, parse, buildSchema, extendSchema } = require('graphql');

let schema = buildSchema(`
  type Person {
    name: String!
  }

  type Query {
    person: Person
  }
`);

schema = extendSchema(schema, parse(`
  extend type Person {
    salary: Int
  }  
`));

var root = { person: () => ({ name: "John Doe", salary: 1234 })};

graphql(schema, '{ person {salary} }', root).then((response) => {
  console.log(response);
});

If it does work, what am I doing wrong in this example that I'm getting this error?

Thanks for example 👍
ATM, buildSchema silently ignore extensions and you need to use extendSchema for that.
This issue is about adding support for extensions in buildSchema.

IvanGoncharov added a commit to IvanGoncharov/graphql-js that referenced this issue Nov 6, 2019
IvanGoncharov added a commit to IvanGoncharov/graphql-js that referenced this issue Dec 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants