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

enums are broken after transformSchema #1056

Closed
nfantone opened this issue Jan 31, 2019 · 13 comments
Closed

enums are broken after transformSchema #1056

nfantone opened this issue Jan 31, 2019 · 13 comments
Labels

Comments

@nfantone
Copy link

nfantone commented Jan 31, 2019

This is a follow up to a sub-issue derived from #1006 - opening a new one hoping it'll get more attention.

I have a simple schema defining an enum and a custom resolver for it.

enum PersonType {
  ADULT
  CHILDREN
  INFANT
}
{
  PersonType: {
    ADULT: 'ADT',
    CHILDREN: 'CHD',
    INFANT: 'INF'
  }
}

This works fine in queries/mutations if I use the schema returned by makeExecutableSchema, but doesn't if using the schema returned by transformSchema. Queries that would return a PersonType all end up with the following error:

{
  "timeThrown": "2019-01-09T20:00:11.786Z",
  "name": "GraphQLError",
  "message": "Expected a value of type \"PersonType\" but received: \"ADULT\""
}

If you don't ask for this particular field in your response, everything works just fine.

I'm using transformSchema to filter out some fields - but found out that even if I don't pass in any transformations (which I'd expect to return an exact copy of the given schema), the error is still there. Basically:

/* schema.js */
const schema = makeExecutableSchema({ ... });

// Works as expected
// module.exports = schema;

// Breaks enums (empty transformations array)
module.exports = transformSchema(schema, []);

This happens for every other enum in my schema. So there's evidently some difference between the original schema and the returned value from transformSchema. Schema introspection reveals the enum values correctly for both cases, however. And, after debugging, I can confirm that the definition for PersonType is present in both of them.

image

Debug breakpoint before return line of transformSchema function. targetSchema is the original while schema is the result after applying transformations.

Moreover, here are the resolve functions for the passengerType field (the enum on the query output): original schema (targetSchema) vs. transformed schema (schema).

image

You can see that they are most definitely not the same, starting from its name ("defaultMergedResolver").

Any advice on this?

@pthrasher
Copy link

I think the main issue here stems from your override. The GraphQL spec appears to claim that enums are meant to be what you said they were, and not to have custom resolvers.

https://graphql.github.io/graphql-spec/draft/#sec-Enums

My guess would be that a bug in graphql-js / graphql-tools is what allowed this to work for you at all in the first place.

I'm not a contributor on this project... was hunting for something else in issues and stumbled across your post which made me curious.

I know the above info is not super helpful, but my suggestion would be to not have custom resolvers for enums. Enums should return the value shown in the schema.

@nfantone
Copy link
Author

@pthrasher Defining custom resolvers for enum values is absolutely supported by Apollo, according to its documentation. Whether they should do this or not as per spec, is another discussion altogether. Regardless, the fact remains that transformSchema result does not conform to the schema being passed.

@yaacovCR
Copy link
Collaborator

@nfantone, I am having trouble reproducing this with direct execution via graphql. Are you able to share a repo that reproduces this error?

@633kh4ck
Copy link

Same issue (using mergeSchemas), any updates?
@nfantone Did you find a solution (or at least workaround)?

@nfantone
Copy link
Author

nfantone commented Jun 12, 2019

@633kh4ck We ended up not using schema transformations at all. As it turned out, for our use case (filtering some queries/mutations), graphql-import works wonders.

@yaacovCR
Copy link
Collaborator

Can you share a more complete code example? Would love to work on fixing this if I could reproduce it.

Looks like this area is covered in test suite...

https://github.com/apollographql/graphql-tools/blob/305466b18d510d6e27493a326a6110d0dfafb64d/src/test/testMergeSchemas.ts#L624

@nfantone
Copy link
Author

@yaacovCR Good to read there's movement over here! Unfortunately, I opened this over 5 months ago and I'm no longer trying to use transformSchema in any of our projects.

However, I remember that reproducing the problem was as simple as setting up a schema as was described in the original issue:

  1. Declare an enum and give it a custom resolver.
  2. Then, call transformSchema(schema, []) and use its result to mount your /graphql.
  3. Finally, try running any query that would read the enum field.

Maybe the problem was fixed already during this time? Also, I'm not entirely sure what that unit test you linked is trying to evaluate, but the resulting enum definitions from both schemas are most definitely not the same, as demonstrated in my analysis some months ago. A deep equality comparison should fail. In my use case, I'm not explicitly using mergeSchemas, but transformSchema, though.

@yaacovCR
Copy link
Collaborator

The unit test is comparing the original result with the merged result. The resolvers in the merged schema are wrapped, but (a) the tests don't crash and (b) the results are the same, so the different resolvers are supposedly working correctly. I think this issue may have been fixed, but I could feel differently very soon if I saw a more fleshed out recent reproduction. I tried from the original issue and it looked ok to me. :)

In general, not sure whether there is movement here... I opened up graphql-tools-fork to merge my own PRs and I am working on fixing what I can there.

@yaacovCR
Copy link
Collaborator

yaacovCR commented Jun 16, 2019

I was able to reproduce -- it had been fixed in root fields returning enum, but not nested fields.

This should be fixed now in graphql-tools-fork v5.2.0. Let me know!

(As always, I will try to separate this out to PR for upstream graphql-tools, but this may be difficult for this fix, as relies on my refactoring from other fixes.)

@yaacovCR
Copy link
Collaborator

I am not great at predicting the future, but I don't see this getting merged to upstream graphql-tools any time soon.

In general, I would love to hear from the package maintainers... It has been radio silence from them re: pull requests and issues for some time, hence the fork.

I would love to just bring everything back in, it's just so far a handful of (important?) bug fixes and a few new transformers.

@wmertens
Copy link

I can confirm that graphql-tools-fork fixes this issue I had with mergeSchemas

@kamilkisiela
Copy link
Collaborator

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

Please update graphql-tools to next or run:

npx match-version graphql-tools next

Let us know if it solves the 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

6 participants