epugh commented on code in PR #3248:
URL: https://github.com/apache/solr/pull/3248#discussion_r1987149392
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/NoOpResponseParser.java:
##########
Review Comment:
Not specific to your changes, but a bit of JavaDocs on why you would use a
NoOpResponseParser might be nice.
##########
solr/core/src/test/org/apache/solr/cloud/TestRandomFlRTGCloud.java:
##########
@@ -423,14 +423,8 @@ private String makeXml(String s) {
}
}
- private static final ResponseParser RAW_XML_RESPONSE_PARSER = new
NoOpResponseParser();
- private static final ResponseParser RAW_JSON_RESPONSE_PARSER =
- new NoOpResponseParser() {
- @Override
- public String getWriterType() {
- return "json";
- }
- };
+ private static final ResponseParser RAW_XML_RESPONSE_PARSER = new
NoOpResponseParser("xml");
Review Comment:
This is much nicer, versus what we had before.
##########
solr/core/src/test/org/apache/solr/handler/admin/ShowFileRequestHandlerTest.java:
##########
@@ -100,18 +100,15 @@ public String getWriterType() {
}
@Override
- public NamedList<Object> processResponse(InputStream body, String
encoding) {
- try {
- if (body.read() >= 0) readFile.set(true);
- } catch (IOException e) {
- throw new RuntimeException(e);
- }
+ public NamedList<Object> processResponse(InputStream body, String
encoding)
+ throws IOException {
+ if (body.read() >= 0) readFile.set(true);
return null;
}
@Override
- public NamedList<Object> processResponse(Reader reader) {
- throw new UnsupportedOperationException("TODO unimplemented"); //
TODO
+ public Set<String> getContentTypes() {
+ return Set.of(); // don't enforce
Review Comment:
if you don't know about the previous `UOE` usage, then the comment doesn't
seem useful. I think it isn't adding value.
##########
solr/core/src/test/org/apache/solr/response/TestRawTransformer.java:
##########
@@ -283,12 +283,7 @@ public void testJsonTransformer() throws Exception {
strResponse.contains("\"links\":[\""));
}
- private static final NoOpResponseParser XML_NOOP_RESPONSE_PARSER = new
NoOpResponseParser();
+ private static final NoOpResponseParser XML_NOOP_RESPONSE_PARSER = new
NoOpResponseParser("xml");
private static final NoOpResponseParser JSON_NOOP_RESPONSE_PARSER =
- new NoOpResponseParser() {
- @Override
- public String getWriterType() {
- return "json";
- }
- };
+ new NoOpResponseParser("json");
Review Comment:
could this fit on line 287? regardless, this is much niver...
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClient.java:
##########
@@ -622,7 +622,7 @@ protected NamedList<Object> executeMethod(
}
final Collection<String> processorSupportedContentTypes =
processor.getContentTypes();
- if (processorSupportedContentTypes != null &&
!processorSupportedContentTypes.isEmpty()) {
+ if (!processorSupportedContentTypes.isEmpty()) {
Review Comment:
Loving this!
##########
solr/solrj/src/java/org/apache/solr/client/solrj/ResponseParser.java:
##########
@@ -16,45 +16,31 @@
*/
package org.apache.solr.client.solrj;
+import java.io.IOException;
import java.io.InputStream;
-import java.io.Reader;
import java.util.Collection;
-import java.util.Set;
import org.apache.solr.common.util.NamedList;
/**
+ * SolrJ Solr response parser.
+ *
* @since solr 1.3
*/
public abstract class ResponseParser {
- public abstract String getWriterType(); // for example: wt=XML, JSON, etc
- public abstract NamedList<Object> processResponse(InputStream body, String
encoding);
+ /** The writer type placed onto the request as the {@code wt} param. */
+ public abstract String getWriterType(); // for example: wt=XML, JSON, etc
- public abstract NamedList<Object> processResponse(Reader reader);
-
- /**
- * A well-behaved ResponseParser will return its content-type.
- *
- * @return the content-type this parser expects to parse
- * @deprecated use {@link #getContentTypes()} instead
- */
- @Deprecated
Review Comment:
nice to see deprecated code actually be removed!
##########
solr/core/src/test/org/apache/solr/search/TestSmileRequest.java:
##########
@@ -99,13 +99,9 @@ public String getWriterType() {
@Override
@SuppressWarnings({"unchecked", "rawtypes"})
- public NamedList<Object> processResponse(InputStream body, String
encoding) {
- try {
- Map m = (Map) SmileWriterTest.decodeSmile(body);
- return new NamedList(m);
- } catch (IOException e) {
- throw new RuntimeException(e);
- }
+ public NamedList<Object> processResponse(InputStream body, String
encoding) throws IOException {
Review Comment:
the wisdom of throwing a IOE shows up through out these changes ;-)
##########
solr/solrj/src/java/org/apache/solr/client/solrj/ResponseParser.java:
##########
@@ -16,45 +16,31 @@
*/
package org.apache.solr.client.solrj;
+import java.io.IOException;
import java.io.InputStream;
-import java.io.Reader;
import java.util.Collection;
-import java.util.Set;
import org.apache.solr.common.util.NamedList;
/**
+ * SolrJ Solr response parser.
+ *
* @since solr 1.3
*/
public abstract class ResponseParser {
- public abstract String getWriterType(); // for example: wt=XML, JSON, etc
- public abstract NamedList<Object> processResponse(InputStream body, String
encoding);
+ /** The writer type placed onto the request as the {@code wt} param. */
+ public abstract String getWriterType(); // for example: wt=XML, JSON, etc
- public abstract NamedList<Object> processResponse(Reader reader);
-
- /**
- * A well-behaved ResponseParser will return its content-type.
- *
- * @return the content-type this parser expects to parse
- * @deprecated use {@link #getContentTypes()} instead
- */
- @Deprecated
- public String getContentType() {
- return null;
- }
+ public abstract NamedList<Object> processResponse(InputStream body, String
encoding)
+ throws IOException;
/**
* A well-behaved ResponseParser will return the content-types it supports.
*
- * @return the content-type values that this parser is capable of parsing.
+ * @return the content-type values that this parser is capable of parsing.
Never null. Empty means
+ * no enforcement.
Review Comment:
Somewhat similar to the previous comment, I don't kno wthat "enforcement" is
a thing I would expect on response parsers around content-types...
--
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]
