TriPSs / nestjs-query

Easy CRUD for GraphQL.
https://tripss.github.io/nestjs-query/
MIT License
164 stars 46 forks source link

Dataloader not working correctly with Mongoose 6 and 7 #281

Closed mareksuscak closed 4 months ago

mareksuscak commented 4 months ago

Describe the bug

After the recent Dataloader fixes, there's now a bug that affects certain versions of Mongoose - namely v6 and v7 because of incompatibility with class-transformer and specifically https://github.com/typestack/class-transformer/issues/991 which causes that values of type ObjectId that are not explicitly decorated with @ObjectId decorator are randomized during the conversion when their parent entity is being converted to a DTO. This was previously not an issue because findRelation and queryRelations were reloading the parent entity from the database here and here - I'd argue unnecessarily but what's more important is that the entity's fields were always correct despite the previous conversion. But now we only use the DTOs passed into findRelation and queryRelations and the DTOs all suffer from the class-transformer bug. I'm not sure what's the best way to fix this as it only affects certain versions of Mongoose. We're not seeing this behavior in tests because this repository already uses Mongoose v8 which isn't affected anymore.

Have you read the Contributing Guidelines?

Yes

To Reproduce See https://github.com/mareksuscak/nestjs-query-bugs/pull/3

Expected behavior ObjectIds do not change randomly and relationships are correctly fetched

Screenshots If applicable, add screenshots to help explain your problem.

Desktop (please complete the following information):

Additional context None

TriPSs commented 4 months ago

We can just drop support for the older versions? Don't see why we would need to support those?

mareksuscak commented 4 months ago

That's what I was thinking as well @TriPSs. The only question is - do we revert my change in 6.1.3 and then cut 7.0.0, re-introduce the change and drop support for mongoose < 8? The only other alternative I could think of was detecting the installed version of Mongoose and if >5 and <8, we could use the old implementation. It seems to be possible to read the version number by accessing mongoose.version - see the docs.

TriPSs commented 4 months ago

Nha let's take this moment and drop those versions. I created a PR to revert it.

Could you create a new PR readding it again together with the removal of the older versions?

mareksuscak commented 4 months ago

Yes sir 🫡

TriPSs commented 4 months ago

Also add a commit with:

feat(query-mongoose): Dropped support of older versions

BREAKING CHANGE: Versions 6 and 7 of mongoose are no longer supported

This will make sure the bump is correct :)

mareksuscak commented 4 months ago

Done @TriPSs https://github.com/TriPSs/nestjs-query/pull/283

mareksuscak commented 4 months ago

This was addressed in 6.1.3 (change reverted) and 7.0.0 (change re-introduced and mongoose version range updated).