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

misc(provider): Change User-Agent header signature #2964

Merged
merged 3 commits into from Feb 21, 2024

Conversation

BoboTiG
Copy link
Contributor

@BoboTiG BoboTiG commented May 23, 2023

What was wrong?

The User-Agent used to include a stringiffied class name like:

"web3.py/6.4.0/<class 'web3.providers.rpc.HTTPProvider'>"
"web3.py/6.4.0/<class 'web3.providers.async_rpc.AsyncHTTPProvider'>"

This patch sligthly changes it to:

"web3.py/6.4.0/web3.providers.rpc.HTTPProvider"
"web3.py/6.4.0/web3.providers.async_rpc.AsyncHTTPProvider"

How was it fixed?

Use type.__module__, and type.__qualname__, rather then str(type)).

Todo:

Cute Animal Picture

Put a link to a cute animal picture inside the parenthesis-->

@kclowes
Copy link
Collaborator

kclowes commented May 24, 2023

Thanks @BoboTiG! I like this change, but it's (arguably) breaking so I'd prefer to wait until v7 to pull it in. I added it to our v7 issue.

@kclowes kclowes added the v7 breaking changes considered for v7 label May 24, 2023
@reedsa reedsa force-pushed the feat-improve-provder-user-agent branch from 27cfc0d to 4c72874 Compare February 20, 2024 21:29
Copy link
Collaborator

@fselmo fselmo left a comment

Choose a reason for hiding this comment

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

I think the change here is a good one. I made some comments to clean up the tests around this. @reedsa, it looks like you requested a review here. Did you want to take care of those if you agree with them and merge? Otherwise I'm good with this. Just seems like a more explicit test makes sense here since we are trying to get a very specific outcome out of the user-agent field.

"""
provider = AsyncHTTPProvider()
headers = provider.get_request_headers()
assert "<" not in headers["User-Agent"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

These are kinda clunky.

  • We aren't testing construct_user_agent(). We are maybe indirectly "testing" it but there's no way to prove that get_request_headers() is actually calling it. So let's rename the test to test_get_request_headers().

  • Since we are now testing the request headers method, let's test both fields that should be there. We are certain of what the value should be for the content-type, so let's validate that logic entirely.

  • The docstring can go here. I think the test name along with the logic inside it is pretty clear of what this is testing for and if it's not it should be changed to reflect it.

from web3 import (
    __version__ as web3_version,
)

...

def test_get_request_headers():
    provider = AsyncHTTPProvider()

    headers = provider.get_request_headers()
    assert len(headers) == 2
    assert headers["Content-Type"] == "application/json"
    assert (
        headers["User-Agent"] == f"web3.py/{web3py_version}/"
        f"{AsyncHTTPProvider.__module__}.{AsyncHTTPProvider.__qualname__}"
    )

@@ -103,3 +103,20 @@ def test_user_provided_session():
assert isinstance(adapter, HTTPAdapter)
assert adapter._pool_connections == 20
assert adapter._pool_maxsize == 20


def test_construct_user_agent():
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should do the same with this test. See previous comment.

BoboTiG and others added 3 commits February 21, 2024 10:05
The User-Agent used to include a stringiffied class name like:

    "web3.py/6.4.0/<class 'web3.providers.rpc.HTTPProvider'>"
    "web3.py/6.4.0/<class 'web3.providers.async_rpc.AsyncHTTPProvider'>"

This patch sligthly changes it to:

    "web3.py/6.4.0/web3.providers.rpc.HTTPProvider"
    "web3.py/6.4.0/web3.providers.async_rpc.AsyncHTTPProvider"
@reedsa reedsa force-pushed the feat-improve-provder-user-agent branch from 5cb5533 to 751808b Compare February 21, 2024 17:05
@reedsa reedsa requested a review from fselmo February 21, 2024 17:13
Copy link
Collaborator

@fselmo fselmo left a comment

Choose a reason for hiding this comment

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

looks good!

@reedsa reedsa merged commit c6db197 into ethereum:main Feb 21, 2024
81 checks passed
@BoboTiG BoboTiG deleted the feat-improve-provder-user-agent branch February 21, 2024 17:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v7 breaking changes considered for v7
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants