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

[Question] Using libraries such as join-monster, which rely on mutating schema instance #198

Closed
jbroadice opened this issue Nov 25, 2018 · 21 comments
Assignees
Labels
bug Something isn't working

Comments

@jbroadice
Copy link

jbroadice commented Nov 25, 2018

Hi again.

I've recently discovered a few GraphQL libraries that work by decorating / mutating the schema (GraphQLSchema) instance, and depend on those changes being persisted and not overwritten, such that those decorations can be accessed in resolvers with the resolverInfo argument (fourth argument). For example, join-monster requires adding custom properties to GraphQLObjectType constructors in order to be configured. When using Apollo, this can be achieved using an adapter, such as join-monster-graphql-tools-adapter, which works by decorating / mutating the schema based on a custom object you pass to it.

As far as I can tell, I cannot rely on mutating the schema instance in such a way when using GraphQL Modules, as the schema are built using a factory method, which is called at strategic points in the control flow.

This has lead me to abandon the idea of being able to use such libraries with GraphQL Modules, viewing them as being idiomatically incompatible with one another. Is this an accurate conclusion, or have I missed something? Perhaps the solution could be as simple as formalising an interface for mutating / transforming a schema at a strategic point before the resolvers get called?

Cheers.

@ardatan
Copy link
Collaborator

ardatan commented Nov 26, 2018

We can add a field for this kind of middleware tools.

new GraphQLModule({
  // some definition
  schemaMiddleware: (schema: GraphQLSchema) => {
   //some logic here
   return modifiedSchema;
}
})

@jbroadice
Copy link
Author

Great, that would do the trick!

@ardatan
Copy link
Collaborator

ardatan commented Nov 26, 2018

After v0.2.10 you can mutate schema by using

new GraphQLModule({
  // some definition
  middleware({schema}){
              //your logic
     return {
            schema
      };
   }
}
})

Let me know if that worked for you.

@ardatan ardatan self-assigned this Nov 26, 2018
@jbroadice
Copy link
Author

Thanks! I tried that, but it didn't seem to work.

As a test, I tried:

new GraphQLModule({
  // ...
  middleware({ schema }) {
    schema['__DIRTY__'] = true;
    return { schema };
  }
})

I see that the middleware function is getting called once, which seems to be when the GraphQLModule is initially bootstrapped.

However, when inspecting resolverInfo.schema from within a resolver, I see that the schema remains unchanged (e.g. I can't see the __DIRTY__ property).

@ardatan
Copy link
Collaborator

ardatan commented Nov 29, 2018

Hi @JChapple!
I added tests for your case, and it passes.
24eac3d

@ghost
Copy link

ghost commented Nov 29, 2018

Hello!

Thanks for putting the test together. I've investigated further and discovered that the problem seems to occur when adding middleware to a module that is being imported by another module.

In my case, I have an entry AppModule:

export const AppModule = new GraphQLModule<AppModuleConfig, {}, AppModuleContext>({
  imports: () => [
    AuthModule,
  ],
});

and I am attempting to add the middleware to AuthModule, which I have stripped down for testing to:

export const AuthModule = new GraphQLModule<{}, AuthModuleRequest, AuthModuleContext>({
  typeDefs, // imported above

  resolvers: {
    Query: {
      me: (_root, _args, _context, info) => {
        console.log('resolver', info.schema);
      }
    }
  },

  middleware: ({ schema }) => {
    schema['__DIRTY__'] = true;
    return { schema };
  },
});

If ApolloServer is instantiated using AuthModule as the root entry point (e.g. const { schema, context } = AuthModule.forRoot({});), I see the __DIRTY__ property when resolving, in info.schema, as expected. However, if I instantiate Apollo using AppModule as the entry point, the resolver does not contain the mutated property.

@ardatan
Copy link
Collaborator

ardatan commented Nov 30, 2018

Could you try the following versions?

Changes:
 - @graphql-modules/core: 0.3.0-alpha.aca8a400
 - @graphql-modules/epoxy:  0.3.0-alpha.aca8a400
 - @graphql-modules/sonar: 0.3.0-alpha.aca8a400
 - @graphql-modules/utils:  0.3.0-alpha.aca8a400

@ghost
Copy link

ghost commented Nov 30, 2018

Great, that seems to have fixed it. Many thanks! 🎉

@ardatan
Copy link
Collaborator

ardatan commented Nov 30, 2018

Great!!! So, we can close this issue then :)

@ardatan ardatan closed this as completed Nov 30, 2018
@BeeHiveJava
Copy link

BeeHiveJava commented Feb 8, 2019

Could you try the following versions?

Changes:
 - @graphql-modules/core: 0.3.0-alpha.aca8a400
 - @graphql-modules/epoxy:  0.3.0-alpha.aca8a400
 - @graphql-modules/sonar: 0.3.0-alpha.aca8a400
 - @graphql-modules/utils:  0.3.0-alpha.aca8a400

I just tested this today and it does not seem to work. I tested versions 0.4.2 and 0.5.0-alpha.e4344183. The modified schema only seems to get through when the middleware is added to the root module (so changing the root module or moving the middleware to the root module).

I'm using the exact sample supplied above by @ghost so I'm not certain where to go from here @ardatan?

@ardatan
Copy link
Collaborator

ardatan commented Feb 22, 2019

#335 We are planning to change our merge logic, so you won't lose your modifications on child modules if you use middlewares.
Could you try it with the following versions?

Changes:
 - @graphql-modules/core: 0.4.2 => 0.5.0-alpha.b12e4322
 - @graphql-modules/di: 0.4.2 => 0.5.0-alpha.b12e4322

@ardatan ardatan added the bug Something isn't working label Feb 22, 2019
@ardatan ardatan reopened this Feb 22, 2019
@BeeHiveJava
Copy link

#335 We are planning to change our merge logic, so you won't lose your modifications on child modules if you use middlewares.
Could you try it with the following versions?

Changes:
 - @graphql-modules/core: 0.4.2 => 0.5.0-alpha.b12e4322
 - @graphql-modules/di: 0.4.2 => 0.5.0-alpha.b12e4322

I just tested this and it does not seem to work. The schema only gets modified when applying the middleware on the root module.

@ardatan
Copy link
Collaborator

ardatan commented Feb 22, 2019

@BeeHiveJava How do you test it?

@BeeHiveJava
Copy link

BeeHiveJava commented Feb 22, 2019

Root module:

export const AppModule = new GraphQLModule({

    imports: [
        UserModule
    ]

});

Child module:

export const UserModule = new GraphQLModule({

    typeDefs: definitions,
    resolvers: resolvers,
    middleware: ({schema}: any) => {
        schema["__DIRTY__"] = true;
        return {schema};
    }

});

This does not seem to modify the final schema for me. But again, when I place the middleware in the root module it does work:

export const AppModule = new GraphQLModule({

    middleware: ({schema}: any) => {
        schema["__DIRTY__"] = true;
        return {schema};
    },
    imports: [
        UserModule
    ]

});

@ardatan
Copy link
Collaborator

ardatan commented Feb 22, 2019

@BeeHiveJava
The schema of each module is created independently from each other recursively.
First, UserModule creates a schema with its typeDefs and resolvers. Then, middleware gets called with that schema not the root module's.
Second, AppModule gets schemas from each imported module then merges it with its own typeDefs and resolvers.
So, I don't think we should reflect your custom property to AppModule's schema.
In addition, all your changes in the schema such as types, directives, resolvers etc will be reflected to the root module as expected; because schema merger can analyze these but I am not sure about the custom properties inside the schema like __DIRTY__.
Although we want this, how can we understand which property should be added to the root module while every module is independent from each other?
Please let me know if there is a reason/scenerio needs that, so we can discuss more.

@BeeHiveJava
Copy link

BeeHiveJava commented Feb 22, 2019

I guess that the biggest use case would be libraries such as join-monster. I think we can get these libraries to work by modifying the generated schema with custom properties afterwards, but it would be real nice if we could add custom properties to the schema from a module.

In addition I think that this is also solvable by implementing some custom directives and a middleware that parses them on the root module? That might even be a more elegant approach than monkey-patching the schema. It seems like this is going to be supported natively: graphql/graphql-js#1527

@ardatan
Copy link
Collaborator

ardatan commented Feb 22, 2019

So, the point is to get that field inside the resolver right?
Your child module can take the custom properties of its module's schema like this;

export const UserModule = new GraphQLModule({

    typeDefs: definitions,
    resolvers: {
        SomeType: {
          someField: (root, args, context, info) => {
             // info.schema has __DIRTY__ here
          }
         }
    },
    middleware: ({schema}) => {
        schema["__DIRTY__"] = true;
        return {schema};
    }

});

I just added a test for that case;
https://github.com/Urigo/graphql-modules/pull/335/files#diff-d906e55e6c8b73e6a4d023a5deb41485R1008

@BeeHiveJava
Copy link

You're right, I missed that point! I'll do some testing and I'll let you know if I get my use case for GraphQL-shield and join-monster to work.

@BeeHiveJava
Copy link

I tested and debugged some and your provided sample seems to work, the new properties are present on the schema @ardatan.

I couldn't, however, get join-monster to work, but it seems like the issue I'm having now is irrelevant. Thanks for all the support thus far, fantastic library . 👍

@ardatan
Copy link
Collaborator

ardatan commented Feb 27, 2019

@BeeHiveJava Let's keep this open until your issue is solved.

@ardatan
Copy link
Collaborator

ardatan commented Apr 2, 2019

We are using different approach for merging schemas, so I think your issue is solved in the latest version. Feel free to open this issue again if your problem persists.

@ardatan ardatan closed this as completed Apr 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants