Skip to content

Enhance record update tests and refactor attribute handling#452

Open
Strift wants to merge 2 commits into
mainfrom
rails-441-document-payload-computed-twice-when-using-attribute
Open

Enhance record update tests and refactor attribute handling#452
Strift wants to merge 2 commits into
mainfrom
rails-441-document-payload-computed-twice-when-using-attribute

Conversation

@Strift

@Strift Strift commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Pull Request

Related issue

Fixes #450

What does this PR do?

  • Fixes a performance bug in the attribute / add_attribute indexing path where custom document fields could be computed twice during updates.
  • Separates “which fields are indexed” from “build the document payload”, so change detection no longer evaluates expensive custom attribute builders.

AI disclosure

Cursor gpt-5.4-high and codex-5.3-high

PR checklist

Please check if your PR fulfills the following requirements:

  • Did you use any AI tool while implementing this PR (code, tests, docs, etc.)? If yes, disclose it in the PR description and describe what it was used for. AI usage is allowed when it is disclosed.
  • Does this PR fix an existing issue, or have you listed the changes applied in the PR description (and why they are needed)?
  • Have you read the contributing guidelines?
  • Have you made sure that the title is accurate and descriptive of the changes?

Thank you so much for contributing to Meilisearch!

Summary by CodeRabbit

  • Bug Fixes & Improvements
    • Added clearer warnings when configured searchable attributes aren’t present in indexed document attributes.
    • Improved reindex/change-detection behavior to ensure custom attribute builders run only when indexed attributes actually change (no-op updates won’t trigger extra indexing).
  • Refactor
    • Unified document attribute extraction and indexed field name computation for more consistent payload generation and dirty checking.
  • Tests
    • Expanded settings and integration coverage for serializer and custom attribute-builder scenarios, including update/no-op behavior.

@Strift Strift added the bug Something isn't working label Jun 23, 2026
@coderabbitai

coderabbitai Bot commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 9caa212b-5cb6-451d-8d48-9bcf9176b91d

📥 Commits

Reviewing files that changed from the base of the PR and between bc82063 and bfb1484.

📒 Files selected for processing (1)
  • spec/settings_spec.rb
🚧 Files skipped from review as they are similar to previous changes (1)
  • spec/settings_spec.rb

📝 Walkthrough

Walkthrough

IndexSettings now centralizes indexed-field derivation and document attribute building, and the indexing path uses those methods for reindexing, normal indexing, and dirty checks. Test support models and specs were added to cover custom attribute builders and evaluation counts.

Changes

Document Attribute Centralization and Single-Evaluation Fix

Layer / File(s) Summary
Settings cleanup and indexed field warnings
lib/meilisearch-rails.rb
warn_searchable_missing_attributes compares searchable fields against configured attributes, use_serializer keeps only serializer assignment, and get_default_attributes removes the legacy comment.
Field-name and document attribute builders
lib/meilisearch-rails.rb
Adds indexed_field_names(document) and refactors document building into build_document_attributes(document), with consistent handling for nil values, proc evaluation, sanitization, and UTF-8 forcing.
Indexing and dirty-checking use the new builders
lib/meilisearch-rails.rb
ms_reindex!, ms_index_documents, and ms_index! switch to build_document_attributes, and ms_must_reindex? uses indexed_field_names for change detection.
Support models for attribute-builder evaluation
spec/support/models/custom_attribute_builder_models.rb
Adds two ActiveRecord support models, per-model evaluation counters, reset helpers, and will_save_change_to_search_blob? wiring for attribute and add_attribute configurations.
Specs for new document-building behavior
spec/settings_spec.rb, spec/integration/active_record/record_is_updated_spec.rb
Updates attribute-building specs to use build_document_attributes and adds integration coverage for single evaluation on updates and no extra evaluation on no-op saves.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Poem

🐇 I hopped through code both night and day,
One payload build now leads the way.
The indexed fields are clear and neat,
No double work beneath my feet.
With tests in tow, the counters sing,
A tidy change for indexing!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title is concise and accurately reflects the main changes: record update tests and attribute handling refactor.
Linked Issues check ✅ Passed The change separates indexed-field detection from payload building, preventing custom attribute builders from running during dirty checks.
Out of Scope Changes check ✅ Passed The added tests, support models, and warning cleanup are all tied to the attribute-handling refactor.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch rails-441-document-payload-computed-twice-when-using-attribute

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@Strift
Strift marked this pull request as draft June 23, 2026 08:34

@coderabbitai coderabbitai Bot 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.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
lib/meilisearch-rails.rb (1)

96-108: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Include add_attribute fields in the searchable warning check.

Line 98 only compares searchable_attributes against @attributes, so a valid configuration like attribute :name; add_attribute :search_blob; searchable_attributes ['search_blob'] will log a misleading “not in attributes” warning even though search_blob is configured for the document payload.

Proposed fix
-        attrs = get_setting(:attributes)&.map { |k, _| k.to_s }
+        attrs = [get_setting(:attributes), get_setting(:additional_attributes)]
+          .compact
+          .flat_map { |configured_attributes| configured_attributes.keys.map(&:to_s) }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@lib/meilisearch-rails.rb` around lines 96 - 108, The
warn_searchable_missing_attributes method only includes regular attributes from
get_setting(:attributes) when checking against searchable_attributes, but it
ignores attributes added via add_attribute declarations. To fix this, modify the
attrs variable assignment to also collect and include any attributes that were
added through add_attribute calls, so the warning comparison includes both
regular and added attributes in the searchable warning check.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@lib/meilisearch-rails.rb`:
- Around line 96-108: The warn_searchable_missing_attributes method only
includes regular attributes from get_setting(:attributes) when checking against
searchable_attributes, but it ignores attributes added via add_attribute
declarations. To fix this, modify the attrs variable assignment to also collect
and include any attributes that were added through add_attribute calls, so the
warning comparison includes both regular and added attributes in the searchable
warning check.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 13a9d5d3-bace-4e11-a98f-90a3c36b4e64

📥 Commits

Reviewing files that changed from the base of the PR and between 16a153d and 8efb252.

📒 Files selected for processing (4)
  • lib/meilisearch-rails.rb
  • spec/integration/active_record/record_is_updated_spec.rb
  • spec/settings_spec.rb
  • spec/support/models/custom_attribute_builder_models.rb

@Strift

Strift commented Jun 30, 2026

Copy link
Copy Markdown
Contributor Author

CI is failing due to meilisearch/meilisearch#6482

@Strift
Strift force-pushed the rails-441-document-payload-computed-twice-when-using-attribute branch from 8efb252 to bc82063 Compare July 7, 2026 02:27
@codecov

codecov Bot commented Jul 7, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 95.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 90.79%. Comparing base (c76d310) to head (bfb1484).

Files with missing lines Patch % Lines
lib/meilisearch-rails.rb 95.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #452      +/-   ##
==========================================
+ Coverage   90.69%   90.79%   +0.09%     
==========================================
  Files          14       14              
  Lines         849      847       -2     
==========================================
- Hits          770      769       -1     
+ Misses         79       78       -1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Co-authored-by: Cursor <cursoragent@cursor.com>
@Strift
Strift marked this pull request as ready for review July 7, 2026 02:43
@Strift
Strift requested review from StephaneRob and curquiza July 7, 2026 02:44
@curquiza
curquiza removed their request for review July 7, 2026 07:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Document payload computed twice when using attribute

1 participant