Skip to content

Rename QueryLimiting.whitelist to .disable! [RUN AS-IF-FOSS]

Luke Duncalfe requested to merge ld-rename-query_limiting_whitelist-to-allowlist into master

What does this MR do?

This MR renames QueryLimiting.whitelist to QueryLimiting.disable! as whitelist has racist connotations.

See the Query count limits docs page to read about the Gitlab::QueryLimiting class. In short, tests that make more than 100 SQL queries fail so we can better see badly performing parts of the application, and we have the ability to disable query limiting if we need to.

This MR also changes the way we persist the state of disabling query limiting. It was previously done by setting an instance variable on Gitlab::QueryLimiting::Transaction, but is now done using Gitlab::SafeRequestStore. In order to not leak disabled state between tests, we disable explicitly after each test in spec_helper.rb.

Issue: #324285 (closed)

Verify

We can check if query limiting still works in this branch by switching it off for some code and running the corresponding request test. For example:

diff --git a/lib/api/helpers/notes_helpers.rb b/lib/api/helpers/notes_helpers.rb
index cb938bc8a14..c2d65138c91 100644
--- a/lib/api/helpers/notes_helpers.rb
+++ b/lib/api/helpers/notes_helpers.rb
@@ -145,7 +145,7 @@ def resolve_discussion(noteable, discussion_id, resolved)
       end

       def disable_query_limiting
-        Gitlab::QueryLimiting.disable!('https://gitlab.com/gitlab-org/gitlab/-/issues/211538')
+        # Gitlab::QueryLimiting.disable!('https://gitlab.com/gitlab-org/gitlab/-/issues/211538')
       end
     end
   end

Then run:

bundle exec rspec spec/requests/api/notes_spec.rb

You'll see Gitlab::QueryLimiting::Transaction::ThresholdExceededError errors.

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

Edited by Luke Duncalfe

Merge request reports