Skip to content
This repository was archived by the owner on Apr 26, 2024. It is now read-only.

Fix possible variable shadow in create_new_client_event #14575

Merged
merged 2 commits into from
Nov 29, 2022
Merged

Fix possible variable shadow in create_new_client_event #14575

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ea2e6f3
fix a possible variable shadow
H-Shay Nov 28, 2022
c8e3903
newsfragment + lints
H-Shay Nov 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/14575.misc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix a possible variable shadow in `create_new_client_event`.
6 changes: 4 additions & 2 deletions synapse/handlers/message.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1135,11 +1135,13 @@ async def create_new_client_event(
)
state_events = await self.store.get_events_as_list(state_event_ids)
# Create a StateMap[str]
state_map = {(e.type, e.state_key): e.event_id for e in state_events}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really wish we could add a lint for variable shadowing (parameter re-assigning) problem. Previously discussed at #10439 (comment)

The mypy issue still exists: python/mypy#11076

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like the state_map function parameter was added more recently in #13487 which caused the collision. Whereas the state_map variable was in place since #12083 and was only used as a temporary variable within itself at the time so this variable shadowing case seems legit.

(just pulled out of #12083)

synapse/synapse/handlers/message.py

Lines 972 to 976 in cd93056

state_map = {(e.type, e.state_key): e.event_id for e in state_events}
# Actually strip down and only use the necessary auth events
auth_event_ids = self._event_auth_handler.compute_auth_events(
event=temp_event,
current_state_ids=state_map,

For reference and confusingly, all of the for_batch and state_map parameter stuff isn't for MSC2716 /batch_send (it's all new from the room creation batch event creation). It really feels like #13487 added some duplication of existing logic that could be covered by state_event_ids/auth_event_ids but I haven't delved into all of that code.

This could happen if for_batch=True, state_event_ids is not-None.

So I guess the question is -- is it valid input if state_event_ids and state_map to both be not-None?

I think this might be a question for @MadLittleMods

-- @clokep, #14575 (review)

As mentioned, since none of the stuff I've worked on for MSC2716 uses these parameters, I don't think there is a case where they interact unless @H-Shay has refactored some of that code and is probably the one to better answer.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, my misunderstanding! Sorry about that. I assumed it all had to do with MSC2716 due to the "batch" comment in it. 😢

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It really feels like #13487 added some duplication of existing logic that could be covered by state_event_ids/auth_event_ids but I haven't delved into all of that code.

@H-Shay Can you create a follow-up issue to track this?

And a separate one for "If we could somehow differentiate our internal batched event creating from MSC2716 that would be fabulous..." (#14575 (review))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you create a follow-up issue to track this?

Sure thing, I have created #14689 for the naming issue and #14688 for the possible logic duplication in create_new_client_event.

current_state_ids = {
(e.type, e.state_key): e.event_id for e in state_events
}
# Actually strip down and only use the necessary auth events
auth_event_ids = self._event_auth_handler.compute_auth_events(
event=temp_event,
current_state_ids=state_map,
current_state_ids=current_state_ids,
for_verification=False,
)

Expand Down