Backend support for advanced search multi-select project [RUN ALL RSPEC] [RUN AS-IF-FOSS]
What does this MR do?
Related to Epic: &6110 and Issue: #332631
Backend support for allowing multi-select project search in Advanced Search only. Basic Search support will be handled in a separate MR. This feature is behind a disabled by default feature flag: advanced_search_multi_project_select
for groups
Added a new param project_ids
which will take the form of project_ids=1,2,3
or project_ids=1
and is only set if a group is selected and the Feature Flag is enabled for the selected group.
If any of the projects selected have Advanced Search enabled, the query will go to Elasticsearch. Once Basic Search supports multi-select projects, additional work will be done to combine results when projects results come from Postgres and Elasticsearch.
How to test
Note: You need to have Elasticsearch enabled in GDK, have Advanced Search enabled for search + indexing, and have created the index using rake commands or the Admin UI (rake gitlab:elastic:index
)
- Enable the feature for a specific group in the rails console:
Feature.enable(:advanced_search_multi_project_select, group)
- Navigate to the search page: http://localhost:3000/search
- Select the group you enabled the feature for in the drop down
- Take note of 2 project ids for that group
- Manually add
&project_ids=AA,BB
to the URL where AA and BB are the project_ids from the previous step - perform some searches and make sure you are getting data back from both projects
Note: I also did some testing by turning on Namespace limiting and trying to send the project_ids
parameter to groups and projects that only support Basic Search to make sure it didn't break anything.
Database
There is a count query and select query for each search scope and will detail both here. Project ids used: 278964 and 7764
Select
Explain plan: https://explain.depesz.com/s/PbPt
SELECT
“users”. *
FROM
“users”
WHERE (((“users”.“name” ILIKE ‘%test%’
OR “users”.“username” ILIKE ‘%test%’)
OR “users”.“email” = ‘test’)
OR “users”.“id” = (
SELECT
“emails”.“user_id”
FROM
“emails”
WHERE
“emails”.“email” = ‘test’
LIMIT 1))
AND “users”.“id” IN (
SELECT
“users”.“id”
FROM
“users”
INNER JOIN “project_authorizations” ON “project_authorizations”.“user_id” = “users”.“id”
WHERE
“project_authorizations”.“project_id” IN (278964, 7764))
ORDER BY
CASE WHEN users.name = ‘test’ THEN
0
WHEN users.username = ‘test’ THEN
1
WHEN users.email = ‘test’ THEN
2
ELSE
3
END,
“users”.“name” ASC
LIMIT 20 OFFSET 0
Count
Explain plan: https://explain.depesz.com/s/HWrU
SELECT
COUNT(*)
FROM (
SELECT
1 AS one
FROM
“users”
WHERE (((“users”.“name” ILIKE ‘%test%’
OR “users”.“username” ILIKE ‘%test%’)
OR “users”.“email” = ‘test’)
OR “users”.“id” = (
SELECT
“emails”.“user_id”
FROM
“emails”
WHERE
“emails”.“email” = ‘test’
LIMIT 1))
AND “users”.“id” IN (
SELECT
“users”.“id”
FROM
“users”
INNER JOIN “project_authorizations” ON “project_authorizations”.“user_id” = “users”.“id”
WHERE
“project_authorizations”.“project_id” IN (278964, 7764))
LIMIT 100) subquery_for_count
Screenshots (strongly suggested)
Tested with two project ids which map to gnome-1
and Fdroidclient
projects (which you can see in the screenshots)
Does this MR meet the acceptance criteria?
Conformity
-
I have included a changelog entry, or it's not needed. (Does this MR need a changelog?) -
I have added/updated documentation, or it's not needed. (Is documentation required?) -
I have properly separated EE content from FOSS, or this MR is FOSS only. (Where should EE code go?) -
I have added information for database reviewers in the MR description, or it's not needed. (Does this MR have database related changes?) -
I have self-reviewed this MR per code review guidelines. -
This MR does not harm performance, or I have asked a reviewer to help assess the performance impact. (Merge request performance guidelines) -
I have followed the style guides.
Availability and Testing
-
I have added/updated tests following the Testing Guide, or it's not needed. (Consider all test levels. See the Test Planning Process.) -
I have tested this MR in all supported browsers, or it's not needed. -
I have informed the Infrastructure department of a default or new setting change per definition of done, or it's not needed.
Security
Does this MR contain changes to processing or storing of credentials or tokens, authorization and authentication methods or other items described in the security review guidelines? If not, then delete this Security section.
-
Label as security and @ mention @gitlab-com/gl-security/appsec
-
The MR includes necessary changes to maintain consistency between UI, API, email, or other methods -
Security reports checked/validated by a reviewer from the AppSec team
Merge request reports
Activity
changed milestone to %14.0
added Category:Global Search Deliverable UX backend devopssystems frontend groupglobal search sectioncore platform workflowin dev + 1 deleted label
assigned to @terrichu
3 Warnings 4e6d228e: Commits that change 30 or more lines across at least 3 files should describe these changes in the commit body. For more information, take a look at our Commit message guidelines. 76eedcf2: Commits that change 30 or more lines across at least 3 files should describe these changes in the commit body. For more information, take a look at our Commit message guidelines. featureaddition and featureenhancement merge requests normally have a documentation change. Consider adding a documentation update or confirming the documentation plan with the Technical Writer counterpart.
For more information, see:
- The Handbook page on merge request types.
- The definition of done documentation.
1 Message You're adding or removing a feature flag, your MR title needs to include [RUN ALL RSPEC] [RUN AS-IF-FOSS]
(we may have updated it automatically for you and started a new MR pipeline) to ensure everything is covered.Reviewer roulette
Changes that require review have been detected!
Please refer to the table below for assigning reviewers and maintainers suggested by Danger in the specified category:
Category Reviewer Maintainer backend Krasimir Angelov ( @krasio
) (UTC+12, 16 hours ahead of@terrichu
)Imre Farkas ( @ifarkas
) (UTC+2, 6 hours ahead of@terrichu
)database Alex Buijs ( @alexbuijs
) (UTC+2, 6 hours ahead of@terrichu
)Steve Abrams ( @sabrams
) (UTC-6, 2 hours behind@terrichu
)To spread load more evenly across eligible reviewers, Danger has picked a candidate for each review slot, based on their timezone. Feel free to override these selections if you think someone else would be better-suited or use the GitLab Review Workload Dashboard to find other available reviewers.
To read more on how to use the reviewer roulette, please take a look at the Engineering workflow and code review guidelines. Please consider assigning a reviewer or maintainer who is a domain expert in the area of the merge request.
Once you've decided who will review this merge request, assign them as a reviewer! Danger does not automatically notify them for you.
If needed, you can retry the
danger-review
job that generated this comment.Generated by
Dangeradded featureaddition label
added typefeature label
- A deleted user
added feature flag label
- Resolved by Terri Chu
- Resolved by Terri Chu
added 1 commit
- 1c0dbc12 - Backend support for multi-select project search
added 1 commit
- e04123c2 - Backend support for multi-select project search
added 1 commit
- c865e79a - Backend support for multi-select project search
added 1 commit
- e8c1accf - Backend support for multi-select project search
marked the checklist item I have included a changelog entry, or it's not needed. (Does this MR need a changelog?) as completed
marked the checklist item I have added/updated documentation, or it's not needed. (Is documentation required?) as completed
marked the checklist item I have properly separated EE content from FOSS, or this MR is FOSS only. (Where should EE code go?) as completed
added 1 commit
- 1967e0f3 - Backend support for multi-select project search
marked the checklist item This MR does not harm performance, or I have asked a reviewer to help assess the performance impact. (Merge request performance guidelines) as completed
marked the checklist item I have added information for database reviewers in the MR description, or it's not needed. (Does this MR have database related changes?) as completed
marked the checklist item I have followed the style guides. as completed
marked the checklist item I have added/updated tests following the Testing Guide, or it's not needed. (Consider all test levels. See the Test Planning Process.) as completed
marked the checklist item I have tested this MR in all supported browsers, or it's not needed. as completed
marked the checklist item I have informed the Infrastructure department of a default or new setting change per definition of done, or it's not needed. as completed
added 1088 commits
-
1967e0f3...99a60957 - 1087 commits from branch
master
- ae3e904d - Backend support for multi-select project search
-
1967e0f3...99a60957 - 1087 commits from branch
- Resolved by Matthias Käppler
Allure report
allure-report-publisher
generated test report for 1ef24268!review-qa-smoke:
test reportadded 1 commit
- 2668a578 - Backend support for multi-select project search
added database label
- A deleted user
added databasereview pending label
added 1 commit
- 9e065b3a - Backend support for multi-select project search
- Resolved by Terri Chu
- Resolved by Dmitry Gruzd
@dgruzd would you mind the initial review here? I wanted to get your take on it first and hope that is ok since this is a complex feature
I feel like I should be adding more tests, but am struggling with where. Do you have any suggestions?
added workflowin review label and removed workflowin dev label
requested review from @dgruzd
marked the checklist item I have self-reviewed this MR per code review guidelines. as completed
- Resolved by Terri Chu
@alexbuijs are you available for an initial database review? I believe I got all the database info required but feel free to ping me if I missed anything