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

Add POC for fixing duplicated name with generics #2606

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

patrick91
Copy link
Member

@patrick91 patrick91 commented Mar 2, 2023

Closes #2599

Not 100% sure why the check fails, for now my fix is a workaround. Just for reference this is what happens with the types inside validate_same_type_definition:

(Pdb++) type1 != type2
True
(Pdb++) type1
<class 'strawberry.types.types.Wrapper'>
(Pdb++) type2
<class 'strawberry.types.types.Wrapper'>
(Pdb++) type1._type_definition
TypeDefinition(name='Wrapper', is_input=False, is_interface=False, origin=<class 'strawberry.types.types.Wrapper'>, description=None, interfaces=[], extend=False, directives=(), is_type_of=None, _fields=[Field(name='value',type=<class 'int'>,default=<dataclasses._MISSING_TYPE object at 0x101527280>,default_factory=<dataclasses._MISSING_TYPE object at 0x101527280>,init=True,repr=True,hash=None,compare=True,metadata=mappingproxy({}),kw_only=<dataclasses._MISSING_TYPE object at 0x101527280>,_field_type=None)], concrete_of=TypeDefinition(name='Wrapper', is_input=False, is_interface=False, origin=<class 'tests.schema.test_generics.test_nested_generics.<locals>.Wrapper'>, description=None, interfaces=[], extend=False, directives=(), is_type_of=None, _fields=[Field(name='value',type=<strawberry.type.StrawberryTypeVar object at 0x10510b1f0>,default=<dataclasses._MISSING_TYPE object at 0x101527280>,default_factory=<dataclasses._MISSING_TYPE object at 0x101527280>,init=True,repr=True,hash=None,compare=True,metadata=mappingproxy({}),kw_only=<dataclasses._MISSING_TYPE object at 0x101527280>,_field_type=None)], concrete_of=None, type_var_map={}), type_var_map={~T: <class 'int'>})
(Pdb++) type2._type_definition
TypeDefinition(name='Wrapper', is_input=False, is_interface=False, origin=<class 'strawberry.types.types.Wrapper'>, description=None, interfaces=[], extend=False, directives=(), is_type_of=None, _fields=[Field(name='value',type=<class 'int'>,default=<dataclasses._MISSING_TYPE object at 0x101527280>,default_factory=<dataclasses._MISSING_TYPE object at 0x101527280>,init=True,repr=True,hash=None,compare=True,metadata=mappingproxy({}),kw_only=<dataclasses._MISSING_TYPE object at 0x101527280>,_field_type=None)], concrete_of=TypeDefinition(name='Wrapper', is_input=False, is_interface=False, origin=<class 'tests.schema.test_generics.test_nested_generics.<locals>.Wrapper'>, description=None, interfaces=[], extend=False, directives=(), is_type_of=None, _fields=[Field(name='value',type=<strawberry.type.StrawberryTypeVar object at 0x10510a650>,default=<dataclasses._MISSING_TYPE object at 0x101527280>,default_factory=<dataclasses._MISSING_TYPE object at 0x101527280>,init=True,repr=True,hash=None,compare=True,metadata=mappingproxy({}),kw_only=<dataclasses._MISSING_TYPE object at 0x101527280>,_field_type=None)], concrete_of=None, type_var_map={}), type_var_map={~T: <class 'int'>})
(Pdb++)

I didn't spend much time on this because I think we could refactor the whole logic.

@BryceBeagle @bellini666 what do you think?

@codecov
Copy link

codecov bot commented Mar 2, 2023

Codecov Report

Merging #2606 (c1d30ee) into main (8586e3a) will decrease coverage by 0.03%.
The diff coverage is 50.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2606      +/-   ##
==========================================
- Coverage   96.32%   96.30%   -0.03%     
==========================================
  Files         184      184              
  Lines        7627     7632       +5     
  Branches     1400     1400              
==========================================
+ Hits         7347     7350       +3     
- Misses        179      181       +2     
  Partials      101      101              

@bellini666
Copy link
Member

I think the main reason for why that is happening is because of this line: https://github.com/strawberry-graphql/strawberry/blob/main/strawberry/types/types.py#L93

When resolving a generic, the TypeDefinition not only creates a copy of himself, but also creates a copy of the type.

If I understood the issue correctly, although the generics are being specialized the same way, they are both creating new types and definitions. Those types and type definitions holds the same attributes, but are different objects and python will compare two classes by their identity (unless the metaclass defines an __eq__)

Maybe what can be done is to cache that new type based on the type_var_map? So when calling copy_with again with the same type_var_map it would return the already specialized type instead of creating a new one.

What do you think?

@bellini666
Copy link
Member

Something like this:

diff --git strawberry/types/types.py strawberry/types/types.py
index 7d484e3e..e189bc3d 100644
--- strawberry/types/types.py
+++ strawberry/types/types.py
@@ -5,6 +5,8 @@
     TYPE_CHECKING,
     Any,
     Callable,
+    Dict,
+    FrozenSet,
     List,
     Mapping,
     Optional,
@@ -44,6 +46,7 @@ class TypeDefinition(StrawberryType):
     type_var_map: Mapping[TypeVar, Union[StrawberryType, type]] = dataclasses.field(
         default_factory=dict
     )
+    _generics_cache: Dict[FrozenSet, type] = dataclasses.field(default_factory=dict)
 
     def __post_init__(self):
         # resolve `Self` annotation with the origin type
@@ -73,6 +76,10 @@ def copy_with(
         self, type_var_map: Mapping[TypeVar, Union[StrawberryType, type]]
     ) -> type:
         # TODO: Logic unnecessary with StrawberryObject
+        cache_key = frozenset(type_var_map.items())
+        if cache_key in self._generics_cache:
+            return self._generics_cache[cache_key]
+
         fields = [field.copy_with(type_var_map) for field in self.fields]
 
         new_type_definition = TypeDefinition(
@@ -98,6 +105,7 @@ def copy_with(
 
         new_type_definition.origin = new_type
 
+        self._generics_cache[cache_key] = new_type
         return new_type
 
     def get_field(self, python_name: str) -> Optional[StrawberryField]:

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.

DuplicatedTypeName exception raised for nested generics
2 participants