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

save without update the entity #46

Open
Alnyz opened this issue Nov 26, 2023 · 2 comments
Open

save without update the entity #46

Alnyz opened this issue Nov 26, 2023 · 2 comments

Comments

@Alnyz
Copy link

Alnyz commented Nov 26, 2023

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.

Artucuno added a commit to Artucuno/pydantic-mongo that referenced this issue Nov 27, 2023
@Artucuno
Copy link
Contributor

Artucuno commented Nov 27, 2023

@Alnyz I've made sorta what you are talking about while maintaining backward compatibility. Are the changes in 7459ffc 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  # <-----
            )
        ....

@Alnyz
Copy link
Author

Alnyz commented Nov 28, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants