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

transformSchema simplifies schema returned from makeExecutableSchema #1006

Closed
taylorgoolsby opened this issue Nov 21, 2018 · 11 comments
Closed

Comments

@taylorgoolsby
Copy link

taylorgoolsby commented Nov 21, 2018

Here is an example which you can use to replicate the problem:
https://github.com/taylorgoolsby/graphql-directive-private/tree/graphql-tools-transform-bug

The README on that branch explains the bug in detail.
And this file has code to reproduce the bug:
https://github.com/taylorgoolsby/graphql-directive-private/blob/graphql-tools-transform-bug/__tests__/main-test.ts

I need this because I want my SchemaDirectiveVisitor to visit object, fields, and whatnot, and I want to mark each object or field with some special markings.
Then I want to use transformSchema to discover these markings and transform the schema accordingly.

This is my workaround right now:

const firstSchema = makeExecutableSchema({
  typeDefs,
  schemaDirectives: {
    deprecated: DeprecatedDirective
  }
});
console.log('first schema', firstSchema._typeMap.User.customFlag)
const thirdSchema = transformSchema(firstSchema, [{
  transformSchema: (secondSchema: any) => {
    // ignore secondSchema and transform firstSchema instead.
    console.log('first schema reference directly used in here', firstSchema._typeMap.User.customFlag)
    return firstSchema
  }
}])
@slaymance
Copy link

This looks to me like a naming issue, which your second answer resolved by creating new variable names for all the schemas you're working with here. You're linter should have caught that you're trying to reassign a const variable in your first implementation when building transformSchema.

@taylorgoolsby
Copy link
Author

taylorgoolsby commented Dec 2, 2018

@slaymance Oh sorry about that. I didn't test that example.

Here is a working example which anyone can run to reproduce the bug: https://github.com/taylorgoolsby/graphql-directive-private/tree/graphql-tools-transform-bug

I will also edit my original post to correct it.

@slaymance
Copy link

@taylorgoolsby Ah thanks for putting together that repo, it's super helpful for understanding this behavior!

When calling transformSchema, the tool recreates the schema passed in originally (the one created by makeExecutableSchema). That recreated schema is based upon the GraphQLNamedType defined by the GraphQL spec, so any custom properties that were added to the schema won't be carried over as they would break that contract definition.

This brings up some interesting discussion points regarding schema transforms. Should the GraphQL spec include the possibility of custom properties for GraphQL types? If not, should the Apollo transformSchema tool still allow for the carrying over of custom type properties when transforming a schema? I think this would be really useful functionality, since transformSchema implies you're manipulating the schema passed in to the function in its entirety.

By the way, the cause of this behavior is a function in graphql-tools called recreateType. You can see the behavior you want by going into node-modules/graphql-tools/dist/stitching/schemaRecreation.js and changing lines 10-19:

const recreatedGraphQLObject = new graphql_1.GraphQLObjectType({
            name: type.name,
            description: type.description,
            astNode: type.astNode,
            isTypeOf: keepResolvers ? type.isTypeOf : undefined,
            fields: function () {
                return fieldMapToFieldConfigMap(fields_1, resolveType, keepResolvers);
            },
            interfaces: function () { return interfaces_1.map(function (iface) { return resolveType(iface); }); },
        });
return { ...type, ...recreatedGraphQLObject };

That should cause the tests in your bug demonstration repo to pass.

@taylorgoolsby
Copy link
Author

@slaymance Thanks for the explanation. Now, I understand why the custom properties are lost, but I also agree that I was expecting the parameter into the Transform functions to be equal to the schema passed into transformSchema's first argument.

I don't think it's a big deal, since even though custom properties are lost, the directives are not, so you can still reference schema._typeMap.User.astNode.directives[0].name.value inside a Transform function.

@nfantone
Copy link

nfantone commented Jan 10, 2019

Hi and apologies for hijacking the topic - but I think I stumbled upon some behaviour related to what @taylorgoolsby has noted. If it's not, let me know and I'll open a new issue.

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 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.

Any pointers?

@nfantone
Copy link

Ok, have some new insight into this. 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").

@taylorgoolsby
Copy link
Author

taylorgoolsby commented Jan 10, 2019

@nfantone I think this line is the beginning of why the input and out schemas are different: https://github.com/apollographql/graphql-tools/blob/master/src/transforms/transformSchema.ts#L15

And above @slaymance describes recreateType as the ultimate culprit.

@nfantone
Copy link

@taylorgoolsby Right. So are we in agreement that this is, indeed, a bug? I'm not sure what to make of this as I never got around this situation.

@taylorgoolsby
Copy link
Author

@taylorgoolsby Right. So are we in agreement that this is, indeed, a bug? I'm not sure what to make of this as I never got around this situation.

I'm not sure if it's a bug or a feature. I'm not part of the apollo team. But as an end user of transformSchema, I think it's a bug.

@nfantone
Copy link

Thanks 👍. An official take would be nice here. Words seem slow around these parts, unfortunately.

@yaacovCR
Copy link
Collaborator

Closing.

See solution above in #1006 (comment)

Additional comments are tracked in #1056

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

No branches or pull requests

4 participants