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

feat(dto): more iteration. #2

Closed
wants to merge 4 commits into from

Conversation

peterschutt
Copy link

  • Adds dto.Attrib.pydantic_type and dto.Attrib.validators where pydantic_type allows the dto field type to be explicitly set, and validators allows arbitrary single argument callables to be specified as dto validators for any model attribute.
  • factory() should now handle MappedAsDataclass sqla models.
  • decorator() is for decorating pydantic models, and uses the decorated class as the base class for the model.
  • adds some tests.

I spent a bit of time experimenting. I think best for the decorator to wrap subtypes of BaseModel and we use the decorated class as the base class for the produced dto. Seems to be the simplest approach IMO, but LMK what you think.

Cheers!

- Adds `dto.Attrib.pydantic_type` and `dto.Attrib.validators` where
`pydantic_type` allows the dto field type to be explicitly set, and
`validators` allows arbitrary single argument callables to be specified
as dto validators for any model attribute.
- `factory()` should now handle `MappedAsDataclass` sqla models
- `decorator()` is for decorating pydantic models, and uses the
decorated class as the base class for the model.
- adds some tests.
if isclass(type_) and issubclass(type_, DeclarativeBase):
type_ = factory(f"{name}_{type_.__name__}", type_, purpose=purpose)

fields[key] = (type_, _construct_field_info(elem, purpose))

return create_model( # type:ignore[no-any-return,call-overload]
name,
__module__=getattr(model, "__module__", __name__),
__base__=base,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If base is explictly passed the produced does not inherit MapperBind. We should make sure that MapperBind is part of bases in all cases.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yah, this does need more thought.

The decorated class could inherit from MapperBind instead of BaseModel.

If the decorated class inherits from BaseModel then we can't simply mix in MapperBind as it inherits from BaseModel also.

If the decorated class inherits from MapperBind there is the issue that model needs to be passed as a class kwarg, so the model then gets declared in the decorator signature, and in the class signature on the very next line.. and I was unsure about the ergonomic of that.

I'll take another look with fresh eyes tomorrow.. I experimented with making MapperBind a mixin class, rather than inherit from BaseModel - maybe that's worth revisiting.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think MapperBind should be a mixin rather than a full pydantic model. Both the decorated class and MapperBind should be in bases, so we only want one of them to inherit BaseModel, or none.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got this where I'm pretty happy implementation-wise, does it feel close enough to what you were heading toward with the original PR? This test is a good demo:

def test_dto_decorator() -> None:

    @dto.decorator(Author, dto.Purpose.WRITE)
    class AuthorDTO(BaseModel):
        name: constr(to_upper=True)  # pyright:ignore

        @validator("name")
        def validate_name(cls, val: str) -> str:
            """We're shouting!"""
            return f"{val}!"

        @validator("dob", check_fields=False)
        def validate_dob(cls, val: date) -> date:
            """Off by one."""
            val += timedelta(days=1)
            return val

    assert AuthorDTO.parse_obj({"name": "Bill Bryson", "dob": "1951-12-08"}).dict() == {
        "name": "BILL BRYSON!",
        "dob": date(1951, 12, 9),
    }

Still need to do docs, more tests, I think there'll be an issue with self-referencing relationships to sort out, and will probably break up the module into a sub-package as a bit going on in there now..

Main issue is that mypy isn't recognising the to_mapped() method, either via the factory() or decorator() method, just get these:

tests/utils/controllers.py:33: error: "CreateDTO" has no attribute "to_mapped"  [attr-defined]
tests/utils/controllers.py:45: error: UpdateDTO? has no attribute "to_mapped"  [attr-defined]
tests/unit/test_dto.py:253: error: "CreateDTO" has no attribute "to_mapped"  [attr-defined]

Seems like there is a very longstanding issue with mypy and class decorators, python/mypy#3135 (comment) but I wouldn't have thought that would affect the factory() generated DTOs.

Further development of the decorator pattern.
@gazorby
Copy link
Owner

gazorby commented Nov 25, 2022

Hi @peterschutt!

Seems good enough regarding decorator impementation plus validator support. I got a working iteration concerning the references cycles. It's quite a bit of work, maybe we want a new PR on your repo for this. I'll merge this so I can rebase on what your last changes here.

@gazorby
Copy link
Owner

gazorby commented Nov 25, 2022

@peterschutt Could you resolve the conflicts?

@peterschutt
Copy link
Author

Hey @gazorby - using https://github.com/topsport-com-au/starlite-saqlalchemy/tree/dto-improvements as feature branch for this, and have opened topsport-com-au#139. All of your original work still in there. Thanks again, dto definitely in a better place now!

I'll start working on restructuring/tests/documentation and if you'd like to chip in for breaking the reference cycles, that'd be great!

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

Successfully merging this pull request may close these issues.

2 participants