[GitHub] [solr] epugh commented on a change in pull request #513: SOLR-9376: [xml] and [json] RawValue DocTransformers should work in cloud mode

[GitBox <gi...@apache.org>]

epugh commented on a change in pull request #513:
URL: https://github.com/apache/solr/pull/513#discussion_r783061509



##########
File path: solr/core/src/java/org/apache/solr/response/RawShimTextResponseWriter.java
##########
@@ -0,0 +1,107 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.response;
+
+import org.apache.solr.common.SolrDocument;
+import org.apache.solr.common.util.NamedList;
+import org.apache.solr.search.ReturnFields;
+
+import java.io.IOException;
+import java.util.Iterator;
+import java.util.Map;
+
+public class RawShimTextResponseWriter extends TextResponseWriter {
+
+  private final TextResponseWriter backing;
+
+  RawShimTextResponseWriter(TextResponseWriter backing) {
+    super(null, false);
+    this.backing = backing;
+  }
+
+  // convert non-raw to raw. These are the reason this class exists

Review comment:
       Could you elaborate on this comment?   "These"?   I don't quite get why this exists.    Does this add that `needsEscaping` method?  COuld we just modify `TextResponseWriter` and not need this class?

##########
File path: solr/core/src/test/org/apache/solr/response/TestRawTransformer.java
##########
@@ -16,55 +16,151 @@
  */
 package org.apache.solr.response;
 
+import org.apache.commons.io.FileUtils;
 import org.apache.solr.SolrTestCaseJ4;
+import org.apache.solr.client.solrj.embedded.JettySolrRunner;
+import org.apache.solr.client.solrj.impl.CloudSolrClient;
+import org.apache.solr.client.solrj.impl.HttpSolrClient;
+import org.apache.solr.client.solrj.impl.NoOpResponseParser;
+import org.apache.solr.client.solrj.request.CollectionAdminRequest;
+import org.apache.solr.client.solrj.request.QueryRequest;
+import org.apache.solr.cloud.MiniSolrCloudCluster;
+import org.apache.solr.cloud.SolrCloudTestCase;
 import org.apache.solr.common.SolrInputDocument;
-import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.common.params.ModifiableSolrParams;
 import org.junit.After;
+import org.junit.AfterClass;
 import org.junit.BeforeClass;
 import org.junit.Test;
 
+import java.io.File;
+import java.lang.invoke.MethodHandles;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.LinkedHashMap;
+import java.util.Map;
+import java.util.Properties;
+
 /**
  * Tests Raw JSON output for fields when used with and without the unique key field.
  *
  * See SOLR-7993
  */
-public class TestRawTransformer extends SolrTestCaseJ4 {
+public class TestRawTransformer extends SolrCloudTestCase {
+
+  private static final String DEBUG_LABEL = MethodHandles.lookup().lookupClass().getName();
+
+  /** A basic client for operations at the cloud level, default collection will be set */
+  private static JettySolrRunner JSR;

Review comment:
       I think this isn't the normal pattern for variables?!

##########
File path: solr/core/src/test/org/apache/solr/response/TestRawTransformer.java
##########
@@ -16,55 +16,151 @@
  */
 package org.apache.solr.response;
 
+import org.apache.commons.io.FileUtils;
 import org.apache.solr.SolrTestCaseJ4;
+import org.apache.solr.client.solrj.embedded.JettySolrRunner;
+import org.apache.solr.client.solrj.impl.CloudSolrClient;
+import org.apache.solr.client.solrj.impl.HttpSolrClient;
+import org.apache.solr.client.solrj.impl.NoOpResponseParser;
+import org.apache.solr.client.solrj.request.CollectionAdminRequest;
+import org.apache.solr.client.solrj.request.QueryRequest;
+import org.apache.solr.cloud.MiniSolrCloudCluster;
+import org.apache.solr.cloud.SolrCloudTestCase;
 import org.apache.solr.common.SolrInputDocument;
-import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.common.params.ModifiableSolrParams;
 import org.junit.After;
+import org.junit.AfterClass;
 import org.junit.BeforeClass;
 import org.junit.Test;
 
+import java.io.File;
+import java.lang.invoke.MethodHandles;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.LinkedHashMap;
+import java.util.Map;
+import java.util.Properties;
+
 /**
  * Tests Raw JSON output for fields when used with and without the unique key field.
  *
  * See SOLR-7993
  */
-public class TestRawTransformer extends SolrTestCaseJ4 {
+public class TestRawTransformer extends SolrCloudTestCase {
+
+  private static final String DEBUG_LABEL = MethodHandles.lookup().lookupClass().getName();
+
+  /** A basic client for operations at the cloud level, default collection will be set */
+  private static JettySolrRunner JSR;
+  private static HttpSolrClient CLIENT;
 
   @BeforeClass
   public static void beforeClass() throws Exception {
-    initCore("solrconfig-doctransformers.xml", "schema.xml");
+    if (random().nextBoolean()) {

Review comment:
       ONe more thing, is this a common pattenr for doing solrcloud versus NON solr cloud?   It just looks a bit odd to me, is there a better way of swapping the two?

##########
File path: solr/core/src/java/org/apache/solr/response/TextResponseWriter.java
##########
@@ -55,6 +58,10 @@
 
   protected Calendar cal;  // reusable calendar instance
 
+  private static final ReturnFields DUMMY_RETURN_FIELDS = new SolrReturnFields();

Review comment:
       Is there a way to avoid having DUMMY_RETURN_FIELDS?   It took me a bit to figure out WHY we have them, and I'm still a bit unsure why.   Versus say if `rawFields == null`, will then having a null.

##########
File path: solr/core/src/java/org/apache/solr/response/transform/RawValueTransformerFactory.java
##########
@@ -109,19 +131,8 @@ public String getName()
     @Override
     public void transform(SolrDocument doc, int docid) {
       Object val = doc.remove(field);
-      if(val==null) {

Review comment:
       I love this change...    This seemed VERY confusiong...




-- 
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: issues-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org