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

Change default Relay input m2m types from ListInput[NodeInputPartial] to ListInput[NodeInput] #630

Conversation

SupImDos
Copy link
Contributor

@SupImDos SupImDos commented Sep 24, 2024

Description

The default Relay input field type for a ManyToManyField or ManyToManyRel marked with strawberry.auto is ListInput[NodeInputPartial].

This means the following is allowed in mutations:

// Set
{
  "data": {
    "my_related_field": {
      "set": [{"id": null}, {"id": null}, ...]
    }
  }
}

// Add
{
  "data": {
    "my_related_field": {
      "add": [{"id": null}, {"id": null}, ...]
    }
  }
}

// Remove
{
  "data": {
    "my_related_field": {
      "remove": [{"id": null}, {"id": null}, ...]
    }
  }
}

I can't think of a scenario where allowing {"id": null} would be the expected / desired behaviour (at least by default), and it causes some weird errors in the default CUD mutations. For add / set / remove it seems appropriate that the "id" Global ID field should be required by default.

This PR changes the default Relay input type for ManyToManyField and ManyToManyRel from ListInput[NodeInputPartial] to ListInput[NodeInput] so that the above is no longer possible.

For reference, this seems to match the non-Relay behaviour of ManyToManyInput, which doesn't allow null for its IDs.

The unit tests all still pass with this change, but if this actually isn't a bug then I'm happy to close this PR.

Types of Changes

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Documentation

Issues Fixed or Closed by This PR

  • N/A

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

Update the default input type for ManyToManyField and ManyToManyRel in Relay to require non-null IDs, aligning with non-Relay behavior and preventing unexpected errors in CUD mutations.

Bug Fixes:

  • Change the default Relay input type for ManyToManyField and ManyToManyRel from ListInput[NodeInputPartial] to ListInput[NodeInput] to prevent allowing null IDs in mutations.

Copy link
Contributor

sourcery-ai bot commented Sep 24, 2024

Reviewer's Guide by Sourcery

This pull request changes the default Relay input field type for ManyToManyField and ManyToManyRel from ListInput[NodeInputPartial] to ListInput[NodeInput]. This change ensures that the 'id' field in these inputs is required, preventing potential errors and unexpected behavior in CUD (Create, Update, Delete) mutations.

File-Level Changes

Change Details Files
Change default Relay input type for ManyToManyField and ManyToManyRel
  • Replace ListInput[NodeInputPartial] with ListInput[NodeInput] for ManyToManyField
  • Replace ListInput[NodeInputPartial] with ListInput[NodeInput] for ManyToManyRel
strawberry_django/fields/types.py

Sequence Diagram

No sequence diagram generated.


Tips
  • Trigger a new Sourcery review by commenting @sourcery-ai review on the pull request.
  • Continue your discussion with Sourcery by replying directly to review comments.
  • You can change your review settings at any time by accessing your dashboard:
    • Enable or disable the Sourcery-generated pull request summary or reviewer's guide;
    • Change the review language;
  • You can always contact us if you have any questions or feedback.

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.

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

Overall Comments:

  • Consider adding new tests that specifically verify null IDs are no longer accepted for ManyToManyField and ManyToManyRel fields.
  • It would be helpful to update the documentation to explain this change in behavior, as it could potentially affect existing users of the library.
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 to tell me if it was helpful.

Copy link
Member

@bellini666 bellini666 left a comment

Choose a reason for hiding this comment

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

For the auto I think you are completely right! Thank you for the PR :)

@bellini666 bellini666 merged commit 63c9bd2 into strawberry-graphql:main Sep 24, 2024
20 checks passed
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.

2 participants