Skip to content

chore: Use transaction for saveUser function#35203

Merged
ggazzo merged 41 commits intodevelopfrom
chore/updateUserTransaction
Mar 17, 2025
Merged

chore: Use transaction for saveUser function#35203
ggazzo merged 41 commits intodevelopfrom
chore/updateUserTransaction

Conversation

@gabriellsh
Copy link
Member

@gabriellsh gabriellsh commented Feb 13, 2025

Proposed changes (including videos or screenshots)

Currently the saveUser function has a lot of side-effects and interacts with many collections multiple times. This can cause sync issues between records because references can change in one place, and fail to change in another, shortcutting the method execution without a proper rollback. For example, if you change a user's username, and the method that updates the avatar fails, the username will have been changed but an error will be thrown in the users.update api, causing confusion since part of the record change but the rest of the changes might not have happened.

This issue was specially prominent during the development of the user auditing mechanism, because it can cause discrepancies between what is being logged and what actually hapenned. It would require to watch the operation step by step or compare records before and after the operation, which also causes discrepancies, as the record can have changed in the mean time that this operation occurs.

This PR aims to make the saveUser helper function more solid with the use of transactions. The use of a transaction allows side effects to be skipped in case it fails, and automatically rolls back all operations that are executed within the transaction. It also will throw errors if by any chance there is a concurrent attempt to change the same record, avoiding unintended conflicts due to race conditions.

Issue(s)

CORE-1022

Steps to test or reproduce

Further comments

@dionisio-bot
Copy link
Contributor

dionisio-bot bot commented Feb 13, 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 13, 2025

⚠️ No Changeset found

Latest commit: 8238e17

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

@codecov
Copy link

codecov bot commented Feb 13, 2025

Codecov Report

Attention: Patch coverage is 48.64865% with 19 lines in your changes missing coverage. Please review.

Project coverage is 59.64%. Comparing base (dfde562) to head (8238e17).
Report is 8 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff            @@
##           develop   #35203   +/-   ##
========================================
  Coverage    59.64%   59.64%           
========================================
  Files         2827     2826    -1     
  Lines        68475    68475           
  Branches     15177    15182    +5     
========================================
  Hits         40839    40839           
  Misses       24989    24989           
  Partials      2647     2647           
Flag Coverage Δ
unit 75.86% <48.64%> (-0.01%) ⬇️

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

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Base automatically changed from refactor/saveCustomFields to develop February 18, 2025 17:02
@gabriellsh gabriellsh force-pushed the chore/updateUserTransaction branch from 199176a to c9de44b Compare February 18, 2025 19:16
@gabriellsh gabriellsh marked this pull request as ready for review February 18, 2025 19:19
@gabriellsh gabriellsh requested review from a team as code owners February 18, 2025 19:19
Copy link
Member

@ricardogarim ricardogarim left a comment

Choose a reason for hiding this comment

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

Since this is the first time we are implementing session passing, we don’t currently have a guideline or code style for handling it. I’m not sure if we have enough time to use this PR as the foundation for that, but it would be great to start defining some best practices.

For example, we typically aim to remove the options property from service-to-database calls and ensure that model calls are as specific as possible to their use case. Here, we’re introducing more flexibility with options (which I’m okay with), but this could lead to inconsistent calls being made to the same model functions.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 6, 2025

PR Preview Action v1.6.0

🚀 View preview at
https://RocketChat.github.io/Rocket.Chat/pr-preview/pr-35203/

Built to branch gh-pages at 2025-03-17 17:08 UTC.
Preview will be ready when the GitHub Pages deployment is complete.

@gabriellsh gabriellsh added this to the 7.5.0 milestone Mar 13, 2025
Copy link
Contributor

@Gustrb Gustrb left a comment

Choose a reason for hiding this comment

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

Awesome work my dude 🚀

@gabriellsh gabriellsh added the stat: QA assured Means it has been tested and approved by a company insider label Mar 17, 2025
@dionisio-bot dionisio-bot bot added the stat: ready to merge PR tested and approved waiting for merge label Mar 17, 2025
@gabriellsh gabriellsh added stat: QA assured Means it has been tested and approved by a company insider and removed stat: QA assured Means it has been tested and approved by a company insider labels Mar 17, 2025
@dionisio-bot dionisio-bot bot removed stat: ready to merge PR tested and approved waiting for merge stat: QA assured Means it has been tested and approved by a company insider labels Mar 17, 2025
@gabriellsh gabriellsh added the stat: QA assured Means it has been tested and approved by a company insider label Mar 17, 2025
@dionisio-bot dionisio-bot bot added the stat: ready to merge PR tested and approved waiting for merge label Mar 17, 2025
@ggazzo ggazzo merged commit 79a6c1f into develop Mar 17, 2025
56 checks passed
@ggazzo ggazzo deleted the chore/updateUserTransaction branch March 17, 2025 21:23
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.

6 participants