dsmiley commented on code in PR #4528:
URL: https://github.com/apache/solr/pull/4528#discussion_r3409985859
##########
solr/solrj/src/java/org/apache/solr/client/solrj/response/ResponseParser.java:
##########
@@ -28,17 +30,40 @@
*/
public abstract class ResponseParser {
+ protected ResponseParser() {
+ assert validateContentTypes();
+ }
+
+ private boolean validateContentTypes() {
Review Comment:
The best place for validation is here when instantiating. These are new
rules... if someone actually has a custom non-compliant impl, they'll find out.
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/HttpJdkSolrClientTest.java:
##########
@@ -437,29 +437,6 @@ public void testUseOptionalCredentialsWithNull() throws
Exception {
}
}
- @Test
- public void testProcessorMimeTypes() throws Exception {
Review Comment:
from Claude:
The deleted test was really checking the behavior of
processorAcceptsMimeType — specifically, that it correctly stripped the ;
charset=UTF-8 from "application/xml;
charset=UTF-8" before comparing. That was non-trivial because the parser
used to return full content-type strings.
Now that getContentTypes() returns pre-normalized MIME types and
checkContentType just does contains(mimeType.toLowerCase(...)), the interesting
logic has moved into the
parsers themselves. A rewritten test in the base class would basically
just be:
assertTrue(new
XMLResponseParser().getContentTypes().contains("application/xml"));
assertFalse(new
XMLResponseParser().getContentTypes().contains("application/json"));
That's testing the parser, not the client. It would belong in a
XMLResponseParserTest or similar, not in an HTTP client integration test. And
since it's just verifying
static constants, it's of marginal value — the constructor assertion in
ResponseParser already catches malformed values.
I'd leave it deleted.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/response/ResponseParser.java:
##########
@@ -28,17 +30,40 @@
*/
public abstract class ResponseParser {
+ protected ResponseParser() {
+ assert validateContentTypes();
+ }
+
+ private boolean validateContentTypes() {
+ Collection<String> contentTypes = getContentTypes();
+ assert contentTypes == getContentTypes()
+ : getClass().getName() + ".getContentTypes() must return the same
instance on every call";
+ for (String ct : contentTypes) {
+ assert !ct.contains(";") && ct.equals(ct.toLowerCase(Locale.ROOT))
+ : getClass().getName()
+ + ".getContentTypes() must return lowercase MIME types without
semicolons, got: "
+ + ct;
+ }
+ return true;
+ }
+
/** 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(InputStream body, String
encoding)
throws IOException;
/**
- * A well-behaved ResponseParser will return the content-types it supports.
+ * Returns the MIME types this parser supports. Return an empty set to
disable. Implementations
+ * must:
+ *
+ * <ul>
+ * <li>Returns the same instance (same reference on every call).
+ * <li>Use only lowercase MIME types without charset or other parameters
(no semicolons)
+ * <li>
+ * </ul>
*
- * @return the content-type values that this parser is capable of parsing.
Never null. Empty means
- * no enforcement.
+ * @return the MIME types that this parser is capable of parsing. Never null.
*/
- public abstract Collection<String> getContentTypes();
+ public abstract Set<String> getContentTypes();
Review Comment:
This is a backwards incompatible change... and it it's only caller should be
our own code, and I think *very* few people would have a custom one of these.
--
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]
