Tracing method "on_request_start" does not receive full URL

[selcukcihan]

Describe the bug

After a release in yarl which had a change in behaviour for URL's update_query aio-libs/yarl#854 (comment) method, the URL received in the on_request_start tracing hook lacks the query string part.

The problem only happens with yarl version 1.9.1. It works fine with previous version which is 1.8.2

To Reproduce

1. Add a tracer
2. Implement on_request_start for the tracer
3. Issue a request that has a query string
4. Observe that the params.url does not have the query string.

Expected behavior

I expect that the URL passed within params to the tracer hook contains the query string part as well and resembles the original URL exactly.

Logs/tracebacks

See https://github.com/aio-libs/yarl/issues/854#issuecomment-1521670598 please for the exact issue that's causing this problem

Python Version

$ python --version
Python 3.9.16 (main, Mar 15 2023, 15:55:54) 
[Clang 14.0.0 (clang-1400.0.29.202)] on darwin

aiohttp Version

$ python -m pip show aiohttp
3.8.4

multidict Version

$ python -m pip show multidict
6.0.4

yarl Version

$ python -m pip show yarl
1.9.1 (the issue is not seen with version 1.8.2 as explained above)

OS

macOS

Related component

Client

Additional context

In this piece of the code

aiohttp/aiohttp/client.py

Line 417 in 315ae90

await trace.send_request_start(method, url.update_query(params), headers)

params is None and the url actually has the query string in it. But the call to url.update_query(params) is removing the query string which is passed to the tracing hook method.

Code of Conduct

• I agree to follow the aio-libs Code of Conduct

[Dreamsorcerer]

Probably just needs a params = params or {} line.
This line looks like it'll also need changing:

aiohttp/aiohttp/client.py

Line 600 in 315ae90

params = None

Would you like try creating a test to reproduce the issue (or maybe there's already a failing test if the requirements pinning is updated)?

[danigm]

Would you like try creating a test to reproduce the issue (or maybe there's already a failing test if the requirements pinning is updated)?

There's currently a broken test, right now with the latest yarl release you can see this error:

_____________________________________________________________ test_request_tracing_url_params[pyloop] ______________________________________________________________

loop = <_UnixSelectorEventLoop running=False closed=False debug=False>, aiohttp_client = <function aiohttp_client.<locals>.go at 0x7f34024fcaf0>

    async def test_request_tracing_url_params(loop: Any, aiohttp_client: Any) -> None:
        async def root_handler(request):
            return web.Response()
    
        async def redirect_handler(request):
            raise web.HTTPFound("/")
    
        app = web.Application()
        app.router.add_get("/", root_handler)
        app.router.add_get("/redirect", redirect_handler)
    
        mocks = [mock.Mock(side_effect=make_mocked_coro(mock.Mock())) for _ in range(7)]
        (
            on_request_start,
            on_request_redirect,
            on_request_end,
            on_request_exception,
            on_request_chunk_sent,
            on_response_chunk_received,
            on_request_headers_sent,
        ) = mocks
    
        trace_config = aiohttp.TraceConfig(
            trace_config_ctx_factory=mock.Mock(return_value=mock.Mock())
        )
        trace_config.on_request_start.append(on_request_start)
        trace_config.on_request_redirect.append(on_request_redirect)
        trace_config.on_request_end.append(on_request_end)
        trace_config.on_request_exception.append(on_request_exception)
        trace_config.on_request_chunk_sent.append(on_request_chunk_sent)
        trace_config.on_response_chunk_received.append(on_response_chunk_received)
        trace_config.on_request_headers_sent.append(on_request_headers_sent)
    
        session = await aiohttp_client(app, trace_configs=[trace_config])
    
        def reset_mocks() -> None:
            for m in mocks:
                m.reset_mock()
    
        def to_trace_urls(mock_func: mock.Mock) -> List[URL]:
            return [call_args[0][-1].url for call_args in mock_func.call_args_list]
    
        def to_url(path: str) -> URL:
            return session.make_url(path)
    
        # Standard
        for req in [
            lambda: session.get("/?x=0"),
            lambda: session.get("/", params=dict(x=0)),
        ]:
            reset_mocks()
            async with req() as resp:
                await resp.text()
                assert to_trace_urls(on_request_start) == [to_url("/?x=0")]
                assert to_trace_urls(on_request_redirect) == []
>               assert to_trace_urls(on_request_end) == [to_url("/?x=0")]
E               AssertionError: assert [URL('http://....0.1:44135/')] == [URL('http://...:44135/?x=0')]
E                 At index 0 diff: URL('http://127.0.0.1:44135/') != URL('http://127.0.0.1:44135/?x=0')
E                 Full diff:
E                 - [URL('http://127.0.0.1:44135/?x=0')]
E                 ?                              ----
E                 + [URL('http://127.0.0.1:44135/')]

[danigm]

Probably the problem is related to this change:

aio-libs/yarl@dd86b34