gerlowskija commented on code in PR #4264:
URL: https://github.com/apache/solr/pull/4264#discussion_r3125213411
##########
solr/api/src/java/org/apache/solr/client/api/endpoint/ConfigsetsApi.java:
##########
@@ -143,12 +152,20 @@ SolrJerseyResponse uploadConfigSet(
@Path("/configsets/{configSetName}")
interface PutFile {
@PUT
- @Path("{filePath:.+}")
+ @Path("/files/{filePath:.+}")
Review Comment:
[0] Worth highlighting that this change doesn't just put `/files` into the
path - it also changes the 'filePath' variable so that it will no longer
contain a leading slash.
i.e. under the previous code, the path `/configsets/myCs/some/file/path.txt`
would result in `filePath` getting the value `/some/file/path.txt`. With the
latest change, an equivalent path like
`/configsets/myCs/files/some/file/path.txt` will now result in `filePath`
getting the value `some/file/path.txt`.
Again, not right or wrong - just want to make sure you're aware of the
change.
##########
solr/core/src/test/org/apache/solr/cloud/TestConfigSetsAPI.java:
##########
@@ -1177,21 +1177,20 @@ private int uploadGivenConfigSet(
throws Exception {
if (v2) {
- // TODO: switch to using V2Request
-
- final ByteBuffer fileBytes =
TestSolrConfigHandler.getFileContent(file.toString(), false);
- final String uriEnding =
- "/configsets/"
- + configSetName
- + suffix
- + (!overwrite ? "?overwrite=false" : "")
- + (cleanup ? "?cleanup=true" : "");
- final boolean usePut = true;
- JettySolrRunner jetty = cluster.getJettySolrRunners().getFirst();
- Map<?, ?> map =
- postDataAndGetResponse(
- jetty, jetty.getBaseURLV2() + uriEnding, fileBytes, username,
usePut);
- return getStatusCode(map);
+ // Use generated v2 SolrJ client (ConfigsetsApi)
+ try (InputStream fileBytes = new FileInputStream(file.toFile())) {
+ // Create and execute the upload request using generated client
+ final var uploadRequest =
+ new ConfigsetsApi.UploadConfigSet(configSetName + suffix,
fileBytes);
+ uploadRequest.setOverwrite(overwrite);
Review Comment:
[+1] Beautiful; love it.
##########
solr/api/src/java/org/apache/solr/client/api/endpoint/ConfigsetsApi.java:
##########
@@ -83,7 +84,7 @@ SolrJerseyResponse
deleteConfigSet(@PathParam("configSetName") String configSetN
@Path("/configsets/{configSetName}")
interface Download {
@GET
- @Path("/download")
+ @Path("/files")
Review Comment:
[0] This is much better from a REST-ful-ness perspective!
But IMO we could make it slightly more consistent with our other APIs by
dropping `/files` here and having the "download the whole thing" API be at `GET
/configsets/{configSetName}`. We have several other APIs that use a `/files`
path segment (e.g. filestore) but none of them allow a "get" on the /files path
itself, so IMO it makes sense to avoid the same here.
But if you don't immediately agree, then I'm happy to move on and stop
holding this up!
##########
solr/api/src/java/org/apache/solr/client/api/endpoint/ConfigsetsApi.java:
##########
@@ -130,7 +131,15 @@ SolrJerseyResponse uploadConfigSet(
@PathParam("configSetName") String configSetName,
@QueryParam("overwrite") Boolean overwrite,
@QueryParam("cleanup") Boolean cleanup,
- @RequestBody(required = true) InputStream requestBody)
+ @RequestBody(
+ required = true,
+ extensions = {
+ @Extension(
+ properties = {
+ @ExtensionProperty(name = GENERIC_ENTITY_PROPERTY,
value = "true")
Review Comment:
[+1] Good catch!
--
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]
