topsport-com-au / starlite-saqlalchemy

Starlite API boilerplate abstraction and utilities.
https://topsport-com-au.github.io/starlite-saqlalchemy/
MIT License
31 stars 8 forks source link

feat(dto): generate dtos with all fields marked as optional #210

Open gazorby opened 1 year ago

gazorby commented 1 year ago

I'm implementing an handler to update some resource where input only contains optional fields, except for the resource identifier.

There are two things that make implementing this a bit cumbersome:

Update DTO

As we need input fields to be optional only, dto fields need to optional too. Currently, dtos can only be generating for read or write purpose, meaning that I need to rewrite a full model for the update.

Proposal: A new update purpose that generate a dto with all fields marked as optional.

to_mapped() set all attributes

Currently, to_mapped() set all attributes from the pydantic instance, included ones with default values. This is good when inserting rows, but may not be desirable when updating them as basically all attributes will be marked as modified by sqlalchemy which will try to update them in database.

Proposal: New exclude_defaults/exclude_unset params on to_mapped() ensuring that only setted attributes from pydantic model are set on sqlalchemy instance.

Use case

Use case using the above proposals:

class User(Base):
    username: Mapped[str] = mapped_column(default="default_username")
    age: Mapped[int] = mapped_column(default=0)

class UserRead(dto.FromMapped[Annotated[User, "read"]]):
    pass

#                                                new dto config "update", makes all field optional    
#                                                vvvvvv
class UserUpdate(dto.FromMapped[Annotated[User, "update"]]):
    id: uuid.UUID

@post("/update")
async def update_user(data: UserUpdate, session: AsyncSession) -> None:

    updated_user = UserUpdate(username="new_username")
    updated_instance = updated_user.to_mapped(exclude_defaults=True)

    assert updated_instance.__dict__["username"] == "new_username"
    assert "age" not in updated_instance.__dict__

    # Only username should be updated
    await UserService(session=session).update(updated_instance)
peterschutt commented 1 year ago

Hey @gazorby - I've done some thinking along these lines too for @patch() route handling.

In your example, what happens if a user sends through '{"id": "...", "age": null}'. Due to the fact that the DTO types everything as optional, the error won't be picked up by validation on the DTO and will instead be raised from the repository when the null value is flushed to the db.

So instead of making every attribute optional, I've thought of defining an UNSET type and making every attribute type a union of its explict type and UNSET, so the generated DTO would be equivalent to:

UnsetType = type("UnsetType", (), {})
UNSET = UnsetType()

class UserUpdate(BaseModel):
  id: uuid.UUID
  username: str | UnsetType = UNSET
  age: int | UnsetType = UNSET

In to_mapped() we can skip setting any attribute on the model that has the UNSET value without worrying about feature flags like exclude_defaults=....

Also, for Annotated[User, "update"] - I'd prefer we use a name that better conveys the meaning, e.g., Annotated[User, "partial"] or Annotated[User, "patch"] etc. Whatever it is, it should behave exactly as "write" except for generating the union types.

Thoughts?

gazorby commented 1 year ago

If a user sends '{"id": "...", "age": null}', it won't lands in sqla model if I use .to_mapped(exclude_defaults=True), (It's what I'm doing with my internal implementation). Setting null is the same as not specifying age field as in both case we exclude the default value, which is None.

I should have given more context about my use case, but using a custom unset type won't work for me actually, because I use DTOs to generate GraphQL input types, using strawberry. A DTO with optional fields will lead to a GraphQL input type like this:

# Only id is required
input UserInput {
    id: ID!
    username: String
    age: Int
}

Which is what I want. As UNSET does not mean anything in GraphQL I think I would have rewritten a model for the input type or maybe wrote some custom conversion logic.

As for the name, I agree with "partial", that is more inline with "read" and "write".

gazorby commented 1 year ago

Actually, I prefer your way design wise, but it does not fit my need well. Also, in my previous example, using exclude_defaults prevent from updating any field to null, if it's an acceptable value, so it's still buggy.

peterschutt commented 1 year ago

Maybe we should do up some POCs and look at the conditions under which they work/don't work. Good chance that there isn't a one-size-fits-all solution and we end up with multiple new features to cover these cases.

Another thing to consider is the openapi that would be generated for these.