model_copy(deep=False) on a persisted table instance shares SQLAlchemy InstanceState — mutations silently lost on commit, no error #2089
Replies: 4 comments
|
Hi @tritsystem, are you still planning to work on this? If not, I’d be interested in taking it up once the maintainers confirm the expected approach. |
|
yes |
|
Thanks for the interest in this, @jagadeepmamidi — while sitting down to work on it I dug a bit further and the root cause turned out to be a little sharper than my original report, so sharing that first in case it changes which fix direction makes the most sense to you.
deep = hero.model_copy(update={"age": 31}, deep=True)
sa_inspect(hero).key == sa_inspect(deep).key
# True — deepcopy faithfully copies InstanceState.key too, so the "new"
# state still claims the SAME identity as the already-persistent original
session.add(deep)
session.commit()
# "succeeds", no exception — but session.new and session.dirty are both
# empty throughout, and the DB row is never touchedThe common root cause for both The tool that already exists in SQLModel and handles this correctly is A few directions this could go, each with different scope/tradeoffs — happy to send a PR for whichever fits best, or if you'd rather take it from here yourselves that's completely understood, just let me know which way (if any) you'd like it to go:
(1) and (2) combine well and are both low-risk if you'd rather ship something quickly while leaving (3)'s behavior change for more discussion. Happy to scope a PR to just one of these, any combination, or none if you'd prefer to handle it internally — whatever's easiest on your end. I have repro scripts for both the |
|
@tiangolo tagging you directly since this touches |
Uh oh!
There was an error while loading. Please reload this page.
First check
Commit to Help
Example Code
Description
Calling
model_copy(deep=False)on atable=Truemodel instance that isalready attached to a
Sessionproduces an object that looks independent(different
id(), mutatingcopy.namedoes not touchhero.nameinPython), but is not independent from SQLAlchemy's point of view: the copy
and the original share the exact same
_sa_instance_state(
InstanceState) object.Because the shared
InstanceState.obj()still resolves tohero, notcopy, everything downstream keys offhero:session.add(copy)does not register a second pending/dirty object —session.newstays empty.session.dirtyshowsheroas the modified object (notcopy).session.commit()succeeds with no exception.Deadpond, notRENAMED) — theedit made through
copyis silently discarded.This is not simple field-aliasing (like a shared mutable list) — it's a
silent lost update: no error is raised anywhere,
copy.namereallydoes read
"RENAMED"right up until commit, and there is no way to tellfrom the copy alone that the edit will not persist.
I'd guess the root cause is that
model_copy()'s shallow copy of__dict__(Pydantic's mechanism) also shallow-copies the private_sa_instance_stateattribute that SQLAlchemy's instrumentation storesthere, instead of giving the copy a fresh
InstanceStatebound to itself.model_copy(deep=True)also shares the same_sa_instance_stateobject(deep-copying a
weakref-bearing SQLAlchemy internal doesn't produce anindependent one either), so
deep=Trueis not a workaround.This seems like a real correctness hazard for any code that treats
model_copy()as "make an independent snapshot I can edit and save" —e.g. an update/PATCH endpoint pattern like:
which silently no-ops instead of updating the row or raising.
Operating System
Windows
Operating System Details
Windows 11
SQLModel Version
0.0.42
Python Version
3.12.10
Additional Context
Happy to open a PR — the most surgical fix I can see is having
model_copy()(or a documented safe pattern) give the copy a freshInstanceStaterather than sharing the original's, but I wanted toconfirm with maintainers whether this is considered a bug in
model_copy()'s interaction with the ORM state, a SQLAlchemy-levellimitation to document, or something to solve with a "use
session.merge/ re-fetch instead" recommendation in the docs.
All reactions