Feat: Add AsyncClient, search API, and streaming bulk export#28
Feat: Add AsyncClient, search API, and streaming bulk export#28dannywillems merged 8 commits intomainfrom
Conversation
716aaeb to
702b65e
Compare
|
|
||
| ## Quick Start | ||
|
|
||
| ```python |
There was a problem hiding this comment.
It is a good practice to never leave code in README, because it can be outdated very quickly. A more maintainable way to add code from files that is tested in the CI.
| Each object can be transformed back into a Python dictionary/JSON using the method `to_dict()`. | ||
| For instance, for the response of the subdomains endpoint, you can get back individual JSON by using: | ||
|
|
||
| ```python |
There was a problem hiding this comment.
We will remove this, based on https://github.com/LeakIX/LeakIXClient-Python/pull/28/changes#r2941216156
5f44186 to
aa7e53a
Compare
dannywillems
left a comment
There was a problem hiding this comment.
This patch does many things, which should be in individual commits and individual patches.
Can you split in individual patches please?
Additionnaly, the async and sync code are duplicated. There should be an elegant way to share code between both.
Finally, the async client does not follow the same pattern as the sync client (AbstractResponse). The class AbstractResponse with its implementation SuccessResponse and ErrorResponse abstract LeakIX backend.
With the async client, the user does not know what actually happened.
| ) | ||
| if status == 200 and isinstance(data, list): | ||
| return [l9format.L9Event.from_dict(item) for item in data] | ||
| return [] |
There was a problem hiding this comment.
This motivates my comment regarding using AbstractResponse. In this case, the user does not know what actually happened (an error happened?).
| async def search( | ||
| self, | ||
| query: str, | ||
| scope: str = "leak", |
There was a problem hiding this comment.
You could use an enum instead.
| } | ||
| return {"services": [], "leaks": []} | ||
|
|
||
| def _parse_events(self, items: list) -> list: |
There was a problem hiding this comment.
We want to have an homogenenous list. l9format should never fail. And if it fails, the user must know, and we should get a report for it.
| return self.get_service(queries=queries, page=page) | ||
| return self.get_leak(queries=queries, page=page) | ||
|
|
||
| def bulk_export_stream(self, queries: list[Query] | None = None): |
There was a problem hiding this comment.
What is the returned type?
e848685 to
362ce2b
Compare
010ea80 to
e547391
Compare
| if page < 0: | ||
| raise ValueError("Page argument must be a positive integer") | ||
| url = f"{self.base_url}/search" | ||
| r = self.__get( |
There was a problem hiding this comment.
Why do you introduce __get instead of _get? Having a uniform API would be better. Keep one of them.
There was a problem hiding this comment.
__get is already on main, it's the existing sync client code (client.py:53). I didn't change that. The async client uses _get since it's called from subclasses via BaseClient.
There was a problem hiding this comment.
Why do you introduce __get instead of _get?
I meant why do you introduce _get instead of __get.
The async client uses _get since it's called from subclasses via BaseClient.
Wdym?
|
|
||
| class Client(BaseClient): | ||
| def __get(self, url: str, params: dict[str, Any] | None) -> AbstractResponse: | ||
| def _get(self, url: str, params: dict[str, Any] | None) -> AbstractResponse: |
There was a problem hiding this comment.
Why do you use _get instead of __get?
There was a problem hiding this comment.
You asked for a uniform API. Both clients now use _get
There was a problem hiding this comment.
This does not answer the question. Can you explain why you picked _get instead of __get?
There was a problem hiding this comment.
Here some useful documentation: https://docs.python.org/3/tutorial/classes.html#private-variables.
|
Can you clean this history please? You introduce a commit that changes |
d05e130 to
c29ab56
Compare
c29ab56 to
44b55b6
Compare
|
This should be removed, as requested in #28 (comment). |
Summary
This PR enhances the LeakIX Python client with async support, reduced code duplication, and improved developer experience.
New Features
get_domain()method for domain-level service/leak queriesbulk_export_stream()generatorCode Quality
BaseClientto eliminate duplication between sync and async clientsAsyncClientreturnsAbstractResponse(not raw lists/dicts), consistent with sync clientScopeenum instead of raw strings for type safetyserialize_queries()helper to centralize query serializationAPI Examples
Changes
leakix/base.pywithBaseClientshared logicleakix/async_client.pywith full async implementationsearch(),get_domain(),bulk_export_stream()to both clientsserialize_queries()helper inquery.pyAsyncClientfrom package root