-
Notifications
You must be signed in to change notification settings - Fork 451
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
[Dragos]: feat implement api client #3505
base: feat/2570-dragos-create-the-connector
Are you sure you want to change the base?
[Dragos]: feat implement api client #3505
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ASIDE : Minor typing fix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Core of the PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ code is clear
✅ dev server is really cool 🙌
✅ tests pass
✅ I succeed to do a minimal use case with ProductClientAPIV1 (once with dev server and once with Dragos API)👍
Minimal use case:
from datetime import timedelta
import asyncio
from yarl import URL
from client_api.v1.product import ProductClientAPIV1
client = ProductClientAPIV1(
base_url=URL("http://localhost:4000"),
token="dev",
secret="dev",
timeout=timedelta(seconds=10),
retry=3,
backoff=timedelta(seconds=2),
)
async def get_products():
products = client.iter_products()
async for product in products:
print(product)
asyncio.run(get_products())
|
||
### Results | ||
|
||
see [docs](./dev/docs/) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you want to display images here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just wanted to provide a link to keep README readable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Crystal clear and the examples work fine 👍
|
||
from pydantic import AwareDatetime, Field | ||
|
||
from client_api.v1.common import BaseAPIV1BulkResponse, BaseClientAPIV1, ResponseModel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would probably used PermissiveBaseModel
instead of its alias ResponseModel
, as ResponseModel
makes me think of a whole response and it doesn't explicitly expose its intent. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That’s an interesting point. I generally prefer to hide technical details that might be implied by naming in public methods and classes. I’d say the "Permissive" aspect (not raising a technical error) is more of a concern for the client developer, whereas the end-user is concerned with the possibility of receiving a None value, accompanied by a warning log.
This is why I introduced a user-oriented alias, ResponseModel, for the technical PermissiveBaseModel object. However, I do agree that the term might not be ideal, as it is also used in the context of "Sub Response Elements" modeling.
I would prefer to keep an alias and I’d be happy to consider any naming suggestions you might have !
|
||
""" | ||
# first page of indicators | ||
indicators: IndicatorsResponse = await self._get_1_page( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could it be called first_page_data
or equivalent to have consistent naming with pages_data
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is later extended to gather all the indicator.
], | ||
"release_date": "2024-03-01T01:31:09.000Z", | ||
"type": "Suspect Domain Report", | ||
"report_link": "BASE_URL/api/v1/products/DEMO_SERIAL/report", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hardcoded BASE_URL
in links lead to this error:
products.0.report_link
Input should be a valid URL, relative URL without a base [type=url_parsing, input_value='BASE_URL/api/v1/products/DEMO_SERIAL/report', input_type=str]
For further information visit https://errors.pydantic.dev/2.10/v/url_parsing
products.0.ioc_csv_link
Input should be a valid URL, relative URL without a base [type=url_parsing, input_value='BASE_URL/api/v1/products/DEMO_SERIAL/csv', input_type=str]
For further information visit https://errors.pydantic.dev/2.10/v/url_parsing
products.0.ioc_stix2_link
Input should be a valid URL, relative URL without a base [type=url_parsing, input_value='BASE_URL/api/v1/products/DEMO_SERIAL/stix2', input_type=str]
For further information visit https://errors.pydantic.dev/2.10/v/url_parsing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your remark, I correct this !
``` | ||
|
||
### Data | ||
The fake server uses a simple json file to store the data. The data should be stored in the `client_api/dev/fake_server` directory under `products.json`and `indicator.json` files. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be stored in the client_api/dev/fake_server/v1/data
directory
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for noticing this mistake !
"""Return the string representation of the validation warning.""" | ||
warnings_repr = os.linesep.join(str(warning) for warning in self.warnings) | ||
return ( | ||
f"{self.warnings_count} validation warning{'s' if self.warnings_count>1 else ''} for {self.model.__name__}{os.linesep}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Proposed changes
Related issues
Checklist
Further comments
Related Notion Page : https://www.notion.so/filigran/Integration-Dragos-Create-the-connector-1528fce17f2a80dda61ed990b8e78b75?pvs=4