[WIP] - Feature: Discriminator Support on composeMongoose #340
Conversation
… supporting extended types
…tion using options
…m dscriminators where present
|
Wow, wOw, woW! 🤯 Now I'm working under graphql-compose v9 where will be refactored directives. It does NOT now affect your PR it affects only my free time. I'll return today evening or tomorrow for reviewing your PR and giving recommendations. Sorry for the small delay from my side. |
Codecov Report
@@ Coverage Diff @@
## master #340 +/- ##
==========================================
+ Coverage 92.84% 93.15% +0.30%
==========================================
Files 57 59 +2
Lines 2195 2337 +142
Branches 673 689 +16
==========================================
+ Hits 2038 2177 +139
- Misses 155 157 +2
- Partials 2 3 +1
Continue to review full report at Codecov.
|
src/resolvers/findMany.ts
Outdated
| @@ -58,15 +58,15 @@ export function findMany<TSource = any, TContext = any, TDoc extends Document = | |||
| throw new Error('First arg for Resolver findMany() should be instance of Mongoose Model.'); | |||
| } | |||
|
|
|||
| if (!tc || tc.constructor.name !== 'ObjectTypeComposer') { | |||
| if (!tc || !(tc instanceof ObjectTypeComposer)) { | |||
There was a problem hiding this comment.
Good catch! This is a very old hack before I started to use peerDependencies for graphql-compose.
I use to use this when I had multiple graphql-compose packages in node_modules. But for now, there is no need in constructor.name check.
src/resolvers/findMany.ts
Outdated
| throw new Error('Second arg for Resolver findMany() should be instance of ObjectTypeComposer.'); | ||
| } | ||
|
|
||
| const aliases = prepareNestedAliases(model.schema); | ||
| const aliasesReverse = prepareAliasesReverse(model.schema); | ||
|
|
||
| return tc.schemaComposer.createResolver<TSource, TArgs>({ | ||
| type: tc.NonNull.List.NonNull, | ||
| type: tc.schemaComposer.getAnyTC(tc.getTypeName()).NonNull.List.NonNull, |
There was a problem hiding this comment.
Maybe better to change '[' + tc.getTypeName() + '!]!' if you want to override type in future.
But I'm still unsure that use type as a string is a good idea. Maybe we just allow passing Interfaces to all resolvers:
export function findMany<TSource = any, TContext = any, TDoc extends Document = any>(
model: Model<TDoc>,
- tc: ObjectTypeComposer<TDoc, TContext>,
+ tc: ObjectTypeComposer<TDoc, TContext> | InterfaceTypeComposer<TDoc, TContext>,
opts?: FindManyResolverOpts
):Resolvers in graphql-compose === FieldConfigs` in graphql.
So we can generate as many Resolvers as we want. And maybe we just need generate a new resolver for every Discriminator type?
There was a problem hiding this comment.
That would definitely be a much cleaner change for now.
I definitely agree on the changing the typing to allow interface TCs to resolvers as a long term solution, especially since it seems that interfaces don't cause issues when 'hacked' in as a string type currently.
My original intention with these changes was to avoid messing with existing code and types as much as possible to avoid accidentally breaking anything, though until I read through what the old DTCs did I was thinking something like this would be needed anyway.
Passing in an Interface TC directly to the resolver (and presumably letting it be passed through composeMongoose() and convertModelToGraphQL() as needed) means we should be able to cut down on code inside eDTC for dealing with the interface. Perhaps EDTC should extend InterfaceTypeComposer() instead of OTC in that case, and we could just have an Object TC as a property for creating and manipulating the custom input type. (like how we have now as eDTC.DInputObject)
Currently for top level model discriminators each sub-model being created using composeMongoose() instead of convertModelToGraphQL() so they have a set of mongoose resolvers generated. (since they have a full mongoose model already stored on the main model which can be used by the resolvers)
// foreach discrimTC
const discrimTC =
discrimType instanceof Schema
? convertModelToGraphQL(
{ schema: discrimType },
this.getTypeName() + key,
schemaComposer,
{ ...this.opts, name: this.getTypeName() + key }
)
: composeMongoose(discrimType as Model<any>, { ...this.opts, name: this.getTypeName() + key });These can then be accessed and used by calling getDisciminatorTCs() which ensures resolvers are present for better typing when importing and using it
getDiscriminatorTCs(): {
[key: string]: ObjectTypeComposer<any, TContext> & {
mongooseResolvers: GenerateResolverType<any, TContext>;
};
} {
// check if mongooseResolvers are present (assume on one = on all)
if ((this.discrimTCs[Object.keys(this.discrimTCs)[0]] as any).mongooseResolvers) {
return this.discrimTCs as any;
} else {
return {};
}
}This is roughly how I've been using it in my test setup:
...
const composeOpts: ComposeMongooseOpts = {
includeBaseDiscriminators: true,
includeNestedDiscriminators: true,
};
export const PClass = mongoose.model('ClassGraph', pClass);
const ExtraClass = PClass.discriminator('extraToken', classSecondSchema);
export const ClassTC = composeMongoose(PClass, composeOpts);
const ExtraClassTC = (ClassTC as EDiscriminatorTypeComposer<any, any>).getDiscriminatorTCs()
?.extraToken;
export { ExtraClassTC };in a 'routes.js' file then:
schemaComposer.Query.addFields({
classById: ClassTC.mongooseResolvers.findById(),
extraClassById: ExtraClassTC.mongooseResolvers.findById()
}I'd like to get rid of the need to use the (... as EDTC) if discriminators are enabled in the options to use EDTC only props/methods, not sure if there is a way to do that in typescript. (Edit: discriminators enabled in options is not going to be a valid way to check for EDTC typing)
There was a problem hiding this comment.
Maybe better to change
'[' + tc.getTypeName() + '!]!'if you want to override type in future.But I'm still unsure that use type as a string is a good idea. Maybe we just allow passing Interfaces to all resolvers:
export function findMany<TSource = any, TContext = any, TDoc extends Document = any>( model: Model<TDoc>, - tc: ObjectTypeComposer<TDoc, TContext>, + tc: ObjectTypeComposer<TDoc, TContext> | InterfaceTypeComposer<TDoc, TContext>, opts?: FindManyResolverOpts ):
Resolversin graphql-compose === FieldConfigs` in graphql.
So we can generate as many Resolvers as we want. And maybe we just need generate a new resolver for every Discriminator type?
@nodkz Is there any progress on allowing InterfaceTCs directly in resolvers, or should I continue with this PR as is?
There was a problem hiding this comment.
@jhbuchanan45 I've just updated master which allows passing InterfaceTCs to resolvers. Please take a look.
There was a problem hiding this comment.
@nodkz I've pushed an updated version of the eDTC which now extends the InterfaceTypeComposer and is passed directly. It's definitely a neater solution, and it was a lot less work to allow IFTCs as well as OTCs than I had expected.
Cheers for making those changes to the resolvers, I had mistakenly assumed that it would need changes in graphql-compose.
|
Is there any update in this PR? Any way the community can help | continue the work? |
I've putting some work into getting discriminator support for the composeMongoose function.
Currently my approach is loosely based on the old system, (extending ObjectTypeComposer and building TCs which reflect the mongoose schema more closely) but it is enabled through composeMongooseOpts rather than a different compose function.
There's also theoretical support for nested discriminator fields with another option, but I'm not as confident in those working, especially on the input types.
Changes to Existing Code
The following are changes to the current composeMongoose code which are not exclusive to this feature, none of them should be breaking, certainly all existing tests are passing:
Using instanceof instead of constructor.name checking ObjectTypeComposer for resolvers in 643bae0.
In src/fieldsConverter.ts the options are now passed through any type conversion function which could be a discriminator type (embedded, embedded array, etc.) for supporting nested discriminator behaviour.
In d586798 I also changed most of the resolvers to use tc.getTypeName() instead of directly passing tc for type, since that points to the correct TC internally, and allows it to be overridden for the EDiscriminatorTypeComposer to point to the InterfaceTC.
Implementation Overview
The EDiscriminatorTypeComposer class, (E being for Enhanced because DTC is already taken by the old system) which extends the ObjectTypeComposer class, (requiring minimal typing changes in the existing code) contains methods for building and using the discriminator system, and has overrides so field modification works as expected.
Representing the discriminated model/schema:
*(Since GraphQL currently only supports Interface types on outputs, this rudimentary solution should allow all valid input objects to be accepted, with mongoose responsible for validating the exact discriminated type, and is necessary for nested discriminators to be supported for input)
To allow eDTCs to be used throughout the current code with minimal changes, I switched the resolvers in d586798 to use tc.getTypeName(), since I could override it inside the eDTC class to point to DInterface without making any other changes to the resolver logic specific to the eDTC class:
Overriding OTC 'Set' methods
Currently the following 'set' methods (and derivatives) should be supported so they change the correct fields/properties on each component of the eDTC:
In addition, I intend to support these methods, but I haven't implemented them yet:
There are also a bunch of methods like these which I don't entirely understand the purpose of yet, like those relating to fieldArgs, (are these for input types?) and some which I don't think need a special case for discriminators, but they could be changed on request.
To Do
Somewhere along the line I think this addresses #326 since it includes the discriminatorKey field when discriminators are enabled (also currently a custom dKey is required since fields beginning with '__', importantly here including '__t', are stripped from the field list)
Conclusion
While I think this is is decent solution for supporting discriminators with the new composeMongoose function while requiring minimal changes to existing code, I'm still not overly happy with the way it works, several key parts feel much too 'hacky' like overriding getTypeName() and reassigning the InputTC functions. While they work at the minute, I suspect they may be susceptible to breaking from minor changes either in the other code in this plugin, or in graphql-compose.
The input types are also far from ideal, but hopefully this can be revisited when/if graphql introduce input Interface types. As best as I could find this is currently being discussed, and with a spec for it agreed in their repo
Having said all this, I'm still not really sure what form a better implementation would take, but hopefully this can server to kickstart further discussion on the subject. I've also spent a good amount of time looking at the older implementation of DTCs as part of working on this, so I should be able to answer questions about that.
Finally, this is my first PR, so please point out anything stupid I've done.