Fix: Any invalid message in a response crashed the entire response #27
Conversation
The code serving the API may return messages that are not valid according the latest version of the schemas. Ignore them with a warning instead of failing the parsing of the entire response.
| @@ -440,5 +445,22 @@ class MessagesResponse(BaseModel): | |||
| pagination_per_page: int | |||
| pagination_item: str | |||
|
|
|||
| @validator("messages", pre=True) | |||
| def handle_invalid_messages(cls, v: List[Dict]): | |||
There was a problem hiding this comment.
You shouldn't assume that v is a list of dicts coming into this function. It should be Any with an isinstance check.
There was a problem hiding this comment.
Also, not 100% sure on this: are we sure that the validator is not run a second time on the return value of this validator? I wouldn't be surprised it it's the case.
| result = [] | ||
| for message_raw in v: | ||
| try: | ||
| message = Message(**message_raw) |
There was a problem hiding this comment.
Why not Message.parse_obj(message_raw)? iirc that would avoid the KeyError.
| path = Path( | ||
| os.path.abspath( | ||
| os.path.join(__file__, "../messages/contains_invalid_messages.json") | ||
| ) | ||
| ) | ||
| with open(path) as fd: |
There was a problem hiding this comment.
Mixing Path and os.path is weird. Also, pretty sure the join would not work because you never specify to take the parent directory of __file__?
| path = Path( | |
| os.path.abspath( | |
| os.path.join(__file__, "../messages/contains_invalid_messages.json") | |
| ) | |
| ) | |
| with open(path) as fd: | |
| path = Path(__file__).parent.parent / "messages" / "contains_invalid_messages.json" | |
| with path.open() as fd: |
|
I am wondering if this would not make more sense in |
|
Replaced by a fix in Aleph-client. |
The code serving the API may return messages that are not valid according the latest version of the schemas. Ignore them with a warning instead of failing the parsing of the entire response.
Closes aleph-im/aleph-client#108