gerlowskija commented on code in PR #4178: URL: https://github.com/apache/solr/pull/4178#discussion_r3034179762
########## changelog/unreleased/migrate-packageapi-to-jax-rs.yml: ########## @@ -0,0 +1,7 @@ +title: Migrate PackageAPI to JAX-RS. PackageAPI now has OpenAPI and SolrJ support. Review Comment: [-0] A user likely doesn't care what framework they APIs are written in. But they probably do care that some of the actual API paths have changed, there's new SolrJ classes for these APIs, etc. ########## changelog/unreleased/migrate-packageapi-to-jax-rs.yml: ########## @@ -0,0 +1,7 @@ +title: Migrate PackageAPI to JAX-RS. PackageAPI now has OpenAPI and SolrJ support. +type: changed +authors: + - name: Eric Pugh +links: +- name: PR#4178 Review Comment: [0] Probably worth having a JIRA reference for this guy, even if it's just a tie-back to one of the v2 umbrella tickets? It's changing APIs and stuff, so worth tracking in more than just the PR description I'd imagine. ########## solr/api/src/java/org/apache/solr/client/api/endpoint/PackageApis.java: ########## @@ -0,0 +1,94 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.solr.client.api.endpoint; + +import io.swagger.v3.oas.annotations.Operation; +import io.swagger.v3.oas.annotations.Parameter; +import io.swagger.v3.oas.annotations.parameters.RequestBody; +import jakarta.ws.rs.DELETE; +import jakarta.ws.rs.GET; +import jakarta.ws.rs.POST; +import jakarta.ws.rs.Path; +import jakarta.ws.rs.PathParam; +import jakarta.ws.rs.QueryParam; +import org.apache.solr.client.api.model.AddPackageVersionRequestBody; +import org.apache.solr.client.api.model.PackagesResponse; +import org.apache.solr.client.api.model.SolrJerseyResponse; + +/** V2 API definitions for managing Solr packages. */ +@Path("/cluster/package") +public interface PackageApis { + + @GET + @Operation( + summary = "List all packages registered in this Solr cluster.", + tags = {"package"}) + PackagesResponse listPackages( + @Parameter(description = "If provided, the named package is refreshed on this node.") Review Comment: [Q] The "named package"? what named package? This API doesn't specify a package name anywhere afaict? Did you mean to put this param on the 'getPackage' method below, which _does_ have a packageName? ########## solr/core/src/test/org/apache/solr/pkg/PackageAPITest.java: ########## @@ -0,0 +1,148 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.solr.pkg; + +import static org.apache.solr.filestore.TestDistribFileStore.uploadKey; + +import java.util.List; +import org.apache.solr.client.api.model.PackagesResponse; +import org.apache.solr.client.solrj.apache.HttpSolrClient; +import org.apache.solr.client.solrj.request.PackageApi; +import org.apache.solr.cloud.SolrCloudTestCase; +import org.apache.solr.filestore.ClusterFileStore; +import org.junit.BeforeClass; +import org.junit.Test; + +/** + * Integration tests for the JAX-RS-based {@link PackageAPI}. + * + * <p>Note: SolrJettyTestRule cannot be used here because the Package API requires ZooKeeper for its + * cluster-level operations. A one-node SolrCloud cluster is used instead. + */ +public class PackageAPITest extends SolrCloudTestCase { Review Comment: [Q] What's the difference between this class, and the existing `TestPackages`? They seem to overlap a good bit. Should they be combined perhaps? Or put differently: if I'm adding a new test case a month from now how would I know whether it belongs in PackageAPITest or in TestPackages? ########## solr/core/src/test/org/apache/solr/pkg/PackageStoreSchemaPluginsTest.java: ########## @@ -133,22 +133,10 @@ private void uploadPluginJar(String version, Path jarPath) throws Exception { } private void registerPackage(String version) throws Exception { - var packageRequest = - new V2Request.Builder("/cluster/package") - .POST() - .forceV2(true) - .withPayload( - Map.of( - "add", - Map.of( - "package", - "mypkg", - "version", - version, - "files", - List.of("/my-plugin/plugin-" + version + ".jar")))) - .build(); - processRequest(client, packageRequest); + var addRequest = new PackageApi.AddPackageVersion("mypkg"); Review Comment: [+1] Lovely! ########## solr/core/src/java/org/apache/solr/packagemanager/PackageManager.java: ########## @@ -152,20 +150,9 @@ public void uninstall(String packageName, String version) // Delete the package by calling the Package API and remove the Jar printGreen("Executing Package API to remove this package..."); - PackagePayload.DelVersion del = new PackagePayload.DelVersion(); - del.version = version; - del.pkg = packageName; - - V2Request req = - new V2Request.Builder(PackageUtils.PACKAGE_PATH) - .forceV2(true) - .withMethod(SolrRequest.METHOD.POST) - .withPayload(Collections.singletonMap("delete", del)) - .build(); - try { - V2Response resp = req.process(solrClient); - printGreen("Response: " + resp.jsonStr()); + new PackageApi.DeletePackageVersion(packageName, version).process(solrClient); + printGreen("Package version deleted from Package API."); Review Comment: [Q] Is there a reason that we no longer print out the response in the new version of this method? I don't really care whether it's JSON or whatever, but IMO this code had the right idea in trying to print out some information that could aid in debugging in case of issues. ########## solr/api/src/java/org/apache/solr/client/api/endpoint/PackageApis.java: ########## @@ -0,0 +1,94 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.solr.client.api.endpoint; + +import io.swagger.v3.oas.annotations.Operation; +import io.swagger.v3.oas.annotations.Parameter; +import io.swagger.v3.oas.annotations.parameters.RequestBody; +import jakarta.ws.rs.DELETE; +import jakarta.ws.rs.GET; +import jakarta.ws.rs.POST; +import jakarta.ws.rs.Path; +import jakarta.ws.rs.PathParam; +import jakarta.ws.rs.QueryParam; +import org.apache.solr.client.api.model.AddPackageVersionRequestBody; +import org.apache.solr.client.api.model.PackagesResponse; +import org.apache.solr.client.api.model.SolrJerseyResponse; + +/** V2 API definitions for managing Solr packages. */ +@Path("/cluster/package") +public interface PackageApis { + + @GET + @Operation( + summary = "List all packages registered in this Solr cluster.", + tags = {"package"}) + PackagesResponse listPackages( + @Parameter(description = "If provided, the named package is refreshed on this node.") + @QueryParam("refreshPackage") + String refreshPackage, + @Parameter( + description = + "If provided, the node waits until its package data matches this ZooKeeper version.") + @QueryParam("expectedVersion") + Integer expectedVersion); + + @GET + @Path("/{packageName}") + @Operation( + summary = "Get information about a specific package in this Solr cluster.", + tags = {"package"}) + PackagesResponse getPackage( Review Comment: [Q] Should 'expectedVersion' be supported here, in addition to the "list" method above? In the current code on main it looks like the query param is supported for both the "list-all" and "get-single-package" usecases. ########## solr/core/src/java/org/apache/solr/packagemanager/PackageManager.java: ########## @@ -152,20 +150,9 @@ public void uninstall(String packageName, String version) // Delete the package by calling the Package API and remove the Jar printGreen("Executing Package API to remove this package..."); - PackagePayload.DelVersion del = new PackagePayload.DelVersion(); - del.version = version; - del.pkg = packageName; - - V2Request req = - new V2Request.Builder(PackageUtils.PACKAGE_PATH) - .forceV2(true) - .withMethod(SolrRequest.METHOD.POST) - .withPayload(Collections.singletonMap("delete", del)) - .build(); - try { - V2Response resp = req.process(solrClient); - printGreen("Response: " + resp.jsonStr()); + new PackageApi.DeletePackageVersion(packageName, version).process(solrClient); Review Comment: [+1] Love the V2Request removal! ########## solr/core/src/java/org/apache/solr/packagemanager/PackageManager.java: ########## @@ -467,10 +454,7 @@ private Pair<List<String>, List<String>> deployCollectionPackage( // If updating, refresh the package version for this to take effect if (isUpdate || pegToLatest) { try { - SolrCLI.postJsonToSolr( - solrClient, - PackageUtils.PACKAGE_PATH, - "{\"refresh\": \"" + packageInstance.name + "\"}"); + new PackageApi.RefreshPackage(packageInstance.name).process(solrClient); Review Comment: [+1] So much nicer! ########## solr/core/src/java/org/apache/solr/packagemanager/RepositoryManager.java: ########## @@ -240,21 +233,12 @@ private boolean installPackage(String packageName, String version) throws SolrEx packageName, version, file.getFileName().toString())) - .collect(Collectors.toList()); - add.manifest = "/package/" + packageName + "/" + version + "/manifest.json"; - add.manifestSHA512 = manifestSHA512; - - GenericSolrRequest request = - new GenericV2SolrRequest(SolrRequest.METHOD.POST, PackageUtils.PACKAGE_PATH) { - @Override - public RequestWriter.ContentWriter getContentWriter(String expectedType) { - return new RequestWriter.StringPayloadContentWriter( - "{add:" + add.jsonStr() + "}", "application/json"); - } - }; + .collect(Collectors.toList())); + addRequest.setManifest("/package/" + packageName + "/" + version + "/manifest.json"); + addRequest.setManifestSHA512(manifestSHA512); try { - NamedList<Object> resp = solrClient.request(request); - printGreen("Response: " + resp.jsonStr()); + addRequest.process(solrClient); + printGreen("Package version registered successfully."); Review Comment: [-0] Ditto, re: this line used to print an actual HTTP response but now it just assumes success ########## solr/core/src/java/org/apache/solr/pkg/PackageAPI.java: ########## @@ -14,426 +14,291 @@ * See the License for the specific language governing permissions and * limitations under the License. */ - package org.apache.solr.pkg; import static org.apache.solr.common.cloud.ZkStateReader.SOLR_PKGS_PATH; import static org.apache.solr.security.PermissionNameProvider.Name.PACKAGE_EDIT_PERM; import static org.apache.solr.security.PermissionNameProvider.Name.PACKAGE_READ_PERM; -import com.fasterxml.jackson.databind.ObjectMapper; +import jakarta.inject.Inject; import java.io.IOException; -import java.io.StringWriter; import java.lang.invoke.MethodHandles; import java.util.ArrayList; import java.util.Collections; -import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import java.util.Objects; -import org.apache.solr.api.Command; -import org.apache.solr.api.EndPoint; -import org.apache.solr.api.PayloadObj; +import java.util.stream.Collectors; +import org.apache.solr.api.JerseyResource; +import org.apache.solr.client.api.endpoint.PackageApis; +import org.apache.solr.client.api.model.AddPackageVersionRequestBody; +import org.apache.solr.client.api.model.PackagesResponse; +import org.apache.solr.client.api.model.SolrJerseyResponse; import org.apache.solr.client.solrj.SolrRequest; import org.apache.solr.client.solrj.SolrServerException; import org.apache.solr.client.solrj.request.GenericSolrRequest; -import org.apache.solr.client.solrj.request.beans.PackagePayload; import org.apache.solr.client.solrj.response.JavaBinResponseParser; import org.apache.solr.common.SolrException; -import org.apache.solr.common.annotation.JsonProperty; -import org.apache.solr.common.cloud.SolrZkClient; -import org.apache.solr.common.cloud.ZooKeeperException; import org.apache.solr.common.params.ModifiableSolrParams; -import org.apache.solr.common.util.CommandOperation; -import org.apache.solr.common.util.EnvUtils; -import org.apache.solr.common.util.ReflectMapWriter; import org.apache.solr.common.util.Utils; import org.apache.solr.core.CoreContainer; import org.apache.solr.filestore.FileStoreUtils; +import org.apache.solr.jersey.PermissionName; import org.apache.solr.request.SolrQueryRequest; import org.apache.solr.response.SolrQueryResponse; -import org.apache.solr.util.SolrJacksonAnnotationInspector; import org.apache.zookeeper.KeeperException; -import org.apache.zookeeper.WatchedEvent; -import org.apache.zookeeper.Watcher; -import org.apache.zookeeper.data.Stat; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -/** This implements the public end points (/api/cluster/package) of package API. */ -public class PackageAPI { - public final boolean enablePackages = EnvUtils.getPropertyAsBool("solr.packages.enabled", false); +/** + * JAX-RS implementation of the package management API ({@code /api/cluster/package}). + * + * @see PackageApis + */ +public class PackageAPI extends JerseyResource implements PackageApis { Review Comment: [-0] Our naming convention up to this point has been for the API definition (i.e. the interface in the `api` module) to be called `FooApi`, and for the implementing class to be named `Foo`. If we wanted to follow that here, then this should probably just be called `Package`. (Or since that's really generic and possibly confusing in Java, perhaps `PackageImpl`, `PackageStore` or something similar would be better here?) ########## solr/core/src/test/org/apache/solr/pkg/TestPackages.java: ########## @@ -130,15 +130,14 @@ public void testCoreReloadingPlugin() throws Exception { FILE1, "L3q/qIGs4NaF6JiO0ZkMUFa88j0OmYc+I6O7BOdNuMct/xoZ4h73aZHZGc0+nmI1f/U3bOlMPINlSOM6LK3JpQ=="); - PackagePayload.AddVersion add = new PackagePayload.AddVersion(); + AddPackageVersionRequestBody add = new AddPackageVersionRequestBody(); add.version = "1.0"; - add.pkg = "mypkg"; add.files = Arrays.asList(new String[] {FILE1}); V2Request req = - new V2Request.Builder("/cluster/package") + new V2Request.Builder("/cluster/package/mypkg/versions") Review Comment: Ditto, re: choosing to retain `V2Request` usage vs. replacing with the auto-generated type. We don't need to make that switchover in this PR, just pointing it out in case it's unintentional. ########## solr/core/src/java/org/apache/solr/pkg/PackageAPI.java: ########## @@ -14,426 +14,291 @@ * See the License for the specific language governing permissions and * limitations under the License. */ - package org.apache.solr.pkg; import static org.apache.solr.common.cloud.ZkStateReader.SOLR_PKGS_PATH; import static org.apache.solr.security.PermissionNameProvider.Name.PACKAGE_EDIT_PERM; import static org.apache.solr.security.PermissionNameProvider.Name.PACKAGE_READ_PERM; -import com.fasterxml.jackson.databind.ObjectMapper; +import jakarta.inject.Inject; import java.io.IOException; -import java.io.StringWriter; import java.lang.invoke.MethodHandles; import java.util.ArrayList; import java.util.Collections; -import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import java.util.Objects; -import org.apache.solr.api.Command; -import org.apache.solr.api.EndPoint; -import org.apache.solr.api.PayloadObj; +import java.util.stream.Collectors; +import org.apache.solr.api.JerseyResource; +import org.apache.solr.client.api.endpoint.PackageApis; +import org.apache.solr.client.api.model.AddPackageVersionRequestBody; +import org.apache.solr.client.api.model.PackagesResponse; +import org.apache.solr.client.api.model.SolrJerseyResponse; import org.apache.solr.client.solrj.SolrRequest; import org.apache.solr.client.solrj.SolrServerException; import org.apache.solr.client.solrj.request.GenericSolrRequest; -import org.apache.solr.client.solrj.request.beans.PackagePayload; import org.apache.solr.client.solrj.response.JavaBinResponseParser; import org.apache.solr.common.SolrException; -import org.apache.solr.common.annotation.JsonProperty; -import org.apache.solr.common.cloud.SolrZkClient; -import org.apache.solr.common.cloud.ZooKeeperException; import org.apache.solr.common.params.ModifiableSolrParams; -import org.apache.solr.common.util.CommandOperation; -import org.apache.solr.common.util.EnvUtils; -import org.apache.solr.common.util.ReflectMapWriter; import org.apache.solr.common.util.Utils; import org.apache.solr.core.CoreContainer; import org.apache.solr.filestore.FileStoreUtils; +import org.apache.solr.jersey.PermissionName; import org.apache.solr.request.SolrQueryRequest; import org.apache.solr.response.SolrQueryResponse; -import org.apache.solr.util.SolrJacksonAnnotationInspector; import org.apache.zookeeper.KeeperException; -import org.apache.zookeeper.WatchedEvent; -import org.apache.zookeeper.Watcher; -import org.apache.zookeeper.data.Stat; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -/** This implements the public end points (/api/cluster/package) of package API. */ -public class PackageAPI { - public final boolean enablePackages = EnvUtils.getPropertyAsBool("solr.packages.enabled", false); +/** + * JAX-RS implementation of the package management API ({@code /api/cluster/package}). + * + * @see PackageApis + */ +public class PackageAPI extends JerseyResource implements PackageApis { private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass()); - public static final String ERR_MSG = - "Package loading is not enabled , Start your nodes with -Dsolr.packages.enabled=true"; + private static final int SYNC_MAX_RETRIES = 10; + private static final long SYNC_SLEEP_MS = 10L; - final CoreContainer coreContainer; - private final ObjectMapper mapper = SolrJacksonAnnotationInspector.createObjectMapper(); - private final SolrPackageLoader packageLoader; - Packages pkgs; + private final CoreContainer coreContainer; + private final SolrQueryRequest solrQueryRequest; + private final SolrQueryResponse solrQueryResponse; - public final Edit editAPI = new Edit(); - public final Read readAPI = new Read(); - - public PackageAPI(CoreContainer coreContainer, SolrPackageLoader loader) { + @Inject + public PackageAPI( + CoreContainer coreContainer, + SolrQueryRequest solrQueryRequest, + SolrQueryResponse solrQueryResponse) { this.coreContainer = coreContainer; - this.packageLoader = loader; - pkgs = new Packages(); - SolrZkClient zkClient = coreContainer.getZkController().getZkClient(); - try { - pkgs = readPkgsFromZk(null, null); - } catch (KeeperException | InterruptedException e) { - pkgs = new Packages(); - // ignore + this.solrQueryRequest = solrQueryRequest; + this.solrQueryResponse = solrQueryResponse; + } + + @Override + @PermissionName(PACKAGE_READ_PERM) + public PackagesResponse listPackages(String refreshPackage, Integer expectedVersion) { + PackageStore packageStore = coreContainer.getPackageLoader().getPackageStore(); + + if (refreshPackage != null) { + packageStore.packageLoader.notifyListeners(refreshPackage); + return instantiateJerseyResponse(PackagesResponse.class); } - try { - registerListener(zkClient); - } catch (KeeperException | InterruptedException e) { - SolrZkClient.checkInterrupted(e); + + if (expectedVersion != null) { + syncToVersion(packageStore, expectedVersion); } - } - private void registerListener(SolrZkClient zkClient) - throws KeeperException, InterruptedException { - zkClient.exists( - SOLR_PKGS_PATH, - new Watcher() { - - @Override - public void process(WatchedEvent event) { - // session events are not change events, and do not remove the watcher - if (Event.EventType.None.equals(event.getType())) { - return; - } - synchronized (this) { - log.debug("Updating [{}] ... ", SOLR_PKGS_PATH); - // remake watch - final Watcher thisWatch = this; - refreshPackages(thisWatch); - } - } - }); + final var response = instantiateJerseyResponse(PackagesResponse.class); + response.result = toPackageData(packageStore.pkgs); + return response; } - public void refreshPackages(Watcher watcher) { - final Stat stat = new Stat(); - try { - final byte[] data = - coreContainer.getZkController().getZkClient().getData(SOLR_PKGS_PATH, watcher, stat); - pkgs = readPkgsFromZk(data, stat); - packageLoader.refreshPackageConf(); - } catch (KeeperException.ConnectionLossException | KeeperException.SessionExpiredException e) { - log.warn("ZooKeeper watch triggered, but Solr cannot talk to ZK: ", e); - } catch (KeeperException e) { - log.error("A ZK error has occurred", e); - throw new ZooKeeperException(SolrException.ErrorCode.SERVER_ERROR, "", e); - } catch (InterruptedException e) { - // Restore the interrupted status - Thread.currentThread().interrupt(); - log.warn("Interrupted", e); + @Override + @PermissionName(PACKAGE_READ_PERM) + public PackagesResponse getPackage(String packageName) { + PackageStore packageStore = coreContainer.getPackageLoader().getPackageStore(); + final var response = instantiateJerseyResponse(PackagesResponse.class); + response.result = toPackageData(packageStore.pkgs); + // Filter to only the requested package + if (response.result != null && response.result.packages != null) { + final var pkgVersions = response.result.packages.get(packageName); + response.result.packages = Collections.singletonMap(packageName, pkgVersions); } + return response; } - private Packages readPkgsFromZk(byte[] data, Stat stat) - throws KeeperException, InterruptedException { + @Override + @PermissionName(PACKAGE_EDIT_PERM) + public SolrJerseyResponse addPackageVersion( + String packageName, AddPackageVersionRequestBody requestBody) { + final var response = instantiateJerseyResponse(SolrJerseyResponse.class); + PackageStore packageStore = coreContainer.getPackageLoader().getPackageStore(); - if (data == null || stat == null) { - stat = new Stat(); - data = coreContainer.getZkController().getZkClient().getData(SOLR_PKGS_PATH, null, stat); + if (!packageStore.isEnabled()) { + throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, PackageStore.ERR_MSG); } - Packages packages = null; - if (data == null || data.length == 0) { - packages = new Packages(); - } else { - try { - packages = mapper.readValue(data, Packages.class); - packages.znodeVersion = stat.getVersion(); - } catch (IOException e) { - // invalid data in packages - // TODO handle properly; - return new Packages(); - } + if (requestBody == null || requestBody.files == null || requestBody.files.isEmpty()) { + throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "No files specified"); } - return packages; - } - - public static class Packages implements ReflectMapWriter { - @JsonProperty public int znodeVersion = -1; - @JsonProperty public Map<String, List<PkgVersion>> packages = new LinkedHashMap<>(); + final List<String> errors = new ArrayList<>(); + FileStoreUtils.validateFiles( + coreContainer.getFileStore(), requestBody.files, true, errors::add); + if (!errors.isEmpty()) { + throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, String.join("; ", errors)); + } - public Packages copy() { - Packages p = new Packages(); - p.znodeVersion = this.znodeVersion; - p.packages = new LinkedHashMap<>(); - packages.forEach((s, versions) -> p.packages.put(s, new ArrayList<>(versions))); - return p; + final PackageStore.Packages[] finalState = new PackageStore.Packages[1]; + try { + coreContainer + .getZkController() + .getZkClient() + .atomicUpdate( + SOLR_PKGS_PATH, + (stat, bytes) -> { + PackageStore.Packages packages; + try { + packages = + bytes == null + ? new PackageStore.Packages() + : packageStore.mapper.readValue(bytes, PackageStore.Packages.class); + packages = packages.copy(); + } catch (IOException e) { + log.error("Error deserializing packages.json", e); + packages = new PackageStore.Packages(); + } + List<PackageStore.PkgVersion> list = + packages.packages.computeIfAbsent(packageName, o -> new ArrayList<>()); + for (PackageStore.PkgVersion pkgVersion : list) { + if (Objects.equals(pkgVersion.version, requestBody.version)) { + throw new SolrException( + SolrException.ErrorCode.BAD_REQUEST, + "Version '" + requestBody.version + "' exists already"); + } + } + list.add(new PackageStore.PkgVersion(packageName, requestBody)); + packages.znodeVersion = stat.getVersion() + 1; + finalState[0] = packages; + return Utils.toJSON(packages); + }); + } catch (KeeperException | InterruptedException e) { + finalState[0] = null; + packageStore.handleZkErr(e); } - } - public static class PkgVersion implements ReflectMapWriter { + if (finalState[0] != null) { + packageStore.pkgs = finalState[0]; + notifyAllNodesToSync(packageStore.pkgs.znodeVersion); + coreContainer.getPackageLoader().refreshPackageConf(); + } - @JsonProperty("package") - public String pkg; + return response; + } - @JsonProperty public String version; + @Override + @PermissionName(PACKAGE_EDIT_PERM) + public SolrJerseyResponse deletePackageVersion(String packageName, String version) { + final var response = instantiateJerseyResponse(SolrJerseyResponse.class); + PackageStore packageStore = coreContainer.getPackageLoader().getPackageStore(); - @JsonProperty public List<String> files; + if (!packageStore.isEnabled()) { + throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, PackageStore.ERR_MSG); + } - @JsonProperty public String manifest; + try { + coreContainer + .getZkController() + .getZkClient() + .atomicUpdate( + SOLR_PKGS_PATH, + (stat, bytes) -> { + PackageStore.Packages packages; + try { + packages = packageStore.mapper.readValue(bytes, PackageStore.Packages.class); + packages = packages.copy(); + } catch (IOException e) { + packages = new PackageStore.Packages(); + } + + List<PackageStore.PkgVersion> versions = packages.packages.get(packageName); + if (versions == null || versions.isEmpty()) { + throw new SolrException( + SolrException.ErrorCode.BAD_REQUEST, "No such package: " + packageName); + } + int idxToRemove = -1; + for (int i = 0; i < versions.size(); i++) { + if (Objects.equals(versions.get(i).version, version)) { + idxToRemove = i; + break; + } + } + if (idxToRemove == -1) { + throw new SolrException( + SolrException.ErrorCode.BAD_REQUEST, "No such version: " + version); + } + versions.remove(idxToRemove); + packages.znodeVersion = stat.getVersion() + 1; + return Utils.toJSON(packages); + }); + } catch (KeeperException | InterruptedException e) { + packageStore.handleZkErr(e); + } - @JsonProperty public String manifestSHA512; + return response; + } - public PkgVersion() {} + @Override + @PermissionName(PACKAGE_EDIT_PERM) + public SolrJerseyResponse refreshPackage(String packageName) { + final var response = instantiateJerseyResponse(SolrJerseyResponse.class); + PackageStore packageStore = coreContainer.getPackageLoader().getPackageStore(); - public PkgVersion(PackagePayload.AddVersion addVersion) { - this.pkg = addVersion.pkg; - this.version = addVersion.version; - this.files = addVersion.files == null ? null : Collections.unmodifiableList(addVersion.files); - this.manifest = addVersion.manifest; - this.manifestSHA512 = addVersion.manifestSHA512; + SolrPackageLoader.SolrPackage pkg = coreContainer.getPackageLoader().getPackage(packageName); + if (pkg == null) { + throw new SolrException( + SolrException.ErrorCode.BAD_REQUEST, "No such package: " + packageName); } + // first refresh on the current node + packageStore.packageLoader.notifyListeners(packageName); - @Override - public boolean equals(Object obj) { - if (obj instanceof PkgVersion that) { - return Objects.equals(this.version, that.version) && Objects.equals(this.files, that.files); - } - return false; - } + final var solrParams = new ModifiableSolrParams(); + solrParams.add("omitHeader", "true"); + solrParams.add("refreshPackage", packageName); - @Override - public int hashCode() { - return Objects.hash(version); - } + final var request = Review Comment: [Q] Is there a reason you're not using the auto-generated SolrJ type here? We should have one now that the API is JAX-RS! If this was an intentional decision made to prevent scope-creep, I'm fine with that. Just making sure it's not oversight. ########## solr/core/src/java/org/apache/solr/pkg/PackageAPI.java: ########## @@ -14,426 +14,291 @@ * See the License for the specific language governing permissions and * limitations under the License. */ - package org.apache.solr.pkg; import static org.apache.solr.common.cloud.ZkStateReader.SOLR_PKGS_PATH; import static org.apache.solr.security.PermissionNameProvider.Name.PACKAGE_EDIT_PERM; import static org.apache.solr.security.PermissionNameProvider.Name.PACKAGE_READ_PERM; -import com.fasterxml.jackson.databind.ObjectMapper; +import jakarta.inject.Inject; import java.io.IOException; -import java.io.StringWriter; import java.lang.invoke.MethodHandles; import java.util.ArrayList; import java.util.Collections; -import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import java.util.Objects; -import org.apache.solr.api.Command; -import org.apache.solr.api.EndPoint; -import org.apache.solr.api.PayloadObj; +import java.util.stream.Collectors; +import org.apache.solr.api.JerseyResource; +import org.apache.solr.client.api.endpoint.PackageApis; +import org.apache.solr.client.api.model.AddPackageVersionRequestBody; +import org.apache.solr.client.api.model.PackagesResponse; +import org.apache.solr.client.api.model.SolrJerseyResponse; import org.apache.solr.client.solrj.SolrRequest; import org.apache.solr.client.solrj.SolrServerException; import org.apache.solr.client.solrj.request.GenericSolrRequest; -import org.apache.solr.client.solrj.request.beans.PackagePayload; import org.apache.solr.client.solrj.response.JavaBinResponseParser; import org.apache.solr.common.SolrException; -import org.apache.solr.common.annotation.JsonProperty; -import org.apache.solr.common.cloud.SolrZkClient; -import org.apache.solr.common.cloud.ZooKeeperException; import org.apache.solr.common.params.ModifiableSolrParams; -import org.apache.solr.common.util.CommandOperation; -import org.apache.solr.common.util.EnvUtils; -import org.apache.solr.common.util.ReflectMapWriter; import org.apache.solr.common.util.Utils; import org.apache.solr.core.CoreContainer; import org.apache.solr.filestore.FileStoreUtils; +import org.apache.solr.jersey.PermissionName; import org.apache.solr.request.SolrQueryRequest; import org.apache.solr.response.SolrQueryResponse; -import org.apache.solr.util.SolrJacksonAnnotationInspector; import org.apache.zookeeper.KeeperException; -import org.apache.zookeeper.WatchedEvent; -import org.apache.zookeeper.Watcher; -import org.apache.zookeeper.data.Stat; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -/** This implements the public end points (/api/cluster/package) of package API. */ -public class PackageAPI { - public final boolean enablePackages = EnvUtils.getPropertyAsBool("solr.packages.enabled", false); +/** + * JAX-RS implementation of the package management API ({@code /api/cluster/package}). + * + * @see PackageApis + */ +public class PackageAPI extends JerseyResource implements PackageApis { private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass()); - public static final String ERR_MSG = - "Package loading is not enabled , Start your nodes with -Dsolr.packages.enabled=true"; + private static final int SYNC_MAX_RETRIES = 10; + private static final long SYNC_SLEEP_MS = 10L; - final CoreContainer coreContainer; - private final ObjectMapper mapper = SolrJacksonAnnotationInspector.createObjectMapper(); - private final SolrPackageLoader packageLoader; - Packages pkgs; + private final CoreContainer coreContainer; + private final SolrQueryRequest solrQueryRequest; + private final SolrQueryResponse solrQueryResponse; - public final Edit editAPI = new Edit(); - public final Read readAPI = new Read(); - - public PackageAPI(CoreContainer coreContainer, SolrPackageLoader loader) { + @Inject + public PackageAPI( + CoreContainer coreContainer, + SolrQueryRequest solrQueryRequest, + SolrQueryResponse solrQueryResponse) { this.coreContainer = coreContainer; - this.packageLoader = loader; - pkgs = new Packages(); - SolrZkClient zkClient = coreContainer.getZkController().getZkClient(); - try { - pkgs = readPkgsFromZk(null, null); - } catch (KeeperException | InterruptedException e) { - pkgs = new Packages(); - // ignore + this.solrQueryRequest = solrQueryRequest; + this.solrQueryResponse = solrQueryResponse; + } + + @Override + @PermissionName(PACKAGE_READ_PERM) + public PackagesResponse listPackages(String refreshPackage, Integer expectedVersion) { + PackageStore packageStore = coreContainer.getPackageLoader().getPackageStore(); + + if (refreshPackage != null) { + packageStore.packageLoader.notifyListeners(refreshPackage); + return instantiateJerseyResponse(PackagesResponse.class); Review Comment: [0] `instantiateJerseyResponse` is primarily useful for cases when: 1. a response is filled out bit by bit 2. it's possible for an exception to interrupt that process mid-flow, and 3. when the response is sent back to the user, we want it to contain both the partial-success information as well as the error information. A good example is CreateCollection. Create-collection fires off a bunch of core-creations and puts information for each (success or failure) into the user-facing response. Historically, if an error crops up in the middle of those core creations we want the response to return information both on the ones that succeeded and the ones that failed. That's what `instantiateJerseyResponse` is good for - it registers the response object in such a way that our exception handling can find the partial response and knit it together with information pulled out of the exception. None of that seems relevant here (and elsewhere) - you should be able to replace this line with `return new PackagesResponse();` -- 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]
