Skip to content

fix: scope Bulk auto-populate config branch to Document payloads - #2304

Open
ruflin wants to merge 1 commit into
9.xfrom
fix/bulk-process-response-precedence
Open

fix: scope Bulk auto-populate config branch to Document payloads#2304
ruflin wants to merge 1 commit into
9.xfrom
fix/bulk-process-response-precedence

Conversation

@ruflin

@ruflin ruflin commented Apr 30, 2026

Copy link
Copy Markdown
Owner

Summary

Elastica\Bulk::_processResponse() had a precedence bug:

if ($data instanceof Document && $data->isAutoPopulate()
    || $this->_client->getConfigValue(['document', 'autoPopulate'], false)
)

Because && binds tighter than ||, the global document.autoPopulate
config caused setVersionParams() to run on any bulk action data —
including Script payloads — polluting them with response metadata
(_version, _seq_no, _primary_term) they have no use for.

The fix wraps the disjunction so the Document instance check applies
to the whole condition.

A new unit test synthesises a bulk response and asserts a Script payload
remains untouched. The test fails on the previous code and passes after
the fix (verified locally on PHP 8.5).

Identified during a P0/P1 code-review pass.

Test plan

  • `vendor/bin/phpunit --filter testProcessResponseDoesNotPopulateScriptWhenAutoPopulateEnabled` (passes after fix, fails before)
  • `vendor/bin/phpunit --group unit tests/BulkTest.php` (13 tests / 54 assertions, all green)
  • CI matrix (PHP 8.1–8.5)
  • PHPStan + php-cs-fixer

Summary by CodeRabbit

  • Bug Fixes
    • Fixed incorrect application of document auto-population behavior to Script payloads in bulk operations. Auto-population now correctly applies only to Document instances.

`Bulk::_processResponse()` had a missing pair of parentheses around the
auto-populate condition:

    if ($data instanceof Document && $data->isAutoPopulate()
        || $this->_client->getConfigValue(['document', 'autoPopulate'], false)
    )

Because `&&` binds tighter than `||`, the global `document.autoPopulate`
config caused `setVersionParams()` to run on any bulk action data,
including `Script` payloads, polluting them with response metadata they
don't use.

Wrap the disjunction so the `Document` instance check applies to the
whole branch. Add a unit-level regression test that synthesises a bulk
response and asserts a Script's params remain untouched.
Copilot AI review requested due to automatic review settings April 30, 2026 20:33
@coderabbitai

coderabbitai Bot commented Apr 30, 2026

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: ea1dbdf6-f3c5-4a99-8214-2756420cab98

📥 Commits

Reviewing files that changed from the base of the PR and between d0c0d60 and 13774ce.

📒 Files selected for processing (3)
  • CHANGELOG.md
  • src/Bulk.php
  • tests/BulkTest.php

📝 Walkthrough

Walkthrough

This PR fixes a bug in Elastica\Bulk::_processResponse() where the document.autoPopulate configuration was incorrectly being applied to Script payloads. The fix corrects boolean-condition operator precedence to limit auto-population to Document instances only. A regression test is added to prevent recurrence.

Changes

Cohort / File(s) Summary
Bug Fix
src/Bulk.php
Corrects conditional logic by adding parentheses to ensure document.autoPopulate behavior only executes when $data is a Document instance, preventing unintended application to Script payloads.
Testing & Documentation
tests/BulkTest.php, CHANGELOG.md
Adds regression test testProcessResponseDoesNotPopulateScriptWhenAutoPopulateEnabled() to verify Script objects are not affected by auto-population logic; documents the fix in the unreleased changelog.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~15 minutes

Poem

A rabbit fixed logic so tight,
With parentheses placed just right,
Scripts no longer auto-populate,
Documents celebrate their fate,
The bulk response shines bright! 🐰✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.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 accurately summarizes the main change: fixing the scope of the auto-populate config logic to only apply to Document payloads, which directly addresses the precedence bug fixed in src/Bulk.php.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ 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 fix/bulk-process-response-precedence

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
Review rate limit: 3/8 reviews remaining, refill in 30 minutes and 42 seconds.

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

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Fixes a boolean operator precedence issue in bulk response processing so the global document.autoPopulate configuration only triggers version-parameter population for Document payloads (and not Script payloads), aligning Bulk behavior with other response-handling paths in the codebase.

Changes:

  • Parenthesized the auto-populate condition in Bulk::_processResponse() so instanceof Document scopes the entire check.
  • Added a unit regression test asserting Script payloads are not populated with version/seq_no/primary_term when global auto-populate is enabled.
  • Documented the fix in the changelog.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
src/Bulk.php Fixes &&/`
tests/BulkTest.php Adds regression coverage to ensure Script payloads are not mutated by _processResponse() under global auto-populate.
CHANGELOG.md Records the bugfix under Unreleased → Fixed.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

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