Changeset View
Standalone View
swh/core/api/serializers.py
Show All 12 Lines | |||||
import arrow | import arrow | ||||
import iso8601 | import iso8601 | ||||
import msgpack | import msgpack | ||||
from typing import Any, Dict, Union, Tuple | from typing import Any, Dict, Union, Tuple | ||||
from requests import Response | from requests import Response | ||||
from swh.core.api.classes import PagedResult | |||||
def encode_datetime(dt: datetime.datetime) -> str: | def encode_datetime(dt: datetime.datetime) -> str: | ||||
"""Wrapper of datetime.datetime.isoformat() that forbids naive datetimes.""" | """Wrapper of datetime.datetime.isoformat() that forbids naive datetimes.""" | ||||
if dt.tzinfo is None: | if dt.tzinfo is None: | ||||
raise ValueError(f"{dt} is a naive datetime.") | raise ValueError(f"{dt} is a naive datetime.") | ||||
return dt.isoformat() | return dt.isoformat() | ||||
def _encode_paged_result(obj: PagedResult) -> Dict[str, Any]: | |||||
"""Serialize PagedResult to a Dict.""" | |||||
return { | |||||
olasd: I don't think this is needed. | |||||
Done Inline ActionsIt will be if we ever need another class that works like PagedResult vlorentz: It will be if we ever need another class that works like `PagedResult` | |||||
Done Inline ActionsSo, it's not needed yet. olasd: So, it's not needed yet. | |||||
Done Inline Actionsoh yeah, i don't even use it... ardumont: oh yeah, i don't even use it...
Dropping it. | |||||
"results": obj.results, | |||||
"next_page_token": obj.next_page_token, | |||||
} | |||||
def _decode_paged_result(obj: Dict[str, Any]) -> PagedResult: | |||||
"""Deserialize Dict into PagedResult""" | |||||
return PagedResult(results=obj["results"], next_page_token=obj["next_page_token"],) | |||||
ENCODERS = [ | ENCODERS = [ | ||||
(arrow.Arrow, "arrow", arrow.Arrow.isoformat), | (arrow.Arrow, "arrow", arrow.Arrow.isoformat), | ||||
(datetime.datetime, "datetime", encode_datetime), | (datetime.datetime, "datetime", encode_datetime), | ||||
( | ( | ||||
datetime.timedelta, | datetime.timedelta, | ||||
"timedelta", | "timedelta", | ||||
lambda o: { | lambda o: { | ||||
"days": o.days, | "days": o.days, | ||||
"seconds": o.seconds, | "seconds": o.seconds, | ||||
"microseconds": o.microseconds, | "microseconds": o.microseconds, | ||||
}, | }, | ||||
), | ), | ||||
(UUID, "uuid", str), | (UUID, "uuid", str), | ||||
Done Inline ActionsI wanted to name it like this for a moment, but I have a small preference for "api_class" now. Up to you. or "swh_core_class" / "swh_api_class" vlorentz: I wanted to name it like this for a moment, but I have a small preference for "api_class" now. | |||||
Done Inline Actionsapi_class appeals to me ;) ardumont: api_class appeals to me ;)
(more than the swh prefixed one) | |||||
Done Inline ActionsWhy not just call it paged_result? your serializer/deserializer only supports these anyway. olasd: Why not just call it `paged_result`? your serializer/deserializer only supports these anyway. | |||||
Done Inline ActionsIn case we need another class in the future. vlorentz: In case we need another class in the future. | |||||
Done Inline ActionsThen add another encoder and decoder pair then. olasd: Then add another encoder and decoder pair then. | |||||
Done Inline Actionsthen why do we need to abstract now the Optional[str] for the PagedResult next_page_token field? ardumont: then why do we need to abstract now the `Optional[str]` for the PagedResult next_page_token… | |||||
Done Inline ActionsWe have other typed paginated API endpoints already existing in our codebase; One of them uses a composite next page token: https://forge.softwareheritage.org/source/swh-scheduler/browse/master/swh/scheduler/model.py$183-193 If you hardcode the next page token type as a string, then to be able to reuse this code, you would need the API function using this return type to handle encoding / decoding an opaque next page token, instead of just using its own explicit and transparent types. I'm not convinced these internal APIs generally need opaque page tokens, on the contrary. olasd: We have other typed paginated API endpoints already existing in our codebase; One of them uses… | |||||
Done Inline Actions
I missed that part. Thanks.
Well, I recall we decided to go that road for cassandra storage and for storage paginated endpoints in general then. Though i don't have links that demonstrates this... (Note that i'm not entirely clear on what you explained to me yet, still thinking, it's on me heh ;) ardumont: > We have other typed paginated API endpoints already existing in our codebase; One of them… | |||||
Done Inline ActionsYeah, sorry, I didn't mean to question whether the specific swh.storage APIs you're looking at should have opaque tokens or not; I understand why they ended up that way and I think that's the most sensible choice. I'm saying having opaque pagination tokens in internal APIs should be a last resort for specific use-cases where we can't do better. olasd: Yeah, sorry, I didn't mean to question whether the specific swh.storage APIs you're looking at… | |||||
Not Done Inline Actions
Why do you see them as a bad thing? Even if they are for internal APIs, it's good to hide implementation details of one component from the others. And if you want clients to use the data they contain, then this data shouldn't be in a pagination token. vlorentz: > should be a last resort for specific use-cases where we can't do better.
Why do you see them… | |||||
(PagedResult, "paged_result", _encode_paged_result), | |||||
# Only for JSON: | # Only for JSON: | ||||
(bytes, "bytes", lambda o: base64.b85encode(o).decode("ascii")), | (bytes, "bytes", lambda o: base64.b85encode(o).decode("ascii")), | ||||
] | ] | ||||
DECODERS = { | DECODERS = { | ||||
"arrow": arrow.get, | "arrow": arrow.get, | ||||
"datetime": lambda d: iso8601.parse_date(d, default_timezone=None), | "datetime": lambda d: iso8601.parse_date(d, default_timezone=None), | ||||
"timedelta": lambda d: datetime.timedelta(**d), | "timedelta": lambda d: datetime.timedelta(**d), | ||||
"uuid": UUID, | "uuid": UUID, | ||||
"paged_result": _decode_paged_result, | |||||
# Only for JSON: | # Only for JSON: | ||||
"bytes": base64.b85decode, | "bytes": base64.b85decode, | ||||
} | } | ||||
class MsgpackExtTypeCodes(Enum): | class MsgpackExtTypeCodes(Enum): | ||||
LONG_INT = 1 | LONG_INT = 1 | ||||
▲ Show 20 Lines • Show All 219 Lines • Show Last 20 Lines |
I don't think this is needed.