[_]: feat/add unique constraint on member_id and workspace_id in workspace_users and workspace_teams_users#421
Conversation
jzunigax2
had a problem deploying
to
development
GitHub Actions
Failure
jzunigax2
had a problem deploying
to
development
GitHub Actions
Failure
jzunigax2
force-pushed
the
fix/unique-constraint-workspace-users
branch
from
e3596f0 to
c0b1203
Compare
jzunigax2
had a problem deploying
to
development
GitHub Actions
Failure
…into fix/unique-constraint-workspace-users
jzunigax2
had a problem deploying
to
development
GitHub Actions
Failure
|
|
Please add a description of why you are doing this @jzunigax2. In this case, something like "Add UNIQUE constraint with workspace_id and member_id to prevent duplicated members in a given workspace" would be a good concise description. Otherwise the context of the why of this change, is lost. |
jzunigax2
changed the title
…into fix/unique-constraint-workspace-users
… add new constraint for workspace_teams_users
| return this.teamUserToDomain(teamUser); | ||
| } catch (error) { | ||
| if ( | ||
| error.name === 'SequelizeUniqueConstraintError' && |
There was a problem hiding this comment.
This should be previously checked by the use case. This seems to be business logic
There was a problem hiding this comment.
The use case does check this condition before attempting insertion, but due to the race condition that can sometimes affect this use case whenever this insertion fails it would just throw a 500 error. while this ensures to catch the error and rethrow a 400 which is what frontend handles
There was a problem hiding this comment.
If we are doing the proper async management, no race condition should happen, especially on small tables like this one, that is even under a thousand records which is far far less than for instance the files table, which thousands of millions of records. This does not seem logical or fundamented, describe how that happens and which steps need to be done to trigger that race condition.
There was a problem hiding this comment.
We have seen this happen twice now. First, back in November, while working on workspace limit adjustments for members, we applied a script that would grant the workspace owner all unassigned space. After doing so, two owners were assigned negative space limits due to the workspace containing more members than its plan allowed. After further investigation, we found back then that these extra members were duplicates, with a createdAt at almost the exact second. If you take a look at the source code you can see that many db calls are made between the check userAlreadyInWorkspace and the actual insertion. This can be replicated by attempting to quickly click on accept invite when prompted in drive-web
Now, more recently, we had a bug report where a workspace owner reported their workspace appeared duplicated on web's workspace selector. In this case, the use case that sets up a workspace was called in repeated succession. If you take a look at the source code you'll see that a couple of db plus one network call are made between the userAlreadyInWorkspace check and the actual user insertion, leaving enough room for a race condition.
We could simply rerun the checks right before insertion, but still, wouldn't there be some chance of a race condition (albeit very slim, probably negligible)?
…into fix/unique-constraint-workspace-users
…Y for workspace_users and workspace_teams_users
|
|
This PR is stale because it has been open for more than 15 days with no activity. |
|
Closing this as proposed solution was not accepted, we may need to revist this in the future with a different scope. Perhaps only with the migrations that introduce a unique |
2 participants
Add UNIQUE constraints with
workspace_idandmember_idinworkspace_usersandworkspace_teams_usersto prevent duplicated members