✨ Support Annotated approach for Relationship attributes#1192
✨ Support Annotated approach for Relationship attributes#1192bolu61 wants to merge 18 commits intofastapi:mainfrom
Annotated approach for Relationship attributes#1192Conversation
YuriiMotov
left a comment
There was a problem hiding this comment.
@bolu61, thanks for working on this!
So, this PR fixes the following code example:
class Foo(SQLModel, table=True):
id: Annotated[int | None, Field(primary_key=True)] = None
uuid: Annotated[UUID, Field(unique=True)]
class Bar(SQLModel, table=True):
id: Annotated[int | None, Field(primary_key=True)] = None
uuid: Annotated[UUID, Field(unique=True)]
foo_id: Annotated[int, Field(foreign_key="foo.id")]
foo: Annotated[Foo, Relationship()]For now last line (with Relationship()) doesn't work (ValueError: <class '__main__.Foo'> has no matching SQLAlchemy type) and this PR fixes that.
I think this is not the same that was initially proposed in #229. The idea there was to support passing multiple annotations like:
class Hero(SQLModel, table=True):
id: Annotated[Optional[int], Field(examples=....), Column(primary_key=True)] = NoneWhere Field can be pydantic.Field.
So, I suggest we unmark this PR as the one that resolves that issue.
As for changes introduced by this PR, I think it goes in line with modern recommendations to use Annotated approach instead parameterizing fields via default value.
I think we should test this approach more and add some automatic tests. Then it will have all chances to be accepted.
|
@YuriiMotov I agree. I'm not too familiar with this codebase. Can you point me to where I can write those tests? |
You can create an annotated version of the code example from "Relationships" section of docs:
|
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as resolved.
This comment was marked as resolved.
📝 Docs previewLast commit 9302e61 at: https://7e0c97b3.sqlmodel.pages.dev |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Annotated approach for Relationship attributes
This comment was marked as resolved.
This comment was marked as resolved.
|
The test that was failing previously is now fixed thanks to #1607. @YuriiMotov : do you want to give this another review? |
YuriiMotov
left a comment
There was a problem hiding this comment.
This needs some more work and testing..
@bolu61, would you like to continue working on this?
|
Ofc. Might take me some time to reread the rest of the code though. |
|
This pull request has a merge conflict that needs to be resolved. |
|
Heads-up: this will be closed in 3 days unless there's new activity. |
- Detect annotated relationships without a default value by iterating over the union of class_dict and original_annotations. - Use elif and unwrap inner Mapped[T] when handling Annotated[Mapped[T], ...] to avoid double-wrapping in Mapped. - Split the relationship/pydantic partition into three single-purpose loops with comments for clarity. - Add tests covering Annotated relationships with default value, without default value, and with Annotated[Mapped[T], ...].
see #229 (comment)
I'm not really sure how Pydantic does it, but here's a potential workaround to the problem.