Closed Ty-Ni closed 8 months ago
This appears to be an extension of https://github.com/roman-right/beanie/issues/707
Hi! Thank you for the catch. I'll check and fix it this/next week.
Using the above server, follow these steps:
- Perform a POST request to the create endpoint
- Perform a PUT request to the update endpoint using the ID returned in step 1.
- The RevisionIdWasChanged error will be raised in the server.
after reproduce i found the problem cause by step 2 (i got the same error yesterday)
you pass the id from step 1, the object in endpoint update
will generate an new object with the same id, then you call save()
cause the error,
we could find_one by id first, and then use new_obj = obj.model_copy(update=foo.model_dump(exclude={"id"}))
to update model,
finally call new_obj.save()
Using the above server, follow these steps:
- Perform a POST request to the create endpoint
- Perform a PUT request to the update endpoint using the ID returned in step 1.
- The RevisionIdWasChanged error will be raised in the server.
after reproduce i found the problem cause by step 2 (i got the same error yesterday) you pass the id from step 1, the object in endpoint
update
will generate an new object with the same id, then you callsave()
cause the error, we could find_one by id first, and then usenew_obj = obj.model_copy(update=foo.model_dump(exclude={"id"}))
to update model, finally callnew_obj.save()
Doing it like this would not provide any guarantees that your data wasn't changed since you retrieved it, however. If you always get a fresh revision_id
when updating an object, you might as well not use the revision_id
. The way it should work is that you supply the revision_id
that belongs to your version of the object, and the object itself, to your update endpoint. The save
method will then compare the revisions between your supplied one, and the one currently stored in the database, to determine whether or not the object has already been changed since you last retrieved it.
Using the above server, follow these steps:
- Perform a POST request to the create endpoint
- Perform a PUT request to the update endpoint using the ID returned in step 1.
- The RevisionIdWasChanged error will be raised in the server.
after reproduce i found the problem cause by step 2 (i got the same error yesterday) you pass the id from step 1, the object in endpoint
update
will generate an new object with the same id, then you callsave()
cause the error, we could find_one by id first, and then usenew_obj = obj.model_copy(update=foo.model_dump(exclude={"id"}))
to update model, finally callnew_obj.save()
Doing it like this would not provide any guarantees that your data wasn't changed since you retrieved it, however. If you always get a fresh
revision_id
when updating an object, you might as well not use therevision_id
. The way it should work is that you supply therevision_id
that belongs to your version of the object, and the object itself, to your update endpoint. Thesave
method will then compare the revisions between your supplied one, and the one currently stored in the database, to determine whether or not the object has already been changed since you last retrieved it.
Thanks for the clarification. Sorry, I must have misunderstood your question.
Hi just to pick up on this. Not sure if this makes a difference but we recently migrated to Pydantic V2.
We are experiencing a somewhat similar issue. Though in our case this is being thrown after a pymongo DuplicateKeyError has been raised. Here small example:
try:
await m.update(
{
"$set": update.model_dump(
exclude={"id", "_id"} # this does not solve the problem
)
},
upsert=False,
)
except DuplicateKeyError as e:
raise ManufacturersDuplicateKeyException(
description=str(e.details)
) from e
We purposefully created a duplicate key error but instead of just throwing the duplicate key error it says the following:
raise DuplicateKeyError(errmsg, code, response, max_wire_version)
...
During handling of the above exception, another exception occurred:
...
raise RevisionIdWasChanged
Resulting in a 500 error instead of a duplicate key error.
Hi just to pick up on this. Not sure if this makes a difference but we recently migrated to Pydantic V2.
We are experiencing a somewhat similar issue. Though in our case this is being thrown after a pymongo DuplicateKeyError has been raised. Here small example: ...
We purposefully created a duplicate key error but instead of just throwing the duplicate key error it says the following: ... Resulting in a 500 error instead of a duplicate key error.
I the issue highlighted by @Sylver11 seems to be caused by this block of code https://github.com/roman-right/beanie/blob/main/beanie/odm/documents.py#L665
try:
result = await self.find_one(find_query).update(
*arguments,
session=session,
response_type=UpdateResponse.NEW_DOCUMENT,
bulk_writer=bulk_writer,
**pymongo_kwargs,
)
except DuplicateKeyError:
raise RevisionIdWasChanged
if bulk_writer is None:
if use_revision_id and not ignore_revision and result is None:
raise RevisionIdWasChanged
merge_models(self, result)
@roman-right It appears that RevisionIdWasChanged
is being raised immediately following a DuplicateKeyError
, which may not be the intended behavior in this context. This is confusing since a DuplicateKeyError
typically signifies a unique constraint violation and shouldn't necessarily be only linked to a revision ID change.
Hi! Sorry for the late reply. The problem in the original example is that the revision id was not provided. It is an interesting situation, as I was asked many times to not provide revision_id and in response body and in the api doc of the endpoint.
I'll think how I can implement both. Maybe with an additional option in the Settings inner class.
BTW for now you can try this approach to return revisions:
from uuid import UUID
import uvicorn
from pydantic import BaseModel
from beanie import Document, init_beanie, PydanticObjectId
from fastapi import FastAPI
from motor.motor_asyncio import AsyncIOMotorClient
class Foo(Document):
class Settings:
use_revision = True
name = "foos"
bar: str
class ResponseModel(BaseModel):
id: PydanticObjectId
bar: str
revision_id: UUID
app: FastAPI = FastAPI()
@app.post("/create")
async def create() -> ResponseModel:
foo = Foo(bar="bar")
result = await foo.insert()
return result
@app.put("/update")
async def update(foo: Foo) -> None:
result = await foo.save() # <- this always throws RevisionIdWasChanged
return None
@app.on_event("startup")
async def startup():
app.mongodb_client = AsyncIOMotorClient("mongodb://beanie:beanie@localhost:27017/")
app.mongodb = app.mongodb_client["foos"]
await init_beanie(database=app.mongodb, document_models=[Foo])
if __name__ == "__main__":
uvicorn.run(
app,
host="127.0.0.1",
port=8000,
)
If you still face revision id problem, please try this PR: https://github.com/roman-right/beanie/pull/797
Hi,
The snippet you provided gives me the same error when I try to create and then edit an object. This occurs both when I omit and when I provide the revision_id
in the JSON body pf the update request.
On the plus side, I am at least able to retrieve the revision_id from the GET request now, so that at least is progress.
I will have a look at the PR and see if I can get it to work with those changes.
Edit:
It is an interesting situation, as I was asked many times to not provide revision_id and in response body and in the api doc of the endpoint.
I would consider it as something you should return if the use_revision
flag is set to True
, where anyone can still optionally remove it from the server output by excluding the field, but that's just my 2 cents.
Apologies. After updating Beanie to the most recent version, I automatically receive the revision_id
in the response as requested above. If I send that revision_id
to the update endpoint, the update works. The issue is now resolved for me, thank you.
After some more testing, the issue reappeared for me. I must've already applied some changes from your PR when I mentioned that it was working as expected last week. Just wanted to mention that the changes in that PR are what did it, not the update to 1.23.6.
Do you have sight on when you will release a version with the PR merged?
Hi! It will be published today.
Describe the bug I am trying to build a simple CRUD application using FastAPI and beanie, with the setting "use_revision" enabled on the model that I am using for this app. However, it seems that I am unable to update items in the database as the RevisionIdWasChanged error is always raised on calling
.save()
.To Reproduce
Using the above server, follow these steps:
The body used for step 2 is the example body provided by the doc generation of FastAPI ("localhost:8000/docs"):
As a separate question; I would expect
version_id
to be part of the Model that is used for doc generation, but the field is marked ashidden
. How are we supposed to check if a document has changed since it was retrieved, if the user does not send the revision_id for the object it was editing?Even with the following body, the request still fails with RevisionIdWasChanged:
where
_id
andrevision_id
were copied directly from the database.Expected behavior I would expect to be able to call the
save
method on an object that has not been changed since retrieving it. Furthermore, I would expectrevision_id
to be part of the expected body type generated when using theuse_revision = True
statement. Lastly, I would expectfoo.save()
to create a document with a filled inrevision_id
field: however, it seems to create document without setting that field (I need to usefoo.insert()
to actually generate arevision_id
.Additional context This is using beanie with version 1.22.6, fastAPI version 0.103.1, mongodb version 7.
I recognise that I may be using this
use_revision
parameter in the wrong way, but the documentation on it is very sparse and my interpretation seems intuitive for a CRUD application.