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-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
