Skip to content

Prune blank conditions from the low-level c: API - #1687

Merged
scarroll32 merged 6 commits into
fix-1414-deep-stripfrom
fix-1653-advanced-blank
Sep 21, 2026
Merged

scarroll32 merged 6 commits into
fix-1414-deep-stripfrom
fix-1653-advanced-blank

Conversation

@scarroll32

Copy link
Copy Markdown
Member

Note

This PR was opened by Claude (Claude Code), acting on behalf of @scarroll32.
It builds on @gdott9's #1653 — thank you for finding this.

Closes #1653.

The bug

Search#initialize drops conditions whose value is blank, but it only inspects top-level values. The c: API nests its values a level deeper, so they were never seen. The condition then built its attribute and contributed a join, producing a query with a LEFT OUTER JOIN and no WHERE clause — different behaviour from the equivalent shorthand:

Person.ransack(children_name_eq: '').result.to_sql
# => SELECT "people".* FROM "people"

Person.ransack(c: { '0' => { a: ['children_name'], p: 'eq', v: [''] } }).result.to_sql
# => SELECT "people".* FROM "people" LEFT OUTER JOIN "people" "children_people" ON ...

Three changes to @gdott9's version

1. The Hash form of v: was dropped unconditionally. This is the important one. The original guard read:

value[:v].all? { |_, i| value[:value].blank? && value[:value] != false }

value there is the enclosing condition hash ({a:, p:, v:}), so value[:value] is always nil — always blank, never false. Every Hash-form value was therefore pruned regardless of content. Verified against the branch as submitted:

Person.ransack(c: { '0' => { a: ['name'], p: 'eq', v: { '0' => { value: 'Ernie' } } } }).result.to_sql
# => SELECT "people".* FROM "people" ORDER BY "people"."id" DESC
#    the condition is silently gone

A search that quietly returns every row is a worse failure than the one being fixed — that's an unfiltered result set going somewhere a filtered one was intended. The value is now unwrapped from either the bare or { value: ... } envelope form before being tested, matching the two shapes Condition#values= already accepts.

2. String keys were missed. params has not been through with_indifferent_access at that point in initialize, so params.key?(:c) doesn't match the "c" that arrives from an HTTP query string — the main way this API is used. Both spellings are handled.

3. The blank test is factored out into blank_condition_value?, shared by the top-level filter and the c: filter, so the two cannot drift. That also preserves the !i.nil? clause added in #1657, which the original overwrote — without it, name_in: [nil] would have regressed.

Tests

@gdott9's spec, plus three guarding the above:

  • blank values pruned from a string-keyed c: hash
  • a populated Hash-form value survives (fails on the original branch)
  • a populated Array-form value survives

Full suite: 533 examples, 0 failures, 1 pending (Rails 8.1.3 / Ruby 3.4.9 / SQLite). RuboCop clean — the two Layout/SpaceAfterComma offences that were failing build on #1653 are gone.

🤖 Generated with Claude Code

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

The implementation mutates caller input and misses blank conditions inside nested groupings.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 Medium severity

Open (1)
What changed in this PR

Prunes blank values from low-level advanced-search conditions to avoid unnecessary joins.

Changes:

  • Adds blank-condition pruning for symbol- and string-keyed c parameters.
  • Preserves populated array and hash-form values.
  • Adds regression coverage for these cases.
File Description
lib/​ransack/​search.rb Adds advanced-condition pruning helpers.
spec/​ransack/​adapters/​active_record/​base_spec.rb Adds pruning and value-preservation tests.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread lib/ransack/search.rb Outdated
Comment on lines +188 to +190
def prune_blank_advanced_conditions!(params)
conditions = params[:c] || params['c']

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note

Reply from Claude (Claude Code), acting on behalf of @scarroll32.

Agreed. Fixed in 948b14c: the pruning now recurses through g/groupings at every depth (Array or index-keyed Hash) and prunes c/conditions in each. While doing that I found the params were only shallow-duplicated, so the in-place delete_if was editing the caller's nested hash — they are now deep_duped first. Specs cover single, double and long-form nesting, plus a snapshot check that the input comes back unchanged.

A condition given through the `c:` API with an empty value was not
dropped, because the params filter in Search#initialize only looks at
top-level values and `c:` nests its values a level deeper. The condition
still built its attribute and contributed a join, so the query came out
with a LEFT OUTER JOIN and no WHERE clause to go with it — behaving
differently from the equivalent shorthand form.

Builds on @gdott9's work in #1653, with three changes.

The Hash form of `v:` was dropped unconditionally. The original guard
read `value[:value]` where `value` is the enclosing condition hash, so it
was always nil, always blank, and every Hash-form value was pruned:

  Person.ransack(c: { "0" => { a: ["name"], p: "eq",
                               v: { "0" => { value: "Ernie" } } } })
  # => SELECT "people".* FROM "people"    (condition silently gone)

A search that silently returns everything is worse than the bug being
fixed, so the value is now unwrapped from either form before testing it.

`params` is not yet indifferent-access at this point, so a string-keyed
`"c"` — the shape that arrives from an HTTP query string — was missed.
Both spellings are handled.

The blank test is factored out so the top-level filter and the `c:`
filter cannot drift apart, and so `name_in: [nil]` keeps working.

Closes #1653

Co-Authored-By: gdott9 <1830024+gdott9@users.noreply.github.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@scarroll32
scarroll32 force-pushed the fix-1653-advanced-blank branch from 14addc4 to 0428964 Compare September 21, 2026 10:21
Records that the shorthand and low-level `c:` forms now agree: a blank
value is dropped in both, so neither leaves a dangling join behind.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@scarroll32 scarroll32 mentioned this pull request Sep 21, 2026
scarroll32 and others added 4 commits September 21, 2026 12:57
Docusaurus resolved the URL-style relative link against the page's own
path, so ./configuration#blank-values became
/getting-started/advanced-mode/configuration/ and the docs build failed
on a broken link. Linking by file path lets Docusaurus resolve it.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The pruning only looked at a top-level c:, so the same dangling join
survived when the condition sat inside a g: grouping — which nests to any
depth and is the form Grouping#build accepts. It now descends into every
grouping, under the short or long key spellings.

Search#initialize also only shallow-duplicated the params, so deleting
from a nested conditions hash edited the caller's own object. The params
are deep-duplicated first, and a spec snapshots the input to prove it
comes back unchanged.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@scarroll32
scarroll32 changed the base branch from main to fix-1414-deep-strip September 21, 2026 11:14
@scarroll32
scarroll32 added this pull request to stack #1700 September 21, 2026 11:24
@scarroll32
scarroll32 merged commit 1a9d7e7 into main Sep 21, 2026
28 checks passed
@scarroll32
scarroll32 deleted the fix-1653-advanced-blank branch September 21, 2026 11:34
@scarroll32 scarroll32 mentioned this pull request Sep 21, 2026
45 of 46 tasks
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.

2 participants