Fixing cross joins in lib-banzai
What does this MR do and why?
In the MR: !124319 (merged) we temporarily allow-listed cross joins between users
and namespaces
. In this MR we are fixing the cross joins in lib/banzai
between these two tables, to prepare them to be on separate databases, as part of the Cells project.
We follow these guidelines on how to resolve cross joins between tables that would belong to different databases: https://docs.gitlab.com/ee/development/database/multiple_databases.html#removing-joins-between-ci-and-non-ci-tables
Addressing: #417466 (closed)
Changelog: fixed
How to verify the changes locally
UserReferenceParser
A. To verify the change on Create an issue and reference users, using for example @root
. When you save the issue the link should be rendered and point to the User personal namespace.
UserParser
B. To verify the change on Create an issue and reference some group or @all
. When you save the issue:
- The members of the projects are added, as long as the group doesn't have
mentioned_disabled
- With
@all
, all the project members are added to the subscribers as well. You will need to test that by having members added to the project, but not the parent group. This is assuming thatdisable_all_mention
FF is not enabled.
Newly introduced database queries
In UserReferenceFilter#namespaces
- Preloading
users
onnamespaces
SELECT * FROM "users" WHERE "users"."id" IN (1, 2, 3, 1614863)
https://console.postgres.ai/gitlab/gitlab-production-tunnel-pg12/sessions/23217/commands/74690
In UserParser
SELECT "project_authorizations"."user_id" FROM "project_authorizations" WHERE "project_authorizations"."project_id" IN (6, 7, 13083)
https://console.postgres.ai/gitlab/gitlab-production-tunnel-pg12/sessions/23285/commands/74854
This query @all
on project issue. It's not worth optimzing for two reasons:
- It's not a common case
- There is a plan to disable the
@all
. See https://gitlab.com/gitlab-org/gitlab/-/blob/master/config/feature_flags/development/disable_all_mention.yml
SELECT DISTINCT "members"."user_id" FROM "members" WHERE "members"."type" = 'GroupMember' AND "members"."source_type" = 'Namespace' AND "members"."source_id" IN (SELECT "namespaces"."id" FROM "namespaces" WHERE "namespaces"."type" = 'Group' AND "namespaces"."id" = 9970 AND (mentions_disabled IS NOT TRUE)) AND "members"."requested_at" IS NULL AND "members"."invite_token" IS NULL AND (members.access_level > 5)
https://postgres.ai/console/gitlab/gitlab-production-tunnel-pg12/sessions/23315/commands/74944
SELECT * from users where ID (1, 13, 5, 8, 9)
The plan has been shared before
Some remarks
- In
UserReferenceParser
: Even though I am removing the cross joins withroutes
, this cross join is still enforced from insideNamespace.where_full_path_in
, which itself has another allowed cross join to be solved later in #420046. I still have to includepreload(:route)
in this method, because I cannot guarantee that in the futureNamespace.where_full_path_in
will keep:route
preloaded on theNamespace
objects. - I removed the
verifies the number of queries
test because the twoupdate_issue
requests are not equal, and cause different code paths to be executed. The first one caused anNote
with a user mention to be rendered. While the 2nd update has a not that doesn't a mention. This causes discrepancy in the number of queries. So I decided to remove this test, because it doesn't make sense to test requests at such a high level. Many parts of the app are touched by this request.
Open Questions
- Do you think it makes sense to execute the new logic in
UserParser
under a feature flag? what are the consequences of something does not go as planned in theUserParser
? maybe that'a question to @engwan - If a feature flag is introduced, should I make the actor
current_user
, because that's available on theBaseParser
.
MR acceptance checklist
This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.
-
I have evaluated the MR acceptance checklist for this MR.
Related to #417466 (closed)