This commit introduces the usage of Common Table Expressions (CTEs) to
efficiently retrieve nested group hierarchies, without having to rely on
the "routes" table (which is an _incredibly_ inefficient way of getting
the data). This requires a patch to ActiveRecord (found in the added
initializer) to work properly as ActiveRecord doesn't support WITH
statements properly out of the box.
Unfortunately MySQL provides no efficient way of getting nested groups.
For example, the old routes setup could easily take 5-10 seconds
depending on the amount of "routes" in a database. Providing vastly
different logic for both MySQL and PostgreSQL will negatively impact the
development process. Because of this the various nested groups related
methods return empty relations when used in combination with MySQL.
For project authorizations the logic is split up into two classes:
* Gitlab::ProjectAuthorizations::WithNestedGroups
* Gitlab::ProjectAuthorizations::WithoutNestedGroups
Both classes get the fresh project authorizations (= as they should be
in the "project_authorizations" table), including nested groups if
PostgreSQL is used. The logic of these two classes is quite different
apart from their public interface. This complicates development a bit,
but unfortunately there is no way around this.
This commit also introduces Gitlab::GroupHierarchy. This class can be
used to get the ancestors and descendants of a base relation, or both by
using a UNION. This in turn is used by methods such as:
* Namespace#ancestors
* Namespace#descendants
* User#all_expanded_groups
Again this class relies on CTEs and thus only works on PostgreSQL. The
Namespace methods will return an empty relation when MySQL is used,
while User#all_expanded_groups will return only the groups a user is a
direct member of.
Performance wise the impact is quite large. For example, on GitLab.com
Namespace#descendants used to take around 580 ms to retrieve data for a
particular user. Using CTEs we are able to reduce this down to roughly 1
millisecond, returning the exact same data.
== On The Fly Refreshing
Refreshing of authorizations on the fly (= when
users.authorized_projects_populated was not set) is removed with this
commit. This simplifies the code, and ensures any queries used for
authorizations are not mutated because they are executed in a Rails
scope (e.g. Project.visible_to_user).
This commit includes a migration to schedule refreshing authorizations
for all users, ensuring all of them have their authorizations in place.
Said migration schedules users in batches of 5000, with 5 minutes
between every batch to smear the load around a bit.
== Spec Changes
This commit also introduces some changes to various specs. For example,
some specs for ProjectTeam assumed that creating a personal project
would _not_ lead to the owner having access, which is incorrect. Because
we also no longer refresh authorizations on the fly for new users some
code had to be added to the "empty_project" factory. This chunk of code
ensures that the owner's permissions are refreshed after creating the
project, something that is normally done in Projects::CreateService.
- While deleting a user, some of the user's associated records are moved to the
ghost user so they aren't deleted. The user is blocked before these records
are moved, to prevent the user from creating new records while the migration
is happening, and so preventing a data race.
- Previously, if the migration failed, the user would _remain_ blocked, which is
not the expected behavior. On the other hand, we can't just stick the block +
migration into a transaction, because we want the block to be committed before
the migration starts (for the data race reason mentioned above).
- One solution (implemented in this commit) is to block the user in a parent
transaction, migrate the associated records in a nested sub-transaction, and
then unblock the user in the parent transaction if the sub-transaction fails.
1. Have `MigrateToGhostUser` be a service rather than a mixed-in module, to keep
things explicit. Specs testing the behavior of this class are moved into a
separate service spec file.
2. Add a `user.reported_abuse_reports` association to make the
`migrate_abuse_reports` method more consistent with the other `migrate_`
methods.
... when the user is destroyed.
1. Normally, for a given awardable and award emoji name, a user is only allowed
to create a single award emoji.
2. This validation needs to be removed for ghost users, since:
- User A and User B have created award emoji - with the same name and against
the same awardable
- User A is deleted. Their award emoji is moved to the ghost user
- User B is deleted. Their award emoji needs to be moved to the ghost user.
However, this breaks the uniqueness validation, since the ghost user is
only allowed to have one award emoji of a given name for a given awardable
1. When the user is deleted.
2. Refactor out code relating to "migrating records to the ghost user" into a
`MigrateToGhostUser` concern, which is tested using a shared example.
This param is passed to service in two places, one is in the build_user for non ldap oauth users. And the other is in the initial production admin user seed data.
Without this change, when setting up GitLab in a production environment, you were not being given the option of setting the root password on initial setup in the UI.
When deleting a user, the following sequence could happen:
1. Project `mygroup/myproject` is scheduled for deletion
2. The group `mygroup` is deleted
3. Sidekiq worker runs to delete `mygroup/myproject`, but the namespace and routes have
been destroyed.
Closes#30334
- Have `Uniquify` take a block instead of a Proc/function. This is more
idiomatic than passing around a function in Ruby.
- Block a user before moving their issues to the ghost user. This avoids a data
race where an issue is created after the issues are migrated to the ghost user,
and before the destroy takes place.
- No need to migrate issues (to the ghost user) in a transaction, because
we're using `update_all`
- Other minor changes
- "Associated" issues are issues the user has created + issues that the
user is assigned to.
- Issues that a user owns are transferred to a "Ghost User" (just a
regular user with `state = 'ghost'` that is created when
`User.ghost` is called).
- Issues that a user is assigned to are moved to the "Unassigned" state.
- Fix a spec failure in `profile_spec` — a spec was asserting that when a user
is deleted, `User.count` decreases by 1. After this change, deleting a user
creates (potentially) a ghost user, causing `User.count` not to change. The
spec has been updated to look for the relevant user in the assertion.
We saw from a recent incident that the `Users::DestroyService` would
attempt to delete a user over and over. Revoking the permissions
from the current user did not help. We should ensure that the
current user does, in fact, have permissions to delete the user.
Signed-off-by: Rémy Coutable <remy@rymai.me>
* Changed name of delete_user_service and worker to destroy
* Move and change delete_group_service to Groups::DestroyService
* Rename Notes::DeleteService to Notes::DestroyService
Previously a lease would only be obtained to update data. This could
lead to duplicate data being inserted, triggering a UNIQUE constraint
error. To work around this we now acquire a lease before performing
_any_ project authorization work, releasing it at the very end.
Fixes#25987
This column used to be a 32 bits integer, allowing for only a maximum of
2 147 483 647 rows. Given enough users one can hit this limit pretty
quickly, as was the case for GitLab.com.
Changing this type to bigint (= 64 bits) would give us more space, but
we'd eventually hit the same limit given enough users and projects. A
much more sustainable solution is to simply drop the "id" column.
There were only 2 lines of code depending on this column being present,
and neither truly required it to be present. Instead the code now uses
the "project_id" column combined with the "user_id" column. This means
that instead of something like this:
DELETE FROM project_authorizations
WHERE user_id = X
AND id = Y;
We now run the following when removing rows:
DELETE FROM project_authorizations
WHERE user_id = X
AND project_id = Y;
Since both user_id and project_id are indexed this should not slow down
the DELETE query.
This commit also removes the "dependent: destroy" clause from the
"project_authorizations" relation in the User and Project models.
Keeping this prevents Rails from being able to remove data as it relies
on an "id" column being present. Since the "project_authorizations"
table has proper foreign keys set up (with cascading removals) we don't
need to depend on any Rails logic.
Prior to this commit the refreshing of authorized projects was done in
two steps:
1. Remove existing authorizations
2. Insert a new list of all authorizations
This can lead to a high amount of dead tuples as every time all rows are
being replaced. For example, if a user with 100 authorizations is given
access to a new project this would lead to:
* 100 rows being removed
* 101 new rows being inserted
This commit changes the way this system works so it only removes/inserts
what is necessary. Using the above example this would lead to only 1 new
row being inserted, with the initial 100 being left untouched.
Fixes https://gitlab.com/gitlab-org/gitlab-ce/issues/25257
Michael Kozono
Robert Speicher
Rémy Coutable
Jarka Kadlecova
Paul Charlton
James Lopez
Grzegorz Bizon
Nick Thomas
Yorick Peterse
Valery Sizov
Clement Ho
Timothy Andrew
Sean McGivern
Stan Hu
George Andrinopoulos
Douwe Maan
DJ Mountney
Douwe Maan
dixpac
Adam Niedzielski