Simplify ResponseParser.getContentType processing

changelog/unreleased/PR#4528-ResponseParser_getContentType.yml

@@ -0,0 +1,8 @@
+title: >
+  Simplify SolrJ ResponseParser.getContentType processing.  getContentType must now return a Set.
+type: other
+authors:
+  - name: David Smiley
+links:
+  - name: PR#4528
+    url: https://github.com/apache/solr/pull/4528

solr/core/src/test/org/apache/solr/search/TestSmileRequest.java

@@ -18,7 +18,6 @@
 
 import java.io.IOException;
 import java.io.InputStream;
-import java.util.Collection;
 import java.util.Map;
 import java.util.Set;
 import org.apache.solr.JSONTestUtil;
@@ -106,9 +105,12 @@ public NamedList<Object> processResponse(InputStream body, String encoding) thro
       return new NamedList(m);
     }
 
+    private static final Set<String> CONTENT_TYPES =
+        Set.of("application/x-jackson-smile", "application/octet-stream");
+
     @Override
-    public Collection<String> getContentTypes() {
-      return Set.of("application/x-jackson-smile", "application/octet-stream");
+    public Set<String> getContentTypes() {
+      return CONTENT_TYPES;
     }
   }
 }

solr/solrj-jetty/src/java/org/apache/solr/client/solrj/jetty/HttpJettySolrClient.java

@@ -26,7 +26,6 @@
 import java.util.Collection;
 import java.util.Iterator;
 import java.util.List;
-import java.util.Locale;
 import java.util.Map;
 import java.util.concurrent.CancellationException;
 import java.util.concurrent.CompletableFuture;
@@ -37,7 +36,6 @@
 import java.util.concurrent.Semaphore;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.TimeoutException;
-import java.util.stream.Collectors;
 import org.apache.solr.client.api.util.SolrVersion;
 import org.apache.solr.client.solrj.SolrClient;
 import org.apache.solr.client.solrj.SolrRequest;
@@ -161,7 +159,6 @@ protected HttpJettySolrClient(String serverBaseUrl, Builder builder) {
       this.listenerFactory = new ArrayList<>(0);
     }
 
-    updateDefaultMimeTypeForParser();
     this.idleTimeoutMillis = builder.getIdleTimeoutMillis();
 
     try {
@@ -807,31 +804,6 @@ protected boolean isFollowRedirects() {
     return httpClient.isFollowRedirects();
   }
 
-  @Override
-  protected boolean processorAcceptsMimeType(
-      Collection<String> processorSupportedContentTypes, String mimeType) {
-
-    return processorSupportedContentTypes.stream()
-        .map(ct -> MimeTypes.getContentTypeWithoutCharset(ct).trim())
-        .anyMatch(mimeType::equalsIgnoreCase);
-  }
-
-  @Override
-  protected void updateDefaultMimeTypeForParser() {
-    defaultParserMimeTypes =
-        parser.getContentTypes().stream()
-            .map(ct -> MimeTypes.getContentTypeWithoutCharset(ct).trim().toLowerCase(Locale.ROOT))
-            .collect(Collectors.toSet());
-  }
-
-  @Override
-  protected String allProcessorSupportedContentTypesCommaDelimited(
-      Collection<String> processorSupportedContentTypes) {
-    return processorSupportedContentTypes.stream()
-        .map(ct -> MimeTypes.getContentTypeWithoutCharset(ct).trim().toLowerCase(Locale.ROOT))
-        .collect(Collectors.joining(", "));
-  }
-
   /**
    * An HttpJettySolrClient that doesn't close or cleanup any resources
    *

solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpJdkSolrClient.java

@@ -37,7 +37,6 @@
 import java.util.HashMap;
 import java.util.Locale;
 import java.util.Map;
-import java.util.Objects;
 import java.util.concurrent.BlockingQueue;
 import java.util.concurrent.CompletableFuture;
 import java.util.concurrent.ExecutorService;
@@ -46,7 +45,6 @@
 import java.util.concurrent.TimeUnit;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
-import java.util.stream.Collectors;
 import javax.net.ssl.SSLContext;
 import org.apache.solr.client.api.util.SolrVersion;
 import org.apache.solr.client.solrj.SolrRequest;
@@ -139,7 +137,6 @@ protected HttpJdkSolrClient(String serverBaseUrl, HttpJdkSolrClient.Builder buil
           ProxySelector.of(new InetSocketAddress(builder.getProxyHost(), builder.getProxyPort())));
     }
     this.httpClient = b.build();
-    updateDefaultMimeTypeForParser();
 
     assert ObjectReleaseTracker.track(this);
   }
@@ -537,36 +534,6 @@ protected boolean isFollowRedirects() {
     return httpClient.followRedirects() != HttpClient.Redirect.NEVER;
   }
 
-  @Override
-  protected boolean processorAcceptsMimeType(
-      Collection<String> processorSupportedContentTypes, String mimeType) {
-    return processorSupportedContentTypes.stream()
-        .map(this::contentTypeToMimeType)
-        .filter(Objects::nonNull)
-        .map(String::trim)
-        .anyMatch(mimeType::equalsIgnoreCase);
-  }
-
-  @Override
-  protected void updateDefaultMimeTypeForParser() {
-    defaultParserMimeTypes =
-        parser.getContentTypes().stream()
-            .map(this::contentTypeToMimeType)
-            .filter(Objects::nonNull)
-            .map(s -> s.toLowerCase(Locale.ROOT).trim())
-            .collect(Collectors.toSet());
-  }
-
-  @Override
-  protected String allProcessorSupportedContentTypesCommaDelimited(
-      Collection<String> processorSupportedContentTypes) {
-    return processorSupportedContentTypes.stream()
-        .map(this::contentTypeToMimeType)
-        .filter(Objects::nonNull)
-        .map(s -> s.toLowerCase(Locale.ROOT).trim())
-        .collect(Collectors.joining(", "));
-  }
-
   @Override
   protected BuilderBase<?, ?> toBuilder(String baseUrl) {
     return new HttpJdkSolrClient.Builder(baseUrl).withHttpClient(this);

solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClient.java

@@ -27,6 +27,7 @@
 import java.nio.charset.StandardCharsets;
 import java.util.Base64;
 import java.util.Collection;
+import java.util.Locale;
 import java.util.Objects;
 import java.util.Set;
 import java.util.concurrent.CompletableFuture;
@@ -72,11 +73,8 @@ public abstract class HttpSolrClient extends SolrClient {
 
   protected RequestWriter requestWriter = new JavaBinRequestWriter();
 
-  // updating parser instance needs to go via the setter to ensure update of defaultParserMimeTypes
   protected ResponseParser parser = new JavaBinResponseParser();
 
-  protected Set<String> defaultParserMimeTypes;
-
   protected final String basicAuthAuthorizationStr;
 
   protected HttpSolrClient(String serverBaseUrl, BuilderBase<?, ?> builder) {
@@ -266,12 +264,6 @@ protected boolean wantStream(final ResponseParser processor) {
     return processor == null || processor instanceof InputStreamResponseParser;
   }
 
-  protected abstract boolean processorAcceptsMimeType(
-      Collection<String> processorSupportedContentTypes, String mimeType);
-
-  protected abstract String allProcessorSupportedContentTypesCommaDelimited(
-      Collection<String> processorSupportedContentTypes);
-
   /**
    * Validates that the content type in the response can be processed by the Response Parser. Throws
    * a {@code RemoteSolrException} if not.
@@ -283,19 +275,15 @@ private void checkContentType(
       String encoding,
       int httpStatus,
       String urlExceptionMessage) {
-    if (mimeType == null
-        || (processor == this.parser && defaultParserMimeTypes.contains(mimeType))) {
-      // Shortcut the default scenario
+    if (mimeType == null) {
       return;
     }
     final Collection<String> processorSupportedContentTypes = processor.getContentTypes();
     if (!processorSupportedContentTypes.isEmpty()) {
-      boolean processorAcceptsMimeType =
-          processorAcceptsMimeType(processorSupportedContentTypes, mimeType);
-      if (!processorAcceptsMimeType) {
+      final String normalizedMimeType = mimeType.toLowerCase(Locale.ROOT).trim();
+      if (!processorSupportedContentTypes.contains(normalizedMimeType)) {
         // unexpected mime type
-        final String allSupportedTypes =
-            allProcessorSupportedContentTypesCommaDelimited(processorSupportedContentTypes);
+        final String allSupportedTypes = String.join(", ", processorSupportedContentTypes);
         String prefix =
             "Expected mime type in [" + allSupportedTypes + "] but got " + mimeType + ". ";
         String exceptionEncoding = encoding != null ? encoding : FALLBACK_CHARSET.name();
@@ -322,11 +310,8 @@ protected static String basicAuthCredentialsToAuthorizationString(String user, S
 
   protected void setParser(ResponseParser parser) {
     this.parser = parser;
-    updateDefaultMimeTypeForParser();
   }
 
-  protected abstract void updateDefaultMimeTypeForParser();
-
   /**
    * Executes a SolrRequest using the provided URL to temporarily override any "base URL" currently
    * used by this client

solr/solrj/src/java/org/apache/solr/client/solrj/response/JavaBinResponseParser.java

@@ -18,7 +18,6 @@
 
 import java.io.IOException;
 import java.io.InputStream;
-import java.util.Collection;
 import java.util.Set;
 import org.apache.solr.common.util.JavaBinCodec;
 import org.apache.solr.common.util.NamedList;
@@ -52,8 +51,11 @@ protected JavaBinCodec createCodec() {
     return new JavaBinCodec(null, stringCache);
   }
 
+  private static final Set<String> CONTENT_TYPES =
+      Set.of(JAVABIN_CONTENT_TYPE, JAVABIN_CONTENT_TYPE_V2);
+
   @Override
-  public Collection<String> getContentTypes() {
-    return Set.of(JAVABIN_CONTENT_TYPE, JAVABIN_CONTENT_TYPE_V2);
+  public Set<String> getContentTypes() {
+    return CONTENT_TYPES;
   }
 }

solr/solrj/src/java/org/apache/solr/client/solrj/response/ResponseParser.java

@@ -19,6 +19,8 @@
 import java.io.IOException;
 import java.io.InputStream;
 import java.util.Collection;
+import java.util.Locale;
+import java.util.Set;
 import org.apache.solr.common.util.NamedList;
 
 /**
@@ -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();
 }

solr/solrj/src/java/org/apache/solr/client/solrj/response/XMLResponseParser.java

@@ -80,9 +80,11 @@ public String getWriterType() {
     return "xml";
   }
 
+  private static final Set<String> CONTENT_TYPES = Set.of("application/xml");
+
   @Override
   public Set<String> getContentTypes() {
-    return Set.of(XML_CONTENT_TYPE);
+    return CONTENT_TYPES;
   }
 
   public NamedList<Object> processResponse(Reader in) {

solr/solrj/src/java/org/apache/solr/client/solrj/response/json/JacksonDataBindResponseParser.java

@@ -20,9 +20,8 @@
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.InputStreamReader;
-import java.util.Collection;
-import java.util.List;
 import java.util.Map;
+import java.util.Set;
 import org.apache.solr.client.api.model.SolrJerseyResponse;
 import org.apache.solr.client.solrj.request.json.JacksonContentWriter;
 import org.apache.solr.client.solrj.response.ResponseParser;
@@ -47,9 +46,11 @@ public String getWriterType() {
     return "json";
   }
 
+  private static final Set<String> CONTENT_TYPES = Set.of("application/json");
+
   @Override
-  public Collection<String> getContentTypes() {
-    return List.of("application/json");
+  public Set<String> getContentTypes() {
+    return CONTENT_TYPES;
   }
 
   // TODO it'd be nice if the ResponseParser could receive the mime type so it can parse

solr/solrj/src/java/org/apache/solr/client/solrj/response/json/JsonMapResponseParser.java

@@ -20,7 +20,6 @@
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.InputStreamReader;
-import java.util.Collection;
 import java.util.Map;
 import java.util.Set;
 import org.apache.solr.client.solrj.response.ResponseParser;
@@ -55,8 +54,10 @@ public NamedList<Object> processResponse(InputStream body, String encoding) thro
     return list;
   }
 
+  private static final Set<String> CONTENT_TYPES = Set.of("application/json");
+
   @Override
-  public Collection<String> getContentTypes() {
-    return Set.of("application/json");
+  public Set<String> getContentTypes() {
+    return CONTENT_TYPES;
   }
 }

solr/solrj/src/test/org/apache/solr/client/solrj/SolrExampleCborTest.java

@@ -281,9 +281,12 @@ public String getWriterType() {
         return "cbor";
       }
 
+      private static final Set<String> CONTENT_TYPES =
+          Set.of("application/cbor", "application/octet-stream");
+
       @Override
       public Set<String> getContentTypes() {
-        return Set.of("application/cbor", "application/octet-stream");
+        return CONTENT_TYPES;
       }
 
       @Override

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 {
-    ResponseParser rp = new XMLResponseParser();
-
-    try (HttpJdkSolrClient client =
-        builder(solrTestRule.getBaseUrl()).withResponseParser(rp).build()) {
-      assertTrue(client.processorAcceptsMimeType(rp.getContentTypes(), "application/xml"));
-      assertFalse(client.processorAcceptsMimeType(rp.getContentTypes(), "application/json"));
-      queryToHelpJdkReleaseThreads(client);
-    }
-
-    rp = new JavaBinResponseParser();
-    try (HttpJdkSolrClient client =
-        builder(solrTestRule.getBaseUrl()).withResponseParser(rp).build()) {
-      assertTrue(
-          client.processorAcceptsMimeType(
-              rp.getContentTypes(), "application/vnd.apache.solr.javabin"));
-      assertTrue(client.processorAcceptsMimeType(rp.getContentTypes(), "application/octet-stream"));
-      assertFalse(client.processorAcceptsMimeType(rp.getContentTypes(), "application/xml"));
-      queryToHelpJdkReleaseThreads(client);
-    }
-  }
-
   @Test
   public void testContentTypeToEncoding() throws Exception {
     try (HttpJdkSolrClient client = builder(solrTestRule.getBaseUrl()).build()) {