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

improve injection code, make backwards compat explicit, make ssl-api explicit #268

Merged
merged 4 commits into from
Nov 25, 2024

Conversation

betaboon
Copy link
Contributor

@betaboon betaboon commented Nov 23, 2024

This PR introduces the following changes:

  • make the injection-code more readable
  • make the injection-code more explicit with regards to version-specific backwards-compat
  • introduce dedicated ssl.context.mock_wrap_socket to mirror stdlib api
  • convert MocketSSLContext.wrap_socket and .wrap_bio to instance-methods to mirror stdlib api
  • introduce dedicated urllib3.mock_ssl_wrap_socket methods mirror urllib3 api
  • use a real ssl.SSLContext in MocketSSLSocket instead of urllib3.ssl_wrap_socket

@coveralls
Copy link

coveralls commented Nov 23, 2024

Coverage Status

coverage: 99.025% (-0.2%) from 99.255%
when pulling a155190 on betaboon:refactor-inject
into 0da2722 on mindflayer:main.

@coveralls
Copy link

Coverage Status

coverage: 98.876% (-0.4%) from 99.255%
when pulling 199e903 on betaboon:refactor-inject
into 0da2722 on mindflayer:main.

3 similar comments
@coveralls
Copy link

Coverage Status

coverage: 98.876% (-0.4%) from 99.255%
when pulling 199e903 on betaboon:refactor-inject
into 0da2722 on mindflayer:main.

@coveralls
Copy link

Coverage Status

coverage: 98.876% (-0.4%) from 99.255%
when pulling 199e903 on betaboon:refactor-inject
into 0da2722 on mindflayer:main.

@coveralls
Copy link

Coverage Status

coverage: 98.876% (-0.4%) from 99.255%
when pulling 199e903 on betaboon:refactor-inject
into 0da2722 on mindflayer:main.

@betaboon betaboon marked this pull request as draft November 23, 2024 19:47
@betaboon betaboon changed the title Improve readability and backwards-compatibility of injection-code improve injection code, make backwards compat explicit, make ssl-api explicit Nov 23, 2024
@betaboon betaboon marked this pull request as ready for review November 23, 2024 20:05
@betaboon betaboon marked this pull request as draft November 23, 2024 20:11
@betaboon betaboon marked this pull request as ready for review November 23, 2024 20:14
@mindflayer
Copy link
Owner

mindflayer commented Nov 23, 2024

I like the function _replace, it makes the code cleaner. I am not sure I see the need for dividing the injecting code into sub-functions. I feet it's a touch of over-engineering with no real benefit. The module we are patching is clearly there already (socket/ssl/etc), and dividing them in blocks should be enough to keep them tidy.
I will sleep on it, but I am not 100% sure I like this level of abstraction for something that simple as patching Python modules.

@mindflayer
Copy link
Owner

Also, have a look at code coverage, not sure what happened but it went down.

@betaboon
Copy link
Contributor Author

sure. take your time.

i my mind having a dedicated place for each injection logic is more readable, especially considering the python- or urllib3-version specific things.

@mindflayer
Copy link
Owner

mindflayer commented Nov 23, 2024

That's the other thing, I don't think we need to add a new dependency for something as simple as trying to import something from a module.
I'd keep the with contextlib.suppress(ImportError) instead. It's more robust and easier to maintain, and it does not need external modules.

@betaboon
Copy link
Contributor Author

sleep on it and ping me tomorrow and I'll take care of whatever decision you come to :)

@mindflayer
Copy link
Owner

mindflayer commented Nov 23, 2024

I've just had an idea. It would be nice if we had a dictionary of everything we patched, with the real and the mocked version, and use it to bring the status back, instead of using variables to know if I patched or not that thing like it is currently done.
If could be something that the _replace functions does for free. We just need to keep track of the changes.

@betaboon
Copy link
Contributor Author

I'll give it a try

@mindflayer
Copy link
Owner

mindflayer commented Nov 23, 2024

It could be literally used both ways. As a way to define the monkey-patches and to bring back their counterparts.

@betaboon
Copy link
Contributor Author

ok.
i just pushed the following changes:

  • removed packaging
  • removed the individual functions from inject.py
  • introduced something that might be close to your idea

mocket/inject.py Outdated Show resolved Hide resolved
mocket/utils.py Outdated Show resolved Hide resolved
Copy link

Copy link
Owner

@mindflayer mindflayer left a comment

Choose a reason for hiding this comment

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

LGTM!

@mindflayer mindflayer merged commit a5b5e34 into mindflayer:main Nov 25, 2024
8 of 9 checks passed
@betaboon betaboon deleted the refactor-inject branch November 25, 2024 11:00
mindflayer pushed a commit that referenced this pull request Nov 25, 2024
…explicit (#268)

* refactor: make injection code more readable and make backwards-compat explicit
* refactor: move ssl socket-wrapping code to ssl/socket.py
* refactor: convert MocketSSLContext.wrap_socket and wrap_bio to instance-methods
* refactor: MocketSSLSocket use proper ssl-context instead of urllib3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants