solr icon indicating copy to clipboard operation
solr copied to clipboard

Move all '*Payload' classes inside their API counterparts

Open gerlowskija opened this issue 3 years ago • 2 comments

Description

Mike Drob requested in a recent PR (#565), that all v2 "Payload" classes be relocated under their counterpart "API" classes.

Solution

This PR introduces this refactor, as promised.

Tests

Existing v2 API or conversion tests should all still pass - this PR doesn't introduce anything net-new test-wise, since it's a pure refactor.

Checklist

Please review the following and check all that apply:

  • [x] I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
  • [x] I have created a Jira issue and added the issue ID to my pull request title.
  • [x] I have given Solr maintainers access to contribute to my PR branch. (optional but recommended)
  • [x] I have developed this patch against the main branch.
  • [ ] I have run ./gradlew check.
  • [ ] I have added tests for my changes.

gerlowskija avatar Jan 27 '22 13:01 gerlowskija

FYI @madrob - here's the refactor of those 'Payload' classes, as requested.

There's still a few beans missing from this PR. A few (Package, PluginMeta) were named and used a little differently than I'm used to, and I hope to get to them when I get a bit more time.

Two others (DeleteBackupPayload, ListBackupPayload) are already used to implement SolrRequest classes in SolrJ (the eventual other use of these payload classes that I mentioned here and here.) I must've done these two as models when adding them in SIP-12 and forgotten about it. Anyway I don't plan on moving these under their API counterparts, as it'd require undoing the SolrRequest implementations.

gerlowskija avatar Jan 27 '22 13:01 gerlowskija

So after seeing your pointer to the DeleteBackupPayload class, I understand more of the intent, and I no longer think this is a worthwhile change. I'm sorry that you ended up going through the large refactor to illustrate this, and agree with you that we should be moving the other way.

madrob avatar Jan 27 '22 18:01 madrob

Closing, as we're slowly in the process of shifting the v2 API away from the custom annotation-based framework (which relied on these Payload classes) towards an approach utilizing an off-the-shelf JAX-RS library. As APIs are migrated away from the custom framework, these Payload classes should go away altogether.

Ultimately, the duplication that was the impetus of this PR should go away as we use the new JAX-RS framework (along with OpenAPI tooling) to generate SolrRequest objects from the API definitions. (See SOLR-16346 for more information on using OpenAPI for client generation.)

gerlowskija avatar Feb 10 '23 18:02 gerlowskija