Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DTO Factory: piccolo implementation #1533

Closed
peterschutt opened this issue Apr 18, 2023 · 9 comments · Fixed by #1896
Closed

DTO Factory: piccolo implementation #1533

peterschutt opened this issue Apr 18, 2023 · 9 comments · Fixed by #1896
Assignees
Labels
Enhancement This is a new feature or request
Milestone

Comments

@peterschutt
Copy link
Contributor

peterschutt commented Apr 18, 2023

Fund with Polar
@peterschutt peterschutt added the Enhancement This is a new feature or request label Apr 18, 2023
@peterschutt peterschutt added this to the 2.0 milestone Apr 18, 2023
@peterschutt peterschutt self-assigned this Apr 18, 2023
@peterschutt peterschutt modified the milestones: 2.0, 2.0-beta Apr 18, 2023
peterschutt added a commit that referenced this issue Apr 20, 2023
To be reimplemented as DTOs/DTO-based serialization plugins (#1533 & #1555)
peterschutt added a commit that referenced this issue Apr 20, 2023
Piccolo and Tortoise docs to be re-added in #1533 & #1555.
peterschutt added a commit that referenced this issue Apr 21, 2023
* Transfer implementation from `dto-refactor` branch.

* Transfer remaining DTO implementation and tests.

* SQLAlchemy contrib dto factory implementation.

* Add `dto_field` function to `dto.factory` namespace.

* Replace `ParsedType.from_annotation()` with custom `__init__()` method.

This allows us to enforce how invariants are calculated and set on the type
from the annotation.

* `on_registration()` receives `ParsedType`.

This allows the dto type to leverage the logic on the `ParsedType` object.

* Use `ParsedParameter`/`ParsedType` as basis for all type inference.

* Use `ParsedType` for `build_struct_from_model()` util.

* Add 3.10+ test for `UnionType` union.

* Fix building sqlalchemy model with optional scalar relationship and `None` value.

* Add registry mapped classes to namespace for forward ref resolution.

This means that related classes can remain inside `if TYPE_CHECKING` block in
a module where they are declared as a relationship to another orm class.

* Fix optional nested model conversion to struct.

* Fix inconsistent parsing of unix timestamp between pydantic and cattrs.

Pydantic timestamp parsed as date returns UTC date, while cattrs implementation was returning local date.

Correct issue in tests where UTC date were compared to local dates.

Closes #1491

* Makes struct conversion utilities private.

* Initial support for multiple config objects per DTO type.

* Use `ParsedType` for `AbstractDTOFactory` type arg handling.

* Narrowing `AbstractDTOType` with a union more complex than `Optional[T]` not supported.. yet.

* Single DTO type supports multiple handler/annotations combinations.

This makes a single `AbstractDTOFactory` type much more versatile, making it
able to handle both scalar and collection annotations of the narrowed type.

Factory `on_registration()` callback receives the handler, and the annotation
it should represent and builds a dto backend for each annotation and config
that it encounters during route registration.

We also simplify the logic by being more restrictive, throwing errors for
handlers annotated with `AbstractDTOFactory` subtypes, non-homogeneous
collection type annotations, and being narrowed with union types. All things
that we may choose to support, but best to wait for someone to come knocking
with a concrete use-case.

* Refactor `DTOInterface`.

This PR includes refactorings to better integrate DTO encoding with existing
serialization patterns, and modifications to the interface to better support
integration with websocket handlers.

# DTOInterface constructor methods

`from_connection()` reverted back to `from_bytes()`, and receives both the
already awaited raw data from the payload, and the connection instance.

Now that `from_bytes()` receives the raw payload data, already extracted from
the connection, it no longer needs to be an async method, improving symmetry
with the `from_data()` constructor method.

`from_data()` now also receives the connection instance, which further
improves symmetry between the two constructor methods.

# Data extraction methods

The `to_encodable_type()` method no longer receives the connection (this isn't
required any more as the constructor methods receive it, and so it can be
stored on the instance to be accessed in this method if it is required by an
implementation).

# serialization.py

An encoding hook has been added for serialization of DTO instances, so DTO
instances can be processed through the usual channel. This wraps any raw
bytes returned from the DTO in `msgspec.Raw`, which nicely resolves an earlier
issue encountered when trying to build this pattern.

These changes are based on discussion in discord development channel on April
14.

* Incorporates changes from DTOInterface refactoring.

* Handler `on_registration()` hooks receive app.

* Test client helper default sig backend consistent with `Litestar`.

* Alt SerializationPluginProtocol leveraging the DTO factory.

If annotated type is supported by the plugin, and no DTO is otherwise defined,
the plugin creates an `AbstractDTOFactory` type for the model type and
assigns it to the handler.

Only one DTO type is generated per model type, which is cached by the plugin
and used anytime that model is encountered by the plugin.

This is intended to replace `SerializationPluginProtocol`, however, I've
implemented it using the name `DTOSerializationPluginProtocol` and left all
the other plugin implementation handling in place in order to ease initial
reviews of the implementation.

* A test to demonstrate an undesired behavior of the current serialization plugin approach.

* Fixes `FromClause` is not defined error.

#1503 added `__table__: FromClause` to the contrib bases which couldn't be
resolved.

* Decouple `AbstractDTOFactory` from `AbstractDTOBackend`.

There is no need to have a required backend type for each factory type. This
allows us to pick the backend type that best suits our needs per handler,
particularly depending on the media type supported by the handler, and whether
this data can be parsed into the DTO directly from bytes, or not.

* Refactor msgspec backend module into package.

* Adds pydantic dto factory backend.

* Adds `decode_media_type()` serialization utility.

* Add tests for `decode_media_type()`.

* `DTOInterface.on_registration()` receives `dto_for` parameter.

This is a literal `"body"` or `"response"` string, to be used for
discriminating functionality. For example, inferring if the backend model
should exclude read-only model properties, or inspecting whether the data
param has an explicit media type set.

* `DTOInterface.on_registration()` no longer receives `parsed_type`.

Now that the method receives `dto_for`, the handler type can be collected from
`route_handler.parsed_fn_signature`.

Replaces`Purpose` enum with `dto_for` param.

* Removes `dto_for` from `DTOConfig`.

This means we don't support assigning a DTO type to a handler that has
different configuration for "data" and "return" applications. If users need
that functionality, they should create a 2nd DTO type configured for "return"
and assign it to the `return_dto` layer param.

* Remove unused import.

* Removes `dto.factory.field.Purpose`. (#1535)

No longer used but was not removed.

Closes #1527

* Reverts to a single config object per dto factory type.

Closes #1521

* Remove utility methods from `AbstractDTOFactory`.

Closes #1522

* Refactor `DTOInterface` - a better pattern for websockets.

Based on discussion in #1518.

This pattern emphasises and supports that the lifespan of a DTO instance
should be bound to the lifespan of the connection.

Any connection-based logic can be performed once, and the DTO used to parse
and serialize as often as necessary for the life of the connection.

* Dto factory form data (#1511)

* Removes SQLAlchemy v1 plugin.

* Removes Piccolo and Tortoise v1 style serialization plugins.

To be reimplemented as DTOs/DTO-based serialization plugins (#1533 & #1555)

* Remove `SerializationPluginProtocol`.

To be superseded by the DTO-based serialization plugin protocol model.

* Rename `DTOSerializationPluginProtocol` to `SerializationPluginProtocol`

* Refactor SQLAlchemy contrib package.

Adds a `plugins` sub-package.

* Remove obsoleted docs.

Piccolo and Tortoise docs to be re-added in #1533 & #1555.

* Removes old SQLAlchemy plugin docs.

* New SQAlchemy contrib reference docs structure.

* WIP - SQLAlchemy plugin tutorial docs

* Fixes for doc references.

* Linting fixes.

* Apply suggestions from code review

* Fix missing import error.

* Dto interface openapi (#1507)

* Adds `DTOInterface.create_openapi_schema()` method.

* Use `dto_for` param for `DTOInterface.create_openapi_schema()`.

* Tests for backend openapi schema generation.

* Test DTOs are called for schemas where available.

* Test for default DTOInterface openapi schema.

* `on_registration()` hook receives `HandlerContext` object.

This is to make the interface more resilient to changes over time, as it is easier to
add attributes to the context object in a non-breaking fashion, compared to changing
function arguments.

* Removes `DTOConfig.__hash__()` implementation.

* Remove check for DTO as handler annotation.

This is no longer a feature supported by the framework.

* Fix where subclass unnecessarily created.

* Adds `BackendContext` for passing data to DTO backends.

* Removes `AbstractDTOBackend.from_field_definitions()` classmethod.

Backend instances are instantiated directly with `BackendContext` object.

* Model type passed to backed via context.

* Add tests.

* Remove unused type.

* Add nested collection to backend tests.

* Test for `ParsedType` equality.

* Test for optional form data with empty form body.

* Removes the `max_nested_recursion` config.

The `max_nested_depth` parameter will put a limit on self recursive models anyway,
and so I'd rather a use-case pop up for this before we bother with an implementation
for it.

* Add optional model attribute for tests.

* Address sonar smells.

* Add test.

* Adds `dto.interface.ConnectionContext`.

This is received by the dto on instantiation and carries pertinent information about
the current connection.

Both `HandlerContext` and `ConnectionContext` receive `handler_id` instead of
`route_handler` and `connection` respectively.

* Fix context object typing.

Makes all attributes `Final`.

* Fix references.

* Includes collection and additional scalar annotated handler in test.

* Fix plugin DTO assignment order.
@dantownsend
Copy link

It would be nice to get this added - maybe we can help in some way.

We recently added support for litestar to Piccolo (replacing starlite), and came across this in our integration tests. For now, we'll version pin to litestar==2.0.0a3.

@peterschutt
Copy link
Contributor Author

Hey @dantownsend - thanks for reaching out.

I've only implemented support for dataclasses and sqlalchemy so far as PoC. While we are still in alpha/beta I'm keen to make another pass over the internals of the DTO factory ABC before building implementations for all of the remaining libraries that we had support for in v1.

I'm away on a family holiday for the next week or so and will launch into this stuff 2nd week of may.

Any help along the way would be appreciated, am I ok to @ you in here if I have any questions?

@dantownsend
Copy link

@peterschutt Cool, happy to help! Thanks for the update.

@peterschutt peterschutt modified the milestones: 2.0-beta, 2.0 May 11, 2023
@Goldziher Goldziher self-assigned this Jul 1, 2023
@Goldziher Goldziher linked a pull request Jul 1, 2023 that will close this issue
@Goldziher
Copy link
Contributor

@peterschutt Cool, happy to help! Thanks for the update.

Hi @dantownsend - i added a PR with a DTO factory for Piccolo. Can you perhaps take a look? Additionally, it would be great if you or one of your collaborators would contribute improvements to this. I did not set out to handle nested models and relations, and that would be a great addition.

@dantownsend
Copy link

@Goldziher It looks great - thank you! I'll have a play around with it. @sinisaos in case you want to give it a go.

@sinisaos
Copy link
Contributor

sinisaos commented Jul 3, 2023

@dantownsend Just finished trying out PiccoloDTO and everything is working great (great job @Goldziher). I had a little problem with the partial update and then I found a very good documentation with an example that helped me to get everything work. @Goldziher I see you write the documentation for PiccoloDTO and here is a full working example for PiccoloDTO that you can use

import typing as t

from litestar import Litestar, MediaType, asgi, delete, get, patch, post
from litestar.contrib.jinja import JinjaTemplateEngine
from litestar.contrib.piccolo.dto import PiccoloDTO
from litestar.dto.factory import DTOConfig, DTOData
from litestar.exceptions import NotFoundException
from litestar.static_files import StaticFilesConfig
from litestar.template import TemplateConfig
from litestar.types import Receive, Scope, Send
from piccolo.engine import engine_finder
from piccolo_admin.endpoints import create_admin

from home.endpoints import home
from home.piccolo_app import APP_CONFIG
from home.tables import Task


# mounting Piccolo Admin
@asgi("/admin/", is_mount=True)
async def admin(scope: "Scope", receive: "Receive", send: "Send") -> None:
    await create_admin(tables=APP_CONFIG.table_classes)(scope, receive, send)


class PatchDTO(PiccoloDTO[Task]):
    """Don't allow client to set the id, and allow partial updates."""

    config = DTOConfig(exclude={"id"}, partial=True)


@get(
    "/tasks",
    return_dto=PiccoloDTO[Task],
    media_type=MediaType.JSON,
    tags=["Task"],
)
async def tasks() -> t.List[Task]:
    return await Task.select().order_by(Task.id, ascending=False)


@post(
    "/tasks",
    dto=PiccoloDTO[Task],
    return_dto=PiccoloDTO[Task],
    media_type=MediaType.JSON,
    tags=["Task"],
)
async def create_task(data: Task) -> Task:
    await data.save()
    await data.refresh()
    return data


@patch(
    "/tasks/{task_id:int}",
    dto=PatchDTO,
    return_dto=PiccoloDTO[Task],
    media_type=MediaType.JSON,
    tags=["Task"],
)
async def update_task(task_id: int, data: DTOData[Task]) -> Task:
    task = await Task.objects().get(Task.id == task_id)
    if not task:
        raise NotFoundException("Task does not exist")
    result = data.update_instance(task)
    await result.save()
    return result


@delete("/tasks/{task_id:int}", tags=["Task"])
async def delete_task(task_id: int) -> None:
    task = await Task.objects().get(Task.id == task_id)
    if not task:
        raise NotFoundException("Task does not exist")
    await task.remove()


async def open_database_connection_pool():
    try:
        engine = engine_finder()
        await engine.start_connection_pool()
    except Exception:
        print("Unable to connect to the database")


async def close_database_connection_pool():
    try:
        engine = engine_finder()
        await engine.close_connection_pool()
    except Exception:
        print("Unable to connect to the database")


app = Litestar(
    route_handlers=[
        admin,
        home,
        tasks,
        create_task,
        delete_task,
        update_task,
    ],
    template_config=TemplateConfig(
        directory="home/templates", engine=JinjaTemplateEngine
    ),
    static_files_config=[
        StaticFilesConfig(directories=["static"], path="/static/"),
    ],
    on_startup=[open_database_connection_pool],
    on_shutdown=[close_database_connection_pool],
)

Sorry for long comment.

@Goldziher
Copy link
Contributor

@dantownsend Just finished trying out PiccoloDTO and everything is working great (great job @Goldziher). I had a little problem with the partial update and then I found a very good documentation with an example that helped me to get everything work. @Goldziher I see you write the documentation for PiccoloDTO and here is a full working example for PiccoloDTO that you can use

import typing as t

from litestar import Litestar, MediaType, asgi, delete, get, patch, post
from litestar.contrib.jinja import JinjaTemplateEngine
from litestar.contrib.piccolo.dto import PiccoloDTO
from litestar.dto.factory import DTOConfig, DTOData
from litestar.exceptions import NotFoundException
from litestar.static_files import StaticFilesConfig
from litestar.template import TemplateConfig
from litestar.types import Receive, Scope, Send
from piccolo.engine import engine_finder
from piccolo_admin.endpoints import create_admin

from home.endpoints import home
from home.piccolo_app import APP_CONFIG
from home.tables import Task


# mounting Piccolo Admin
@asgi("/admin/", is_mount=True)
async def admin(scope: "Scope", receive: "Receive", send: "Send") -> None:
    await create_admin(tables=APP_CONFIG.table_classes)(scope, receive, send)


class PatchDTO(PiccoloDTO[Task]):
    """Don't allow client to set the id, and allow partial updates."""

    config = DTOConfig(exclude={"id"}, partial=True)


@get(
    "/tasks",
    return_dto=PiccoloDTO[Task],
    media_type=MediaType.JSON,
    tags=["Task"],
)
async def tasks() -> t.List[Task]:
    return await Task.select().order_by(Task.id, ascending=False)


@post(
    "/tasks",
    dto=PiccoloDTO[Task],
    return_dto=PiccoloDTO[Task],
    media_type=MediaType.JSON,
    tags=["Task"],
)
async def create_task(data: Task) -> Task:
    await data.save()
    await data.refresh()
    return data


@patch(
    "/tasks/{task_id:int}",
    dto=PatchDTO,
    return_dto=PiccoloDTO[Task],
    media_type=MediaType.JSON,
    tags=["Task"],
)
async def update_task(task_id: int, data: DTOData[Task]) -> Task:
    task = await Task.objects().get(Task.id == task_id)
    if not task:
        raise NotFoundException("Task does not exist")
    result = data.update_instance(task)
    await result.save()
    return result


@delete("/tasks/{task_id:int}", tags=["Task"])
async def delete_task(task_id: int) -> None:
    task = await Task.objects().get(Task.id == task_id)
    if not task:
        raise NotFoundException("Task does not exist")
    await task.remove()


async def open_database_connection_pool():
    try:
        engine = engine_finder()
        await engine.start_connection_pool()
    except Exception:
        print("Unable to connect to the database")


async def close_database_connection_pool():
    try:
        engine = engine_finder()
        await engine.close_connection_pool()
    except Exception:
        print("Unable to connect to the database")


app = Litestar(
    route_handlers=[
        admin,
        home,
        tasks,
        create_task,
        delete_task,
        update_task,
    ],
    template_config=TemplateConfig(
        directory="home/templates", engine=JinjaTemplateEngine
    ),
    static_files_config=[
        StaticFilesConfig(directories=["static"], path="/static/"),
    ],
    on_startup=[open_database_connection_pool],
    on_shutdown=[close_database_connection_pool],
)

Sorry for long comment.

cheers, would you care to add a PR to the docs?

@Goldziher
Copy link
Contributor

@dantownsend and @sinisaos - if you guys would like to add a followup PR that improves the DTO by supporting recursive relations etc. that would be great. We have very strong support for SQLA, because we have the knowledge to do this. I do not have the knowledge how to handle Piccolo on this level, but it would be great to have.

@sinisaos
Copy link
Contributor

sinisaos commented Jul 4, 2023

@Goldziher Sorry for the slow response. I made a PR #1916 for the PiccoloDTO docs and I hope it's good enough. If not, feel free to change it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement This is a new feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants