Check if default_payload is a dict before using it as a dict
#292
Conversation
|
I think this is waiting on changes to reject the pushkey(?) |
…ult_payload is malformed
…ygnal into shay/fix_type_error
|
Yes I've add changes which reject the pushkey-I am not convinced of their elegance but I figured best to get some feedback on my approach. Once the |
sygnal/apnspushkin.py
Outdated
| payload.update(device.data.get("default_payload", {})) | ||
| default_payload = device.data.get("default_payload", {}) | ||
| payload.update(default_payload) |
There was a problem hiding this comment.
Does this use of default_payload need checking for not being a dict too? (Not sure of the bigger picture here)
There was a problem hiding this comment.
My thinking on this was that we do the check on default_payload several lines before the only call to the function where this code is located (_get_payload_event_id_only), and the check returns/stops execution if the default payload is not a dict, so afaict the execution should never get to this stage if default_payload is not a dict.
There was a problem hiding this comment.
I think your reasoning is correct; but I have in mind:
- maintainability: in the future, we might call
_get_payload_event_id_onlywith adevicewhosedefault_payloadhasn't been validated - ease of reading: it'd be nice to arrange this in a way that the reader doesn't need to repeat this reasoning every time we come to look at
default_payload.
(Warning: the following is splitting hairs.)
Looking at the body of _get_payload_event_id_only, I note that the only time we use the device argument is to retrieve the default_payload (see also #127). I'm tempted to suggest:
- remove
_get_payload_event_id_only'sdeviceargument - have it take a
default_payload: Dict[str, Any]instead - change the caller to pass
default_payloaddirectly, handling the case wheredevice.data is None
There was a problem hiding this comment.
Great I will do that, thanks for outlining your reasoning!
There was a problem hiding this comment.
(To emphasise: this is fine as it is and I am going a little too extreme here. I think this is great as written and definitely mergeable. But I wanted to outline the thinking behind the question.)
There was a problem hiding this comment.
Ha i just re-implmented it with your changes, you let me know if seems better-I honestly think both ways feel a little clunky, but that's because the surrounding code is a little clunky (lots of conditional branches, probably unavoidable).
There was a problem hiding this comment.
Ahh, I had something slightly different in mind. I was thinking:
- above line 288 add
default_payload = {} - this means we can remove lines 297 through 301, and always use 302 (because we always have a default payload)
There was a problem hiding this comment.
Right that makes more sense, have updated.
| if data is None: | ||
| failed.append(device.pushkey) |
There was a problem hiding this comment.
looks like this will go on to attempt delivery anyway?
There was a problem hiding this comment.
It sure does look like that-which probably doesn't make sense. I can fix that along with downgrading the error to a warning.
Fixes #291.