search

graphql-compose / graphql-compose-mongoose

Mongoose model converter to GraphQL types with resolvers for graphql-compose https://github.com/nodkz/graphql-compose
MIT License
708 stars 94 forks source link

[WIP] - Feature: Discriminator Support on composeMongoose #340

Open jhbuchanan45 opened 3 years ago

jhbuchanan45 commented 3 years ago

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.

type ComposeMongooseOpts = {
...
/**
* Support discriminators on base models
*/
includeBaseDiscriminators?: boolean;
/**
* EXPERIMENTAL - Support discriminated fields on nested fields
* May not work as expected on input types for mutations
*/
includeNestedDiscriminators?: boolean;
}

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:

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.

class EDiscriminatorTypeComposer<TSource, TContext> extends ObjectTypeComposer<TSource, TContext> {
  discriminatorKey: string = '';
  discrimTCs: {
      [key: string]: ... ObjectTypeComposer<any, TContext>;
    } = {};
  BaseTC: ObjectTypeComposer<TSource, TContext>;
  DInputObject: ObjectTypeComposer<TSource, TContext>;
  DInterface: InterfaceTypeComposer<TSource, TContext>;
  opts: ComposeMongooseDiscriminatorsOpts<TContext> = {};
  DKeyETC?: EnumTypeComposer<TContext>;
...
}

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 d586798dc80ed0bffc308ababedfe62716d6fcbb 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:

getTypeName(): string {
   return this.DInterface.getTypeName();
}

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.

nodkz commented 3 years ago

Wow, wOw, woW! 🤯 Incredible idea 👍

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-commenter commented 3 years ago

Codecov Report

Merging #340 (425fbb4) into master (8bb278e) will increase coverage by 0.30%. The diff coverage is 97.83%.

:exclamation: Current head 425fbb4 differs from pull request most recent head 0167cb2. Consider uploading reports for the commit 0167cb2 to get more accurate results Impacted file tree graph

@@            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
Impacted Files Coverage Δ
src/resolvers/connection.ts 95.23% <ø> (-0.22%) :arrow_down:
src/resolvers/pagination.ts 100.00% <ø> (ø)
src/fieldsConverter.ts 92.62% <96.00%> (-0.17%) :arrow_down:
...hancedDiscriminators/eDiscriminatorTypeComposer.ts 97.60% <97.60%> (ø)
src/composeMongoose.ts 97.14% <100.00%> (+1.30%) :arrow_up:
src/discriminators/prepareBaseResolvers.ts 100.00% <100.00%> (ø)
src/enhancedDiscriminators/index.ts 100.00% <100.00%> (ø)
src/index.ts 100.00% <100.00%> (ø)
src/resolvers/count.ts 81.81% <100.00%> (+0.86%) :arrow_up:
src/resolvers/createMany.ts 95.55% <100.00%> (+0.10%) :arrow_up:
... and 30 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 8bb278e...0167cb2. Read the comment docs.

fdelucchijr commented 2 years ago

Is there any update in this PR? Any way the community can help | continue the work?