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

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

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



##########
File path: solr/core/src/java/org/apache/solr/response/transform/DocTransformer.java
##########
@@ -52,6 +53,24 @@ public void setContext( ResultContext context ) {
 
   }
 
+  /**
+   * If this transformer wants to bypass escaping in the {@link org.apache.solr.response.TextResponseWriter} and
+   * write content directly to output for certain field(s), the names of any such field(s) should be added to the
+   * specified collection.
+   *
+   * NOTE: normally this will be conditional on the `wt` param in the request, as supplied to the
+   * {@link DocTransformer}'s parent {@link TransformerFactory} at the time of transformer creation.
+   *
+   * @param addToExisting Existing collection to which field names should be added.
+   * @return Collection containing field names to be written raw. If a non-null collection was specified as the
+   * method argument, that same collection should be returned. If the method argument is null, a newly-created
+   * collection should be created and returned if any field renames are necessary (locally created return values
+   * need not be externally modifiable -- i.e., {@link java.util.Collections#singleton(Object)} is acceptable).
+   */
+  public Collection<String> getRawFields(Collection<String> addToExisting) {

Review comment:
       Why bother with the addToExisting param -- seems like over-optimization and not worth the hassle it takes to document it.

##########
File path: solr/core/src/java/org/apache/solr/response/RawShimTextResponseWriter.java
##########
@@ -0,0 +1,111 @@
+/*
+ * 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;
+
+/**
+ * Utility class that delegates to another {@link TextResponseWriter}, but converts normal write requests
+ * into "raw" requests that write field values directly to the delegate {@link TextResponseWriter}'s backing writer.
+ */
+public class RawShimTextResponseWriter extends TextResponseWriter {

Review comment:
       Needs to be public?

##########
File path: solr/core/src/test/org/apache/solr/cloud/TestRandomFlRTGCloud.java
##########
@@ -96,14 +107,12 @@
       new RenameFieldValueValidator("bbb_i", "my_int_field_alias"),
       new SimpleFieldValueValidator("ccc_s"),
       new RenameFieldValueValidator("ddd_s", "my_str_field_alias"),
-      //
-      // SOLR-9376: RawValueTransformerFactory doesn't work in cloud mode 

Review comment:
       awesome :-)

##########
File path: solr/core/src/java/org/apache/solr/response/transform/GeoTransformerFactory.java
##########
@@ -144,88 +148,111 @@ public void transform(SolrDocument doc, int docid) throws IOException {
 
     }
     
+    // if source has been renamed, update reference

Review comment:
       Changes here are hard to follow but if tests pass, I'm good with it.

##########
File path: solr/CHANGES-SOLR-9376.txt
##########
@@ -0,0 +1,29 @@
+TO BE DROPPED INTO CHANGES.txt (and possibly `solr-upgrade-notes.adoc`?)
+FOR WHATEVER VERSION THIS ULTIMATELY LANDS IN:
+
+Upgrade Notes
+----------------------
+
+* `[xml]` raw value DocTransformer: The response format for field values serialized
+  as raw XML (via the `[xml]` raw value DocTransformer and `wt=xml`) has changed.
+  Previously, values were dropped in directly as top-level child elements of each
+  `<doc>`, obscuring associated field names and yielding inconsistent `<doc>`
+  structure. This is changed with SOLR-9376, and raw values are now wrapped in a
+  `<raw name="field_name">[...]</raw>` element at the top level of each `<doc>` (or
+  within an enclosing `<arr name="field_name"><raw>[...]</raw></arr>` element for
+  multi-valued fields). Existing clients that parse field values serialized in this
+  way will need to be updated accordingly.
+
+* The public `WriteableValue` interface has been removed. Its sole purpose was to

Review comment:
       WriteableValue is too low level to even bother talking about here.  It's great to explain in the PR text but that's it IMO.

##########
File path: solr/core/src/java/org/apache/solr/response/transform/TransformerFactory.java
##########
@@ -40,6 +41,47 @@ public void init(NamedList<?> args) {
 
   public abstract DocTransformer create(String field, SolrParams params, SolrQueryRequest req);
 
+  /**
+   * The {@link FieldRenamer} interface should be implemented by any {@link TransformerFactory} capable of generating
+   * transformers that might rename fields, and should implement {@link #create(String, SolrParams, SolrQueryRequest, Map, Set)}
+   * in place of {@link #create(String, SolrParams, SolrQueryRequest)} (with the latter method
+   * overridden to throw {@link UnsupportedOperationException}).
+   *
+   * {@link DocTransformer}s returned via {@link #create(String, SolrParams, SolrQueryRequest, Map, Set)}
+   * will be added in a second pass, allowing simplified logic in {@link TransformerFactory#create(String, SolrParams, SolrQueryRequest)}
+   * for non-renaming factories.
+   *
+   * {@link #create(String, SolrParams, SolrQueryRequest, Map, Set)} must implement extra logic to be aware of
+   * preceding field renames, and to make subsequent {@link FieldRenamer} transformers aware of its own field renames.
+   *
+   * It is harmless for a {@link DocTransformer} that does _not_ in practice rename fields to be returned from a
+   * factory that implements this interface (e.g., for conditional renames?); but doing so opens the possibility of
+   * {@link #create(String, SolrParams, SolrQueryRequest, Map, Set)} being called _after_ fields have been renamed,
+   * so such implementations must still check whether the field with which they are concerned has been renamed ...
+   * and if it _has_, must copy the field back to its original name. This situation also demonstrates the
+   * motivation for separating the creation of {@link DocTransformer}s into two phases: an initial phase involving
+   * no field renames, and a subsequent phase that implement extra logic to properly handle field renames.
+   */
+  public interface FieldRenamer {

Review comment:
       Would it make more sense to subclass TransformerFactory instead to show this?  It's fine as-is I guess.

##########
File path: solr/core/src/java/org/apache/solr/response/TextResponseWriter.java
##########
@@ -55,6 +61,15 @@
 
   protected Calendar cal;  // reusable calendar instance
 
+  /**
+   * NOTE: {@link #NO_RAW_FIELDS} is a signal object that must be used to differentiate from <code>null</code>

Review comment:
       nit: in javadoc style, you can get right to the point and omit "NOTE: {@link #NO_RAW_FIELDS} is" since you are putting this *on* that field after all.




-- 
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