Fix interfaces with duplicate directives by Speedy1991 · Pull Request #3674 · strawberry-graphql/strawberry

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

Thanks for adding the RELEASE.md file!

Here's a preview of the changelog:

This release addresses a bug where directives were being added multiple times when defined in an interface which multiple objects inherits from.

The fix involves deduplicating directives when applying extensions/permissions to a field, ensuring that each directive is only added once.

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 (c738986).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3674   +/-   ##
=======================================
  Coverage   97.01%   97.02%           
=======================================
  Files         503      503           
  Lines       33458    33492   +34     
  Branches     5618     5626    +8     
=======================================
+ Hits        32460    32495   +35     
+ Misses        792      791    -1     
  Partials      206      206

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

CodSpeed Performance Report

Merging #3674 will not alter performance

_{Comparing Speedy1991:fix-3596 (c738986) 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?

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)

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

Speedy1991 · 2024-10-21T07:17:22Z

@bellini666 I refactored the code to keep the order + added a test with a permission class.

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

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

Comment thread strawberry/permission.py Outdated

Arthur Bayr and others added 4 commits October 21, 2024 08:44

keep order

9b4e9c9

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

e89afd1

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

added a test

d77bc51

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

be4faa0

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

Arthur Bayr and others added 4 commits October 21, 2024 12:46

fix untyped **kwargs

315f2a1

keep 'Any' from inheritance

96a2ae5

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

35015d6

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

add a RELEASE.md

8a82009

botberry added bot:has-release-file labels Oct 21, 2024

bellini666 approved these changes Oct 21, 2024

View reviewed changes

Comment thread RELEASE.md Outdated

Update RELEASE.md

0c83f47

bellini666 changed the title ~~Fix create multiple directives~~ Fix interfaces with duplicate directives Oct 21, 2024

Execute the schema to increase coverage

c738986

bellini666 merged commit 9947fee into strawberry-graphql:main Oct 21, 2024

Uh oh!

Conversation

Speedy1991 commented Oct 17, 2024 • edited by sourcery-ai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Types of Changes

Issues Fixed or Closed by This PR

Checklist

Summary by Sourcery

Uh oh!

sourcery-ai Bot commented Oct 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

Uh oh!

sourcery-ai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

botberry commented Oct 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

botberry commented Oct 17, 2024

Uh oh!

codecov Bot commented Oct 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

codspeed-hq Bot commented Oct 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CodSpeed Performance Report

Merging #3674 will not alter performance

Summary

Uh oh!

bellini666 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Speedy1991 commented Oct 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bellini666 commented Oct 19, 2024

Uh oh!

Speedy1991 commented Oct 19, 2024

Uh oh!

bellini666 commented Oct 19, 2024

Uh oh!

Speedy1991 commented Oct 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Speedy1991 commented Oct 17, 2024 •

edited by sourcery-ai Bot

Loading

sourcery-ai Bot commented Oct 17, 2024 •

edited

Loading

botberry commented Oct 17, 2024 •

edited

Loading

codecov Bot commented Oct 17, 2024 •

edited

Loading

codspeed-hq Bot commented Oct 17, 2024 •

edited

Loading

Speedy1991 commented Oct 19, 2024 •

edited

Loading

Speedy1991 commented Oct 21, 2024 •

edited

Loading