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 incorrect query params type #13

Open
wants to merge 4 commits into
base: base-sha/d2c0fb4d2d363929c9ac10161884d004ab9cf555
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions RELEASE.md
Original file line number Diff line number Diff line change
@@ -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.

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

Is this comment correct?

Choose a reason for hiding this comment

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

Is this comment helpful?

Choose a reason for hiding this comment

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

Is the comment type correct?

Choose a reason for hiding this comment

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

Is the comment area correct?

Choose a reason for hiding this comment

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

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

4 changes: 2 additions & 2 deletions strawberry/flask/views.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
from __future__ import annotations

import warnings
from typing import TYPE_CHECKING, Any, List, Mapping, Optional, Union, cast
from typing import TYPE_CHECKING, Any, Mapping, Optional, Union, cast

from flask import Request, Response, render_template_string, request
from flask.views import View
Expand All @@ -26,7 +26,7 @@ def __init__(self, request: Request) -> None:
self.request = request

@property
def query_params(self) -> Mapping[str, Union[str, Optional[List[str]]]]:
def query_params(self) -> QueryParams:
return self.request.args.to_dict()

@property
Expand Down
9 changes: 2 additions & 7 deletions strawberry/http/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

from strawberry.http import GraphQLHTTPResponse
from strawberry.http.ides import GraphQL_IDE, get_graphql_ide_html
from strawberry.http.types import HTTPMethod
from strawberry.http.types import HTTPMethod, QueryParams

from .exceptions import HTTPException
from .typevars import Request
Expand Down Expand Up @@ -50,17 +50,12 @@ def parse_json(self, data: Union[str, bytes]) -> Any:
def encode_json(self, response_data: GraphQLHTTPResponse) -> str:
return json.dumps(response_data)

def parse_query_params(
self, params: Mapping[str, Optional[Union[str, List[str]]]]
) -> Dict[str, Any]:
def parse_query_params(self, params: QueryParams) -> Dict[str, Any]:
params = dict(params)

if "variables" in params:
variables = params["variables"]

if isinstance(variables, list):
variables = variables[0]
Comment on lines -61 to -62

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

Is this comment correct?

Choose a reason for hiding this comment

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

Is this comment helpful?

Choose a reason for hiding this comment

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

Is the comment type correct?

Choose a reason for hiding this comment

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

Is the comment area correct?

Choose a reason for hiding this comment

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

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


if variables:
params["variables"] = self.parse_json(variables)

Expand Down
4 changes: 2 additions & 2 deletions strawberry/http/types.py
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
from typing import Any, List, Mapping, Optional, Union
from typing import Any, Mapping, Optional
from typing_extensions import Literal, TypedDict

HTTPMethod = Literal[
"GET", "POST", "PUT", "DELETE", "PATCH", "HEAD", "OPTIONS", "TRACE"
]

QueryParams = Mapping[str, Optional[Union[str, List[str]]]]
QueryParams = Mapping[str, Optional[str]]


class FormData(TypedDict):
Expand Down
8 changes: 1 addition & 7 deletions strawberry/sanic/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
TYPE_CHECKING,
Any,
Dict,
List,
Mapping,
Optional,
Type,
Expand Down Expand Up @@ -43,12 +42,7 @@ def query_params(self) -> QueryParams:
# the keys are the unique variable names and the values are lists
# of values for each variable name. To ensure consistency, we're
# enforcing the use of the first value in each list.

args = cast(
Dict[str, Optional[List[str]]],
self.request.get_args(keep_blank_values=True),
)

args = self.request.get_args(keep_blank_values=True)
return {k: args.get(k, None) for k in args}

@property
Expand Down
9 changes: 9 additions & 0 deletions tests/http/test_query.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

Is this comment correct?

Choose a reason for hiding this comment

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

Is this comment helpful?

Choose a reason for hiding this comment

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

Is the comment type correct?

Choose a reason for hiding this comment

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

Is the comment area correct?

Choose a reason for hiding this comment

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

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

response = await http_client.get(

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

Is this comment correct?

Choose a reason for hiding this comment

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

Is this comment helpful?

Choose a reason for hiding this comment

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

Is the comment type correct?

Choose a reason for hiding this comment

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

Is the comment area correct?

Choose a reason for hiding this comment

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

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

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

assert response.status_code == 200

Choose a reason for hiding this comment

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

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}"

Choose a reason for hiding this comment

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

Is this comment correct?

Choose a reason for hiding this comment

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

Is this comment helpful?

Choose a reason for hiding this comment

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

Is the comment type correct?

Choose a reason for hiding this comment

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

Is the comment area correct?

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

Is this comment correct?

Choose a reason for hiding this comment

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

Is this comment helpful?

Choose a reason for hiding this comment

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

Is the comment type correct?

Choose a reason for hiding this comment

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

Is the comment area correct?

Choose a reason for hiding this comment

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

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



async def test_missing_query(http_client: HttpClient):
response = await http_client.post(
url="/graphql",
Expand Down