JavaBinUpdateRequestCodec: Dead code removal #3207
Conversation
| @@ -49,15 +42,6 @@ | |||
| * @since solr 1.4 | |||
| */ | |||
| public class JavaBinUpdateRequestCodec { | |||
| private boolean readStringAsCharSeq = false; | |||
|
|
|||
| public JavaBinUpdateRequestCodec setReadStringAsCharSeq(boolean flag) { | |||
There was a problem hiding this comment.
unused; always false
| @@ -69,32 +53,32 @@ public JavaBinUpdateRequestCodec setReadStringAsCharSeq(boolean flag) { | |||
| */ | |||
| public void marshal(UpdateRequest updateRequest, OutputStream os) throws IOException { | |||
There was a problem hiding this comment.
simply moving code around for clarity in this method
|
|
||
| // we don't add any docs, because they were already processed | ||
| // deletes are handled later, and must be passed back on the UpdateRequest |
There was a problem hiding this comment.
the code improvements made this comment needless
| try (var codec = new StreamingCodec(handler)) { | ||
| namedList = codec.unmarshal(is); |
There was a problem hiding this comment.
big improvement, simplifying the request/response contract here with the internal StreamingCodec. No need to have the array of NamedList or pass that in or pass an UpdateRequest. Now a simple NamedList response.
| if (o instanceof List) { | ||
| @SuppressWarnings("unchecked") | ||
| List<NamedList<?>> list = (List<NamedList<?>>) o; | ||
| sdoc = listToSolrInputDocument(list); |
| } else if (o instanceof NamedList) { | ||
| UpdateRequest req = new UpdateRequest(); | ||
| req.setParams(new ModifiableSolrParams(((NamedList) o).toSolrParams())); | ||
| handler.update(null, req, null, null); |
| } else if (o instanceof Map<?, ?> m) { // doc. To imitate JSON style. SOLR-13731 | ||
| sdoc = convertMapToSolrInputDoc(m); | ||
| } else { | ||
| throw new IllegalStateException("Unexpected data type: " + o.getClass()); |
There was a problem hiding this comment.
Not being silent about the unexpected! I'll change this to SolrException(ErrorCode.BAD_REQUEST
| UpdateRequest updateUnmarshalled = | ||
| new JavaBinUpdateRequestCodec() | ||
| .unmarshal( | ||
| is, | ||
| (document, req, commitWithin, override) -> { | ||
| if (commitWithin == null) { | ||
| req.add(document); |
There was a problem hiding this comment.
The test needed an update because JavaBinUpdateRequestCodec no longer uses the same UpdateRequest between the add-docs and delete-docs parts, which doesn't matter. Arguably, the callback shouldn't even take an UpdateRequest anyway.
dsmiley
changed the title
|
Plan to merge Friday to main; no changes.txt / JIRA. Merge commit message:
|
This PR removes some dead code in JavaBinUpdateRequestCodec (no test exercises it), and simplifies related logic.
Disclaimer It's possible and maybe likely that an old SolrJ might have sent data encoded with these types. It's also possible a user is using JavaBinCodec directly to do the same. Henceforth, they will get an error. As we have very little / inadequate backwards compatibility tests, I can't know for sure. The disclaimer is so vague and frankly kind of expected for a major version (that this targets) that I'm not sure anything meaningful could be said in upgrade notes.
I plan to merge for 10.
Context:
While working on something nearby, I noticed that
JavaBinUpdateRequestCodec.readOuterMostDocIterator(server side, processes an update request that's JavaBin formatted) reads the outer most object checking for a variety of possibilities. I'm suspicious of them so added some asserts and found out when some were added.https://issues.apache.org/jira/browse/SOLR-2904 weird
https://issues.apache.org/jira/browse/SOLR-13731 this one is actually tested but not realistically to how users use javabin.
Even if we just end up leaving some comments, it'd be helpful.