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.

Sign up for GitHub

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

Jump to bottom

Fix incorrect query params type #17

Open
wants to merge 4 commits into
base: base-sha/d2c0fb4d2d363929c9ac10161884d004ab9cf555
Choose a base branch
from
Open

Fix incorrect query params type #17

wants to merge 4 commits into from

Conversation

Hellebore
Copy link

@Hellebore Hellebore commented Jul 9, 2024 •
edited by SourceryAI
Loading

Description

This is the result of my deep-dive into this comment.

Our GET query parsing method incorrectly assumed that our query parameters could be lists. As a result some of our types were sligtly incorrect and we had an unnecessary block handling the case.

Generally it would be correct that GET query parameters could contain lists (e.g. ?variables={}&variables={} could be parsed as a list). The sanic integration even contains a warning about this. However, it turns out none of our integrations interprets query parameters which are defined multiple times as lists by default (not even sanic). I manually checked every integration and also added a test to prove this. Also double-checked the graphql http spec to verify the "variables" query parameter must indeed not be a list.

(fyi: most of these frameworks provide a getlist method which can be used to explicitly interpret multi-defined GET query parameters as a list).

Types of Changes

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

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

This pull request enhances the query parameter handling in Strawberry GraphQL by removing unnecessary checks for list-type parameters. The changes simplify the codebase and slightly improve performance, while ensuring consistent behavior across all integrations. The PR includes updates to type annotations, new tests to verify the behavior, and documentation for the release.

  • Enhancements:
    • Simplified and optimized the internal GET query parsing logic by removing unnecessary checks for list-type query parameters.
  • Documentation:
    • Added a RELEASE.md file to document the patch release and its changes.
  • Tests:
    • Added a new test to verify that query parameters are never interpreted as lists across all integrations.
  • Chores:
    • Updated type annotations in various files to reflect the correct types for query parameters.

@Hellebore
Copy link
Author

This is an experiment review for experiment review_of_reviews_20240708
Run ID: review_of_reviews_20240708/run_2024-07-09T20-20-16_v1-20-0-10-g40cd15d62
The benchmark review for this pull request can be found at #13.
This pull request was cloned from https://github.com/strawberry-graphql/strawberry/pull/3558. (Note: the URL is not a link to avoid triggering a notification on the original pull request.)

Experiment configuration 
review_config:
  # User configuration for the review
  # - benchmark - use the user config from the benchmark reviews
  # - <value> - use the value directly
  user_review_config:
    enable_ai_review: true
    enable_rule_comments: false

    enable_complexity_comments: benchmark
    enable_security_comments: benchmark
    enable_tests_comments: benchmark
    enable_comment_suggestions: benchmark
    enable_functionality_review: benchmark

    enable_pull_request_summary: benchmark
    enable_review_guide: benchmark

    enable_approvals: true

  ai_review_config:
    # The model responses to use for the experiment
    # - benchmark - use the model responses from the benchmark reviews
    # - llm - call the language model to generate responses
    model_responses:
      comments_model: llm
      comment_area_model: llm
      comment_validation_model: llm
      comment_suggestion_model: llm
      complexity_model: llm
      docstrings_model: llm
      functionality_model: llm
      security_model: llm
      tests_model: llm
      pull_request_summary_model: llm
      review_guide_model: llm
      documentation_model: llm
      overall_comments_model: llm

# The pull request dataset to run the experiment on
pull_request_dataset:
- https://github.com/mraniki/iamlistening/pull/334
- https://github.com/mraniki/cefi/pull/475
- https://github.com/mraniki/MyLLM/pull/581
- https://github.com/mraniki/dxsp/pull/689
- https://github.com/jschalk/jaar/pull/239
- https://github.com/jschalk/jaar/pull/241
- https://github.com/jschalk/jaar/pull/242
- https://github.com/iptux-src/iptux/pull/620
- https://github.com/iptux-src/iptux/pull/622
- https://github.com/hacksider/Deep-Live-Cam/pull/46
- https://github.com/mnbf9rca/super_simple_tfl_status/pull/114
- https://github.com/mnbf9rca/super_simple_tfl_status/pull/115
- https://github.com/RockProfile/Django-deployment/pull/1
- https://github.com/hbmartin/graphviz2drawio/pull/83
- https://github.com/fairdataihub/codefair-app/pull/28
- https://github.com/totaldebug/atomic-calendar-revive/pull/1518
- https://github.com/iammiracle01/portfolio/pull/1
- https://github.com/iammiracle01/portfolio/pull/2
- https://github.com/iammiracle01/portfolio/pull/3
- https://github.com/supermario-ai/gpt-crawler/pull/1
- https://github.com/hbmartin/graphviz2drawio/pull/84
- https://github.com/hbmartin/graphviz2drawio/pull/85
- https://github.com/dreamerminsk/tasked/pull/85
- https://github.com/dreamerminsk/tasked/pull/86
- https://github.com/dreamerminsk/tasked/pull/84
- https://github.com/haalasz/fm-tools/pull/9
- https://github.com/haalasz/fm-tools/pull/10
- https://github.com/iptux-src/iptux/pull/619
- https://github.com/code-Harsh247/medsymptom/pull/1
- https://github.com/code-Harsh247/medsymptom/pull/2
- https://github.com/cpp-lln-lab/bidspm/pull/1263
- https://github.com/cpp-lln-lab/bidspm/pull/1264
- https://github.com/cpp-lln-lab/bidspm/pull/1265
- https://github.com/luiscarlop/Judge_AI/pull/22
- https://github.com/NoNormalCreeper/nonebot_plugin_wolfram/pull/6
- https://github.com/osama1998H/kalima/pull/39
- https://github.com/osama1998H/kalima/pull/40
- https://github.com/osama1998H/kalima/pull/41
- https://github.com/jackdewinter/pymarkdown/pull/1131
- https://github.com/Eliver-Salazar/PED/pull/12
- https://github.com/NextAlone/Nagram/pull/40
- https://github.com/strawberry-graphql/strawberry-django/pull/575
- https://github.com/strawberry-graphql/strawberry/pull/3558
- https://github.com/strawberry-graphql/strawberry/pull/3559
- https://github.com/Ruin2121/yahooquery/pull/9
- https://github.com/gdsfactory/gdsfactory/pull/2951
- https://github.com/gdsfactory/gdsfactory/pull/2954
- https://github.com/gdsfactory/gdsfactory/pull/2956
- https://github.com/gdsfactory/gdsfactory/pull/2957
- https://github.com/gdsfactory/cspdk/pull/51
review_comment_labels:
- label: correct
  question: Is this comment correct?
- label: helpful
  question: Is this comment helpful?
- label: comment-type
  question: Is the comment type correct?
- label: comment-area
  question: Is the comment area correct?
- label: llm-test
  question: |
    What type of LLM test could this comment become?
    - 👍 - this comment is really good/important and we should always make it
    - 👎 - this comment is really bad and we should never make it
    - no reaction - don't turn this comment into an LLM test

# Benchmark reviews generated by running
#   python -m scripts.experiment benchmark <experiment_name>
benchmark_reviews:
- dataset_pull_request: https://github.com/mraniki/iamlistening/pull/334
  review_pull_request: https://github.com/sourcery-ai-experiments/iamlistening/pull/35
- dataset_pull_request: https://github.com/mraniki/cefi/pull/475
  review_pull_request: https://github.com/sourcery-ai-experiments/cefi/pull/36
- dataset_pull_request: https://github.com/mraniki/MyLLM/pull/581
  review_pull_request: https://github.com/sourcery-ai-experiments/MyLLM/pull/4
- dataset_pull_request: https://github.com/mraniki/dxsp/pull/689
  review_pull_request: https://github.com/sourcery-ai-experiments/dxsp/pull/14
- dataset_pull_request: https://github.com/iptux-src/iptux/pull/620
  review_pull_request: https://github.com/sourcery-ai-experiments/iptux/pull/4
- dataset_pull_request: https://github.com/iptux-src/iptux/pull/622
  review_pull_request: https://github.com/sourcery-ai-experiments/iptux/pull/5
- dataset_pull_request: https://github.com/hacksider/Deep-Live-Cam/pull/46
  review_pull_request: https://github.com/sourcery-ai-experiments/Deep-Live-Cam/pull/2
- dataset_pull_request: https://github.com/mnbf9rca/super_simple_tfl_status/pull/114
  review_pull_request: https://github.com/sourcery-ai-experiments/super_simple_tfl_status/pull/1
- dataset_pull_request: https://github.com/mnbf9rca/super_simple_tfl_status/pull/115
  review_pull_request: https://github.com/sourcery-ai-experiments/super_simple_tfl_status/pull/2
- dataset_pull_request: https://github.com/hbmartin/graphviz2drawio/pull/83
  review_pull_request: https://github.com/sourcery-ai-experiments/graphviz2drawio/pull/1
- dataset_pull_request: https://github.com/iammiracle01/portfolio/pull/1
  review_pull_request: https://github.com/sourcery-ai-experiments/portfolio/pull/2
- dataset_pull_request: https://github.com/iammiracle01/portfolio/pull/2
  review_pull_request: https://github.com/sourcery-ai-experiments/portfolio/pull/1
- dataset_pull_request: https://github.com/iammiracle01/portfolio/pull/3
  review_pull_request: https://github.com/sourcery-ai-experiments/portfolio/pull/3
- dataset_pull_request: https://github.com/supermario-ai/gpt-crawler/pull/1
  review_pull_request: https://github.com/sourcery-ai-experiments/gpt-crawler/pull/1
- dataset_pull_request: https://github.com/hbmartin/graphviz2drawio/pull/84
  review_pull_request: https://github.com/sourcery-ai-experiments/graphviz2drawio/pull/2
- dataset_pull_request: https://github.com/dreamerminsk/tasked/pull/86
  review_pull_request: https://github.com/sourcery-ai-experiments/tasked/pull/14
- dataset_pull_request: https://github.com/dreamerminsk/tasked/pull/84
  review_pull_request: https://github.com/sourcery-ai-experiments/tasked/pull/15
- dataset_pull_request: https://github.com/haalasz/fm-tools/pull/9
  review_pull_request: https://github.com/sourcery-ai-experiments/fm-tools/pull/2
- dataset_pull_request: https://github.com/haalasz/fm-tools/pull/10
  review_pull_request: https://github.com/sourcery-ai-experiments/fm-tools/pull/3
- dataset_pull_request: https://github.com/iptux-src/iptux/pull/619
  review_pull_request: https://github.com/sourcery-ai-experiments/iptux/pull/6
- dataset_pull_request: https://github.com/code-Harsh247/medsymptom/pull/1
  review_pull_request: https://github.com/sourcery-ai-experiments/medsymptom/pull/1
- dataset_pull_request: https://github.com/code-Harsh247/medsymptom/pull/2
  review_pull_request: https://github.com/sourcery-ai-experiments/medsymptom/pull/2
- dataset_pull_request: https://github.com/NoNormalCreeper/nonebot_plugin_wolfram/pull/6
  review_pull_request: https://github.com/sourcery-ai-experiments/nonebot_plugin_wolfram/pull/1
- dataset_pull_request: https://github.com/osama1998H/kalima/pull/39
  review_pull_request: https://github.com/sourcery-ai-experiments/kalima/pull/1
- dataset_pull_request: https://github.com/osama1998H/kalima/pull/40
  review_pull_request: https://github.com/sourcery-ai-experiments/kalima/pull/2
- dataset_pull_request: https://github.com/osama1998H/kalima/pull/41
  review_pull_request: https://github.com/sourcery-ai-experiments/kalima/pull/3
- dataset_pull_request: https://github.com/jackdewinter/pymarkdown/pull/1131
  review_pull_request: https://github.com/sourcery-ai-experiments/pymarkdown/pull/9
- dataset_pull_request: https://github.com/Eliver-Salazar/PED/pull/12
  review_pull_request: https://github.com/sourcery-ai-experiments/PED/pull/3
- dataset_pull_request: https://github.com/NextAlone/Nagram/pull/40
  review_pull_request: https://github.com/sourcery-ai-experiments/Nagram/pull/1
- dataset_pull_request: https://github.com/strawberry-graphql/strawberry-django/pull/575
  review_pull_request: https://github.com/sourcery-ai-experiments/strawberry-django/pull/2
- dataset_pull_request: https://github.com/strawberry-graphql/strawberry/pull/3558
  review_pull_request: https://github.com/sourcery-ai-experiments/strawberry/pull/13
- dataset_pull_request: https://github.com/strawberry-graphql/strawberry/pull/3559
  review_pull_request: https://github.com/sourcery-ai-experiments/strawberry/pull/14
- dataset_pull_request: https://github.com/Ruin2121/yahooquery/pull/9
  review_pull_request: https://github.com/sourcery-ai-experiments/yahooquery/pull/1

@SourceryAI
Copy link

SourceryAI commented Jul 9, 2024 •
edited
Loading

Reviewer's Guide by Sourcery

This pull request fixes incorrect query parameter type handling in GET requests by removing unnecessary list checks and updating type annotations. It simplifies the internal GET query parsing logic, adds a test to verify the behavior across all integrations, and includes a release note for the patch update.

File-Level Changes

Files Changes
strawberry/sanic/views.py
strawberry/http/base.py
strawberry/http/types.py
strawberry/flask/views.py		 Removed unnecessary list handling for query parameters and updated type annotations to reflect that query parameters are not interpreted as lists
tests/http/test_query.py
RELEASE.md		 Added a new test case and release notes to verify and document the changes in query parameter handling
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.

SourceryAI
SourceryAI approved these changes Jul 9, 2024
View reviewed changes
Copy link

@SourceryAI SourceryAI left a comment

Choose a reason for hiding this comment

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

Hey @Hellebore - 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: all looks good

LangSmith trace

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SourceryAI SourceryAI SourceryAI approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Hellebore @SourceryAI @DoctorJohn