Update mongoose 8 #439
Update mongoose 8 #439
Conversation
|
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 |
| @@ -90,7 +90,7 @@ describe('Resolver helper `filter` ->', () => { | |||
| const itc = args.filter.type as InputTypeComposer; | |||
| expect(itc.hasField('_id')).toBe(true); | |||
| expect(itc.hasField('name')).toBe(true); | |||
| expect(itc.hasField('age')).toBe(false); | |||
| expect(itc.hasField('age')).toBe(true); | |||
There was a problem hiding this comment.
Ouch, tricky case!
If client makes query with name and age then YES this mongo query will be fast.
But if be requested only age then NO this query will be slow, because this field on the second place of compound index.
And I don't know what behavior is better...
There was a problem hiding this comment.
Compound index will only be used if both fields request together, so it will only work on sort
On normal query with one field ( name or age ) the compound index will not be used.
So the correct solution will be actually no filters if no singular index for the field.
There was a problem hiding this comment.
So I suggest either keep them both or remove them both.
Currently they are both there which i think it's fine.
There was a problem hiding this comment.
Compound index will only be used if both fields request together
Sorry, but this statement is incorrect. Mongodb can use fields partially from compound index (from left to right).
https://www.mongodb.com/docs/manual/core/indexes/index-types/index-compound/
So for compound index ({name:1, age: 1}) a query with filter:
({ name: 'ABC', age: 33 })will use this index({ name: 'ABC' })will use this index({ age: 33 })will NOT use this index
So name can be present in filter GraphQL type at any case. But age is tricky because it will work fast only if client also requested a name. And it's sad that with GraphQL is quite difficult to construct such conditional types.
Anyway, I think we can keep your change and publish under major version.
There was a problem hiding this comment.
Oh yea thank you for highlighting it!
I could make the first field of the compound index available if you prefer.
It also maybe worth mention this in the readme to make sure users understand index behavior and how best they get the queries.
src/utils/patchMongoose.ts
Outdated
| ); | ||
| } | ||
|
|
||
| const oldMongooseSchemaIndexDef = mongoose.Schema.prototype.index; |
There was a problem hiding this comment.
Is mongoose patching is crucial? Can we find another solution to avoid modifying alien module?
With such thing we can get a security violation.
There was a problem hiding this comment.
Yes I don't like this method - but was the only way to modify this method.
I have PR in mongoose here: Automattic/mongoose#14042
Seems it will take time to merge - and the issue will be still the issue will be in 7 and older versions - so I would suggest keeping this patch.
The patching is very small as well and if mongoose changed we can update to remove it.
There was a problem hiding this comment.
Why not create the correct index right away in the developer code?
Aliases are mongoose doc/code/operational level helpers. Indexes are DB/storage/data level staff. And mix aliases with indexes might bring a lot of headache on huge databases - it's quite easy rename alias in code, but it quite difficult to create/rename the real index in database. On my opinion, to be error prone protected it's better keep away aliases from indexes.
const UserSchema = new Schema(
{
n: {
type: String,
alias: 'name',
},
age: {
type: Number,
}
}
)
- UserSchema.index({ name: 1, age: -1 });
+ UserSchema.index({ n: 1, age: -1 });There was a problem hiding this comment.
I agree, this case only happen with aliases :) however if its working now I think it's good to keep it 😄
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ 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 ☔ View full report in Codecov by Sentry. |
|
Amazing works @meabed hopefully we will have this enhancement release soon. |
|
@nodkz do you think anything pending for this to be released? |
```diff
- UserSchema.index({ name: 1, age: -1 });
+ UserSchema.index({ n: 1, age: -1 });
```
|
@meabed could you please review my last commit 2b80ad2 ? 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 ( I made appropriate changes in |
|
Sounds good 😌 also my pr in mongoose is merged |
|
@meabed thank you a lot for the gorgeous PR 💪 |
|
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 |
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: meabed#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 Automattic/mongoose#14042