Async Support

[sam-kleiner]

Here is a first pass for async support. #17

I wanted to get some feedback before moving forward with outstanding todos. This implementation was focused on 100% backwards compatibility and no code duplication. Because of that I had to use some generator magic that made the code more complicated than if I had made breaking changes or had some code duplication. If you would prefer another direction in that regard let me know.

I did not yet take on the "decrease in SSL cipher suite strength." Its just hard coded to verify false. I would appreciate some example calls that would produce these errors you mentioned so I can do some manual testing on the async methods.

All existing tests still pass on 3.12, didnt test others yet

TODOS:

• async tests
• decrease in SSL cipher suite strength (see comment in Implement an async mode #17)

[meeb]

Thanks for the PR! This is pretty much exactly how I was planning to structure this so very happy to look at merging this. I believe several of the RDAP endpoints which were using extremely out of date SSL handshakes have been updated so the previous examples may not be relevant. This was originally handled and fixed in the first issue #1 with commit 6e67019 however the RDAP endpoint for .work which is now https://rdap.nic.work/ does support modern handshakes and doesn't require downgrading:

$ curl -sI --tlsv1.2 https://rdap.nic.work/ | grep HTTP
HTTP/1.1 400 

Since I made that comment it may be getting to the point where it's viable to just drop the SSL downgrade option unless someone can find a TLD that isn't supported. Even .uz has switched to https://rdap.cctld.uz/ which supports TLSv1.2 and that was previously the sole HTTP only served RDAP endpoint.

If you add a couple of tests for the async stuff the rest looks good to me, I'll give it another review and make any minor nitpick comments.

After merging I'll go through it and depreciate the allow_insecure_ssl option and clean up the README. Thanks again.

[meeb]

Had a local test and further review of this, everything else looks great. Pop that version import back in and add a couple of top level tests for the async interface and this is good to merge.

[sam-kleiner]

Fixed the version issue and did some other misc cleanup.

[sam-kleiner]

Had a local test and further review of this, everything else looks great. Pop that version import back in and add a couple of top level tests for the async interface and this is good to merge.

So I'm not 100% sure what you mean here.

If you mean the AsyncQuery and AsyncBoostrap classes idk how useful that would be since the existing tests all hit the same code of the new BaseQuery and BaseBootstrap classes (the only diff is the part that needs to send the request).

If you mean the public interface, I don't see any tests for the sync version of the public interface. I took the liberty of writing sync and async versions here. I wanted to run this by you because the way I did it requires pytest (I updated your make test command), but some people have strong opinions on pytest vs unittest.

These test use responses and pytest_httpx for mocking the internal http calls, so you can easily run the top level api all the way through without making actual requests. The responses library has a handy record requests option that saves to yaml files. I added a small helper function to load those same files into pytest_httpx for reuse in both sync and async tests.

To save new mocks change your test function from this

@responses.activate
# @_recorder.record(file_path=RESPONSES / 'boostrap.yaml')
def test_bootstrap_interface(self):
    responses._add_from_file(RESPONSES / "boostrap.yaml")

to this

# @responses.activate
@_recorder.record(file_path=RESPONSES / 'boostrap.yaml')
def test_bootstrap_interface(self):
    # responses._add_from_file(RESPONSES / "boostrap.yaml")

run the test and it saves real network requests to the file for later

Thoughts? If you like the tests in the link above I will merge that branch into this PR branch. If not let me know.

[meeb]

Yep I just meant a basic check to make sure that the new parameters or methods exposed to the public API had a check that they were handled correctly and any accidental regressions in the future were caught. Honestly I'd not looked at the test code properly for a couple of years and there not being basic interface checks for the existing interface is an oversight on my part.

Full request replay testing would be awesome, feel free to merge those in, thanks!

I have no objection to using pytest. I generally start with the standard library and add libraries as required so that's why it's initially unittest rather than any specific library zealotry. Full mock end to end testing would be worth the switch.

[sam-kleiner]

Great those tests are merged in, anything else to get this over the finish line?

[meeb]

Excellent, thanks again! Nope, nothing I can think of for now. I'll look at giving this a final once-over and bundling it into a release when I get time over the weekend.

[meeb]

Ignore the checks failing that's just dev packages not being installed in the action.

[sam-kleiner]

Awesome, thanks for the quick review and feedback!

[sam-kleiner]

Ignore the checks failing that's just dev packages not being installed in the action.

Again I did all this in 3.12. I dont think I used anything that would break older ones, but just wanted to call it out.

[meeb]

All good, nothing looks difficult to tweak if it does complain on 3.8, and I probably need to add 3.12 and drop 3.7 by now anyway.

Off topic question but are you using this library for anything interesting?

[sam-kleiner]

Not yet, just doing a PoC for some asset discovery and enrichment, but I'll let you know!

[meeb]

Your async PR along with fixes to the test actions, some other tidying and async usage docs in the README are all bundled into the new 3.0.0 release. Thanks again for your effort.