Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix(#56): error in handling multiple query parameters #57

Merged
merged 5 commits into from
Mar 25, 2025

Conversation

yhl-cs
Copy link
Contributor

@yhl-cs yhl-cs commented Mar 24, 2025

Summary

Fix: #56

Checklist

  • I've read CONTRIBUTING.md.
  • I understand that this PR may be closed in case there was no previous discussion. (This doesn't apply to typos!)
  • I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
  • I've updated the documentation accordingly.

Copy link

codecov bot commented Mar 24, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.43%. Comparing base (64a7be4) to head (365651e).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #57   +/-   ##
=======================================
  Coverage   97.43%   97.43%           
=======================================
  Files           9        9           
  Lines         467      467           
  Branches       36       36           
=======================================
  Hits          455      455           
  Misses          7        7           
  Partials        5        5           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@WSH032
Copy link
Owner

WSH032 commented Mar 24, 2025

Uhmmm, the type checker is complaining (although I believe multi_items() is of the correct type), can you investigate this?

If possible, could you add a test?

ref:

  • @pytest.mark.anyio()
    async def test_if_the_proxy_forwarding_is_correct(
    self, tool_4_test_fixture: Tool4TestFixture
    ) -> None:
    """测试代理服务器的转发功能是否正常."""
    client_for_conn_to_proxy_server = (
    tool_4_test_fixture.client_for_conn_to_proxy_server
    )
    proxy_server_base_url = tool_4_test_fixture.proxy_server_base_url
    # 测试目标服务器响应体转发正常
    r = await client_for_conn_to_proxy_server.get(
    proxy_server_base_url + "get/echo_headers_and_params",
    headers={"foo": "bar"},
    )
    assert r.json()["foo"] == "bar"
    # 测试客户端请求体转发正常
    r = await client_for_conn_to_proxy_server.post(
    proxy_server_base_url + "post/echo_body",
    json={"foo": "bar"},
    )
    assert r.json()["foo"] == "bar"
    # 测试目标服务文件转发正常
    file_str = "你好"
    r = await client_for_conn_to_proxy_server.put(
    proxy_server_base_url + f"put/echo_file?content={file_str}",
    )
    assert r.content.decode("utf-8") == file_str
  • @pytest.mark.anyio()
    async def test_ws_proxy(self, tool_4_test_fixture: Tool4TestFixture) -> None:
    """测试websocket代理."""
    proxy_server_base_url = tool_4_test_fixture.proxy_server_base_url
    client_for_conn_to_proxy_server = (
    tool_4_test_fixture.client_for_conn_to_proxy_server
    )
    get_request = tool_4_test_fixture.get_request
    ########## 测试数据的正常转发 ##########
    async with aconnect_ws(
    proxy_server_base_url + "echo_text", client_for_conn_to_proxy_server
    ) as ws:
    await ws.send_text("foo")
    assert await ws.receive_text() == "foo"
    async with aconnect_ws(
    proxy_server_base_url + "echo_bytes", client_for_conn_to_proxy_server
    ) as ws:
    await ws.send_bytes(b"foo")
    assert await ws.receive_bytes() == b"foo"
    ########## 测试子协议 ##########

If you want to leave it to me, I won't have time until the weekend 😂

@yhl-cs
Copy link
Contributor Author

yhl-cs commented Mar 25, 2025

multi_items() returns list[tuple[str, _CovariantValueType]] (where _CovariantValueType is declared covariant via covariant=True)1, but the HTTP library requires invariant primitive types (str|int|float|bool) in QueryParamTypes2.

The type checker blocks this as a potential type safety violation.

Perhaps this can be fixed by:

  1. Explicit type conversion
    params=cast(httpx.QueryParams, request.query_params.multi_items())

  2. Type checker ignores
    params=request.query_params.multi_items(), # pyright: ignore [reportArgumentType]

Footnotes

  1. https://github.com/encode/starlette/blob/bcdf0ad017168408060b6faab156636ced4c94f7/starlette/datastructures.py#L20

  2. https://github.com/encode/httpx/blob/9e8ab40369bd3ec2cc8bff37ab79bf5769c8b00f/httpx/_types.py#L31

@WSH032
Copy link
Owner

WSH032 commented Mar 25, 2025

I spent some time investigating this.

This is actually a design issue with httpx. They should have used Sequence[Tuple[str, PrimitiveData],] in QueryParamTypes instead of List[...]/Tuple[...].

To fix this issue, you only need to make the following modification:

- params = request.query_params.multi_items()
+ params = tuple(request.query_params.multi_items())  # list(...) will also work

This way, params will be inferred as Tuple[Tuple[str, PrimitiveData], ...] instead of List[Tuple[str, str]].


You might wonder why pyright rejected List[Tuple[str, str]]: https://stackoverflow.com/a/78532825

Fix: WSH032#56

Signed-off-by: yhl-cs <yuhl18@zju.edu.cn>
@WSH032
Copy link
Owner

WSH032 commented Mar 25, 2025

Thanks! I will release v0.3.0 now.

@WSH032 WSH032 changed the title handling duplicate query parameters fix: error in handling multiple query parameters (#56) Mar 25, 2025
@WSH032 WSH032 changed the title fix: error in handling multiple query parameters (#56) fix: error in handling multiple query parameters #56 Mar 25, 2025
@WSH032 WSH032 changed the title fix: error in handling multiple query parameters #56 fix(#56): error in handling multiple query parameters Mar 25, 2025
@WSH032 WSH032 merged commit d464f27 into WSH032:main Mar 25, 2025
19 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error in handling duplicate query parameters
2 participants