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

prototype: object type extending #3379

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

Conversation

Kitefiko
Copy link

Prototype for #2605

@botberry
Copy link
Member

Hi, thanks for contributing to Strawberry 🍓!

We noticed that this PR is missing a RELEASE.md file. We use that to automatically do releases here on GitHub and, most importantly, to PyPI!

So as soon as this PR is merged, a release will be made 🚀.

Here's an example of RELEASE.md:

Release type: patch

Description of the changes, ideally with some examples, if adding a new feature.

Release type can be one of patch, minor or major. We use semver, so make sure to pick the appropriate type. If in doubt feel free to ask :)

Here's the tweet text:

🆕 Release (next) is out! Thanks to Kitefiko for the PR 👏

Get it here 👉 https://beta.strawberry.rocks/release/(next)

Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

PR Type: Enhancement

PR Summary: This pull request introduces a new feature aimed at extending the functionality of object types within the Strawberry GraphQL framework. It adds a StrawberryObjectBuilder class that allows for customization at various stages of the object type creation process, including before wrapping a dataclass, during field processing, and after the entire process. This enhancement is designed to provide developers with more flexibility and control over how object definitions are constructed, potentially translating 'auto' types or changing field classes to custom ones. Additionally, it integrates this builder pattern into existing functions such as _process_type, type, input, and interface by allowing an optional builder parameter.

Decision: Comment

📝 Type: 'Enhancement' - not supported yet.
  • Sourcery currently only approves 'Typo fix' PRs.
✅ Issue addressed: this change correctly addresses the issue or implements the desired feature.
No details provided.
✅ Small diff: the diff is small enough to approve with confidence.
No details provided.

General suggestions:

  • Ensure that the new StrawberryObjectBuilder class and its methods are thoroughly documented, including clear examples of how and when to use them. This will help users understand the benefits and capabilities of this new feature.
  • Consider the impact of making the builder parameter required in the future, as noted in the TODO comment. Evaluate whether this change would significantly improve the internal design or usability of the framework, and if so, plan for a deprecation path that minimizes disruption for users.
  • Review and address any linter warnings directly rather than suppressing them, to maintain code quality and readability. If suppression is absolutely necessary, provide clear comments explaining the reason.
  • Clarify the purpose and expected behavior of the after_process method in the StrawberryObjectBuilder class. The current placeholder docstring ('Dont know tbh') should be replaced with a meaningful description.

Thanks for using Sourcery. We offer it for free for open source projects and would be very grateful if you could help us grow. If you like it, would you consider sharing Sourcery on your favourite social media? ✨

Share Sourcery

Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.

Callable[[Any, GraphQLResolveInfo, GraphQLAbstractType], str]
],
) -> StrawberryObjectDefinition:
"""Posibility to use custom StrawberryObjectDefinition"""
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick (llm): There's a typo in the docstring: 'Posibility' should be corrected to 'Possibility'.

)

def after_process(self, cls: Type[WithStrawberryObjectDefinition]):
"""Dont know tbh"""
Copy link
Contributor

Choose a reason for hiding this comment

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

issue (llm): The docstring for 'after_process' is unclear and unprofessional. It should be updated to accurately describe the method's purpose or effect, even if it's currently a placeholder for future implementation.

directives: Optional[Sequence[object]],
extend: bool,
fields: List[StrawberryField],
is_type_of: Optional[Callable[[Any, GraphQLResolveInfo], bool]],
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion (llm): Consider providing a brief explanation or example usage of 'is_type_of' and 'resolve_type' in the docstring for 'create_object_definition'. This would help users understand when and how to use these parameters effectively.

is_interface: bool,
interfaces: List[StrawberryObjectDefinition],
description: Optional[str],
directives: Optional[Sequence[object]],
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion (llm): It might be beneficial to specify a more precise type than 'object' for the elements of 'directives'. If there's a common interface or base class that all directives should adhere to, using that would enhance type safety and code readability.

@@ -76,6 +87,9 @@ class if one is not set by either using an explicit strawberry.field(name=...) o

# then we can proceed with finding the fields for the current class
for field in dataclasses.fields(cls): # type: ignore
# Builder field hook
field = builder.on_field(field=field) # noqa: PLW2901
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion (llm): The use of 'noqa: PLW2901' suggests that there's a linter warning being suppressed. It's generally a good practice to address the root cause of the warning if possible, rather than suppressing it. If this suppression is necessary, consider adding a comment explaining why.

@Kitefiko Kitefiko mentioned this pull request Feb 11, 2024
2 tasks
Copy link

codecov bot commented Feb 11, 2024

Codecov Report

Attention: Patch coverage is 97.05882% with 1 line in your changes missing coverage. Please review.

Project coverage is 96.57%. Comparing base (f15bcc7) to head (41a44b1).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3379   +/-   ##
=======================================
  Coverage   96.57%   96.57%           
=======================================
  Files         524      525    +1     
  Lines       33630    33651   +21     
  Branches     5578     5580    +2     
=======================================
+ Hits        32479    32500   +21     
+ Misses        915      914    -1     
- Partials      236      237    +1     

Copy link

codspeed-hq bot commented Feb 11, 2024

CodSpeed Performance Report

Merging #3379 will not alter performance

Comparing Kitefiko:object-extension (41a44b1) with main (f15bcc7)

Summary

✅ 13 untouched benchmarks

@Kitefiko Kitefiko marked this pull request as draft February 14, 2024 21:33
@Kitefiko Kitefiko marked this pull request as draft February 14, 2024 21:33
Comment on lines 17 to 18
def before_wrap_dataclass(self, cls: Type) -> None:
pass
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should use something similar to our extensions api and define this as on_wrap_dataclass, in which the user can yield inside, meaning he can call "before" code before it, and "after" code after it. That also allows they to except exceptions

What do you think?

Copy link
Author

Choose a reason for hiding this comment

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

Maybe we should use something similar to our extensions api and define this as on_wrap_dataclass, in which the user can yield inside, meaning he can call "before" code before it, and "after" code after it. That also allows they to except exceptions

What do you think?

I was modeling this based on FieldExtension. Where these hooks are more similar to it's apply function. While I can imagine on_wrap_dataclass possibly being context manager for both before_wrap_dataclass & after_process, I don't see how would that work for on_field.

Comment on lines 20 to 22
def on_field(self, field: Field | StrawberryField) -> Any:
return field
Copy link
Member

Choose a reason for hiding this comment

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

Maybe the same here

@Kitefiko
Copy link
Author

Kitefiko commented May 3, 2024

Just rebased and adjusted so solution is no longer outdated - original_type_annotations.

@bellini666
Copy link
Member

bellini666 commented May 11, 2024

I like where this is going...

Checking it today I'm wondering if we should alllow a list of type extensions instead of just one.

Then when doing, field = extension.on_field you just replace it with

for extension in extensions:
    field = extension.on_field(field)

And so on.

This would allow for much more customization.

What do you think about this @patrick91 ?

@bellini666 bellini666 requested a review from patrick91 May 11, 2024 17:47
@patrick91
Copy link
Member

Checking it today I'm wondering if we should alllow a list of type extensions instead of just one.

yup, we do this for fields already 😊

@Kitefiko
Copy link
Author

Then when doing, field = extension.on_field you just replace it with

for extension in extensions:
    field = extension.on_field(field)

This would allow for much more customization.

There are 2 reasons why I didn't do that.

  • Reduce performance hit to minimum - no looping (this might not matter tho)
  • Keep it more generic. Since you already have access to lower level and thus can modify entire object (including fields), it seemed redundant to enforce this via library

You could do something like

class MyTypeExtension(TypeExtension):
    def __init__(self, field_extensions: Iterable[...]):
        self._field_extensions = field_extensions

    def on_field(self, field: dataclasses.Field[Any]):
        for ext in self._field_extensions:
            field = ext.apply(field)
        return field

What do you guys think?

Also could you explain a little more how you envision the usage of context manager styled hooks @bellini666 ?

@bellini666
Copy link
Member

Then when doing, field = extension.on_field you just replace it with

for extension in extensions:
    field = extension.on_field(field)

This would allow for much more customization.

There are 2 reasons why I didn't do that.

  • Reduce performance hit to minimum - no looping (this might not matter tho)
  • Keep it more generic. Since you already have access to lower level and thus can modify entire object (including fields), it seemed redundant to enforce this via library

IMO I think it is still worth it. This only runs once, and the performance hit here should be insignificant.

Also could you explain a little more how you envision the usage of context manager styled hooks @bellini666 ?

Hrm, I think the code that I mentioned changed now, but I was thinking in something like this: https://github.com/strawberry-graphql/strawberry/blob/main/strawberry/extensions/base_extension.py#L27 . With that you can have some code that executes "before" and "after" without having to define 2 functions for that.

But I don't think that makes sense for the on_field for example, as it is supposed to be able to actually change the field.

@Kitefiko
Copy link
Author

Kitefiko commented Jun 12, 2024

Then when doing, field = extension.on_field you just replace it with

for extension in extensions:
    field = extension.on_field(field)

This would allow for much more customization.

There are 2 reasons why I didn't do that.

  • Reduce performance hit to minimum - no looping (this might not matter tho)
  • Keep it more generic. Since you already have access to lower level and thus can modify entire object (including fields), it seemed redundant to enforce this via library

IMO I think it is still worth it. This only runs once, and the performance hit here should be insignificant.

True that, some more things to consider between single and multi TypeExtension per object.
With one TypeExtension you will be allowed to subclass StrawberryObjectDefinition. This would allow to add custom methods, custom data as attrs, and use isinstance on cls.__strawberry_definition__.
With multiple I don't really see a way how to subclass StrawberryObjectDefinition so the only "modifiable" thing might be metadata?

I personally like single TypeExtension mainly due to bigger freedom and ability to directly modify __strawberry_definition__. With multiple approach I would be back to monkey patching. One could get multiple approach functionality from single approach one, but not vice versa.

Also could you explain a little more how you envision the usage of context manager styled hooks @bellini666 ?

Hrm, I think the code that I mentioned changed now, but I was thinking in something like this: https://github.com/strawberry-graphql/strawberry/blob/main/strawberry/extensions/base_extension.py#L27 . With that you can have some code that executes "before" and "after" without having to define 2 functions for that.

But I don't think that makes sense for the on_field for example, as it is supposed to be able to actually change the field.

Thanks for clarifying this. I do like context manager approach here too, after figuring out how it works. I might have added this at the latest update but, after_process currently returns cls. I added this due to how pydantic wrapper replaces original class with new. I think however that that is very unlikely scenario so context manager is better, unless someone thinks otherwise.

I was playing with integration into django library and thing popped up: should on_field hook have additionally also origin info from _get_fields.origins that is gather from all bases?

Copy link
Member

@erikwrede erikwrede left a comment

Choose a reason for hiding this comment

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

I'm a bit late to the party, but thanks for picking this up again! I see that it picks up some ideas from my initial concept, but also opens up to some changes. I've added a few comments that came to my mind immediately.

The main open point I see is wether we want to support one or multiple type extensions on the same type. My initial draft aimed for the support of multiple extensions, however I am open to other opinions and curious to hear why you chose to take this approach.

@@ -177,7 +178,7 @@ def wrap(cls: Any) -> Type[StrawberryTypeFromPydantic[PydanticModel]]:
)

wrapped = _wrap_dataclass(cls)
extra_strawberry_fields = _get_fields(wrapped, {})
extra_strawberry_fields = _get_fields(wrapped, TypeExtension(), {})
Copy link
Member

Choose a reason for hiding this comment

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

Instantiating an empty type extension feels wrong to me.

Copy link
Author

Choose a reason for hiding this comment

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

This I currently take as a temp code, I am planning on implementing PydanticTypeExtension once API is agreed upon.

is_type_of = getattr(cls, "is_type_of", None)
resolve_type = getattr(cls, "resolve_type", None)

cls.__strawberry_definition__ = StrawberryObjectDefinition(
cls.__strawberry_definition__ = extension.create_object_definition(
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure about letting the extension influence the instantiation of the strawberry object definition. This severely limits our ability to add new parameters to the constructor later on without breaking the public API. Addtionally, this implies that only one type extension is supported on a type.

However, having the extension change all of the fields manually after creation could also lead to worse DevX. Open for other opinions on this /cc @strawberry-graphql/core

@Kitefiko
Copy link
Author

The main open point I see is wether we want to support one or multiple type extensions on the same type.

Yes. This is currently blocker right now. I would really appreciate for more people to weight in.

My initial draft aimed for the support of multiple extensions, however I am open to other opinions and curious to hear why you chose to take this approach.

I have touched on this in previous comment but the main points IMO are:

Single

  • Ability to hook into objects' field creation
  • Ability to directly inherit and modify StrawberryObjectDefinition used for __strawberry_definition__. This allows for custom attrs, methods, has_MY_object_definition, get_MY_object_definition, ...

Multiple

  • Ability to hook into objects' field creation
  • You can use multiple TypeExtension out of the box
  • Ability to add extra info via metadata? field on StrawberryObjectDefinition
  • Strawberry internals are less exposed
  • Non-official (current approach) ability to add custom attrs, methods etc. via setattr e.g. __strawberry_django_definition__

You could create TypeExtension that would act as Multiple TypeExtension, but not vice versa.
I think that Multiple kinda clashes with and takes responsibility from strawberry.field(extensions=[...] and FieldExtension concept.

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.

5 participants