Urigo / graphql-modules

Enterprise Grade Tooling For Your GraphQL Server
https://graphql-modules.com/
MIT License
1.31k stars 114 forks source link

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

Closed jbroadice closed 5 years ago

jbroadice commented 5 years ago

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 commented 5 years ago

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

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

Great, that would do the trick!

ardatan commented 5 years ago

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.

jbroadice commented 5 years ago

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 commented 5 years ago

Hi @jchapple! I added tests for your case, and it passes. https://github.com/Urigo/graphql-modules/commit/24eac3dac8463b07993680709816396988cfe7cb

ghost commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

ardatan commented 5 years ago

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

BeeHiveJava commented 5 years ago

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 commented 5 years ago

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
BeeHiveJava commented 5 years ago

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 commented 5 years ago

@BeeHiveJava How do you test it?

BeeHiveJava commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

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: https://github.com/graphql/graphql-js/issues/1527

ardatan commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

ardatan commented 5 years ago

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.