pydantic-mongo icon indicating copy to clipboard operation
pydantic-mongo copied to clipboard

save without update the entity

Open Alnyz opened this issue 1 year ago • 2 comments

Hello, I recently discovered this project and found it interesting and useful for certain cases. However, upon examining the source code, I observed that the save method updates an existing entity with its _id/id. I suggest raising an error if the existing id already exists and introducing a separate method, like update, for modifying existing entities.

This approach eliminates the need for a query to check whether the id already exists, preventing potential errors when developers intend to save the latest data.

Alnyz avatar Nov 26 '23 15:11 Alnyz

@Alnyz I've made sorta what you are talking about while maintaining backward compatibility. Are the changes in 7459ffcc82dc6526c6fd06d39e32ee451ead649a what you meant?

This approach eliminates the need for a query to check whether the id already exists, preventing potential errors when developers intend to save the latest data.

This part doesn't make sense (to me atleast), because upsert=True in update_one automatically creates a document if it doesn't exist. However, when reading the pymongo docs, I've found a small issue in the way pydantic-mongo handles upserts, so I'll also fix that in a pull request.

    def save(self, model: T, **kwargs) -> Union[InsertOneResult, UpdateResult]:
        """
        Save entity to database. It will update the entity if it has id, otherwise it will insert it.
        :param model: Model to save
        :param kwargs: kwargs for pymongo insert_one or update_one
        :return: Union[InsertOneResult, UpdateResult]
        """
        document = self.to_document(model)

        if model.id:
            mongo_id = document.pop("_id")
            return self.get_collection().update_one(
                {"_id": mongo_id}, {"$set": document}, upsert=True, **kwargs  # <-----
            )
        ....

Artucuno avatar Nov 27 '23 23:11 Artucuno

Yes, that's what I meant. In your suggested change, you used $set as the default filter for the update. I think it would be better to make it a parameter, allowing people to use their own filters.

This approach eliminates the need for a query to check whether the ID already exists, preventing potential errors when developers intend to save the latest data.

The main point of this section is to raise an error if 'entity' already exists in the database, eliminating the need to use find_one_by_id or a similar method for checking.

I believe this step offers better performance since there's no need to query first to check if entity already exists. This can be especially useful if a developer wants to send a message like (You're already registered) to the user.

    def save(self, model: T, **kwargs) -> InsertOneResult:
        """
        Save entity to database. It will update the entity if it has id, otherwise it will insert it.
        :param model: Model to save
        :param kwargs: kwargs for pymongo insert_one or update_one
        :return: Union[InsertOneResult, UpdateResult]
        :raise: pymongo.errors.DuplicateKeyError if id already exists in db
        """
        document = self.to_document(model)
        return self.get_collection().insert_one(document, **kwargs)
        ....

Alnyz avatar Nov 28 '23 01:11 Alnyz