Further avatar fixes and improvements #63
Conversation
horazont
force-pushed
the
feature/avatar
branch
from
950283d
to
d06434e
Compare
* they where parsed to bytes before, but since they are reused as ids and the encoding is not uniquely defined in the XEP, we preserve the original string for interaction with pubsub and define an normalized_id property on AbstractAvatarHandle, which can be used for caching.
horazont
force-pushed
the
feature/avatar
branch
from
7c3b0ee
to
906ab21
Compare
aioxmpp/avatar/service.py
Outdated
| """ | ||
|
|
||
| def __init__(self, remote_jid, mime_type, id_, nbytes, width=None, | ||
| height=None, url=None, pubsub=None): | ||
| self._remote_jid = remote_jid | ||
| self._mime_type = mime_type | ||
| self._id = id_ | ||
| self._normalized_id = normalize_id(self._id) |
There was a problem hiding this comment.
Handle it in the property function. If it is a performance concern at some point, we can still fix it.
| except KeyError: | ||
| pass | ||
| with (yield from self._lock): | ||
| if jid in self._metadata_cache: |
There was a problem hiding this comment.
I feel this can be written down nicer with try. Maybe:
if require_fresh:
self._metadata_cache.pop(jid, None)
else:
try:
return self._metadata_cache[jid]
except KeyError:
passThere was a problem hiding this comment.
Hmm, pop of course has the notion of using the value we pop off the dict, instead of just discarding it. I think the explicit test is the right way to go here (and shorter!).
There was a problem hiding this comment.
Okay, if you prefer it that way it’s fine.
| avatar_xso.Data(avatar_set.image_bytes), | ||
| id_=id_ | ||
| ) | ||
| with (yield from self._lock): |
There was a problem hiding this comment.
This still lacks the race condition fix if we race with other clients, right? Do you plan to do this in this PR?
There was a problem hiding this comment.
Nope. This will probably need a new pull request an possible additional support from PubSub (but I'll have to read the PubSub spec to be sure). As I do not want to download the image data to verify it matches, but simply check whether the image data pubsub item has the appropriate id.
There was a problem hiding this comment.
Actually, I wonder whether we already get a push notification with the avatar data or at least the item ID because PEP. Can you investigate that? Does that even help us? (I’m not sure about that.)
Downloading the full avatar does seem like not a very bad thing to do here, unless we find a saner way. Matching the metadata won’t be sufficient to detect more than half of the collision scenarios.
There was a problem hiding this comment.
XEP-0163 section 3 is a bit unclear on whether we always get a notification:
If the publication logic dictates that event notifications shall be sent, the account owner's server generates notifications and sends them to all appropriate entities as described in the Receiving Event Notifications section of this document, as well as to any of the account owner's available resources.
If we would get a notification about the data, it would be easy to check consistency. If the server is clever enough to not send the payload along (but only notify about the ID), this would be the desired behaviour.
tests/avatar/test_service.py
Outdated
| @@ -186,22 +196,61 @@ def test_png_id_is_normalized(self): | |||
| ) | |||
|
|
|||
| def test_error_checking(self): | |||
There was a problem hiding this comment.
I prefer to have those tests (a) in individual test case methods and (b) using assertRaisesRegex to at least roughly check you are catching the error you are expecting to catch.
- (a) brings the advantage of overview if a single condition is violated and sense of graveness if the whole method is borked
- (b) helps with testing that it’s not a "raise RuntimeError()" at the beginning but actual different code paths being taken
tests/avatar/test_service.py
Outdated
| @@ -419,6 +489,19 @@ def test_get_image_bytes(self): | |||
| ) | |||
| self.assertEqual(res, TEST_IMAGE) | |||
|
|
|||
| def test_HttpAvatarDescriptr(self): | |||
There was a problem hiding this comment.
Typo in method name (Descriptor vs. Descriptr)
|
Github claims one of my comments is outdated, but I don’t think it in fact is. |
get_avatar_metdata(..., require_fresh=True)