fix: raise clear error when default_factory/default passed to WriteOnlyMapped/DynamicMapped relationship#13289
Conversation
… default passed to WriteOnlyMapped relationship WriteOnlyMapped relationships have no in-memory collection, so default_factory and default are not meaningful. Previously, passing default_factory=list would crash with a cryptic error (e.g., 'LoaderCallableStatus' object is not iterable). Now, declarative_scan() raises a clear ArgumentError suggesting init=False as the correct approach. Added test verifying the expected error message.
CaselIT
left a comment
CaselIT
left a comment
There was a problem hiding this comment.
thanks, left a few suggestions. let's also wait mike input on whatever we should include also dynamic here
There was a problem hiding this comment.
@zzzeek do default / default_factory make sense with dynamic? if I'm not mistaken it should be similar to write only
There was a problem hiding this comment.
dynamic is not exactly the same as write_only but yes, these are descriptors that probably shouldnt be considered within the dataclass regime, unless someone can show us a use case for that
|
|
||
| if is_write_only: | ||
| if ( | ||
| self._attribute_options.dataclasses_default_factory |
There was a problem hiding this comment.
I think we could use a single error, there isn't really a need to duplicate them here.
Something like
WriteOnlyMapped relationship {self._format_as_string(cls, key)} does not support 'default' or 'default_factory' properties. Use 'init=False' to exclude this attribute from init
|
|
||
| self._assertions(A, B, "write_only") | ||
|
|
||
| def test_write_only_default_factory_raises(self, decl_base): |
There was a problem hiding this comment.
would be nice to also test the default
|
Thanks for the review! Addressed both suggestions:
Waiting for Mike's input on the dynamic question. |
| else: | ||
| is_write_only = is_dynamic = False | ||
|
|
||
| if is_write_only: |
There was a problem hiding this comment.
dynamic is not exactly the same as write_only but yes, these are descriptors that probably shouldnt be considered within the dataclass regime, unless someone can show us a use case for that
| f"For relationship " | ||
| f"{self._format_as_string(cls, key)}: " | ||
| "'default' and 'default_factory' are not supported for " | ||
| "WriteOnlyMapped relationships, which have " |
There was a problem hiding this comment.
yeah would mention dynamic here also
| "no in-memory collection. Use 'init=False' " | ||
| "to exclude this attribute from __init__ " | ||
| "and add items after construction via " | ||
| "direct assignment or " |
There was a problem hiding this comment.
this message is a bit wordy. if there's no way to make it more succinct consider linking to errors.rst (see the "code" parameter of the ArgumentError class hierarchy)
There was a problem hiding this comment.
I'd like to move these tests. test_typed_mapping is not specific to dataclasses nor writeonly/dynamic.
these tests should be either in:
test_dc_transforms.py
or
test_dynamic.py
and there should be tests for both the writeonly and dynamic cases
…dback - Single error message mentioning both WriteOnlyMapped and DynamicMapped - Error message: 'WriteOnlyMapped / DynamicMapped relationship does not support default or default_factory properties. Use init=False' - Add tests for DynamicMapped with default_factory and default - Bypass early default rejection in _RelationshipDeclared for write_only/dynamic lazy so the specific error fires
algojogacor
changed the title
3 participants
Description
Fixes: #13227
When
default_factory=listordefaultis passed to aWriteOnlyMappedorDynamicMappedrelationship, it previously caused cryptic crashes (e.g.,TypeError: LoaderCallableStatus object is not iterablein_WriteOnlyAttributeImpl.set()). This happens because these mapped types have no in-memory collection, sodefault_factoryanddefaultare not meaningful.This PR adds an explicit
ArgumentErrorinRelationshipProperty.__init__()that fires whendefault_factoryordefaultis set on a write-only or dynamic relationship, with a clear message:Changes:
lib/sqlalchemy/orm/relationships.py: Added check foris_write_only or is_dynamicwithdataclasses_default_factory/dataclasses_defaultraisingArgumentError. Also bypassed early default rejection in_RelationshipDeclaredfor write_only/dynamic lazy so the specific error fires.test/orm/declarative/test_typed_mapping.py: Added tests for both WriteOnlyMapped and DynamicMapped withdefault_factoryanddefault.Checklist
Fixes: #13227in the commit message