couchbaselabs / node-ottoman

Node.js ODM for Couchbase
https://ottomanjs.com/
Apache License 2.0
287 stars 98 forks source link

Check the model type when using `findById` #651

Closed xavierchow closed 2 years ago

xavierchow commented 2 years ago

I'm not quite sure if it's a bug, but the behavior of ottoman doesn't match my assumption.

I have a use case that uses uuid without prefix as the key of a document, and multiple models reside in a single bucket/scope/collection. The problem is that when applying an Id of model A to ModelB.findById, it just returns the document of model A.

I know it's not expected to apply the wrong Id to the model.findById, but it would be ideal if it can return a DocumentNotFoundError instead of a data from other models.

Attaching a minimum reproducible code as follows, https://gist.github.com/xavierchow/a995771877a9bac3ff56e192ae3aef2f

I'm wondering if an improvement with checking the model type for findById makes sense?

AV25242 commented 2 years ago

@xavierchow probably a bug like you had mentioned. @gsi-alejandro can you please review and provide a feedback ?

gsi-alejandro commented 2 years ago

hi @xavierchow

We need to discuss it. This issue will never arise while using scopes and collections.

The problem is: to ensure this now we have the metadata _type, this way we know if the document belongs to the model that requests it maybe in the future, we have to remove permanently or by a flag option the _type, then this implementation will not work anymore.

xavierchow commented 2 years ago

@gsi-alejandro IMHO, the Ottoman as the ODM maintains the mapping between the Model to the documents in the DB. It's strange that I can use A.findById to get a B document.

Good to know you may have a plan about the metadata _type, but how to distinguish the models (by collection separation or by metadata _type) doesn't block the improvement that brings the concept integrity, what do you think?

gsi-alejandro commented 2 years ago

Hi @xavierchow, thank you for your feedback.

We will provide a solution for its next release (check for doc _type against the model)

gsi-alejandro commented 2 years ago

released on version 2.2.1