IDEV-2020 handle large response feeds improved #148
Conversation
domaintools/api.py
Outdated
| from datetime import datetime, timedelta, timezone | ||
| from hashlib import sha1, sha256, md5 | ||
| from hmac import new as hmac | ||
| from types import GeneratorType |
There was a problem hiding this comment.
maybe we should use the typing.Generator to add type hinting as I think this GeneratorType is use to check if the object is a generator type object.
domaintools/results.py
Outdated
| def response(self) -> Generator: | ||
| status_code = None | ||
| while status_code != 200: | ||
| resp_data = super().response() |
There was a problem hiding this comment.
we can directly call the self.data() and override it here in the subclass, and remove the special handling case in the data() in the base class, to isolate the feeds results, as the response path is not actually being used for feeds as the results is always a text not a json/dict. wdyt?
There was a problem hiding this comment.
In my opinion I would like to have it as is. why? because I want it to undergo the same processes as the normal endpoint would take (e.g. checking the limit exceeded, retry action, rate limiting checking, etc.) If we would override the self.data() method we would still need those processes to be implemented as well causing code duplication. what are your thoughts about it?
There was a problem hiding this comment.
yeah make sense, hmm I think not actually code duplication rather code isolation for single responsibility, we don't want to put too much if-else condition if there will be another products in the future to support different kinds of results.
also side note: I think we can move the raise condition inside check_limit_exceeded so we dont have the 'code duplication' in the async request as it will automatically handle the ServiceException.
Checking the async request, it seems it doesn't raise the 503 exception unless it returns the actual 503 status code and it will be catched in the setStatus which returns a different exception.
…) that is of Generator type.
| ) | ||
|
|
||
| def domaindiscovery(self, **kwargs): | ||
| def domaindiscovery(self, **kwargs) -> FeedsResults: |
…specific conditions in base Results class.
domaintools/base_results.py
Outdated
| if isinstance(self._data, dict) and ( | ||
| "response" in self._data and "limit_exceeded" in self._data["response"] and self._data["response"]["limit_exceeded"] is True | ||
| ): | ||
| raise ServiceException(503, f"Limit Exceeded: {self._data['response']['message']}") |
There was a problem hiding this comment.
maybe we could move it in the end of condition?
limit_exceeded, reason = False, "Limit Exceeded"
if limit_exceeded:
raise ServiceException(503, reason)
| return self._data | ||
|
|
||
| def check_limit_exceeded(self): | ||
| limit_exceeded, reason = False, "" |
There was a problem hiding this comment.
just a pedantic change:
can we put the Limit Exceeded as default reason? so we dont need to define it in elif and just concat the message in the if statement.
Overall, LGTM ✅ . Nice!!
No description provided.