Introduce NoteQueryBuilder for note Elasticsearch queries

Ravi Kumarrequested to merge
462686-note-query-builder into master

What does this MR do and why?

Adds Search::Elastic::NoteQueryBuilder and uses it for note search when the search_notes_use_query_builder feature flag is on. This fixes a crash on blank note searches.

Notes were the last scope that mixed the legacy ApplicationClassProxy#basic_query_hash with the new Search::Elastic::Filters. For a blank search term, the legacy hash had no filter array. The filters then raised NoMethodError: undefined method 'concat' for nil. BoolExpr always starts with an empty filter array, so the new builder does not have this problem.

The builder uses Search::Elastic::Queries.by_full_text and BoolExpr, like the milestone, project and user builders. NoteClassProxy#elastic_search uses it when the flag (type gitlab_com_derisk, actor: current user) is on. The legacy path stays for rollback. This is the same approach as the users migration: #462683 (closed)

Snippets have the same crash. They will be migrated separately, after the project snippet work for #35760 merges.

An earlier one-line fix, !261069 (closed), was closed in favour of this MR.

Query components

QUERY_COMPONENTS run in this order:

  1. Formats: source_fields, size
  2. Filters: by_search_level_and_membership, by_note_confidentiality (Planner minimum), by_not_hidden (only after the backfill_hidden_on_notes migration), by_archived, by_noteable_type

Formats run first so by_noteable_type can override _source and size for the related-ids lookup used by merge request and issue search.

Behaviour changes for reviewers
  • The named query for the archived filter changes from note:archived:non_archived to filters:non_archived.
  • Advanced syntax detection now uses the shared Search::Elastic::Queries regex, like other scopes. For example, foo-bar is no longer treated as advanced syntax. # and ! now are.
  • There is no archived filter at project level on either path. Production passes search_level: 'project' as a string. The old proxy spec passed a symbol, which made the legacy path apply the filter. The spec now uses the string.
Tests and verification
  • New spec: ee/spec/lib/search/elastic/note_query_builder_spec.rb.
  • ee/spec/lib/elastic/latest/note_class_proxy_spec.rb runs its integration examples with the flag on and off. It also has a blank query example with the flag on.
  • In GDK, Note.elastic_search returned the same first page of note IDs with the flag on and off. I checked an admin, a project member and a non-member, at global, group and project level, with six search terms. The terms included foo-bar, test* and a quoted phrase.

References

Rollout issue: #633081

Screenshots or screen recordings

Request in GDK: GET /api/v4/projects/1/search?scope=merge_requests&state=opened&search=%20

Flag off (before) Flag on (after)
nqb_flag_off nqb_flag_on

With the flag off, the request returns 500 Internal Server Error. With the flag on, it returns the open merge requests.

How to set up and validate locally

  1. Use a GDK with Advanced Search enabled. In Admin > Settings > Search, turn on "Search with Advanced Search" and wait for indexing to finish.
  2. Sign in.
  3. In a Rails console, enable the flag: 
    Feature.enable(:search_notes_use_query_builder)
  4. Open https://gdk.test:3443/api/v4/projects/<id>/search?scope=merge_requests&state=opened&search=%20 for a project with open merge requests. You get JSON results.
  5. Disable the flag and reload: 
    Feature.disable(:search_notes_use_query_builder)
    You get a 500 error. This is the legacy path.

MR acceptance checklist

Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

🤖 Generated with Claude Code

Edited by Ravi Kumar

Merge request reports