fix(mongo): sorting with UnderscoreNamingStrategy #4314
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #4314 +/- ##
==========================================
- Coverage 99.69% 99.61% -0.09%
==========================================
Files 214 214
Lines 13785 14110 +325
Branches 3232 3325 +93
==========================================
+ Hits 13743 14055 +312
- Misses 39 54 +15
+ Partials 3 1 -2
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
thanks! interesting this wasnt reported before, i guess most of the mongo folks dont really use different strategies/field names
left a few comments about the test case, i'd like to see that greatly simplified, but will be happy to do that myself after merging if you'd struggle somehow with that.
tests/issues/GH4313.test.ts
Outdated
| it('Init orm with basic naming strategy',async () => { | ||
| orm = await MikroORM.init({ | ||
| entities: [A], | ||
| clientUrl: await initMongoReplSet('mikro-orm-test'), |
There was a problem hiding this comment.
no need for replica sets, that memory server implementation is rather unstable and i am using it only in places that are actually testing transactions
There was a problem hiding this comment.
Wow, how much faster the test have become without replset! Thanks for hint!
tests/issues/GH4313.test.ts
Outdated
| }); | ||
| } | ||
|
|
||
| describe('GH issue 4313', () => { |
There was a problem hiding this comment.
no need for nested describe blocks - actually no need for any of them, use just one test block on top level
There was a problem hiding this comment.
Ok, I'll make it shorter
tests/issues/GH4313.test.ts
Outdated
| it('Close connection', async () => { | ||
| await orm.close(); | ||
| await closeReplSets(); | ||
| }); |
There was a problem hiding this comment.
this should be afterAll hook, not a separate test case
tests/issues/GH4313.test.ts
Outdated
| it('Init orm with underscore naming strategy',async () => { | ||
| orm = await MikroORM.init({ | ||
| entities: [A], | ||
| clientUrl: await initMongoReplSet('mikro-orm-test'), | ||
| driver: MongoDriver, | ||
| namingStrategy: UnderscoreNamingStrategy, | ||
| }); | ||
| await orm.schema.clearDatabase(); | ||
| }); |
There was a problem hiding this comment.
this should be beforeAll hook, not a separate test case
tests/issues/GH4313.test.ts
Outdated
| it('Init orm with underscore naming strategy',async () => { | ||
| orm = await MikroORM.init({ | ||
| entities: [A], | ||
| clientUrl: await initMongoReplSet('mikro-orm-test'), |
There was a problem hiding this comment.
keep the defaults, just set the database name via dbName
tests/issues/GH4313.test.ts
Outdated
| test('Insert entities', async () => { | ||
| const a = new A(); | ||
| a.complexName = 'a'; | ||
| const b = new A(); | ||
| b.complexName = 'b'; | ||
| await orm.em.persistAndFlush(a); | ||
| await orm.em.persistAndFlush(b); | ||
| orm.em.clear(); | ||
| }); |
There was a problem hiding this comment.
this should be also part of that (single) beforeAll hook
tests/issues/GH4313.test.ts
Outdated
| await orm.em.persistAndFlush(a); | ||
| await orm.em.persistAndFlush(b); |
There was a problem hiding this comment.
you can pass both in a single call
| await orm.em.persistAndFlush(a); | |
| await orm.em.persistAndFlush(b); | |
| await orm.em.persistAndFlush([a, b]); |
| const data1 = await orm.em.findOne(A, { complexName: { $ne: null } }, { | ||
| orderBy: { complexName: 'asc' }, | ||
| }); | ||
| expect(data1!.complexName).toBe('a'); | ||
|
|
||
| const data2 = await orm.em.findOne(A, { complexName: { $ne: null } }, { | ||
| orderBy: { complexName: 'desc' }, | ||
| }); | ||
| expect(data2!.complexName).toBe('b'); | ||
|
|
||
| const data3 = await orm.em.findOne(A, { complexName: { $ne: null } }, { | ||
| orderBy: [{ complexName: 'asc' }], | ||
| }); | ||
| expect(data3!.complexName).toBe('a'); | ||
|
|
||
| const data4 = await orm.em.findOne(A, { complexName: { $ne: null } }, { | ||
| orderBy: [{ complexName: 'desc' }], | ||
| }); | ||
| expect(data4!.complexName).toBe('b'); |
There was a problem hiding this comment.
i dont think we need to test asc vs desc, that's kinda irrelevant to the issue, one direction is enough, the problem is about the left side, not right side
There was a problem hiding this comment.
one case may pass just by an accident, but both wont
tests/issues/GH4313.test.ts
Outdated
|
|
||
| let orm: MikroORM<MongoDriver>; | ||
|
|
||
| function testCase() { |
funduck
force-pushed
the
fix-mongo-sorting-underscore-naming
branch
from
ec4fbc8
to
97ee08f
Compare
|
Fixed most of the issues you've mentioned |
Fixes #4313
I propose to apply
this.renameFieldstooptions.orderBythe same way it is used forwhere