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

fix: DTO factory narrowed with a generic alias. #2791

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

peterschutt
Copy link
Contributor

Pull Request Checklist

  • New code has 100% test coverage
  • (If applicable) The prose documentation has been updated to reflect the changes introduced by this PR
  • (If applicable) The reference documentation has been updated to reflect the changes introduced by this PR
  • Pre-Commit Checks were ran and passed
  • Tests were ran and passed

Description

This PR is an attempt at handling DTOs that are narrowed with a _GenericAlias of a type supported by the DTO factory type.

Close Issue(s)

Closes #2500

This PR is an attempt at handling DTOs that are narrowed with a `_GenericAlias` of a type supported by the DTO factory type.

Closes #2500
Copy link
Contributor Author

@peterschutt peterschutt left a comment

Choose a reason for hiding this comment

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

This'll need a few iterations.

@@ -110,7 +111,7 @@ def __init__(
rename_fields=self.dto_factory.config.rename_fields,
)
self.transfer_model_type = self.create_transfer_model_type(
model_name=model_type.__name__, field_definitions=self.parsed_field_definitions
model_name=(get_origin(model_type) or model_type).__name__, field_definitions=self.parsed_field_definitions
Copy link
Contributor Author

Choose a reason for hiding this comment

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

With this feature, anywhere that we could previously assume that the type that narrowed the dto was just a regular class, we now have to account for the fact that it could be an instance of _GenericAlias.

In the my first pass of this PR I've taken the quickest and dirtiest approach to get things passing, but we might need some abstraction over the type that handles the differences.

@@ -29,7 +31,8 @@ class DataclassDTO(AbstractDTO[T], Generic[T]):
def generate_field_definitions(
cls, model_type: type[DataclassProtocol]
) -> Generator[DTOFieldDefinition, None, None]:
dc_fields = {f.name: f for f in fields(model_type)}
model_origin = get_origin(model_type) or model_type
dc_fields = {f.name: f for f in fields(model_origin)}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We'll have to test this against all of the other dto factory types that support generics too, b/c there is bound to be cases where the _GenericAlias instances break things in there too.

Comment on lines +459 to +471
if isinstance(self.annotation, _GenericAlias) and self.origin not in (ClassVar, Literal):
cl_args = get_args(cl)
cl_origin = get_origin(cl) or cl
return (
issubclass(self.origin, cl_origin)
and (len(cl_args) == len(self.args) if cl_args else True)
and (
all(t.is_subclass_of(cl_arg) for t, cl_arg in zip(self.inner_types, cl_args))
if cl_args
else True
)
)

Copy link
Contributor Author

@peterschutt peterschutt Nov 28, 2023

Choose a reason for hiding this comment

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

Trying to determine when a _GenericAlias type should be considered a sub-type of another type.

This says that when A is a _GenericAlias instance, then it is a subclass of B when:

  1. if B is parametrized, then As origin is a subtype of Bs origin, otherwise A's origin is a subtype of B.
  2. if B is parametrized, then A and B must have the same number of type parameters declared
  3. if B is parametrized, then As type params are pairwise subtypes of Bs type params

With 2 and 3, if B is not parametrized, and origin of A is a subtype of B then I think we should treat that as B[Any, ...] so the type parameters only come into it if both types have args declared.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll have to add a lot of tests for this.

Comment on lines +252 to +254
if not (parameters := getattr(annotation, "__parameters__", None)):
return type_hints
typevar_map = {p: p for p in parameters}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is to support using this function without knowing up front if the type we are passing in is a generic type or not. If not, it won't have the parameters attribute, and so we just return the type hints without any post processing.

@@ -161,3 +163,28 @@ class SubType(Model):
assert (
dto_type._dto_backends["handler_id"]["data_backend"].parsed_field_definitions[-1].name == "c" # pyright: ignore
)


def test_type_narrowing_with_generic_type() -> None:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

should name it test_get_model_type_hints_with_generic_type()

@peterschutt peterschutt self-assigned this Dec 1, 2023
@peterschutt peterschutt added the Bug 🐛 This is something that is not working as expected label Dec 1, 2023
Copy link

sonarcloud bot commented Dec 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

100.0% 100.0% Coverage
0.0% 0.0% Duplication

Copy link

github-actions bot commented Dec 3, 2023

Documentation preview will be available shortly at https://litestar-org.github.io/litestar-docs-preview/2791

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug 🐛 This is something that is not working as expected
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enhancement: Support generic types in DTOs
1 participant