-
Notifications
You must be signed in to change notification settings - Fork 54
Fix conditional indexing with enqueue mode in ms_enqueue_index! #427
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Fix conditional indexing with enqueue mode in ms_enqueue_index! #427
Conversation
WalkthroughThe Changes
Sequence DiagramsequenceDiagram
participant Caller
participant ms_enqueue_index!
participant Utilities
participant Enqueue Handler
participant Index
participant ms_index!
Caller->>ms_enqueue_index!: ms_enqueue_index!(synchronous)
ms_enqueue_index!->>Utilities: indexable?(document, options)
alt Document is indexable
Utilities-->>ms_enqueue_index!: true
alt Enqueue configured
ms_enqueue_index!->>Enqueue Handler: call(self, remove=false)
Enqueue Handler->>Index: background job → add_documents
else No enqueue config
ms_enqueue_index!->>ms_index!: ms_index!(synchronous)
ms_index!->>Index: add_documents
end
else Document not indexable
Utilities-->>ms_enqueue_index!: false
ms_enqueue_index!->>Utilities: ms_conditional_index?(options)
alt Conditional indexing enabled
Utilities-->>ms_enqueue_index!: true
alt Enqueue configured
ms_enqueue_index!->>Enqueue Handler: call(self, remove=true)
Enqueue Handler->>Index: background job → delete_document
else No enqueue config
ms_enqueue_index!->>ms_index!: ms_remove_from_index!(synchronous)
ms_index!->>Index: delete_document
end
else Conditional indexing disabled
Utilities-->>ms_enqueue_index!: false
Note over ms_enqueue_index!: No action taken
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
d0ea7be to
d08785c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
lib/meilisearch-rails.rb (1)
956-974: LGTM! Successfully aligns withms_index!behavior.The implementation correctly addresses the issue where non-indexable documents weren't being removed from the index when using enqueue mode. The two-branch structure now mirrors the logic in
ms_index!(lines 562-591):
- Indexable documents → enqueue indexing (or fallback to synchronous)
- Non-indexable with conditional indexing → enqueue removal (or fallback to synchronous)
The nested conditional blocks at lines 958-961 and 966-969 are nearly identical and could be refactored to reduce duplication:
def ms_enqueue_index!(synchronous) - if Utilities.indexable?(self, meilisearch_options) - if meilisearch_options[:enqueue] - unless self.class.send(:ms_indexing_disabled?, meilisearch_options) - meilisearch_options[:enqueue].call(self, false) - end - else - ms_index!(synchronous) - end - elsif self.class.send(:ms_conditional_index?, meilisearch_options) - if meilisearch_options[:enqueue] - unless self.class.send(:ms_indexing_disabled?, meilisearch_options) - meilisearch_options[:enqueue].call(self, true) - end - else - ms_remove_from_index!(synchronous) + is_indexable = Utilities.indexable?(self, meilisearch_options) + should_process = is_indexable || self.class.send(:ms_conditional_index?, meilisearch_options) + + return unless should_process + + remove = !is_indexable + + if meilisearch_options[:enqueue] + unless self.class.send(:ms_indexing_disabled?, meilisearch_options) + meilisearch_options[:enqueue].call(self, remove) end + else + remove ? ms_remove_from_index!(synchronous) : ms_index!(synchronous) end endThis reduces cognitive complexity while preserving the same logic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/meilisearch-rails.rb(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
lib/meilisearch-rails.rb (1)
lib/meilisearch/rails/utilities.rb (1)
indexable?(44-49)
|
I will add unit tests once the logic will be pre-approved - but looking at default mode and it's specs - that's valid approach. |
|
@coderabbitai generate docstrings |
✅ Actions performedInitiated docstring generation; will generate only if new commits exist. |
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @whitemerry. * #427 (comment) The following files were modified: * `lib/meilisearch-rails.rb`
Pull Request
Related issue
Fixes #426
What does this PR do?
Fixes conditional indexing when using
enqueue: trueoption. Previously, when a record no longer met the indexing condition (e.g.,if: :published?returnedfalse), the document would remain in the Meilisearch index instead of being removed.Code reflects logic implemented in
ms_index!method.PR checklist
Please check if your PR fulfills the following requirements:
Thank you so much for contributing to Meilisearch!
Summary by CodeRabbit