nick-boss-tech commented on PR #5220:
URL: https://github.com/apache/solr/pull/5220#issuecomment-6388770177
**Draft Review TL/DR:**
No concrete implementation bug found, but testing is incomplete.
- P2: No tests for the new PUT endpoint. Add coverage for enable/disable
behavior, missing or null input, actual HTTP routing, and authorization.
Existing v1 tests do not exercise this API.
- The PR body checks “tests added” and “Reference Guide added,” but neither
appears in the diff.
The implementation preserves the existing replication flag and update
permission.
**Draft Review Details:**
🤖 *AI text below and above* 🤖 *(reviewed by Nick Shanin)*
@jaykay12 I reviewed `e6113dc093a9`, including the new PUT endpoint, request
validation, resource registration, and the existing v1 enable/disable commands.
I did not find a concrete implementation defect in those paths.
The implementation appears consistent with the existing behavior: both API
versions update the same replication flag, the new method requires
`UPDATE_PERM`, and using a boxed `Boolean` allows the validation to distinguish
a missing value from `enabled: false`. `CoreReplication` is already registered
as a Jersey resource, so no additional resource registration appears necessary.
The main remaining gap is test coverage, which the PR’s Tests section
already identifies as “To be added.” There are no test changes in this
revision. Existing replication tests exercise the v1 commands, so they would
not establish that the new PUT route accepts the intended JSON body and changes
the correct core’s state.
Before considering this ready, I suggest covering:
- **Both state transitions:** send `{"enabled": false}` and `{"enabled":
true}`, then verify the resulting replication state or observable behavior.
Checking only a successful response would miss a setter that does nothing.
- **Required input:** verify that a missing body, an empty object, and
`{"enabled": null}` return 400 without changing the current state.
- **The actual HTTP route:** exercise `PUT
/api/cores/{coreName}/replication` so the test covers routing and JSON
deserialization, alongside any direct method tests in `CoreReplicationAPITest`.
- **Authorization:** verify that a user with read permission alone cannot
change replication state, while a user with the required update permission can.
The existing `CoreReplicationAPITest` and leader/follower setup in
`TestReplicationHandler` look like useful starting points. I would keep the
scope focused on this API’s contract and parity with the v1 commands.
There is also a small description inconsistency: the tests-added and
Reference Guide-added boxes are checked, although neither is present in the
diff. Those should reflect what is included when the PR is ready.
This was a static review.
--
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]
