Skip to content

Conversation

@d-gubert
Copy link
Member

@d-gubert d-gubert commented Feb 5, 2025

Proposed changes (including videos or screenshots)

When apps update an entity (a room or a message) via their respective ModifyUpdater method, the bridge will actually update ALL fields of in the DB record. That is because it uses a $set instruction with the whole record sent by the app. This creates a problem where the app could have queried stale data and would inadvertedly overwrite the previously updated DB record with such stale data.

To prevent this scenario, this PR modifies the builder instances used by the updaters, RoomBuilder and MessageBuilder, to actually keep track of the changes applied by the app, and send only the changed properties to be updated in the bridge.

Issue(s)

CONN-480

Steps to test or reproduce

Further comments

@dionisio-bot
Copy link
Contributor

dionisio-bot bot commented Feb 5, 2025

Looks like this PR is ready to merge! 🎉
If you have any trouble, please check the PR guidelines

@changeset-bot
Copy link

changeset-bot bot commented Feb 5, 2025

🦋 Changeset detected

Latest commit: 3d1c334

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 38 packages
Name Type
@rocket.chat/omnichannel-transcript Patch
@rocket.chat/authorization-service Patch
@rocket.chat/stream-hub-service Patch
@rocket.chat/network-broker Patch
@rocket.chat/presence-service Patch
@rocket.chat/fuselage-ui-kit Patch
@rocket.chat/account-service Patch
@rocket.chat/ui-video-conf Patch
@rocket.chat/core-typings Patch
@rocket.chat/ddp-streamer Patch
@rocket.chat/queue-worker Patch
@rocket.chat/apps-engine Patch
@rocket.chat/ui-client Patch
@rocket.chat/apps Patch
@rocket.chat/i18n Patch
@rocket.chat/meteor Patch
rocketchat-services Patch
@rocket.chat/uikit-playground Patch
@rocket.chat/mock-providers Patch
@rocket.chat/api-client Patch
@rocket.chat/core-services Patch
@rocket.chat/cron Patch
@rocket.chat/ddp-client Patch
@rocket.chat/freeswitch Patch
@rocket.chat/gazzodown Patch
@rocket.chat/livechat Patch
@rocket.chat/model-typings Patch
@rocket.chat/rest-typings Patch
@rocket.chat/ui-contexts Patch
@rocket.chat/license Patch
@rocket.chat/omnichannel-services Patch
@rocket.chat/pdf-worker Patch
@rocket.chat/presence Patch
@rocket.chat/ui-voip Patch
@rocket.chat/web-ui-registration Patch
@rocket.chat/models Patch
@rocket.chat/ui-avatar Patch
@rocket.chat/instance-status Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@d-gubert d-gubert added this to the 7.4.0 milestone Feb 5, 2025
@codecov
Copy link

codecov bot commented Feb 13, 2025

Codecov Report

Attention: Patch coverage is 93.93939% with 2 lines in your changes missing coverage. Please review.

Project coverage is 59.46%. Comparing base (dac213d) to head (3d1c334).
Report is 1 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop   #35120      +/-   ##
===========================================
+ Coverage    59.45%   59.46%   +0.01%     
===========================================
  Files         2828     2829       +1     
  Lines        68159    68290     +131     
  Branches     15124    15124              
===========================================
+ Hits         40521    40606      +85     
- Misses       24982    25028      +46     
  Partials      2656     2656              
Flag Coverage Δ
unit 75.63% <93.93%> (-0.07%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

@d-gubert d-gubert force-pushed the fix/apps-prevent-whole-update branch from 0c68a4d to dd802e2 Compare February 13, 2025 23:42
@d-gubert d-gubert changed the title fix: prevent greedy update via apps fix(apps): prevent greedy update via apps Feb 14, 2025
@d-gubert d-gubert marked this pull request as ready for review February 14, 2025 18:29
@d-gubert d-gubert requested review from a team as code owners February 14, 2025 18:29
@d-gubert d-gubert force-pushed the fix/apps-prevent-whole-update branch from 040628e to 48978a6 Compare February 14, 2025 21:43
@KevLehman KevLehman added the stat: QA assured Means it has been tested and approved by a company insider label Feb 19, 2025
@dionisio-bot dionisio-bot bot added the stat: ready to merge PR tested and approved waiting for merge label Feb 19, 2025
@kodiakhq kodiakhq bot merged commit 8996414 into develop Feb 20, 2025
47 of 48 checks passed
@kodiakhq kodiakhq bot deleted the fix/apps-prevent-whole-update branch February 20, 2025 02:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stat: QA assured Means it has been tested and approved by a company insider stat: ready to merge PR tested and approved waiting for merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants