Fix incorrect query params type #13

sourcery-ai-experiments-bot · 2024-07-08T03:05:13Z

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

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 fixes the incorrect handling of query parameters by ensuring they are never interpreted as lists. It simplifies the internal GET query parsing logic and adds a test to verify the correct behavior. Additionally, a RELEASE.md file has been added to document the patch release.

Bug Fixes:
- Fixed incorrect handling of query parameters by ensuring they are never interpreted as lists.
Enhancements:
- Simplified the internal GET query parsing logic by removing unnecessary checks for list types.
Documentation:
- Added a RELEASE.md file to document the patch release.
Tests:
- Added a test to verify that query parameters are never interpreted as lists.

sourcery-ai-experiments-bot · 2024-07-08T03:05:14Z

This is a benchmark review for experiment review_of_reviews_20240708.
Run ID: review_of_reviews_20240708/benchmark_2024-07-08T00-18-37_v1-20-0-4-gb56c705c9.

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: benchmark
      comment_area_model: benchmark
      comment_validation_model: benchmark
      comment_suggestion_model: benchmark
      complexity_model: benchmark
      docstrings_model: benchmark
      functionality_model: benchmark
      security_model: benchmark
      tests_model: benchmark
      pull_request_summary_model: benchmark
      review_guide_model: benchmark

# 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: []

SourceryAI · 2024-07-08T03:05:40Z

Reviewer's Guide by Sourcery

This pull request fixes the incorrect type handling of query parameters in GET requests by ensuring they are not interpreted as lists. It simplifies the internal GET query parsing logic by removing unnecessary checks, adds a test to verify the correct behavior, and includes a release note documenting the changes.

File-Level Changes

Files	Changes
`strawberry/sanic/views.py` `strawberry/http/base.py` `strawberry/http/types.py` `strawberry/flask/views.py`	Simplified the internal GET query parsing logic by removing unnecessary checks for list types and updating the QueryParams type.

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

Hey @sourcery-ai-experiments-bot - I've reviewed your changes and they look great!

Here's what I looked at during the review

🟡 General issues: 1 issue found
🟢 Security: all looks good
🟡 Testing: 4 issues found
🟢 Complexity: all looks good
🟡 Documentation: 1 issue found

LangSmith trace

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

SourceryAI · 2024-07-08T03:07:53Z

strawberry/http/base.py

-            if isinstance(variables, list):
-                variables = variables[0]


issue (bug_risk): Removal of list handling for variables

The code no longer checks if variables is a list and processes it accordingly. Ensure that variables will never be a list, or this change might introduce bugs.

Is this comment correct?

Is this comment helpful?

Is the comment type correct?

Is the comment area correct?

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

SourceryAI · 2024-07-08T03:07:53Z

tests/http/test_query.py

@@ -168,6 +168,15 @@ async def test_passing_invalid_json_get(http_client: HttpClient):
    assert "Unable to parse request body as JSON" in response.text


+async def test_query_parameters_are_never_interpreted_as_list(http_client: HttpClient):


suggestion (testing): Consider adding edge cases for query parameters

The test test_query_parameters_are_never_interpreted_as_list is a good start. However, it would be beneficial to add more edge cases, such as testing with different data types, empty values, and malformed JSON in the variables parameter to ensure robustness.

Is this comment correct?

Is this comment helpful?

Is the comment type correct?

Is the comment area correct?

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

SourceryAI · 2024-07-08T03:07:53Z

tests/http/test_query.py

@@ -168,6 +168,15 @@ async def test_passing_invalid_json_get(http_client: HttpClient):
    assert "Unable to parse request body as JSON" in response.text


+async def test_query_parameters_are_never_interpreted_as_list(http_client: HttpClient):
+    response = await http_client.get(


suggestion (testing): Add assertion for error handling

It would be useful to add an assertion to check that the server returns an appropriate error message or status code when the variables parameter is malformed or contains invalid JSON.

Suggested change

response = await http_client.get(

response = await http_client.get(

url='/graphql?query=query($name: String!) { hello(name: $name) }&variables={"name": "Jake"}&variables={"name": "Jake"}',

)

assert response.status_code == 400

assert "Invalid JSON" in response.text

Is this comment correct?

Is this comment helpful?

Is the comment type correct?

Is the comment area correct?

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

SourceryAI · 2024-07-08T03:07:53Z

tests/http/test_query.py

+        url='/graphql?query=query($name: String!) { hello(name: $name) }&variables={"name": "Jake"}&variables={"name": "Jake"}',
+    )
+
+    assert response.status_code == 200


suggestion (testing): Consider using a more descriptive assertion message

Using a more descriptive assertion message can help with debugging if the test fails. For example: assert response.status_code == 200, f"Expected status code 200 but got {response.status_code}".

Suggested change

assert response.status_code == 200

assert response.status_code == 200, f"Expected status code 200 but got {response.status_code}"

Is this comment correct?

Is this comment helpful?

Is the comment type correct?

Is the comment area correct?

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

SourceryAI · 2024-07-08T03:07:53Z

tests/http/test_query.py

+    )
+
+    assert response.status_code == 200
+    assert response.json["data"] == {"hello": "Hello Jake"}


suggestion (testing): Check for additional response fields

Consider checking for additional fields in the response, such as errors or extensions, to ensure the response is fully validated.

Suggested change

assert response.json["data"] == {"hello": "Hello Jake"}

assert response.json["data"] == {"hello": "Hello Jake"}

assert "errors" not in response.json

assert "extensions" not in response.json

Is this comment correct?

Is this comment helpful?

Is the comment type correct?

Is the comment area correct?

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

SourceryAI · 2024-07-08T03:07:53Z

RELEASE.md

@@ -0,0 +1,3 @@
+Release type: patch
+
+This release removes an unnecessary check from our internal GET query parsing logic making it simpler and (insignificantly) faster.


suggestion (documentation): Consider adding a comma for clarity.

Adding a comma after 'logic' would improve readability: 'This release removes an unnecessary check from our internal GET query parsing logic, making it simpler and (insignificantly) faster.'

Suggested change

This release removes an unnecessary check from our internal GET query parsing logic making it simpler and (insignificantly) faster.

This release removes an unnecessary check from our internal GET query parsing logic, making it simpler and (insignificantly) faster.

Is this comment correct?

Is this comment helpful?

Is the comment type correct?

Is the comment area correct?

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

DoctorJohn added 4 commits July 7, 2024 18:32

Fix assumption query params could be lists

562070a

Add release file

b6fe116

Test that variables are still successfully used

073d353

Lint and format

36da65e

SourceryAI approved these changes Jul 8, 2024

View reviewed changes

This was referenced Jul 8, 2024

Fix incorrect query params type #15

Open

Fix incorrect query params type #17

Open

		@@ -168,6 +168,15 @@ async def test_passing_invalid_json_get(http_client: HttpClient):
		assert "Unable to parse request body as JSON" in response.text


		async def test_query_parameters_are_never_interpreted_as_list(http_client: HttpClient):

	assert response.status_code == 200
	assert response.status_code == 200, f"Expected status code 200 but got {response.status_code}"

		@@ -0,0 +1,3 @@
		Release type: patch

		This release removes an unnecessary check from our internal GET query parsing logic making it simpler and (insignificantly) faster.

Fix incorrect query params type #13

Are you sure you want to change the base?

Fix incorrect query params type #13

Conversation

sourcery-ai-experiments-bot commented Jul 8, 2024 • edited by SourceryAI Loading

Description

Types of Changes

Checklist

Summary by Sourcery

sourcery-ai-experiments-bot commented Jul 8, 2024

SourceryAI commented Jul 8, 2024 • edited Loading

Reviewer's Guide by Sourcery

File-Level Changes

SourceryAI left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sourcery-ai-experiments-bot commented Jul 8, 2024 •

edited by SourceryAI

Loading

SourceryAI commented Jul 8, 2024 •

edited

Loading