-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Fix application service not being able to join remote federated room without a profile set #13131
Fix application service not being able to join remote federated room without a profile set #13131
Conversation
"Failed to get profile information while processing remote join for %r: %s", | ||
target, | ||
e, | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same pattern that we already use in
synapse/synapse/handlers/message.py
Lines 640 to 652 in e714b8a
try: | |
if "displayname" not in content: | |
displayname = await profile.get_displayname(target) | |
if displayname is not None: | |
content["displayname"] = displayname | |
if "avatar_url" not in content: | |
avatar_url = await profile.get_avatar_url(target) | |
if avatar_url is not None: | |
content["avatar_url"] = avatar_url | |
except Exception as e: | |
logger.info( | |
"Failed to get profile information for %r: %s", target, e | |
) |
profile = self.profile_handler | ||
if not content_specified: | ||
content["displayname"] = await profile.get_displayname(target) | ||
content["avatar_url"] = await profile.get_avatar_url(target) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feels like these profile functions would be better if they just returned None
when there is no profile for the local user. Profiles aren't required.
synapse/synapse/handlers/profile.py
Lines 104 to 129 in e714b8a
async def get_displayname(self, target_user: UserID) -> Optional[str]: if self.hs.is_mine(target_user): try: displayname = await self.store.get_profile_displayname( target_user.localpart ) except StoreError as e: if e.code == 404: raise SynapseError(404, "Profile was not found", Codes.NOT_FOUND) raise return displayname else: try: result = await self.federation.make_query( destination=target_user.domain, query_type="profile", args={"user_id": target_user.to_string(), "field": "displayname"}, ignore_backoff=True, ) except RequestSendFailed as e: raise SynapseError(502, "Failed to fetch profile") from e except HttpResponseException as e: raise e.to_synapse_error() return result.get("displayname") synapse/synapse/handlers/profile.py
Lines 201 to 225 in e714b8a
async def get_avatar_url(self, target_user: UserID) -> Optional[str]: if self.hs.is_mine(target_user): try: avatar_url = await self.store.get_profile_avatar_url( target_user.localpart ) except StoreError as e: if e.code == 404: raise SynapseError(404, "Profile was not found", Codes.NOT_FOUND) raise return avatar_url else: try: result = await self.federation.make_query( destination=target_user.domain, query_type="profile", args={"user_id": target_user.to_string(), "field": "avatar_url"}, ignore_backoff=True, ) except RequestSendFailed as e: raise SynapseError(502, "Failed to fetch profile") from e except HttpResponseException as e: raise e.to_synapse_error() return result.get("avatar_url")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I agree. Pretty much everywhere uses the same pattern as here, and we already return None
if the user sets and then deletes their display name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy for this to land as is, or to refactor calls to the profile store
Will merge for now ⏩ Thanks for the review @erikjohnston 🐹 |
#399) Synapse changes: matrix-org/synapse#13131 to address matrix-org/synapse#4778
Fix application service not being able to join remote federated room without a profile set
Fix #4778
Complement tests: matrix-org/complement#399
Pull Request Checklist
EventStore
toEventWorkerStore
.".code blocks
.Pull request includes a sign off(run the linters)