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

BugFix: Default fields do not work when passing schema as string to mergeSchemas #1121

Closed
JazminElkan-Gonzalez opened this issue May 2, 2019 · 5 comments
Labels

Comments

@JazminElkan-Gonzalez
Copy link

JazminElkan-Gonzalez commented May 2, 2019

Notes:
If you pass your schema as a string to mergeSchemas it drops the default values.

I validated this by creating a unit test -> #1120

I have found exactly where it is failing -> https://github.com/apollographql/graphql-tools/blob/master/src/stitching/typeFromAST.ts#L186

reasoning -> valueFromAST(node.defaultValue, type) is returning undefined. If I replace type with the hard coded value expected (I.E. valueFromAST(node.defaultValue, GraphQLString) in this case) then I see the defaultValue is Foo as expected and the mergedSchema includes the defaults. so it seems type is not evaluated correctly... I tried printing it out to console and see String as expected... but I'm not super familiar with GraphQLInputType and GraphQLInputObjectType so this is where my investigation came to a head. it feels like it should be a quick fix for someone more familiar with the code base? help would be appreciated!

Intended outcome: When passing schemas as strings to mergeSchemas I expect the result to have defaults defined in the original schemas (see draft PR above for basic example)

Actual outcome: defaults are dropped see output from test:
Screen Shot 2019-05-02 at 4 17 15 PM

How to reproduce the issue: Pull down my branch and run yarn test -g 'input object with default and raw schema' I have added console Logs that should highlight the issue

@JazminElkan-Gonzalez JazminElkan-Gonzalez changed the title Default fields do not work when passing schema as string to mergeSchemas BugFix: Default fields do not work when passing schema as string to mergeSchemas May 3, 2019
@yaacovCR
Copy link
Collaborator

yaacovCR commented Jun 19, 2019

I think this is because we should be using valueFromAstUntyped as the subfield types are just stubs.

If advanced parseLiteral functioning is needed (ie the stuff besides validation done with the type argument), this should be after the merge. This would also come in handy in case the input type is changed, even if just the parseLiteral function is changed (for example if a different internal representation is used). In general, we should probably serialize from the default value using the old schema and then parse from that with the new schema.

@yaacovCR
Copy link
Collaborator

Fix to this is released in graphql-tools-fork v6.0.1. Please let me know how performs.

@yaacovCR
Copy link
Collaborator

yaacovCR commented Jun 27, 2019

The way it shook out, valueFromAstUntyped does not work from stub types either, so I just stored the entire ast. When recreating a schema, it serializes with the old type and parses with the new, unless it is working from a stub, in which case it calls parseLiteral

yaacovCR added a commit to yaacovCR/graphql-tools-fork that referenced this issue Sep 22, 2019
yaacovCR added a commit to yaacovCR/graphql-tools-fork that referenced this issue Sep 22, 2019
yaacovCR added a commit to yaacovCR/graphql-tools-fork that referenced this issue Sep 22, 2019
yaacovCR added a commit to yaacovCR/graphql-tools-fork that referenced this issue Sep 22, 2019
yaacovCR added a commit to yaacovCR/graphql-tools-fork that referenced this issue Oct 3, 2019
yaacovCR added a commit to yaacovCR/graphql-tools-fork that referenced this issue Oct 25, 2019
yaacovCR added a commit to yaacovCR/graphql-tools-fork that referenced this issue Oct 25, 2019
yaacovCR added a commit to yaacovCR/graphql-tools-fork that referenced this issue Nov 4, 2019
yaacovCR added a commit to yaacovCR/graphql-tools-fork that referenced this issue Dec 31, 2019
yaacovCR added a commit to yaacovCR/graphql-tools-fork that referenced this issue Dec 31, 2019
yaacovCR added a commit to yaacovCR/graphql-tools-fork that referenced this issue Jan 8, 2020
yaacovCR added a commit to yaacovCR/graphql-tools-fork that referenced this issue Jan 21, 2020
yaacovCR added a commit to yaacovCR/graphql-tools-fork that referenced this issue Feb 27, 2020
yaacovCR added a commit to yaacovCR/graphql-tools-fork that referenced this issue Mar 26, 2020
yaacovCR added a commit to yaacovCR/graphql-tools-fork that referenced this issue Mar 26, 2020
@kamilkisiela
Copy link
Collaborator

🔈@JazminGonzalez-Rivero

We recently released an alpha version of GraphQL Tools (#1308) that should fix your issue.

Please update graphql-tools to next or run:

npx match-version graphql-tools next

Let us know if it solves your problem, we're counting for your feedback! :)

@yaacovCR yaacovCR mentioned this issue Mar 27, 2020
22 tasks
@yaacovCR
Copy link
Collaborator

Rolled into #1306

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants