Migrate RenameCoreAPI from homegrown @EndPoint/@Command to JAX-RS#4172
Migrate RenameCoreAPI from homegrown @EndPoint/@Command to JAX-RS#4172epugh merged 9 commits intoapache:mainfrom
Conversation
…ate tests Co-authored-by: epugh <22395+epugh@users.noreply.github.com>
Co-authored-by: epugh <22395+epugh@users.noreply.github.com>
|
I've done a bit of work in my own |
| solrTestRule.startSolr(createTempDir()); | ||
| solrTestRule.newCollection(DEFAULT_TEST_CORENAME).withConfigSet(ExternalPaths.DEFAULT_CONFIGSET).create(); | ||
| } | ||
|
|
There was a problem hiding this comment.
code above seems fine... but the setup and tests below are low-level, reaching directly into Solr instead of using SolrClient. Why?
There was a problem hiding this comment.
humm... Good question.... a part of me likes the "Hey, look, we are testing the exact API" instead of a integration style test...
I am going to make another test that usese just SolrClient...
There was a problem hiding this comment.
after digging around a bit more, you are right that we want to do a more integration test to cover the generated code....
There was a problem hiding this comment.
@dsmiley I rewrote the tests to be integration style. Also, there is a todo about v2 generated not raising exceptions, so we'll fix it when that is sorted. Can I get another reivew?
epugh
changed the title
|
@dsmiley thoguths on if this needs a changelog? It's "internal"... |
| // Sending JSON "null" as the body causes Jersey to deserialize it to a null requestBody, | ||
| // triggering the handler's "Required request-body is missing" guard. | ||
| GenericV2SolrRequest renameRequest = | ||
| new GenericV2SolrRequest( |
There was a problem hiding this comment.
Isn't this PR supposed to introduced a nice typed V2 request we can use instead of this generic one?
There was a problem hiding this comment.
it does, but the typed one doesn't raise the nice error.. I also thought maybe having both ways tested was of some value? 🤷
There was a problem hiding this comment.
Ah, I see that's above test but this and lower ones are triggering erroneous cases that maybe can't be done with the nice API.
So Nevermind.
There was a problem hiding this comment.
I suspect, when we get the error thing sorted that all these tests will need another looksee...
|
IMO Each V2 API addition is worth a changelog. |
|
I still need to fix one javadoc reference... |
At a minimum each of these JAX-RS migrations creates a new SolrRequest/SolrResponse object to SolrJ that folks can now use, and that's usually worth a changelog in its own right! |
|
Hey @epugh - the PR name and commit message here are really misleading and don't reflect that this is largely dead-code deletion. Friendly reminder to keep an eye on that stuff for the next one. |
Thanks, you are quite right! This was so far at least an outlier, in that it wasn't a migration, though we added a test. |
I hadn't really thoguht about that! I guess this shows up in our JavaDocs right? We don't have special documentation around htis in the Ref Guide beyond the rather shallow https://solr.apache.org/guide/solr/latest/deployment-guide/solrj.html#solrj-overview page? Which I think doesnt' even link to javadocs? |
RenameCoreAPI.javawas the last remnant of the old@EndPoint/@CommandV2 API style for core renaming. The JAX-RS implementation (RenameCore.java,RenameCoreApi.java,RenameCoreRequestBody.java) already existed and was fully wired intoCoreAdminHandler.getJerseyResources()andCoreAdminOperation.RENAME_OP— the old class was dead production code only referenced in a mock-based test.Changes
RenameCoreAPI.java— superseded byRenameCore.javaV2CoreAPIMappingTest— removedRenameCoreAPIregistration and its mock-basedtestRenameCoreAllParamstestRenameCoreAPITest— unit tests forRenameCoreusingSolrJettyTestRule(no mocks, noinitCore), following the pattern ofShowFileRequestHandlerTestTests cover: same-name early return, missing request body, and missing
toparameter.