nick-boss-tech commented on PR #5219:
URL: https://github.com/apache/solr/pull/5219#issuecomment-6389433907
**Review TL/DR:**
The implementation looks consistent with the existing v1 enable/disable
behavior. I see tests are already planned; the main gap is exercising the
actual PUT route with both enabled: false and enabled: true, checking the
resulting replication state, and covering missing/null input and permission
enforcement. In particular, a read-only user should be unable to change the
flag. This is a coverage request, not a demonstrated implementation failure. My
review was static; I haven’t run the tests.
**Review Details:**
🤖 AI text below and above 🤖 (reviewed by Nick Shanin)
@jaykay12 I reviewed e6113dc093a9. The implementation appears consistent
with the existing v1 commands: both paths update the same AtomicBoolean, the
new endpoint declares the matching UPDATE_PERM, and the null checks allow
enabled: false as a valid value. I didn’t find a concrete implementation bug in
those paths.
I see the Tests section already says “To be added.” Before considering the
endpoint ready, I’d suggest covering these parts of its HTTP contract:
- Issue actual PUT requests to /api/cores/{coreName}/replication with
{"enabled": false} and then {"enabled": true}, asserting the resulting handler
state or observable replication behavior after each request.
- Exercise an absent body, an empty object, and {"enabled": null}. Verify
the intended 400 response and that validation leaves the replication state
unchanged.
- Exercise authorization with an appropriately permitted user and a
read-only user, verifying that the latter is denied and cannot change the state.
At least one test should go through HTTP so it covers resource discovery and
JSON mapping as well as the Java method. Existing v1 enable/disable tests don’t
establish that the new route works.
There’s also a small description mismatch: the tests-added and Reference
Guide-added boxes are checked, but neither kind of file appears in the current
diff. Please reconcile those as the remaining work lands.
These are verification requests, not claims that those cases currently fail.
My review was static; I haven’t compiled or run the tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
