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

Fix codegen crash on nullable lists of non-scalars #3653

Merged
merged 4 commits into from
Oct 6, 2024

Conversation

enoua5
Copy link
Contributor

@enoua5 enoua5 commented Sep 30, 2024

Description

Codegen would previously crash when working with a nullable list of non-scalars. This was due to a lambda referencing a value that would get rewritten with that same lambda causing an endless recursion.

Types of Changes

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

Issues Fixed or Closed by This PR

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 code generation crash when processing nullable lists of non-scalar types by adjusting the lambda function handling and add corresponding tests.

Bug Fixes:

  • Fix a crash in the code generation tool when handling nullable lists of non-scalar types by correcting the lambda function handling.

Tests:

  • Add tests to cover the scenario of nullable lists of non-scalar types to ensure the code generation tool handles them correctly.

Chores:

  • Add a release note indicating a patch release to fix the codegen crash issue.

@enoua5 enoua5 marked this pull request as ready for review September 30, 2024 17:51
Copy link
Contributor

sourcery-ai bot commented Sep 30, 2024

Reviewer's Guide by Sourcery

This pull request fixes a bug in the codegen tool where it would crash when working with a nullable list of non-scalar types. The fix involves modifying the _unwrap_type function in the query_codegen.py file to correctly handle nested optional and list types. Additionally, new test cases and snapshots have been added to ensure the functionality works as expected.

Sequence Diagram

sequenceDiagram
    participant Caller
    participant _unwrap_type
    participant StrawberryOptional
    participant StrawberryList
    Caller->>_unwrap_type: Call with type
    alt type is StrawberryOptional
        _unwrap_type->>StrawberryOptional: Unwrap of_type
        StrawberryOptional-->>_unwrap_type: Return unwrapped type
        _unwrap_type->>_unwrap_type: Recursive call
        _unwrap_type->>_unwrap_type: Create wrapper with previous_wrapper
    else type is StrawberryList
        _unwrap_type->>StrawberryList: Unwrap of_type
        StrawberryList-->>_unwrap_type: Return unwrapped type
        _unwrap_type->>_unwrap_type: Recursive call
        _unwrap_type->>_unwrap_type: Create wrapper with previous_wrapper
    end
    _unwrap_type-->>Caller: Return unwrapped type and wrapper
Loading

File-Level Changes

Change Details Files
Modified _unwrap_type function to handle nested optional and list types
  • Introduced 'previous_wrapper' variable to store the wrapper from recursive calls
  • Updated logic for handling StrawberryOptional and StrawberryList types
  • Adjusted lambda functions to use 'previous_wrapper' instead of 'wrapper'
strawberry/codegen/query_codegen.py
Added new test case for optional list of people
  • Added 'optional_list_of_people' field to Query class
tests/codegen/conftest.py
Added new test snapshots for optional list of people
  • Created Python snapshot for optional list of people
  • Created TypeScript snapshot for optional list of people
tests/codegen/snapshots/python/optional_list_of_people.py
tests/codegen/snapshots/typescript/optional_list_of_people.ts
Added new GraphQL query for optional list of people
  • Created GraphQL query to test optional list of people functionality
tests/codegen/queries/optional_list_of_people.graphql
Added release notes
  • Specified release type as patch
  • Added description of the bug fix
RELEASE.md

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.

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

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 @enoua5 - I've reviewed your changes and they look great!

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: 1 issue found

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.

RELEASE.md Show resolved Hide resolved
@botberry
Copy link
Member

botberry commented Sep 30, 2024

Thanks for adding the RELEASE.md file!

Here's a preview of the changelog:


Fixes an issue where the codegen tool would crash when working with a nullable list of types.

Here's the tweet text:

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

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

Copy link

codecov bot commented Sep 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.82%. Comparing base (36bd99e) to head (4524483).
Report is 7 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3653      +/-   ##
==========================================
+ Coverage   96.76%   96.82%   +0.05%     
==========================================
  Files         522      503      -19     
  Lines       33824    33410     -414     
  Branches     5635     5583      -52     
==========================================
- Hits        32731    32350     -381     
+ Misses        863      830      -33     
  Partials      230      230              

Copy link

codspeed-hq bot commented Sep 30, 2024

CodSpeed Performance Report

Merging #3653 will not alter performance

Comparing enoua5:codegen-optional-list (4524483) with main (ffdd85b)

Summary

✅ 15 untouched benchmarks

Copy link
Member

@DoctorJohn DoctorJohn left a comment

Choose a reason for hiding this comment

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

Hi @enoua5, thanks for the PR. I had to read the corresponding issue to understand the changes and what was going on. Thanks for fixing the recursion. I left two minor suggestions regarding the test's readability/verifiability.

tests/codegen/queries/optional_list_of_people.graphql Outdated Show resolved Hide resolved
tests/codegen/queries/optional_list_of_people.graphql Outdated Show resolved Hide resolved
Copy link
Member

@DoctorJohn DoctorJohn left a comment

Choose a reason for hiding this comment

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

Awesome, thanks again @enoua5. This looks good to me now

@patrick91 patrick91 merged commit 5c0f0b0 into strawberry-graphql:main Oct 6, 2024
117 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants