Fix Elasticsearch pagination with null sortable field values

Dmitry Gruzdrequested to merge
fix-elasticsearch-pagination-null-values into master

What does this MR do and why?

This MR fixes a critical bug in Elasticsearch pagination where records with null sortable field values (e.g., milestone_due_date) were completely unreachable after the first page, even though they existed in the index.

Problem: When using Advanced Search (Elasticsearch) with GLQL or other features that sort by nullable fields, pagination would stop returning results once it reached records with null values. For example, when sorting issues by milestone ascending:

  • Page 1: Issues with milestones ✅
  • Page 2+: Empty results ❌ (issues without milestones were unreachable)

Root Cause:

  1. Elasticsearch uses sentinel values for nulls: Long.MAX_VALUE (9223372036854775807) for ASC, Long.MIN_VALUE (-9223372036854775808) for DESC
  2. These sentinel values were passed directly in pagination cursors
  3. Range queries like field > cursor_value don't match missing/null fields in Elasticsearch
  4. This created a pagination gap where null records were sorted last but unreachable via pagination

Solution:

  1. Convert Elasticsearch sentinel values (Long.MAX_VALUE / Long.MIN_VALUE) back to nil in cursor generation
  2. Add null-aware pagination filter logic that handles nil cursors explicitly
  3. Include must_not exists clause when paginating forward to capture null values that sort after non-null values
  4. Guard the fix behind feature flag search_glql_fix_null_field_pagination for safe rollout

Impact:

  • Fixes pagination for GLQL queries sorting by nullable fields
  • Fixes Advanced Search pagination for issues, merge requests, etc. sorted by nullable columns
  • Works correctly for both ASC and DESC sort orders
  • Maintains backward compatibility via feature flag

References

  • Closes #583599
  • Rollout issue: #583611
  • Feature flag: search_glql_fix_null_field_pagination (type: gitlab_com_derisk)

Screenshots or screen recordings

Before (with feature flag disabled)

Sorting by milestone ascending with 10 issues (6 with milestones, 4 without):

  • Page 1: Shows issues 1, 2, 3 (with milestones) ✅
  • Page 2: Empty results ❌
  • Issues 4-7 (without milestones) are unreachable

After (with feature flag enabled)

Sorting by milestone ascending:

  • Page 1: Issues 1, 2, 3 (Milestones 1, 2)
  • Page 2: Issues 8, 9, 10 (Milestones 3, 4)
  • Page 3: Issues 4, 5, 6 (No milestone)
  • Page 4: Issue 7 (No milestone)
  • All 10 issues accessible ✅

Sorting by milestone descending also works:

  • Page 1: Issues 10, 9, 8 (latest milestones)
  • Page 2: Issues 3, 2, 1 (earlier milestones)
  • Page 3+: Issues 7, 6, 5, 4 (no milestone)

How to set up and validate locally

Setup

  1. Enable Elasticsearch in your GDK:

    gdk config set elasticsearch.enabled true
gdk reconfigure
gdk restart

  2. Create test data with nullable milestone field:

    # In rails console
project = Project.find_by_full_path('your-group/your-project')

# Create issues with milestones
milestone1 = project.milestones.create!(title: 'Milestone 1', due_date: 1.week.from_now)
3.times { |i| project.issues.create!(title: "Issue #{i+1} with milestone", milestone: milestone1) }

# Create issues without milestones
4.times { |i| project.issues.create!(title: "Issue #{i+4} no milestone") }

# Wait for ES indexing or trigger manually
WorkItem.where(project: project).each(&:maintain_elasticsearch_update)
sleep 2  # Give ES time to index

  3. Enable the feature flag:

    Feature.enable(:search_glql_fix_null_field_pagination)

Validation

Test ASC sorting

  1. Navigate to GLQL interface or use GraphQL:

    display: table
fields: title, milestone
limit: 3
query: project = "your-group/your-project" AND type = issue
sort: milestone asc

  2. Verify:

    • Page 1 shows issues WITH milestones
    • Clicking "Load more" shows more issues (not empty)
    • Continue pagination until you see issues WITHOUT milestones
    • All issues are accessible

Test DESC sorting

  1. Change query to sort: milestone desc
  2. Verify:
    • Page 1 shows issues with latest milestones
    • Pagination continues through earlier milestones
    • Final pages show issues WITHOUT milestones (nulls sort last)
    • All issues are accessible

Test with feature flag disabled

  1. Disable the feature flag:

    Feature.disable(:search_glql_fix_null_field_pagination)

  2. Repeat the ASC test

  3. Verify the bug reproduces: pagination stops returning results after issues with milestones

Cleanup 

Feature.disable(:glql_advanced_search_fix_null_field_pagination)

Technical Details

Files Modified

  1. ee/lib/search/elastic/relation.rb

    • Modified cursor_for to convert Elasticsearch sentinel values to nil
    • Handles both Long.MAX_VALUE (ASC) and Long.MIN_VALUE (DESC)

  2. ee/lib/search/elastic/pagination.rb

    • Modified paginate to allow nil sort values
    • Added pagination_filter method with null-aware branching logic
    • Added pagination_filter_for_null_cursor for pagination within null values
    • Added pagination_filter_for_non_null_cursor with must_not exists clause
    • Retained legacy_pagination_filter for feature flag rollback

  3. ee/config/feature_flags/gitlab_com_derisk/search_glql_fix_null_field_pagination.yml

    • Added feature flag definition for controlled rollout

  4. ee/spec/lib/search/elastic/pagination_spec.rb

    • Added comprehensive test coverage for null value pagination
    • Tests for ASC and DESC sorting with nil cursors
    • Tests for backward pagination (should not include nulls)
    • Tests for feature flag enabled/disabled states

Key Implementation Details

Cursor Conversion:

# Convert Long.MAX_VALUE or Long.MIN_VALUE to nil
sort_values.map do |v|
  if v == 9223372036854775807 || v == -9223372036854775808
    nil
  else
    v
  end
end

Null-Aware Filter (ASC/DESC forward pagination):

# Include nulls when paginating forward (both ASC and DESC)
should_clauses << { bool: { must_not: { exists: { field: sort_property } } } } if is_after

Pagination with nil cursor:

# When cursor is [nil, tie_breaker], only filter by tie_breaker
{
  bool: {
    must: [
      { bool: { must_not: { exists: { field: sort_property } } } },
      { range: { id: { gt: tie_breaker_value } } }
    ]
  }
}

MR acceptance checklist

This MR has been evaluated against the MR acceptance checklist:

  • ✅ Performance: No N+1 queries, Elasticsearch query structure is optimal
  • ✅ Security: No security implications, works with existing authorization
  • ✅ Observability: Feature flag allows monitoring and quick rollback
  • ✅ Testing: Comprehensive RSpec tests for all scenarios
  • ✅ Documentation: Feature flag documented, inline code comments added
  • ✅ Quality: RuboCop passes, no syntax errors
  • ✅ Maintainability: Clean separation via feature flag, backward compatible
  • ✅ Reliability: Tested with real Elasticsearch, handles edge cases

Testing Performed

  • Unit tests for pagination logic with null values
  • Manual testing with test project (10 issues, various milestone states)
  • Verified ASC sorting pagination works end-to-end
  • Verified DESC sorting pagination works end-to-end
  • Verified feature flag toggle (enabled/disabled)
  • Verified backward pagination doesn't incorrectly include nulls
  • Confirmed Elasticsearch query structure is correct
Edited by Dmitry Gruzd

Merge request reports