Skip to content

SOLR-18212: Migrate PackageAPI endpoints from @EndPoint/@Command to JAX-RS#4178

Open
epugh wants to merge 22 commits intoapache:mainfrom
epugh:copilot/migrate-packageapi-to-jax-rs
Open

SOLR-18212: Migrate PackageAPI endpoints from @EndPoint/@Command to JAX-RS#4178
epugh wants to merge 22 commits intoapache:mainfrom
epugh:copilot/migrate-packageapi-to-jax-rs

Conversation

@epugh
Copy link
Copy Markdown
Contributor

@epugh epugh commented Mar 2, 2026

Migrates org.apache.solr.pkg.PackageAPI from Solr's homegrown @EndPoint/@Command dispatch to standard JAX-RS annotations, consistent with other V2 APIs (ClusterFileStore, ZookeeperRead, ClusterProperty).

This has a pretty hefty update to the patterns, but I thought it was better to do it now while v2 api's are "experimental".

New REST endpoints

The command-based POST pattern is replaced with RESTful routes:

Old New
POST /cluster/package {"add": {"package":"p","version":"v","files":[...]}} POST /cluster/package/{name}/versions
POST /cluster/package {"delete": {"pkg":"p","version":"v"}} DELETE /cluster/package/{name}/versions/{version}
POST /cluster/package {"refresh": "name"} POST /cluster/package/{name}/refresh
GET /cluster/package unchanged
GET /cluster/package/{name} unchanged

Key structural changes

  • PackageAPI — new JAX-RS implementation (extends JerseyResource implements PackageApis); replaces the old Edit/Read inner classes (~220 lines removed)
  • PackageStore — renamed from the old PackageAPI; now clearly a ZK data/state holder (Packages, PkgVersion, ZK watcher), not an endpoint class. THIS WAS A BIG REFACTORING
  • solr/api module — new PackageApis.java (JAX-RS interface), PackagesResponse.java, AddPackageVersionRequestBody.java; OAS generation produces PackageApi SolrJ request classes automatically
  • PackageAPITest — integration test using SolrCloudTestCase (ZK required) with generated PackageApi SolrJ request classes
  • TestPackages — updated to use new endpoints; error path changed from /details[0]/errorMessages[0] to /msg
  • package-manager-internals.adoc — updated endpoint reference and curl examples

Copilot AI and others added 8 commits March 1, 2026 13:42
- Replace PackagePayload.AddVersion with AddPackageVersionRequestBody
- Move package name from body to URL: POST /cluster/package/{name}/versions
- Replace delete command pattern with DELETE /cluster/package/{name}/versions/{version}
- Replace refresh command with POST /cluster/package/{name}/refresh
- Update errPath in testAPI from /details[0]/errorMessages[0] to /msg
- Remove all add.pkg assignments (pkg now lives in URL path)
- Replace add.pkg references in verifyComponent() with literal package name strings
- Update import: remove PackagePayload, add AddPackageVersionRequestBody (sorted)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: epugh <22395+epugh@users.noreply.github.com>
…sponse.error field

Co-authored-by: epugh <22395+epugh@users.noreply.github.com>
Co-authored-by: epugh <22395+epugh@users.noreply.github.com>
Co-authored-by: epugh <22395+epugh@users.noreply.github.com>
@epugh epugh marked this pull request as ready for review March 2, 2026 19:36
@gerlowskija
Copy link
Copy Markdown
Contributor

Is there a JIRA ticket associated with this change @epugh?

@epugh
Copy link
Copy Markdown
Contributor Author

epugh commented Apr 3, 2026

Is there a JIRA ticket associated with this change @epugh?

No, because I viewed it as "just a refactoring and no one will notice....", and that I was hoping there were no debates that would require a JIRA to hash out!

@@ -0,0 +1,7 @@
title: Migrate PackageAPI to JAX-RS. PackageAPI now has OpenAPI and SolrJ support.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[-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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redone!

authors:
- name: Eric Pugh
links:
- name: PR#4178
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DOne

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.")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so, no, this is correct. It's used internally to signal between nodes that they need refreshing. I am digging more in and I think the description is bad.

@Operation(
summary = "Get information about a specific package in this Solr cluster.",
tags = {"package"})
PackagesResponse getPackage(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep! It's kind of internal tooling, but yeah.

try {
V2Response resp = req.process(solrClient);
printGreen("Response: " + resp.jsonStr());
new PackageApi.DeletePackageVersion(packageName, version).process(solrClient);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[+1] Love the V2Request removal!


if (refreshPackage != null) {
packageStore.packageLoader.notifyListeners(refreshPackage);
return instantiateJerseyResponse(PackagesResponse.class);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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();

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixing now! Thank you... Fixed a number of places with this pattern.

public int hashCode() {
return Objects.hash(version);
}
final var request =
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so many places to update... I suspect in the future we need to do a "Check each GSR for fitness" check ;-)

* <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 {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You know, I was kind of wondering that myself. I suspect at the end of all of this, we'll want to come back and have a single strong pattern of what tests go where for each v2 api... they have grown organically.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, embarking on a reorg of the tests...

List.of("/my-plugin/plugin-" + version + ".jar"))))
.build();
processRequest(client, packageRequest);
var addRequest = new PackageApi.AddPackageVersion("mypkg");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[+1] Lovely!

add.files = Arrays.asList(new String[] {FILE1});
V2Request req =
new V2Request.Builder("/cluster/package")
new V2Request.Builder("/cluster/package/mypkg/versions")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Originally I was trying to change as little as possible, but... It's so much fun to erase V2Request, and I suspoose it also gvies us testing over the generated code...

@epugh epugh changed the title Migrate PackageAPI endpoints from @EndPoint/@Command to JAX-RS SOLR-18212: Migrate PackageAPI endpoints from @EndPoint/@Command to JAX-RS Apr 25, 2026
@epugh
Copy link
Copy Markdown
Contributor Author

epugh commented Apr 25, 2026

@gerlowskija I think I am ready for another review. I renamed PackageAPI to ClusterPackage to not conflict with PackageApis, but not married to that name! I am updating the spreadsheet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants