Keep the caller's foreign key, timestamp and column values on construction - #24
Merged
Merged
Conversation
…ction Four places where an instance quietly disagreed with the row it writes, all found while porting a large application onto yara-orm. - ForeignKeyFieldInstance overrode to_db but not to_python_value, so `create(parent_id=str(pk))` bound correctly yet left the raw str on the instance: `child.parent_id == parent.id` was False until the row was re-fetched. The override mirrors to_db and enrols the `<name>_id` column in MetaInfo.coerced_fields, so __init__ normalises it like every other coerced column. - Model.__init__ applied relation objects after the field loop and unconditionally, so a relation passed alongside an explicit `<name>_id` silently replaced the id that was asked for — exactly what a factory does, since a SubFactory declaration is evaluated even when the caller passes the id. The explicit id now wins (by name or db-column alias), and a relation object whose pk disagrees with it is no longer cached as the prefetched relation, so `await obj.<rel>` cannot hand back an instance the column does not point at. - auto_now_add overwrote a caller-supplied stamp on insert, so backdated fixtures, imports and backfills landed at the current time. It now fills the column only when nothing was set; auto_now still always stamps, and so does a field declaring both. - Model had no __iter__, so `dict(instance)` and consumers that walk an object as pairs (pydantic's from_attributes, serializers, factories) could not take a model directly. Instances now yield `(name, value)` for the columns they carry, in declaration order, skipping columns absent under only()/defer() and private attributes, followed by any extras kept under `Meta.extra_kwargs = "store"`. Tests cover each behaviour and its counter-case; the relations, models-and-fields, querying and API-reference docs describe the precedence rules.
CI installs the latest ty, which now flags two spots that 0.0.57 accepted: - `TransactionWrapper.__aexit__` was annotated `-> bool`, which the newer checker reads as "may suppress the exception", so `atomic()`'s wrapper could fall through and implicitly return None. It always returns False, so annotate it `Literal[False]`. - `_natural_key` narrowed on `isinstance(values, dict)` and needed a `ty: ignore` on the old checker that the new one reports as unused. Narrowing on `Model` first needs no suppression under either version.
The new tests sat in test_model_extras, whose module schema carries a raw CHECK (age >= 0) that Oracle cannot create against its quoted "age" column; every db test in that module is on the Oracle skip list for that reason, and the new ones — not on the list — were the first to reach generate_schemas there (ORA-00904: "AGE": invalid identifier). Move them to tests/test_construction_semantics.py with their own models so the behaviour is exercised on all six backends instead of skipped.
…w_add honours a supplied stamp, Model.__iter__, bulk_update bind-param clamp
Three paths the new tests left untaken, each a real precedence case: - `<relation>=None` alongside an explicit `<name>_id` keeps the id (the relation does not clear it). - A raw pk under the relation name alongside an explicit `<name>_id` loses to the explicit id. - Both coercion directions on a foreign key pass a value through unchanged while the target model is still unregistered.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Four places where a constructed instance quietly disagreed with the row it writes. All four surfaced while porting a large application onto yara-orm, and each one currently needs a shim in application code.
What changes
create(parent_id=str(pk))bound correctly but left the rawstron the instance, sochild.parent_id == parent.idwasFalseuntil re-fetchForeignKeyFieldInstance.to_python_valuemirrorsto_db, and the<name>_idcolumn joinsMetaInfo.coerced_fieldsso__init__normalises it like every other coerced column<name>_idsilently replaced that id (relations were applied last, unconditionally)await obj.<rel>cannot return a row the column does not point atauto_now_addoverwrote a caller-supplied stamp, so backdated fixtures, imports and backfills landed atnow()auto_now_addfills the column only when nothing was set.auto_nowstill always stamps, and so does a field declaring bothModelwas not iterable, sodict(instance)and pair-walking consumers (pydanticfrom_attributes, serializers, factories) could not take a model directlyModel.__iter__yields(name, value)for the columns the instance carries, in declaration order — skipping columns absent underonly()/defer()and private attributes, then any extras kept underMeta.extra_kwargs = "store"The relation precedence matters most for factories: a
SubFactorydeclaration is evaluated even when the caller also passesauthor_id=, so the old order made the explicitly requested id unreachable.Compatibility
Behavioural changes, all in the direction of honouring what the caller passed:
auto_now_addvalue keeps it instead of being restamped.<relation>=objtogether with<relation>_id=xnow storesx, notobj.pk. Passing only one of the two is unchanged.iter(instance)raisingTypeErrorwould change behaviour.FK columns now go through
to_python_valueon the create path (a cached lookup of the target pk field); row hydration builds instances through__new__and its decode plan, so reads are untouched.Tests
Each behaviour and its counter-case, in
tests/test_construction_semantics.py(its own module so it runs on Oracle too —test_model_extras's schema is on the Oracle skip list) andtests/test_fields.py: explicit id wins, mismatched object not cached, relation-only construction unchanged, explicitcreated_atpreserved, unsetcreated_atstamped,auto_nowstamped even when supplied, iteration including extras, deferred columns skipped.Full suite on SQLite + PostgreSQL: 1975 passed, 26 skipped. The
test_concurrency.py/test_mt_concurrency.pypool tests are excluded from that run — this box is at itsmax_connectionsceiling and they fail identically on released 1.15.0.ruff check,ruff format --checkandty checkare clean.Docs: relation precedence in the relations guide, the
auto_now_addrule in models-and-fields,dict(instance)in the querying guide and the API reference.