fix: Teams may have no associated main room on creation

[lucas-a-pelegrino]

Proposed changes (including videos or screenshots)

This PR aims to temporary fix an issue where teams were created with no associated room due to some error occurring in the create room service.

Issue(s)

CORE-552

Steps to test or reproduce

To reproduce the error:

1. Call the api: teams.create with an invalid room name, i.e: team-with-invalid-char*

curl 'http://localhost:3000/api/v1/teams.create' \
  -H 'Accept: application/json' \
  -H 'Accept-Language: en-US,en;q=0.9' \
  -H 'Connection: keep-alive' \
  -H 'Content-Type: application/json' \
  -H 'Cookie: rc_uid=9Xbn8WuPWjpzywoeK; rc_token=HUvemBdcxXNPPp_FKmPAjMLpdBEBOQ0zJTN5Yz2Bk6E' \
  -H 'Origin: http://localhost:3000' \
  -H 'Referer: http://localhost:3000/channel/devchannel' \
  -H 'Sec-Fetch-Dest: empty' \
  -H 'Sec-Fetch-Mode: cors' \
  -H 'Sec-Fetch-Site: same-origin' \
  -H 'User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/129.0.0.0 Safari/537.36' \
  -H 'X-Auth-Token: HUvemBdcxXNPPp_FKmPAjMLpdBEBOQ0zJTN5Yz2Bk6E' \
  -H 'X-User-Id: 9Xbn8WuPWjpzywoeK' \
  -H 'sec-ch-ua: "Google Chrome";v="129", "Not=A?Brand";v="8", "Chromium";v="129"' \
  -H 'sec-ch-ua-mobile: ?0' \
  -H 'sec-ch-ua-platform: "macOS"' \
  --data-raw '{"name":"team-with-invalid-char*","members":[],"type":0,"room":{"readOnly":false,"extraData":{"topic":"","broadcast":false,"encrypted":false}}}'

2. Check the database to see that a team was created even though the room creation service throws an error.

With the fix in place:

1. Call the api: teams.create with an invalid room name, i.e: team-with-invalid-char*

curl 'http://localhost:3000/api/v1/teams.create' \
  -H 'Accept: application/json' \
  -H 'Accept-Language: en-US,en;q=0.9' \
  -H 'Connection: keep-alive' \
  -H 'Content-Type: application/json' \
  -H 'Cookie: rc_uid=9Xbn8WuPWjpzywoeK; rc_token=HUvemBdcxXNPPp_FKmPAjMLpdBEBOQ0zJTN5Yz2Bk6E' \
  -H 'Origin: http://localhost:3000' \
  -H 'Referer: http://localhost:3000/channel/devchannel' \
  -H 'Sec-Fetch-Dest: empty' \
  -H 'Sec-Fetch-Mode: cors' \
  -H 'Sec-Fetch-Site: same-origin' \
  -H 'User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/129.0.0.0 Safari/537.36' \
  -H 'X-Auth-Token: HUvemBdcxXNPPp_FKmPAjMLpdBEBOQ0zJTN5Yz2Bk6E' \
  -H 'X-User-Id: 9Xbn8WuPWjpzywoeK' \
  -H 'sec-ch-ua: "Google Chrome";v="129", "Not=A?Brand";v="8", "Chromium";v="129"' \
  -H 'sec-ch-ua-mobile: ?0' \
  -H 'sec-ch-ua-platform: "macOS"' \
  --data-raw '{"name":"team-with-invalid-char*","members":[],"type":0,"room":{"readOnly":false,"extraData":{"topic":"","broadcast":false,"encrypted":false}}}'

2. Check the database to see that the invalid team was removed afterwards.

Further comments

After taking a good look on how the service is implemented, introducing the use of transactions which would be the better solution would take longer, and this fix is due to be included in the 6.12 release.

After the merge, I'll bring this issue up on our backend discussions so we can design a better flow for this service and maybe implement it in a way that makes using transactions doable, or even make it so the code is simpler and/or more reliable and such edge cases are handled properly.

Also, it would involve adding the database session on multiple methods as optional to be able to manage the transaction correctly. So a more quick and temporary solution was to invert the order the code executes, creating a Team and inserting the Team Members after a Room is successfully created.

The behavior stays the same if converting a Room to a Team.

[changeset-bot]

🦋 Changeset detected

Latest commit: fd37df6

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

This PR includes changesets to release 32 packages

Name	Type
@rocket.chat/meteor	Patch
@rocket.chat/core-typings	Patch
@rocket.chat/rest-typings	Patch
@rocket.chat/api-client	Patch
@rocket.chat/apps	Patch
@rocket.chat/core-services	Patch
@rocket.chat/cron	Patch
@rocket.chat/ddp-client	Patch
@rocket.chat/fuselage-ui-kit	Patch
@rocket.chat/gazzodown	Patch
@rocket.chat/livechat	Patch
@rocket.chat/model-typings	Patch
@rocket.chat/ui-contexts	Patch
@rocket.chat/account-service	Patch
@rocket.chat/authorization-service	Patch
@rocket.chat/ddp-streamer	Patch
@rocket.chat/omnichannel-transcript	Patch
@rocket.chat/presence-service	Patch
@rocket.chat/queue-worker	Patch
@rocket.chat/stream-hub-service	Patch
@rocket.chat/license	Patch
@rocket.chat/omnichannel-services	Patch
@rocket.chat/pdf-worker	Patch
@rocket.chat/presence	Patch
rocketchat-services	Patch
@rocket.chat/uikit-playground	Patch
@rocket.chat/models	Patch
@rocket.chat/ui-avatar	Patch
@rocket.chat/ui-client	Patch
@rocket.chat/ui-video-conf	Patch
@rocket.chat/web-ui-registration	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

[dionisio-bot]

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

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 59.39%. Comparing base (9a69771) to head (fd37df6).
Report is 1 commits behind head on develop.

Additional details and impacted files

@@           Coverage Diff            @@
##           develop   #33101   +/-   ##
========================================
  Coverage    59.38%   59.39%           
========================================
  Files         2547     2547           
  Lines        63255    63241   -14     
  Branches     14228    14231    +3     
========================================
- Hits         37567    37559    -8     
+ Misses       22978    22974    -4     
+ Partials      2710     2708    -2     

Flag	Coverage Δ
unit	75.84% <ø> (ø)

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

[KevLehman]

Curious, the process will rollback and that's cool. But, is there a way to prevent the process from failing? I was thinking if the issue is the room name being invalid, maybe we can call a function that checks for that before attempting to create the team, wdyt?

[lucas-a-pelegrino]

@KevLehman I agree with you that we should add a naming validation for this. The issue is that this is not the only thing that may fail on Room creation, if you take a look at the Room.create() service, you can see that there are a lot of things that may fail, this fix aims to fix this specific edge case that causes a Team to be created with no room associated with it because of the naming (for direct api calls, from our UI it doesn't happen).

[KevLehman]

No no yeah, that's great. I was thinking in having both, cause from your description it seems you already found a consistent way for this to happen. So, was thinking it's faster to cover that way earlier (before creating teams and doing more db calls) rather than waiting for the rollback to kick in 👀

@MarcosSpessatto @lucas-a-pelegrino wdyt? does it makes sense or do we think on these improvements on the "actual" fix on the next release?

[lucas-a-pelegrino]

No no yeah, that's great. I was thinking in having both, cause from your description it seems you already found a consistent way for this to happen. So, was thinking it's faster to cover that way earlier (before creating teams and doing more db calls) rather than waiting for the rollback to kick in 👀

@MarcosSpessatto @lucas-a-pelegrino wdyt? does it makes sense or do we think on these improvements on the "actual" fix on the next release?

I think we can add the name validation early on in the method start, that would help but not resolve the whole issue... :/ A lot of things could still go wrong and a Team and TeamMember records could be created with no associated room. Even the TeamMember insertion could throw an unexpected error and fall into this edge case.

[matheusbsilva137]

Please add a changeset to this PR
(btw we usually describe what's the error in the PR title, not the fix. Reference: https://developer.rocket.chat/docs/pull-requests-tags)