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.

Sign up for GitHub

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

Jump to bottom

feat: rework and simplify DTOs #2088

Merged
merged 14 commits into from
Aug 3, 2023
Merged

feat: rework and simplify DTOs #2088

merged 14 commits into from
Aug 3, 2023

Conversation

Goldziher
Copy link
Contributor

@Goldziher Goldziher commented Jul 29, 2023 •
edited
Loading

This PR does the following:

  1. It removes the DTOInterface class, which was both unnecessary and problematic - all DTOs must include the logic that is on the base DTO class, and this was wrong.
  2. It renames AbstractDTOFactory into AbstractDTO - this is more correct since all its subclasses are called DataclassDTO, MsgspecDTO etc.
  3. It removes the ForType and the logic associated with it - this is no longer necessary and it just added complication.
  4. It removes the unnecessary layers of abstraction added in multiple places - these were not helping and made the code more complicated and harder to grasp.
  5. It simplifies and merges the encoding/decoding logic on the DTO classes.
  6. It simplifies and conventionalizes the registration logic in handlers, which was a bit messy and hacky.
  7. It fixes multiple issues with the registration of DTOs and the handling of nested types.
  8. It cleansup some parts of the tests.
  9. It fixes an issue with pagination and DTOs.

The following are breaking changes:

  • removal of DTOInterface
  • Removal of ForType
  • Rename of AbstractDTOFactory -> AbstractDTO
  • Replacement of the DTO methods builtins_to_data_type and bytes_to_data_type with decode
  • Replace of the DTO method data_to_encodable_type with encode

It closes:

#2018
#1929

This is the last change to DTOs before we go for release. From this point onward its only bug fixes.

Apologies for the scale of this PR - it required a rather deep dive to fully work out the code and simplify it.

@Goldziher Goldziher requested a review from a team as a code owner July 29, 2023 07:50
@Goldziher Goldziher linked an issue Jul 29, 2023 that may be closed by this pull request
Bug: Pagination breaks serialization of SQL Alchemy models #2018
Closed
4 tasks
@Goldziher Goldziher force-pushed the 2018-bug-pagination-breaks-serialization-of-sql-alchemy-models branch 4 times, most recently from 819e0a7 to c9a8888 Compare July 29, 2023 21:46
@JacobCoffee
Copy link
Member

This is a monster ._. 👹

@Goldziher
Copy link
Contributor Author

This is a monster ._. 👹

Not quite done yet...

@Goldziher Goldziher force-pushed the 2018-bug-pagination-breaks-serialization-of-sql-alchemy-models branch 7 times, most recently from 39cc377 to a38aad7 Compare July 30, 2023 21:17
@github-actions
Copy link

github-actions bot commented Jul 30, 2023 •
edited
Loading

Qodana Community for Python

21 new problems were found

Inspection name Severity Problems
Accessing a protected member of a class or a module ◽️ Notice 14
Unused local symbols ◽️ Notice 5
An instance attribute is defined outside init`` ◽️ Notice 2

💡 Qodana analysis was run in the pull request mode: only the changed files were checked
☁️ View the detailed Qodana report

Contact Qodana team

Contact us at qodana-support@jetbrains.com

@Goldziher Goldziher force-pushed the 2018-bug-pagination-breaks-serialization-of-sql-alchemy-models branch from a38aad7 to aaba865 Compare July 30, 2023 21:39
@provinzkraut provinzkraut force-pushed the 2018-bug-pagination-breaks-serialization-of-sql-alchemy-models branch from aaba865 to df78850 Compare July 31, 2023 12:36
@Goldziher Goldziher force-pushed the 2018-bug-pagination-breaks-serialization-of-sql-alchemy-models branch from 87df79d to 1d5b7aa Compare August 1, 2023 15:59
@sourcery-ai sourcery-ai bot mentioned this pull request Aug 1, 2023
Closed
@Goldziher Goldziher force-pushed the 2018-bug-pagination-breaks-serialization-of-sql-alchemy-models branch from 1d5b7aa to 1693db7 Compare August 1, 2023 16:05
provinzkraut
provinzkraut requested changes Aug 3, 2023
View reviewed changes
docs/examples/contrib/sqlalchemy/plugins/tutorial/full_app_with_init_plugin.py Outdated Show resolved Hide resolved
docs/usage/dto/2-dto-interface.rst Outdated Show resolved Hide resolved
litestar/dto/_backend.py Show resolved Hide resolved
litestar/dto/_backend.py Outdated Show resolved Hide resolved
litestar/dto/base_dto.py Outdated Show resolved Hide resolved
tests/unit/test_contrib/test_piccolo_orm/test_piccolo_orm_dto.py Outdated Show resolved Hide resolved
tests/unit/test_contrib/test_prometheus.py Outdated Show resolved Hide resolved
tests/unit/test_dto/test_factory/test_integration.py Show resolved Hide resolved
tests/unit/test_handlers/test_websocket_handlers/test_listeners.py Outdated Show resolved Hide resolved
tests/unit/test_handlers/test_websocket_handlers/test_validations.py Outdated Show resolved Hide resolved
@Goldziher Goldziher force-pushed the 2018-bug-pagination-breaks-serialization-of-sql-alchemy-models branch from f31e427 to 5f11208 Compare August 3, 2023 14:49
Goldziher and others added 3 commits August 3, 2023 16:50
@Goldziher @provinzkraut
Update litestar/dto/base_dto.py
0b137b9 
Co-authored-by: Janek Nouvertné <25355197+provinzkraut@users.noreply.github.com>
@Goldziher
chore: address review comments
0ae9ee7
@Goldziher
chore: merge issue fixes
617e1fe
@Goldziher Goldziher force-pushed the 2018-bug-pagination-breaks-serialization-of-sql-alchemy-models branch from 7b6c274 to 617e1fe Compare August 3, 2023 15:13
Goldziher and others added 3 commits August 3, 2023 17:21
@Goldziher @provinzkraut
Update litestar/handlers/base.py
0673780 
Co-authored-by: Janek Nouvertné <25355197+provinzkraut@users.noreply.github.com>
@Goldziher
chore: address review comments
90a6253
@Goldziher
chore: make handler_id unique
8f1521b
@sonarcloud
Copy link

sonarcloud bot commented Aug 3, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

99.6% 99.6% Coverage
0.0% 0.0% Duplication

cofin
cofin approved these changes Aug 3, 2023
View reviewed changes
Copy link
Member

@cofin cofin left a comment

Choose a reason for hiding this comment

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

Sorry for the long review time on this. It's a big one, but other than the current comments, it LGTM.

@JacobCoffee
Copy link
Member

JacobCoffee commented Aug 3, 2023 •
edited
Loading

Sorry for the delay

(Well that's funny timing and phrasing 🤣... trying to finish from my phone)

@Goldziher Goldziher merged commit b9814c2 into main Aug 3, 2023
17 of 18 checks passed
@Goldziher Goldziher deleted the 2018-bug-pagination-breaks-serialization-of-sql-alchemy-models branch August 3, 2023 18:21
provinzkraut added a commit that referenced this pull request Aug 3, 2023
@provinzkraut
doc: fix a docs reference error introduced in #2088
b1677a3
@provinzkraut provinzkraut mentioned this pull request Aug 3, 2023
Merged
provinzkraut added a commit that referenced this pull request Aug 3, 2023
@provinzkraut
doc: fix a docs reference error introduced in #2088
44d8c6c
provinzkraut added a commit that referenced this pull request Aug 3, 2023
@provinzkraut
docs: fix a docs reference error introduced in #2088 (#2110)
8631f24 
doc: fix a docs reference error introduced in #2088
@peterschutt peterschutt mentioned this pull request Dec 8, 2023
Closed
4 tasks
peterschutt added a commit that referenced this pull request Dec 8, 2023
@peterschutt
fix: no signature model data validation if DTO
42abfcc 
By the time the signature model is parsed, the DTO has already validated and built the object to be injected as `data`, so we don't need the signature model to do it again.

This PR ensures that `data` fields are typed as `Any` on the signature model so that validation doesn't occur twice.

This is a regression introduced in #2088, so I've added an explicit test for it.

Closes #2149
@peterschutt peterschutt mentioned this pull request Dec 8, 2023
Merged
peterschutt added a commit that referenced this pull request Dec 8, 2023
@peterschutt
fix: no signature model data validation if DTO
27fa9f1 
By the time the signature model is parsed, the DTO has already validated and built the object to be injected as `data`, so we don't need the signature model to do it again.

This PR ensures that `data` fields are typed as `Any` on the signature model so that validation doesn't occur twice.

This is a regression introduced in #2088, so I've added an explicit test for it.

Closes #2149
peterschutt added a commit that referenced this pull request Dec 8, 2023
@peterschutt
fix: no signature model data validation if DTO (#2854)
3b0af1a 
By the time the signature model is parsed, the DTO has already validated and built the object to be injected as `data`, so we don't need the signature model to do it again.

This PR ensures that `data` fields are typed as `Any` on the signature model so that validation doesn't occur twice.

This is a regression introduced in #2088, so I've added an explicit test for it.

Closes #2149
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JacobCoffee JacobCoffee JacobCoffee approved these changes

@provinzkraut provinzkraut provinzkraut approved these changes

@cofin cofin cofin approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Bug: Pagination breaks serialization of SQL Alchemy models
4 participants
@Goldziher @JacobCoffee @cofin @provinzkraut