graphql-compose / graphql-compose-mongoose

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

Update mongoose 8 #439

Closed meabed closed 7 months ago

meabed commented 8 months ago

Hey @nodkz, Hope all is well, this PR to support mongoose 8 as it was release few days ago

Summary of the changes:

PR Testing are here: https://github.com/meabed/graphql-compose-mongoose/pull/3

Pending Mongoose fix release: mongoose changed slightly the order of the index attributes when it has an alias - so the order for sorting enum is failing in the test.

I have a workaround already implemented to patch the index method

and the PR landed in mongoose we can remove it : I have a PR in mongoose to fix it https://github.com/Automattic/mongoose/pull/14042

meabed commented 8 months ago

Hi @nodkz this PR is ready now ✅ - would you please have a look - we might as well increase the version to major to address the dropping support of nodejs 14 and mongoose 5.

Even node16 is EOL, and Mongoose 6 is EOL too

codecov-commenter commented 8 months ago

Codecov Report

Attention: 11 lines in your changes are missing coverage. Please review.

Comparison is base (91cdfd0) 92.54% compared to head (acc33b2) 91.67%. Report is 5 commits behind head on master.

:exclamation: Current head acc33b2 differs from pull request most recent head bc859d9. Consider uploading reports for the commit bc859d9 to get more accurate results

Files Patch % Lines
src/resolvers/removeMany.ts 0.00% 7 Missing :warning:
src/fieldsConverter.ts 91.66% 1 Missing :warning:
src/resolvers/helpers/validate.ts 80.00% 0 Missing and 1 partial :warning:
src/resolvers/removeById.ts 75.00% 1 Missing :warning:
src/resolvers/removeOne.ts 75.00% 1 Missing :warning:

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #439 +/- ## ========================================== - Coverage 92.54% 91.67% -0.87% ========================================== Files 57 57 Lines 2213 2283 +70 Branches 665 729 +64 ========================================== + Hits 2048 2093 +45 - Misses 163 187 +24 - Partials 2 3 +1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

loci-admin commented 8 months ago

Amazing works @meabed hopefully we will have this enhancement release soon.

meabed commented 7 months ago

@nodkz do you think anything pending for this to be released?

nodkz commented 7 months ago

@meabed could you please review my last commit https://github.com/graphql-compose/graphql-compose-mongoose/pull/439/commits/2b80ad2bf9b541266428d751d126ddc225f9f628 ?

I dislike the idea of using some patches in runtime for instances of 3rd code/packages. So I revised all logic and found the root of our problem - incorrect use of fieldname in compound index of User mock:

const UserSchema = new Schema(
    n: {
      type: String,
      required: true,
      description: 'Person name',
      alias: 'name',
    },
);
- UserSchema.index({ name: 1, age: -1 });
+ UserSchema.index({ n: 1, age: -1 });

So it's error-prone to use aliases (name) inside compound indexes. Must be used the real field name (n) like it is in database.

I made appropriate changes in src/utils/getIndexesFromModel.ts and now it works with Mongoose 6, 7, 8.

meabed commented 7 months ago

Sounds good 😌 also my pr in mongoose is merged

nodkz commented 7 months ago

@meabed thank you a lot for the gorgeous PR 💪

meabed commented 7 months ago

Thank you @nodkz I have upgraded and tested 🚀 all looks great :)

Also the PR for index order in mongoose has been released: https://github.com/Automattic/mongoose/releases/tag/8.0.1