nick-boss-tech commented on PR #5218: URL: https://github.com/apache/solr/pull/5218#issuecomment-6388873362
**Review TL/DR:** - P2: UI test uses the wrong API version. The [updated helper](https://github.com/apache/solr/blob/2ea1ef96a4f925a5eaed0716a98898d9b9c49e4c/solr/webapp/src/test/org/apache/solr/webapp/AdminUiParamsetsScreenTest.java#L63-L70) requests /config/paramsets through a v1 client, but that path exists only in v2. Use an explicit v2 request and handle 404 as expected absence after deletion. - P2 compatibility concern: Reusing the new lookup changes v1’s missing-paramset response from 200 to 404. Preserve v1 behavior or obtain explicit agreement for that additional change. The ordinary v2 read path otherwise looks coherent. **Review Detail:** 🤖 *AI text below and above* 🤖 *(reviewed by Nick Shanin)* @jaykay12 I reviewed `2ea1ef96a4f9`, including the new JAX-RS endpoints, the v1 delegation, and the Admin UI changes. The new resource registration and `CONFIG_READ_PERM` annotations look consistent, and the browser service correctly separates GET `/config/paramsets` from the existing POST `/config/params`. I found two points to address: 1. **The updated UI test observer still sends a v1 request.** In [`[AdminUiParamsetsScreenTest.paramsetRows()](https://github.com/apache/solr/blob/2ea1ef96a4f925a5eaed0716a98898d9b9c49e4c/solr/webapp/src/test/org/apache/solr/webapp/AdminUiParamsetsScreenTest.java#L63-L70)`](https://github.com/apache/solr/blob/2ea1ef96a4f925a5eaed0716a98898d9b9c49e4c/solr/webapp/src/test/org/apache/solr/webapp/AdminUiParamsetsScreenTest.java#L63-L70), the lookup now uses `/<collection>/config/paramsets/<name>` and expects the flat v2 response. However, `AdminUiTestBase.adminApi()` constructs a `GenericSolrRequest`, which inherits `SolrRequest`’s default API version of V1. That means the observer requests the new suffix through v1 routing, where the handler still recognizes `/config/params`, rather than reaching `/api/collections/<collection>/config/paramsets/<name>`. The first post-create lookup therefore cannot retrieve the paramset. Its exception is wrapped in `RuntimeException`, and `waitUntil()` does not catch exceptions from the condition. I suggest using an explicitly v2 request, or the generated `ParamSetsApi.GetParamSet` request with `IndexType.COLLECTION`. The deletion check also needs to account for the new named lookup returning 404 when the paramset disappears. Treat that specific status as expected absence while allowing other errors to fail the test. Alternatively, list the paramsets through v2 and check whether the name is present. 2. **The shared lookup changes the existing v1 missing-name behavior.** At the base revision, GET `/solr/<collection>/config/params/<missing>` returns a normal 200 response containing `response.znodeVersion`, with no matching paramset entry. The [[new v1 delegation](https://github.com/apache/solr/blob/2ea1ef96a4f925a5eaed0716a98898d9b9c49e4c/solr/core/src/java/org/apache/solr/handler/SolrConfigHandler.java#L199-L205)](https://github.com/apache/solr/blob/2ea1ef96a4f925a5eaed0716a98898d9b9c49e4c/solr/core/src/java/org/apache/solr/handler/SolrConfigHandler.java#L199-L205) calls `GetParamSets.getParamSet()`, which throws 404 for that case. Existing v1 callers checking for absence would consequently receive an exception instead of an inspectable response. The guide explicitly documents 404 for both versions, so I recognize that the patch acknowledges this change. My concern is compatibility and scope: returning 404 for the new v2 resource makes sense, but a v2 migration does not itself require changing the established v1 response. My recommendation is to preserve the existing absence response in the v1 adapter and retain 404 for v2, with a test distinguishing the two contracts. If changing v1 is an agreed part of the migration, that decision should also be explicit in the PR description and changelog. This was a static review; I did not execute the browser test or reproduce these cases locally. At review time, the upstream build and test workflows were awaiting maintainer approval. The first point follows directly from the request-routing code; the second is a compatibility concern for maintainer judgment. -- 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]
