Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
ruudvanbuul opened this issue Sep 11, 2023 · 11 comments
Open

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

ruudvanbuul opened this issue Sep 11, 2023 · 11 comments
Labels
bug Something isn't working

Comments

@ruudvanbuul
Copy link

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.

@ruudvanbuul ruudvanbuul added the bug Something isn't working label Sep 11, 2023
@TriPSs
Copy link
Owner

TriPSs commented Sep 11, 2023

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

@ruudvanbuul
Copy link
Author

ruudvanbuul commented Sep 11, 2023

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

@ruudvanbuul ruudvanbuul changed the title Allow custom id in creation endpoints (Typegoose + Mongoose) Allow custom id in create* endpoints (Typegoose + Mongoose) Sep 11, 2023
@TriPSs
Copy link
Owner

TriPSs commented Sep 11, 2023

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
Copy link
Author

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
Copy link
Author

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
Copy link
Owner

TriPSs commented Sep 12, 2023

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

@ruudvanbuul
Copy link
Author

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
Copy link
Contributor

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
Copy link
Author

ruudvanbuul commented Sep 14, 2023

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
Copy link
Contributor

smolinari commented Sep 14, 2023

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. 😁

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. 😀 🤷

Scott

@ruudvanbuul
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants