TriPSs / nestjs-query

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

Allow custom id in create* endpoints (Typegoose + Mongoose) #177

Open ruudvanbuul opened 1 year ago

ruudvanbuul commented 1 year ago

It's currently impossible to use a custom id when creating entities in Mongoose or Typegoose. However, in Typeorm you are able to do this. I am not sure why this works differently for Mongoose and Typegoose...

I would like to be able to use a custom id and do the same checks as Typeorm does, which is check if the id already exists in the db.

A wok around is to not use the query service to insert your document, but this kind of defeats the point of nestjs-query.

I would be happy to provide a PR if there is concensus to fix this.

TriPSs commented 1 year ago

Is this something related to this library? I feel like this should be a ticket for Mongoose or Typegoose?

ruudvanbuul commented 1 year ago

Yes it is related to this library

It is code specifically in the createOne and createMany functions of the typegoose-query.service.ts and mongoose-query.service.ts. ensureIdIsNotPresent checks if an id is set. This does not happen in typeorm

TriPSs commented 1 year ago

Typeorm also does that but there the metadata of the entity is available so it can check the correct field. See the following in function (on mobile can't share other info)

private async ensureEntityDoesNotExist(e: Entity):

I have never used either of the other two libs so I don't know if that metadata is available or even possible.

ruudvanbuul commented 1 year ago

Typeorm does not check if you have given an id. It only checks that when you have given an id, it does not exist yet. So the functionality is different. I think it would be better to have the same functionality and thus allow giving an id in mongoose/typegoose.

ruudvanbuul commented 1 year ago

In mongo the id field is always _id. So you can just check if that field exists.

In the ensureEntityDoesNotExist function of typeorm it then does a findOne to check if the id already exists. You would do the exact same for mongo.

I am not sure why this was implemented this way. At the moment you can never insert a document with an _id field. So there is no way to create a document with a custom id value in query service.

TriPSs commented 1 year ago

I do not know why this was done, could it be that the Typegoos/Mongoose libs do not support this?

ruudvanbuul commented 1 year ago

No it is fully supported. When I comment out the ensureIdIsNotPresent line everything works fine.

I loaded all my data through custom bulk writes so didn't notice this before, but now I had a use case to use this function and it became a problem.

Really not sure why this is added, especially because it differs from the typeorm functionality. Although I did notice that the original creator of nestjs did not have much experience with mongo. Because the queries are also very inefficient and .lean isn't used (I'll make PRs for this later or create a separate mongo implementation library)

smolinari commented 1 year ago

The lack of supporting custom IDs is probably done because it's a bad idea to make your own ids with MongoDB. Especially sequenced ids. MongoDB ObjectIds were specifically engineered for the purpose of big data storage. If you mess with MongoDB's id setup, you will more than likely experience issues. My advice to anyone is, don't mess with them. There is absolutely no reason to change them.

Scott

ruudvanbuul commented 1 year ago

As stated here _id field can be lots of data types. ObjectID is just the default. In my use case my data has generated unique ids, which is a valid use case even outlined in the docs. Without going into too much detail, this saves me index space and a lot of id lookups when syncing large amounts of data daily.

So completely blocking custom ids seems like a weird functionality to me if the mongo docs say this is a valid use case. Also if ensureIdIsNotPresent is skipped and no id is present it will still create an ObjectID. Now I can not use create functionality in nestjs-query at all.

If making things fool proof is the main consideration, introducing a module setting to enable customIds can fix this (disabled by default). I'm happy to make a PR for that.

smolinari commented 1 year ago

If you are storing UUIDs, then they should be fine for big data.

I can only guess the check to enforce a missing id was built in because it's just smarter and easier to use the default ObjectId and I'd also venture to say, nestjs-query wasn't designed with the ingestion of data from an external system using its own ids in the Mongo id field in mind.

In fact, I'd personally avoid using nestjs-query to ingest data from an external system. I'd use a specialized endpoint just for the ingestion and synchronization and I'd use the Node driver directly (or even use a faster language like Rust or Go). It would perform much, much better.

And, knowing that you only want to store the external system's id, that tells me the creation of the data should only be done in that master system. Granted, UUIDs can be created anywhere, but then you'd need bi-directional sync'ing and well, that is asking for trouble. So, if you ingest and sync the data outside of the application (the one using nestjs-query), and you have to create the data in the master system, you don't necessarily need any creation methods in nestjs-query. Granted, I'm making a lot of assumptions of your use case here.

That all being said, I'm not the owner of this package. Just a user myself, who also did some work on the Typegoose package. :grin:

And, I also don't see any proper technical reason to stop the usage of a self-generated ids. To me, it's just a way to avoid the (less experienced) devland dev from creating his own footgun. :grinning: :shrug:

Scott

ruudvanbuul commented 1 year ago

For the imports I do use mongo driver directly with bulk write operations. But documents can also be added later through the graphql api (1 at a time, but with custom id). I know my use case is pretty specific, but nestjs-query is such a nice tool that I like to use it as much as possible.

I think manually enabling custom ids should be enough of a safe guard to keep less experienced devs from going the wrong path, that means they have to enable it in nestjs-query and in their schema.

I wil just create a PR and then @TriPSs can decide what to do with it.