Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(federation): Sync room properties on join #13072

Merged
merged 2 commits into from
Aug 22, 2024

Conversation

nickvergessen
Copy link
Member

@nickvergessen nickvergessen commented Aug 21, 2024

☑️ Resolves

Follow-up ideas

🛠️ API Checklist

🏁 Checklist

  • ⛑️ Tests (unit and/or integration) are included or not possible
  • 📘 API documentation in docs/ has been updated or is not required
  • 🔖 Capability is added or not needed

@nickvergessen nickvergessen self-assigned this Aug 21, 2024
@nickvergessen nickvergessen added 2. developing bug feature: api 🛠️ OCS API for conversations, chats and participants feature: federation 🌐 labels Aug 21, 2024
@nickvergessen nickvergessen added this to the 🖤 Next Major (31) milestone Aug 21, 2024
@nickvergessen
Copy link
Member Author

/backport to stable30

Copy link
Member

@danxuliu danxuliu left a comment

Choose a reason for hiding this comment

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

I am not fully convinced on adding a specific method to update the database in a single query. While saving database queries is of course good, not many queries might be saved with that, so the standard RoomService methods could be used instead. In the initial sync it may help, but after that it would only help if some cloud notification is missed, as right now it syncs more properties than what is explicitly propagated (for example, the call recording state), but those missing properties should be propagated anyway; otherwise a federated user already in the room would not receive the change until another federated user joins again.

Syncing all the properties in one go in general seems to have the same effect as using the standard RoomService methods, but not sending the individual modification events could also miss some behaviours (like invalidating the reference cache), and there could be also missing behaviours on some specific methods (like resetting the participant permissions when the room permissions are modified). So I am not sure if the few saved database queries are worth the trouble of handling those things 🤔

Independently of that, CI does not seem to be happy.

lib/Service/RoomService.php Outdated Show resolved Hide resolved
lib/Service/RoomService.php Outdated Show resolved Hide resolved
@nickvergessen
Copy link
Member Author

I am not fully convinced on adding a specific method to update the database in a single query.

my main intention was to avoid certain reactions of other components listening to the hooks
e.g. I don't know what happens if clients receive 10 room-modified signaling messages in few milliseconds and some of them with potential wrong/old data on other props
but I will just add a "before" event and make that one disable the normal RoomModified listener for signaling. Other listeners can decide themselves whether they should "pause" and conclude with a summary.
Should work just as fine

@nickvergessen
Copy link
Member Author

Most of red CI is due to nextcloud/server#46991

@nickvergessen nickvergessen force-pushed the bugfix/noid/sync-room-props-on-join branch 2 times, most recently from 73e4c25 to c3c9112 Compare August 22, 2024 07:32
@nickvergessen
Copy link
Member Author

Changed it to the RoomService methods now, made the SignalingListener pause on BeforeRoomSync and send a single signaling messages afterwards.
Some other listeners were modified to not trigger on federated conversations (we don't need to check/stop a recording, etc.)

@nickvergessen
Copy link
Member Author

Copy link
Member

@danxuliu danxuliu left a comment

Choose a reason for hiding this comment

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

Force pushed to fix the check against the last activity.

I have also added a fixup, but not applied it yet, to update the lobby. Before only the lobby state was checked, but not the timer. Therefore, if the lobby was previously set but only the timer changed when the data was synchronized again it was not updated. But given that there was a condition inside the external if that took into account the timer maybe I am missing something, so I prefered not to apply it without a review from @nickvergessen

Other than that and the comment, tested and works 👍

lib/Service/RoomService.php Outdated Show resolved Hide resolved
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen nickvergessen force-pushed the bugfix/noid/sync-room-props-on-join branch from fdb2755 to 9dda21c Compare August 22, 2024 14:24
@nickvergessen nickvergessen merged commit ee75447 into main Aug 22, 2024
67 checks passed
@nickvergessen nickvergessen deleted the bugfix/noid/sync-room-props-on-join branch August 22, 2024 15:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review bug feature: api 🛠️ OCS API for conversations, chats and participants feature: federation 🌐
Projects
None yet
2 participants