fix(base): remove unreachable msgspec.Struct annotation mapping - #775
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #775 +/- ##
==========================================
+ Coverage 82.43% 82.45% +0.02%
==========================================
Files 105 105
Lines 9039 9039
Branches 1219 1219
==========================================
+ Hits 7451 7453 +2
+ Misses 1262 1260 -2
Partials 326 326 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Follow-up to litestar-org#771 for the same class of dead configuration. `create_registry()` seeded `type_annotation_map` with `Struct: JsonB`. SQLAlchemy resolves `type_annotation_map` through an exact/`__mro__` dictionary lookup that rejects supertype matches (`_resolve_for_python_type` returns `None` unless `python_type is matched_on_flattened`). A base `Struct` key is found in a concrete struct's `__mro__` but then rejected, so the entry only ever matched the literal `Mapped[Struct]` annotation and never a user-defined subclass -- the only realistic usage. Like the `DataclassProtocol` entry removed in litestar-org#771, it was misleading dead configuration. Remove the entry (and its now-unused msgspec import). Users who want a struct column register the concrete type via `custom_annotation_map={MyStruct: JsonB}`, which lands as an exact key and resolves. Adds regression tests guarding against reintroducing the `Struct` key and documenting the supported concrete-type registration path. Closes litestar-org#772
eab4246 to
b713410
Compare
There was a problem hiding this comment.
Pull request overview
This PR removes an ineffective default msgspec.Struct -> JsonB mapping from create_registry() because SQLAlchemy’s type_annotation_map resolution rejects supertype matches, making the base Struct key effectively unusable for real-world Struct subclasses. It also adds regression tests to prevent the mapping from being reintroduced and to validate the supported explicit concrete-type registration path.
Changes:
- Remove the unreachable
msgspec.Structentry (and optional import) fromadvanced_alchemy.base.create_registry(). - Add a regression test asserting
Structis not present in the default registry’stype_annotation_map. - Add a test confirming
Structsubclasses resolve when explicitly registered viacustom_annotation_map.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
advanced_alchemy/base.py |
Removes the dead optional msgspec.Struct default mapping from create_registry() to avoid misleading configuration. |
tests/unit/test_base.py |
Adds regression coverage to prevent reintroducing the Struct key and verifies the supported explicit concrete mapping behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
cofin
left a comment
There was a problem hiding this comment.
LGTM - while this is technically breaking, this and the DictProtocol were never actually working as intended. I think it's safe to release now.
Summary
Closes #772. Follow-up to #477 / #771.
create_registry()seedstype_annotation_mapwithStruct: JsonB(msgspec) so struct-typed columns are stored asJsonB. Like theDataclassProtocolentry removed in #771, this entry only ever matches an exactMapped[Struct]annotation — never a concrete subclass, which is the only realistic usage.Root cause — SQLAlchemy resolves
type_annotation_mapby walking the concrete type's__mro__, then rejecting supertype matches inTypeEngine._resolve_for_python_type:MyStruct.__mro__containsStruct, so the key is found, but becauseMyStruct is not Structthe match is rejected. SoStruct: JsonBis effectively dead for the common case of user-defined structs.MCVE (before this change)
Changes
Struct: JsonBentry and its now-unused msgspec import fromadvanced_alchemy/base.py.Structkey, and a test of the supported concrete-registration path.Compatibility
This changes the narrow case where a model is annotated literally as
Mapped[Struct](the msgspec base class); that exact annotation previously selectedJsonB. Concrete annotations such asMapped[MyStruct]did not resolve through this entry before this change and continue to require an explicit concrete mapping.Supported replacement
Notes
This mirrors the direction taken for
DataclassProtocolin #771, whose PR body explicitly flagged the identicalStructlimitation as out of scope. The regression test uses a directStruct not in type_annotation_mapassertion rather than #771's_is_protocolguard, sincemsgspec.Structis a concrete base class, not a Protocol.Testing
uv run pytest tests/unit/test_base.py— all passuv run ruff check— cleanuv run mypy advanced_alchemy/base.py— clean📚 Documentation preview: https://litestar-org.github.io/advanced-alchemy-docs-preview/775