Repository navigation
Treat an unknown polymorphic type as an invalid search key - #1741
Merged
Merged
Conversation
The class in an `_of_Model_type` suffix was resolved with `Kernel.const_get`, which raises `NameError` for a name that is not a constant, or is lowercase, straight from the query string, whatever `ignore_unknown_conditions` is set to. A constant that is not a model (`String`, a module, an abstract class, `ActiveRecord::Base` itself) got past the lookup and raised from `klassify` or the schema instead. Resolve the name with `safe_constantize` and accept only a concrete model, through a new `searchable_class?` hook on the context that the Active Record integration refines. Any other suffix is not a polymorphic reference, so the key is ignored by default and raises `InvalidSearchError` in a strict search, like any unknown attribute. Fixes #1738 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Merged
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Non-module namespace components still raise an unhandled TypeError, bypassing unknown-key handling.
1 open finding
What changed in this PR
Routes invalid polymorphic type suffixes through Ransack’s existing unknown-key handling.
Changes:
- Adds safe constant resolution and model validation.
- Tests invalid conditions and sorts in default and strict modes.
- Documents the updated behavior.
| File | Description |
|---|---|
spec/ransack/search_spec.rb |
Adds invalid-type regression coverage. |
lib/ransack/context.rb |
Resolves and validates polymorphic types. |
lib/ransack/active_record/context.rb |
Restricts types to concrete Active Record models. |
docs/going-further/polymorphic-search.md |
Explains invalid-type handling. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
`safe_constantize` swallows a missing constant but not a namespace that is not a module: `"ENV::Person".safe_constantize` raises `TypeError`, so `notable_of_ENV::Person_type_name_eq` still raised from the query string. Treat it like every other non-model suffix. Found by Copilot review. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Note
This PR was opened by Claude (Claude Code), acting on behalf of @scarroll32.
Reported in #1738, an agent-filed issue that was deleted from the tracker shortly after this PR was opened, so the reference no longer resolves. The bug was reproduced independently and the fix stands on its own.
The bug
The class in an
_of_Model_typesuffix was resolved withKernel.const_get, straight from the query string. Four kinds of input raised instead of followingignore_unknown_conditions:notable_of_NoSuchType_type_name_eqNameError: uninitialized constant NoSuchTypenotable_of_person_type_name_eqNameError: wrong constant name personnotable_of_String_type_name_eq(a module likewise)ArgumentError: Don't know how to klassify Stringnotable_of_ApplicationRecord_type_name_eq(abstract)ActiveRecord::TableNotSpecifiednotable_of_ENV::Person_type_name_eq(namespace that is not a module)TypeError: ENV::Person does not refer to class/moduleThe issue reports the first. The others fall out of the same lookup; the last was found by Copilot's review of this PR.
The fix
Context#unpolymorphize_associationresolves the name withsafe_constantizeand accepts only a concrete model, through a newsearchable_class?hook on the context: by default the class must haveransackable_attributes; the Active Record context requires a non-abstractActiveRecord::Basesubclass. Any other suffix is not a polymorphic reference, so the key is treated like any unknown attribute: ignored by default,InvalidSearchErrorin a strict search. A sort on such a key follows the same rule.Specs cover all eight shapes under both settings, and a sort on an unknown type and on a non-module namespace. The docs' polymorphic page gets a paragraph and a note.
Suite: 802 examples, 0 failures on Rails 8.1 and 7.2.2.1 (SQLite) and on PostgreSQL; RuboCop clean.
Worth a backport to
5-0-stable? A crafted query string currently produces a 500 fromNameError. There is no resource exhaustion, so I have not treated it as an advisory.🤖 Generated with Claude Code