Skip to content

Simplify sort-clause de-duplication and document single-key invariant#1263

Open
jwils wants to merge 1 commit into
mainfrom
joshuaw/fix-sort-dedup-multikey-shortcircuit
Open

Simplify sort-clause de-duplication and document single-key invariant#1263
jwils wants to merge 1 commit into
mainfrom
joshuaw/fix-sort-dedup-multikey-shortcircuit

Conversation

@jwils

@jwils jwils commented Jun 17, 2026

Copy link
Copy Markdown
Collaborator

What

DatastoreQuery#remove_duplicate_sort_clauses used:

sort_clauses.select do |clause|
  clause.keys.all? { |key| seen_fields.add?(key) }
end

This relied on Set#add?'s side effect inside all?, conflating filtering with mutation. For a multi-key clause, all? short-circuits as soon as a previously-seen key is encountered, leaving the remaining keys of that clause unrecorded in seen_fields — a subtle correctness bug.

In practice this branch is unreachable: sort clauses are always single-key, enforced upstream by DecodedCursor::Factory.from_sort_list, which raises Errors::InvalidSortFieldsError ("Each must be a flat hash with one entry") for anything else.

Changes

  • Simplified the method to operate directly on the single key via seen_fields.add?(clause.keys.first).
  • Added a comment documenting the single-key invariant and why add? correctly drops duplicates.

This follows the project guideline to avoid defensive code for impossible cases rather than adding multi-key handling for a scenario that can't occur.

Testing

bundle exec rspec elasticgraph-graphql/spec/unit/elastic_graph/graphql/datastore_query/sorting_spec.rb \
                  elasticgraph-graphql/spec/unit/elastic_graph/graphql/datastore_query/merge_spec.rb
# 45 examples, 0 failures

`remove_duplicate_sort_clauses` used `clause.keys.all? { |key| seen_fields.add?(key) }`,
which relied on `Set#add?`'s side effect inside `all?`. That conflated filtering
with mutation and would have left some keys unrecorded for a multi-key clause due to
`all?`'s short-circuiting. Sort clauses are in fact always single-key (enforced upstream
by `DecodedCursor::Factory.from_sort_list`, which raises `InvalidSortFieldsError` for
anything else), so the multi-key branch was both unreachable and subtly wrong.

Simplified to operate directly on the single key and added a comment documenting the
invariant, per the project's 'avoid defensive code for impossible cases' guideline.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant