Skip to content
This repository has been archived by the owner on Sep 12, 2023. It is now read-only.

DTO refactor, fix reference cycles #147

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

gazorby
Copy link
Collaborator

@gazorby gazorby commented Nov 30, 2022

Hi @peterschutt!

My iteration based on #139 to fix reference cycles leads to another dto refactor.

Improvements

  • Refactor DTO implementation, using a class based design. This introduces a DTOFactory base class that exposes common functionalities used to inspect SQLAlchemy models and may ease the implementation of other validation libraries under something like a dto protocol/interface that would be used by starlite-saqlalchemy. It also encapsulates some global state needed to resolve cycles.
  • Reference cycles are transparently handled, without the need of calling update_forward_refs() on pydantic model
class A(base):
    __tablename__ = "a"

    name: Mapped[str]
    children: Mapped[list["B"] | None] = relationship("B", back_populates="a")

class B(base):
    __tablename__ = "b"

    name: Mapped[str]
    a_id: Mapped[int] = mapped_column(ForeignKey("a.id"))
    a: Mapped["A"] = relationship("A", back_populates="children")

dto_model_child = dto.FromMapped[Annotated[B, "write"]]
# No need to call dto_model_child.update_forward_refs()
dto_child_instance = dto_model_child.parse_obj(
    {"id": 1, "name": "b", "a_id": 1, "a": {"id": 1, "name": "a"}}
)
  • Optional relationships are correctly reflected in dto. This follows PEP 484 which prohibits implicit optional type if None if given as default (which is the case for scalar relationships).
class A(Base):
    b_id: Mapped[int | None] = mapped_column(ForeignKey("b.id"))
    b: Mapped["B" | None] = relationship("B")

# dto.b is an optional field
dto = dto.FromMapped[Annotated[B, "write"]]
  • Model's ForwardRefs are now resolved using registry.mappers (instead of relying solely on vars(module)). It creates a hard dependency between saqlalchemy.db.orm and saqlalchemy.dto.from_mapped but makes inference more reliable in non-trivial scenarios.

@gazorby gazorby changed the title DTO refactor, fix reference cycle DTO refactor, fix reference cycles Nov 30, 2022
@peterschutt
Copy link
Member

Thanks @gazorby I'll review asap!

@gazorby
Copy link
Collaborator Author

gazorby commented Nov 30, 2022

Sorry for the second burst, didn't see that you closed the other PR and I already did some work on it ;)

@peterschutt
Copy link
Member

Just letting you know that I probably won't get too deep into this until early next week as I have a bit on this weekend, but from a quick skim it looks nice and some very interesting ideas!

pre-commit-ci bot and others added 2 commits March 31, 2023 08:34
- Decouple pydantic DTO implementation
- Add support for default callable that accept SQLAlchemy context
- Add support for static mehtod default callable
- Cache generated dto so they can be reused as needed
- Fix generation when relationships use string typed annotation
- Bring back some unit tests
@peterschutt
Copy link
Member

Hey @gazorby - DTOs are getting a big rework in starlite 2.0, so I've been sitting on my heals with this library until the beta is out there so there is some certainty with how it looks.

We could keep the 0-version of the lib compatible with starlite 1.x and make the 1.x version of the lib starlite 2.x perhaps?

@gazorby
Copy link
Collaborator Author

gazorby commented Apr 1, 2023

@peterschutt !

No problem with that, Just want to keep this updated with my latest downstream changes as I need them, so we can choose what we want to keep or ditch. DTO changes for V2 are tracked in litestar-org/litestar/pull/1295 right?

@gazorby
Copy link
Collaborator Author

gazorby commented Apr 1, 2023

Also, do you plan to cover DTO ssaq features in Starlite V2 so we don't need this alternate implementation anymore?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants