Respect model's self.primary_key when creating Meilisearch indexes
#425
Conversation
- Use the model's #primary_key or nil. - Rails' Active Record defaults #primary_key to "id".
- Module size is at the RuboCop limit; consider refactoring soon.
WalkthroughRefactors option construction in meilisearch-rails to a centralized helper that derives defaults (type, per_page, page, primary_key) and simplifies enqueue and hit id selection. Adds a Story model with a non-standard primary key for tests. Expands specs to validate index primary_key alignment via the MeiliSearch client. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant M as Model (includes MeiliSearch::Rails)
participant H as default_meilisearch_options
participant C as MeiliSearch Client
participant I as Index
Note over M,H: Option construction (centralized)
M->>H: Build defaults from settings + options
H-->>M: { type, per_page, page, primary_key, ... }
Note over M,C: Index configuration uses derived primary_key
M->>C: Get index by index_uid
C-->>M: Index handle
M->>I: Set/verify primary_key (from model or option)
I-->>M: Acknowledgement
Note over M,I: Search flow
M->>I: search(query, params)
I-->>M: hits with ids
alt primary key is virtual
M->>M: Extract hit ids via virtual PK key
else
M->>M: Extract hit ids via primary_key
end
M-->>M: Map ids to records
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
lib/meilisearch-rails.rb(4 hunks)spec/options_spec.rb(2 hunks)spec/support/models/story.rb(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
spec/options_spec.rb (4)
spec/support/models/people.rb (1)
reset_people!(27-30)lib/meilisearch/rails/configuration.rb (1)
client(44-53)spec/support/models/story.rb (1)
reset_stories!(17-20)spec/support/models/book.rb (1)
reset_books!(37-41)
spec/support/models/story.rb (2)
spec/support/active_record_schema.rb (1)
ar_schema(20-22)spec/support/2_method_helpers.rb (1)
safe_index_uid(9-11)
lib/meilisearch-rails.rb (1)
spec/support/dummy_classes.rb (1)
model_name(4-6)
| def default_meilisearch_options(meilisearch_settings, options) | ||
| type = model_name.to_s.constantize | ||
| { | ||
| type: type, | ||
| per_page: meilisearch_settings.get_setting(:hitsPerPage) || 20, | ||
| page: 1, | ||
| primary_key: type.respond_to?(:primary_key) ? type.primary_key : nil | ||
| }.merge(options) | ||
| end |
There was a problem hiding this comment.
Avoid constantizing model_name
model_name.to_s is often customized (e.g. ActiveModel::Name.new(self, nil, 'PublicFacingName')). In those cases 'PublicFacingName'.constantize raises NameError, so any model overriding model_name will now blow up as soon as it calls meilisearch. Previously we never constantized and those models worked fine. Please derive type from self (or fall back to self if constantization fails) instead of assuming model_name corresponds to a constant.
Apply this diff to keep the behaviour safe:
- type = model_name.to_s.constantize
+ type = respond_to?(:name) && name ? name.safe_constantize || self : self📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def default_meilisearch_options(meilisearch_settings, options) | |
| type = model_name.to_s.constantize | |
| { | |
| type: type, | |
| per_page: meilisearch_settings.get_setting(:hitsPerPage) || 20, | |
| page: 1, | |
| primary_key: type.respond_to?(:primary_key) ? type.primary_key : nil | |
| }.merge(options) | |
| end | |
| def default_meilisearch_options(meilisearch_settings, options) | |
| type = respond_to?(:name) && name ? name.safe_constantize || self : self | |
| { | |
| type: type, | |
| per_page: meilisearch_settings.get_setting(:hitsPerPage) || 20, | |
| page: 1, | |
| primary_key: type.respond_to?(:primary_key) ? type.primary_key : nil | |
| }.merge(options) | |
| end |
🤖 Prompt for AI Agents
In lib/meilisearch-rails.rb around lines 795 to 803, avoid constantizing
model_name; replace type = model_name.to_s.constantize with logic that derives
type from self (or attempts constantize and rescues NameError), e.g. set type =
self and only try to constantize model_name.to_s if you want to support that,
but rescue NameError and fall back to self; ensure subsequent primary_key lookup
uses the resolved type (type.respond_to?(:primary_key) ? type.primary_key : nil)
so models that override model_name do not raise.
There was a problem hiding this comment.
Previously we never constantized and those models worked fine.
Actually, the current code constantize already. Hence, I'm not changing it. Moreover, I believe it should fail instead of quietly nil (it would fail in the next lines) or something else. WDYT, maintainers? :)
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the current code around line 381 to verify the user's claim
cat -n lib/meilisearch-rails.rb | sed -n '370,390p'
Length of output: 1050
🏁 Script executed:
#!/bin/bash
# Check the main branch to see what line 381 looked like before this PR
git fetch origin main
git show origin/main:lib/meilisearch-rails.rb | cat -n | sed -n '370,395p'
Length of output: 1411
@thicolares You're absolutely right, and I apologize for the confusion!
Looking at the main branch, line 381 indeed shows:
type: model_name.to_s.constantize,Your PR is simply refactoring this existing logic into the default_meilisearch_options method without changing the behavior. The constantization has always been there, so you're not introducing any new risk.
I withdraw my previous suggestion. The refactoring looks good as-is. Thank you for the clarification and for catching my error! 👍
🧬 Code Graph Analysis Results
[lib/meilisearch-rails.rb] - default_meilisearch_options usage
-
Summary
- In the class method default_meilisearch_options(meilisearch_settings, options), the code derives a type by calling model_name.to_s.constantize. This is the point where constantization happens to determine the model class for indexing.
- The method returns a hash including:
- type: the constantized model class
- per_page: from meilisearch_settings[:hitsPerPage] or default 20
- page: default 1
- primary_key: derived from the type if it responds to primary_key, else nil
- Merges the provided options into the resulting hash.
-
Rationale for relevance
- This is the explicit place where constantization is performed within lib/meilisearch-rails.rb. The user comment references a constantize usage at this area; understanding this snippet clarifies how the model class is resolved and what happens if the model name cannot be constantized.
1 participant
Pull Request
Related issue
Fixes #423
What does this PR do?
Before
The
Model.primary_keyis not used to setmeilisearch_options.primary_keyanywhere.After
Set
meilisearch_options.primary_keyfrom the ActiveRecordSomeModel.primary_keywhen Rails loads the model, which calls#meilisearch.SomeModel.primary_keyis"id"by default, or a custom key if overridden.Unchanged
If
primary_keyis explicitly passed to#meilisearch, it takes precedence. If neither is defined, theprimary_keyis set to nil.Why
These properties are better defined in initialize, as they are static model attributes that don’t change. This avoids redundant checks later and keeps the code cleaner and faster.
Future work (suggestion)
I believe there are still some now redundant checks (1, 2, 3) in the might that could be cleaned up in a separate PR.
PR checklist
Please check if your PR fulfills the following requirements:
Thank you so much for contributing to Meilisearch!
Summary by CodeRabbit
Bug Fixes
Refactor
Tests