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

Refactor the middleware setup [starts v7 breaking changes] #3169

Merged
merged 24 commits into from
Jan 18, 2024

Conversation

fselmo
Copy link
Collaborator

@fselmo fselmo commented Dec 15, 2023

What was wrong?

In order to process requests and responses separately and to eventually support batch requests, we need to uncouple the make_request() method from being sandwiched along with all the middleware in each request. If we want APIs that can process request params separately from making a request, we need to refactor quite a bit of the code to massage this sort of logic in.

How was it fixed?

In this PR I am refactoring the middleware from the functional programming paradigm of a sandwiched request into a class-based structure with request processors and response processors. The ultimate end result for most requests is these classes still wrap the make_request, as it is often convenient to be able to replace the make_request function in order for a specific request to not be made under certain conditions some middleware necessitate (currently this is only true for the local_filter_middleware).

Imo this class-based structure makes the middleware a bit easier to read and create. If a middleware only processes request, it only needs to define a request processor; if the response is the only thing the middleware touches, it can override the response processor on the class, etc. In case the make_request method itself needs to be altered, the _wrap_make_request() method can be overridden and that would behave similarly to the way the middlewares currently work.

Some things to note:

  • This refactor removes the request retry middleware from the provider and removes the concept of provider middleware altogether. Instead, exception retry logic is done as a configuration on the HTTP providers.
  • This refactor removes the result generator middlewares and opts for a request_mocker fixture for dealing with mocked requests. This has proved to be quite nice to work with, at least for the current tests, and it removes the need to create, inject, and clean up middleware for each test. Instead, the context manager takes care of mocking while inside the manager and cleaning up when exiting the context manager for both sync and async.
  • The simple_cache_middleware is moved to a decorator on the make_request method for the providers. This can be turned on and configured as configurations on the appropriate provider classes.


Todo:

  • Add entry to the release notes
  • Revise some of the type hinting. In particular the type hinting around the Middleware within manager and provider classes
  • Update documentation to reflect above changes ( will be in a separate PR)

Cute Animal Picture

IMG-20240103-WA0005

@fselmo fselmo changed the title Refactor the middleware setup Refactor the middleware setup [starts v7 breaking changes] Dec 15, 2023
@fselmo fselmo force-pushed the refactor-middleware-setup branch 8 times, most recently from e59a152 to ebd0f01 Compare December 15, 2023 22:16
@fselmo fselmo marked this pull request as ready for review December 19, 2023 23:50
web3/middleware/cache.py Outdated Show resolved Hide resolved
fselmo added a commit to fselmo/web3.py that referenced this pull request Jan 16, 2024
- Refactor middlewares as classes with request processors and response processors
- Apply this refactor to some existing middlewares and test
  - gas_price_strategy_middleware
  - validation / formatting middleware
  - attrdict_middleware
  - (incomplete) ens_name_to_address_middleware
- Refactor the exception_retry_middleware to be a configuration on the http providers
- Remove all caching middleware.
- Refactor request caching via configurations on the provider classes and decorators on their respective ``make_request()`` methods.
- This middleware depends on a request always being made when middleware are processed. This isn't how the middleware is processed for websockets and the separation of middleware into request param processing and response processing further helps with being able to support batch requests. Remove all middleware that were dependent on a request always being sandwiched between request processing and response processing.
- Add a ``RequestMocker`` class with ``request_mocker`` pytest fixture to fascilitate request mocking now that the middleware doesn't sequester the ``make_request()`` call. Since we can't swap the ``make_request()`` on the fly, mock results and errors using this test utility class.
- Linting
- Some cleanup along the way
- closes ethereum#2995
- ``ens_name_to_address_middleware`` would return the ``bytes`` address, rather than checksummed address. Since this happens after the ``request_formatters`` have been applied, this would override the results from the ``ABI_REQUEST_FORMATTERS`` which take ``address`` types and format them to a checksummed hex address.
…ware

- Use request_mocker where we can
- result_processors take care of the formatting... this was causing the ``None`` value for logsBloom to raise when mocked
- The middleware changed names in the refactor, but this commit puts the finishing touches and closes ethereum#899
- Make request caching more robust by re-introducing the lock from the middleware as well as checking for "error" and None responses and not caching those.
- Re-introduce the tests for the simple_cache_middleware as caching utils tests.

[Unrelated]

- Keep the decorator logic on ``make_request`` when using the ``request_mocker`` fixture so we can still utilized the request mocker for testing cached requests.
fselmo added a commit to fselmo/web3.py that referenced this pull request Jan 16, 2024
@fselmo fselmo changed the base branch from v6 to main January 16, 2024 15:47
Copy link
Contributor

@pacrob pacrob left a comment

Choose a reason for hiding this comment

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

Nicely done! 🏋️
GTG for me.

Copy link
Collaborator

@kclowes kclowes left a comment

Choose a reason for hiding this comment

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

Left a few nits to take or leave, but looks good!

docs/middleware.rst Show resolved Hide resolved
web3/_utils/module_testing/utils.py Show resolved Hide resolved
tests/core/caching-utils/test_request_caching.py Outdated Show resolved Hide resolved
web3/main.py Outdated Show resolved Hide resolved
web3/middleware/base.py Show resolved Hide resolved
web3/providers/async_base.py Outdated Show resolved Hide resolved
web3/providers/base.py Outdated Show resolved Hide resolved
@Nov1kov

This comment was marked as off-topic.

- Use an internal method for the ``NamedMiddlewareOnion`` to generate
  the tuple of middlewares, instead of relying on ``tuple()``.
@fselmo
Copy link
Collaborator Author

fselmo commented Jan 18, 2024

@kclowes I think I addressed all your comments. Let me know if there's anything else outstanding and how you feel about the tupleization commit.

@kclowes
Copy link
Collaborator

kclowes commented Jan 18, 2024

LGTM! 🚀

@fselmo fselmo force-pushed the refactor-middleware-setup branch 2 times, most recently from 2b3c3d8 to a548a4e Compare January 18, 2024 23:38
@fselmo fselmo merged commit 635b542 into ethereum:main Jan 18, 2024
99 checks passed
fselmo added a commit that referenced this pull request Jan 18, 2024
@fselmo fselmo mentioned this pull request Jan 22, 2024
1 task
@fselmo fselmo deleted the refactor-middleware-setup branch February 3, 2024 23:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants