GraphQL MergeRequest.pipelines loses offset pagination for BRANCHES and TAGS scopes

Summary

MergeRequest.pipelines silently loses offset pagination when queried with scope: BRANCHES or scope: TAGS (or the equivalent refType: heads / refType: tags, which ResolvesPipelines::REF_TYPE_SCOPE_MAP maps onto those scopes).

For those two scopes the resolver is supposed to return an offset-paginated connection, because the ordering they produce cannot be paginated by keyset. The marker that selects offset pagination is discarded before resolution, so the field falls back to keyset pagination on an ordering the code itself documents as unsupported.

This is pre-existing on master and is not caused by !249797 (merged), which is where it was found.

Mechanism

ResolvesPipelines#resolve_pipelines wraps the relation for these two scopes:

if %w[branches tags].include?(params[:scope])
  # `branches` and `tags` scopes are ordered in a complex way that is not supported by the keyset pagination.
  # We offset pagination here so we return the correct connection.
  offset_pagination(pipelines)
else
  pipelines
end

offset_pagination returns Gitlab::Graphql::Pagination::OffsetPaginatedRelation, a SimpleDelegator whose only job is to mark the relation so the correct connection type is chosen. It re-wraps itself for exactly two methods:

class OffsetPaginatedRelation < SimpleDelegator
  def preload(...)
    self.class.new(super)
  end

  def includes(...)
    self.class.new(super)
  end
end

merge and unscope are not overridden. Both therefore delegate to the wrapped relation and return a bare ActiveRecord::Relation, dropping the marker.

Resolvers::MergeRequestPipelinesResolver#query_for chains both onto the result of resolve_pipelines:

resolve_pipelines(pipeline_project(mr), args.merge(merge_request_event_first: true))
  .unscope(where: :project_id)
  .merge(::Ci::PipelinesForMergeRequestFinder.new(mr, current_user).execute)

So for scope: BRANCHES or scope: TAGS the marker is stripped and offset pagination is never applied. On master the same thing happens through .merge(mr.all_pipelines) alone, so the behaviour predates the unscope call.

The fact that preload and includes were given explicit re-wrapping overrides suggests this class of bug was already known; merge and unscope were missed.

Verified

  • Types::Ci::PipelineScopeEnum exposes BRANCHES and TAGS, and MergeRequestPipelinesResolver includes ResolvesPipelines, so the scope argument is reachable on this field.
  • OffsetPaginatedRelation overrides only preload and includes.
  • MergeRequestPipelinesResolver#query_for calls merge (on master) and merge plus unscope (on the branch above) on the resolve_pipelines result.

Not verified

No query was executed. The user-visible symptom of falling back to keyset pagination on this ordering has not been reproduced or characterised. It may be incorrect page boundaries, duplicated or omitted records, or an error. That should be established before choosing a fix.

Suggested fix

Two options, smallest first:

  1. Add merge and unscope overrides to OffsetPaginatedRelation, following the existing self.class.new(super) pattern. Smallest change, but it keeps the class in a position where every new relation method has to be remembered.
  2. Restructure MergeRequestPipelinesResolver#query_for so the offset wrap is applied last, after all relation chaining. This removes the ordering dependency instead of patching another method into the delegator.

Option 2 is the more durable of the two.

Related

There is a deeper cause worth recording, but it is a larger change and should not be folded in here. Ci::PipelinesFinder is project-scoped, and it is used on this path only because it owns the GraphQL filter arguments (status, ref, sha, source, username, ids, scope). Serving a two-project query with it requires neutralising its base relation with unscope(where: :project_id). Removing that would mean either teaching Ci::PipelinesFinder to accept a relation, or extracting its filters.