fix: scope Index::refresh() to the target index (#2313)#2315
Conversation
Index::refresh() was calling indices()->refresh() without passing the
index name, which the underlying elasticsearch-php client maps to
`POST /_refresh` (a cluster-wide refresh) instead of the expected
`POST /{index}/_refresh`. This caused calls like
`$client->getIndex('my-index')->refresh()` to silently refresh every
index in the cluster, hurting performance on shared clusters.
Pass `['index' => $this->getName()]` so the request is correctly
scoped to the current index, matching the behavior of neighboring
methods (delete, exists, open, close, setSettings, analyze).
Client::refreshAll() is unchanged and remains the explicit
cluster-wide refresh.
Refs #2313
There was a problem hiding this comment.
Pull request overview
This PR fixes Elastica\Index::refresh() so it refreshes only the current index (calling POST /{index}/_refresh) instead of performing an unintended cluster-wide refresh (POST /_refresh), aligning the method behavior with the surrounding Index API and the expectation from issue #2313.
Changes:
- Scope
Index::refresh()to the index by passing theindexparameter to the underlying Elasticsearch client call. - Add a functional test asserting the last request path is
/{index}/_refreshafter calling$index->refresh(). - Add a changelog entry documenting the fix.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/Index.php |
Passes ['index' => $this->getName()] to ensure refresh targets only the current index. |
tests/IndexTest.php |
Adds a functional regression test verifying the refresh request path is index-scoped. |
CHANGELOG.md |
Documents the behavior change under “Fixed” for Unreleased. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| $lastRequest = $client->getLastRequest(); | ||
| $this->assertNotNull($lastRequest); | ||
| $this->assertSame( |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
Index Refresh Targeting Fix
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Address Copilot review feedback on #2315: use assertInstanceOf with Psr\Http\Message\RequestInterface for clearer failures and stronger typing (consistent with ClientFunctionalTest::testLastRequestResponse).
Summary
Fixes #2313.
Elastica\Index::refresh()was callingindices()->refresh()on the underlyingelastic/elasticsearchclient without passing theindexparameter. That maps toPOST /_refresh— a cluster-wide refresh — instead of the expectedPOST /{index}/_refresh.That is surprising for an instance method on
Elastica\Index, especially because every neighboring method (delete(),exists(),open(),close(),setSettings(),analyze(),forcemerge(), …) does pass['index' => $this->getName()]. In practice, code like:```php
$client->getIndex('my-index')->refresh();
```
was silently refreshing every index in the cluster on every call, which is expensive on shared/large clusters.
Change
src/Index.php: pass['index' => \$this->getName()]toindices()->refresh()so the request is correctly scoped to the current index.Client::refreshAll()is unchanged — it is the explicit cluster-wide refresh and still callsindices()->refresh()without an index, mapping toPOST /_refresh.Backward compatibility
This is a bug fix. Anyone who was actually relying on
\$index->refresh()to refresh the whole cluster (almost certainly nobody, given the method name and surrounding API) can call\$client->refreshAll()instead, which has been the dedicated API for that since at least 8.x.Test plan
IndexTest::testRefreshTargetsIndexAndNotClusterthat asserts\$client->getLastRequest()->getUri()->getPath()equals/{index}/_refreshafter calling\$index->refresh()(same pattern used byClientFunctionalTest::testLastRequestResponse).vendor/bin/phpunit --group unit --filter IndexTest).Changelog
Added an entry under `### Fixed` in the `Unreleased` section of `CHANGELOG.md` referencing #2313.
Summary by CodeRabbit
Bug Fixes
Tests