Fix create multiple directives #3674

Speedy1991 · 2024-10-17T14:54:14Z

Description

Directives are added multiple times leading to VSC errors like The directive "@featuresRequired" can only be used once at this location.

Another option to fix this would be to make field.directives a set but this is used on multiple places and I don't think it's worth the rework. If the fix is ok for you I'll add a test with e.g. this sandbox setup

Types of Changes

Issues Fixed or Closed by This PR

Check if the directive is already set on the field as directive

Checklist

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
I have tested the changes and verified that they work and don't break anything (as well as I can manage).

Summary by Sourcery

Fix the issue of multiple directives being added to a field by deduplicating permission directives before extending the field's directives list.

Bug Fixes:

Fix the issue of directives being added multiple times to a field, which caused errors in VSC.

Enhancements:

Optimize the process of applying permission directives by deduplicating them before adding to the field.

sourcery-ai · 2024-10-17T14:54:20Z

Reviewer's Guide by Sourcery

This pull request addresses a bug where directives were being added multiple times, causing VSCode errors. The fix involves deduplicating directives when applying permissions to a field, ensuring that each directive is only added once.

Class diagram for StrawberryField directive handling

classDiagram
    class StrawberryField {
        +List~Directive~ directives
    }
    class Permission {
        +Directive schema_directive
    }
    class PermissionHandler {
        +apply(field: StrawberryField) void
    }
    PermissionHandler --> StrawberryField : apply
    PermissionHandler --> Permission : uses
    note for StrawberryField "Field directives are now deduplicated when applying permissions."

File-Level Changes

Change	Details	Files
Implement deduplication of permission directives	Create a set of permission directives from the permissions Create a set of existing field directives Calculate the difference between permission directives and existing directives Extend field directives with only the new, unique directives	`strawberry/permission.py`

Possibly linked issues

print_schema creates multiple directives if a field is used in multiple schemas #3596: The PR fixes the issue by deduping directives, preventing multiple instances in schemas.

Tips and commands

Interacting with Sourcery

Trigger a new review: Comment @sourcery-ai review on the pull request.
Continue discussions: Reply directly to Sourcery's review comments.
Generate a GitHub issue from a review comment: Ask Sourcery to create an
issue from a review comment by replying to it.
Generate a pull request title: Write @sourcery-ai anywhere in the pull
request title to generate a title at any time.
Generate a pull request summary: Write @sourcery-ai summary anywhere in
the pull request body to generate a PR summary at any time. You can also use
this command to specify where the summary should be inserted.

Customizing Your Experience

Access your dashboard to:

Enable or disable review features such as the Sourcery-generated pull request
summary, the reviewer's guide, and others.
Change the review language.
Add, remove or edit custom review instructions.
Adjust other review settings.

Getting Help

Contact our support team for questions or feedback.
Visit our documentation for detailed guides and information.
Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

for more information, see https://pre-commit.ci

sourcery-ai

Hey @Speedy1991 - I've reviewed your changes - here's some feedback:

Overall Comments:

The fix looks good, but please include the test you mentioned (using the sandbox setup) in this PR to ensure the bug doesn't resurface in the future.
Consider if any user-facing documentation needs to be updated to reflect this fix, especially if users might have encountered this issue before.

Here's what I looked at during the review

🟢 General issues: all looks good
🟢 Security: all looks good
🟢 Testing: all looks good
🟢 Complexity: all looks good
🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

botberry · 2024-10-17T14:57:16Z

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 Arthur for the PR 👏

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

botberry · 2024-10-17T14:57:22Z

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 Arthur for the PR 👏

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

codecov · 2024-10-17T14:59:17Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.02%. Comparing base (56172dc) to head (a736a61).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3674   +/-   ##
=======================================
  Coverage   97.01%   97.02%           
=======================================
  Files         503      503           
  Lines       33458    33461    +3     
  Branches     5618     5618           
=======================================
+ Hits        32460    32465    +5     
+ Misses        792      790    -2     
  Partials      206      206

codspeed-hq · 2024-10-17T15:04:46Z

CodSpeed Performance Report

Merging #3674 will not alter performance

_{Comparing Speedy1991:fix-3596 (a736a61) with main (56172dc)}

Summary

✅ 15 untouched benchmarks

bellini666

Hey @Speedy1991 ,

Thank you very much for this!

Could we have a test for this to avoid any future regressions?

bellini666 · 2024-10-19T14:57:34Z

strawberry/permission.py

@@ -158,9 +158,14 @@ def __init__(
    def apply(self, field: StrawberryField) -> None:
        """Applies all of the permission directives to the schema and sets up silent permissions."""
        if self.use_directives:
-            field.directives.extend(
+            # Dedupe multiple directives
+            # https://github.com/strawberry-graphql/strawberry/issues/3596


I don't think we need the issue linked here as it is easy to check the commit that changed this on git history

Speedy1991 · 2024-10-19T15:28:13Z

Hey @Speedy1991 ,

Thank you very much for this!

Could we have a test for this to avoid any future regressions?

yea, i wasn't sure if refactoring the directives from a list to a set would be a more stable fix, see this comment:

Another option to fix this would be to make field.directives a set but this is used on multiple places and I don't think it's worth the rework. If the fix is ok for you I'll add a test

bellini666 · 2024-10-19T15:38:08Z

yea, i wasn't sure if refactoring the directives from a list to a set would be a more stable fix, see this comment:

Refactoring it into a set could possibly have unwanted consequences. Because they are not "ordered" you can't enforce permissions to run in a given order. So I think your fix is the way to go.

Speedy1991 · 2024-10-19T15:45:43Z

Hm I think I need to refactor this a littlebit if the order matters, because the current fix cast the lists to sets and calculates the difference. As far i know casting to/from a set does not guarantee to keep the order.

bellini666 · 2024-10-19T16:11:50Z

Hm I think I need to refactor this a littlebit if the order matters, because the current fix cast the lists to sets and calculates the difference. As far i know casting to/from a set does not guarantee to keep the order.

Good point, I actually missed that (even though I commented about this same very issue lol)

https://github.com/strawberry-graphql/strawberry/issues/3596

80df22e

[pre-commit.ci] auto fixes from pre-commit.com hooks

a736a61

for more information, see https://pre-commit.ci

sourcery-ai bot reviewed Oct 17, 2024

View reviewed changes

bellini666 reviewed Oct 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix create multiple directives #3674

Fix create multiple directives #3674

Speedy1991 commented Oct 17, 2024 •

edited by sourcery-ai bot

Loading

sourcery-ai bot commented Oct 17, 2024 •

edited

Loading

Interacting with Sourcery

Customizing Your Experience

Getting Help

sourcery-ai bot left a comment

botberry commented Oct 17, 2024

botberry commented Oct 17, 2024

codecov bot commented Oct 17, 2024 •

edited

Loading

codspeed-hq bot commented Oct 17, 2024

bellini666 left a comment

bellini666 Oct 19, 2024

Speedy1991 Oct 19, 2024

Speedy1991 commented Oct 19, 2024 •

edited

Loading

bellini666 commented Oct 19, 2024

Speedy1991 commented Oct 19, 2024

bellini666 commented Oct 19, 2024

Fix create multiple directives #3674

Are you sure you want to change the base?

Fix create multiple directives #3674

Conversation

Speedy1991 commented Oct 17, 2024 • edited by sourcery-ai bot Loading

Description

Types of Changes

Issues Fixed or Closed by This PR

Checklist

Summary by Sourcery

sourcery-ai bot commented Oct 17, 2024 • edited Loading

Reviewer's Guide by Sourcery

Class diagram for StrawberryField directive handling

File-Level Changes

Possibly linked issues

Interacting with Sourcery

Customizing Your Experience

Getting Help

sourcery-ai bot left a comment

Choose a reason for hiding this comment

botberry commented Oct 17, 2024

botberry commented Oct 17, 2024

codecov bot commented Oct 17, 2024 • edited Loading

Codecov Report

codspeed-hq bot commented Oct 17, 2024

CodSpeed Performance Report

Merging #3674 will not alter performance

Summary

bellini666 left a comment

Choose a reason for hiding this comment

bellini666 Oct 19, 2024

Choose a reason for hiding this comment

Speedy1991 Oct 19, 2024

Choose a reason for hiding this comment

Speedy1991 commented Oct 19, 2024 • edited Loading

bellini666 commented Oct 19, 2024

Speedy1991 commented Oct 19, 2024

bellini666 commented Oct 19, 2024

Speedy1991 commented Oct 17, 2024 •

edited by sourcery-ai bot

Loading

sourcery-ai bot commented Oct 17, 2024 •

edited

Loading

codecov bot commented Oct 17, 2024 •

edited

Loading

Speedy1991 commented Oct 19, 2024 •

edited

Loading