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

mergeSchemas throws when typeDefs include default values #1399

Closed
DrewML opened this issue Apr 23, 2020 · 5 comments · Fixed by #1401
Closed

mergeSchemas throws when typeDefs include default values #1399

DrewML opened this issue Apr 23, 2020 · 5 comments · Fixed by #1401

Comments

@DrewML
Copy link

DrewML commented Apr 23, 2020

Minimal Repro

const exampleTypeDefs = gql`
    type Query {
        foo(arg: String = "1"): String
    }
`;
const exampleSchema = mergeSchemas({ typeDefs: exampleTypeDefs });

//TypeError: String cannot represent value: { kind: "StringValue", value: "1", block: false }
//           at GraphQLScalarType.serializeString [as serialize]

Believe this relates to the fix done for #1121

Looks like the whole AST node for defaultValue is being passed to serializeString, serializeInt, etc in the graphql package, but those methods only expect the raw value from the value prop, not the node wrapper.

@DrewML
Copy link
Author

DrewML commented Apr 23, 2020

Looks like this is the rough call path:

  1. mergeSchemas calls addResolversToSchema
  2. addResolversToSchema runs this code:
    // serialize all default values prior to healing fields with new 	scalar/enum types.
    forEachDefaultValue(schema, serializeInputValue);
  3. serializeInputValue above is a wrapper that invokes the serializeX methods from graphql types. It is expecting a type def and a value
  4. forEachDefaultValue runs this code:
    // arg.defaultValue as the second arg to `fn` is an AST node, but fn is expecting a simple value
    arg.defaultValue = fn(arg.type, arg.defaultValue);

yaacovCR added a commit that referenced this issue Apr 23, 2020
mergeSchemas dummy types should handle entire SDL surface prior to better prepare for rewiring with actual types

closes #1399
yaacovCR added a commit that referenced this issue Apr 23, 2020
mergeSchemas dummy types should handle entire SDL surface prior to better prepare for rewiring with actual types

closes #1399
@yaacovCR yaacovCR reopened this Apr 23, 2020
@yaacovCR
Copy link
Collaborator

Thanks for pointing this out, see PR for canary build.

@yaacovCR yaacovCR added the waiting-for-release Fixed/resolved, and waiting for the next stable release label Apr 23, 2020
@DrewML
Copy link
Author

DrewML commented Apr 23, 2020

Thanks for the quick fix @yaacovCR! Confirmed 5.0.1-2def1b9.0 fixes it 🥂

@yaacovCR
Copy link
Collaborator

yaacovCR commented Apr 23, 2020

This change should also preserve directives in the AST for the gateway types -- this missing behavior likely led to bugs with directives on new types using mergeSchemas.

Directives were probably working on type extensions, as those are separated out and handled by extendSchema.

@DrewML -- thank you for bringing this to our attention!!!!!!!

@yaacovCR
Copy link
Collaborator

Closed by #1419.

@yaacovCR yaacovCR removed the waiting-for-release Fixed/resolved, and waiting for the next stable release label May 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants