litestar-org / litestar

Production-ready, Light, Flexible and Extensible ASGI API framework | Effortlessly Build Performant APIs
https://litestar.dev/
MIT License
5.53k stars 378 forks source link

Bug: Using plugin results in DTO given to dto parameter not being used for return #3460

Open ryanolf-tb opened 6 months ago

ryanolf-tb commented 6 months ago

Description

Based on the docs, I expect a DTO given to a handler via the dto parameter to apply to the return value when no return_dto is specified. However, when the SQLALchemyPlugin is used this is not the case.

More generally, it looks like the return_dto resolution order in the BaseRouteHandler puts generated DTOs from serialization plugins ahead of DTOs from explicit dto parameters on the handler. For me this is not the expected behavior based on the documentation, though I can see that one may want to opt-in to the return_dto provided by the plugin while still providing a data DTO, which perhaps is the reason for the current behavior.

It may be that the docs just need an update to clarify? An alternative might be to specify a particular value that can be passed to the return_dto parameter to maintain the current behavior, but to otherwise resolve to dto if it's given.

URL to code causing the issue

No response

MCVE

from typing import Annotated

from advanced_alchemy.extensions.litestar.plugins.init.config.engine import EngineConfig
from litestar import Litestar, get
from litestar.contrib.sqlalchemy.dto import SQLAlchemyDTO
from litestar.contrib.sqlalchemy.plugins import (
    SQLAlchemyPlugin,
    SQLAlchemySyncConfig,
)
from litestar.dto import DTOConfig
from litestar.testing import TestClient
from sqlalchemy.orm import DeclarativeBase, Mapped, mapped_column

class Base(DeclarativeBase):
    pass

class Address(Base):
    __tablename__ = "addresses"

    id: Mapped[int] = mapped_column(primary_key=True)
    street: Mapped[str]
    city: Mapped[str]
    state: Mapped[str]
    zip: Mapped[str]

db_config = SQLAlchemySyncConfig(
    connection_string="sqlite://",
    engine_config=EngineConfig(echo=False),
    create_all=True,
)

AddressDTO = SQLAlchemyDTO[
    Annotated[
        Address,
        DTOConfig(exclude={"zip"}),
    ]
]

@get("/address", dto=AddressDTO, sync_to_thread=False)
def get_address() -> Address:
    return Address(id=1, street="123 Main St", city="Anytown", state="NY", zip="12345")

app_with_plugin = Litestar(
    route_handlers=[get_address],
    plugins=[SQLAlchemyPlugin(config=db_config)],
)

app_without_plugin = Litestar(
    route_handlers=[get_address],
)

with TestClient(app=app_without_plugin) as client:
    if "zip" not in client.get("/address").json():
        print("'zip' is properly excluded without the plugin")

with TestClient(app=app_with_plugin) as client:
    if "zip" in client.get("/address").json():
        print("'zip' is not excluded with the plugin")

Steps to reproduce

1. Run the MCVE
2. Note that 'zip' is not properly excluded from the response JSON when the `SQLALchemyPlugin` is used.

Screenshots

"![SCREENSHOT_DESCRIPTION](SCREENSHOT_LINK.png)"

Logs

'zip' is properly excluded without the plugin
INFO - 2024-05-02 11:27:19,957 - httpx - _client - HTTP Request: GET http://testserver.local/address "HTTP/1.1 200 OK"
'zip' is not excluded with the plugin
INFO - 2024-05-02 11:27:19,968 - httpx - _client - HTTP Request: GET http://testserver.local/address "HTTP/1.1 200 OK"

Litestar Version

2.8.2

Platform


[!NOTE]
While we are open for sponsoring on GitHub Sponsors and OpenCollective, we also utilize Polar.sh to engage in pledge-based sponsorship.

Check out all issues funded or available for funding on our Polar.sh dashboard

  • If you would like to see an issue prioritized, make a pledge towards it!
  • We receive the pledge once the issue is completed & verified
  • This, along with engagement in the community, helps us know which features are a priority to our users.

Fund with Polar

Alc-Alc commented 6 months ago

Not dismissing the behavior, but just pointing this out zip: Mapped[str] = mapped_column(info=dto_field('private')) can be used to ignore it via the plugin

dto_field is litestar.dto.dto_field

ryanolf-tb commented 6 months ago

I think in the typical CRUD use case one would use dto_field. In my example, I can just use return_dto instead of dto. This may not have been brought up before because it is not encountered much in practice, and it's easy to work around if one understands the behavior.