You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by GitBox <gi...@apache.org> on 2022/01/11 20:29:28 UTC
[GitHub] [solr] magibney opened a new pull request #513: SOLR-9376: [xml] and [json] RawValue DocTransformers should work in cloud mode
magibney opened a new pull request #513:
URL: https://github.com/apache/solr/pull/513
See: [SOLR-9376](https://issues.apache.org/jira/browse/SOLR-9376)
This PR fixes test coverage to cover both cloud and standalone mode operation of the `[json]` and `[xml]` DocTransformers. Testing could/should definitely be expanded, and there may be some edge cases not accounted for.
(The bulk of the work was actually in updating the test (such as it was) to cover cloud mode. Very tricky because this issue deals with serialization per se, so you need to bypass a bunch of the common test abstractions that purposefully hide serialization details. Wistfully thinking of [SOLR-11872](https://issues.apache.org/jira/browse/SOLR-11872) :slightly_smiling_face:)
There was a false start, reverted. I left the commit history for clarity and comparison, but I think the approach ultimately taken (after the false start) is much better.
--
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
[GitHub] [solr] epugh commented on a change in pull request #513: SOLR-9376: [xml] and [json] RawValue DocTransformers should work in cloud mode
Posted by 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
[GitHub] [solr] magibney commented on a change in pull request #513: SOLR-9376: [xml] and [json] RawValue DocTransformers should work in cloud mode
Posted by GitBox <gi...@apache.org>.
magibney commented on a change in pull request #513:
URL: https://github.com/apache/solr/pull/513#discussion_r789313401
##########
File path: solr/CHANGES-SOLR-9376.txt
##########
@@ -0,0 +1,21 @@
+TO BE DROPPED INTO CHANGES.txt (and possibly `solr-upgrade-notes.adoc`?)
+FOR WHATEVER VERSION THIS ULTIMATELY LANDS IN:
+
+Upgrade Notes
Review comment:
:+1: Yes, I was drafting it there, but it was never ultimately going to stay in its own file. This is getting close enough now that I'll move it into `major-changes-in-solr-9.adoc`
--
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
[GitHub] [solr] epugh commented on pull request #513: SOLR-9376: [xml] and [json] RawValue DocTransformers should work in cloud mode
Posted by GitBox <gi...@apache.org>.
epugh commented on pull request #513:
URL: https://github.com/apache/solr/pull/513#issuecomment-1010364091
Can you explain *why* this didn't work in Cloud mode? I don't quite see what changed that makes it work in Cloud where it didn't prevously.
--
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
[GitHub] [solr] dsmiley commented on a change in pull request #513: SOLR-9376: [xml] and [json] RawValue DocTransformers should work in cloud mode
Posted by 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
[GitHub] [solr] magibney commented on pull request #513: SOLR-9376: [xml] and [json] RawValue DocTransformers should work in cloud mode
Posted by GitBox <gi...@apache.org>.
magibney commented on pull request #513:
URL: https://github.com/apache/solr/pull/513#issuecomment-1015742408
>if you wanted to make it easier to write new transformers like this, is there any other changes you would make?
I don't think so. Indeed, if someone went to implement their own custom `DocTransformer` that sought to write directly (unescaped) to output, they would currently replace the field value on each doc with an instance of the `WriteableValue` interface ... and their custom plugin would consequently be incompatible with cloud.
The new way would be to override the new (default no-op) `DocTransformer.getRawFields(Collection<String> addToExisting)` method to add the output field (to be written raw) to the specified collection. Should then be compatible with cloud, but otherwise work the same as currently.
--
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
[GitHub] [solr] magibney commented on a change in pull request #513: SOLR-9376: [xml] and [json] RawValue DocTransformers should work in cloud mode
Posted by GitBox <gi...@apache.org>.
magibney commented on a change in pull request #513:
URL: https://github.com/apache/solr/pull/513#discussion_r788339171
##########
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:
No indeed, will change this to package-private.
--
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
[GitHub] [solr] magibney commented on a change in pull request #513: SOLR-9376: [xml] and [json] RawValue DocTransformers should work in cloud mode
Posted by GitBox <gi...@apache.org>.
magibney commented on a change in pull request #513:
URL: https://github.com/apache/solr/pull/513#discussion_r783523080
##########
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:
Thanks! ... fwiw I wouldn't have characterized this as confusing exactly ... or no more so than the proposed alternative. In any case it's hard to judge from this particular change in isolation. Hopefully overall though this is clearer -- or _differently_ confusing, and in a more appropriate way.
--
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
[GitHub] [solr] epugh commented on pull request #513: SOLR-9376: [xml] and [json] RawValue DocTransformers should work in cloud mode
Posted by GitBox <gi...@apache.org>.
epugh commented on pull request #513:
URL: https://github.com/apache/solr/pull/513#issuecomment-1011029402
Thanks for the explanation, and the use of `wt=javabin` makes sense on the _why_ this is happening.
--
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
[GitHub] [solr] magibney commented on a change in pull request #513: SOLR-9376: [xml] and [json] RawValue DocTransformers should work in cloud mode
Posted by GitBox <gi...@apache.org>.
magibney commented on a change in pull request #513:
URL: https://github.com/apache/solr/pull/513#discussion_r783530761
##########
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:
I will add javadoc comments to this class to clarify. This class is needed b/c in the case where control of serialization is delegated to `SchemaField`, the API (appropriately) gives no way to indicate that the serialization should be raw. So this class exists as a shim TextResponseWriter that delegates to another TextResponseWriter, with the slight modification that it causes calls to `writeStr(...)` and `writeArray(...)` to delegate to their "raw" counterparts.
--
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
[GitHub] [solr] epugh commented on a change in pull request #513: SOLR-9376: [xml] and [json] RawValue DocTransformers should work in cloud mode
Posted by GitBox <gi...@apache.org>.
epugh commented on a change in pull request #513:
URL: https://github.com/apache/solr/pull/513#discussion_r784073844
##########
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:
Honestly, I think that we might be bikeshedding here, and in lieu of any commonly accepted standard for these types of variable names (that is enforced by a linter!), this is fine. I looked at the `TestCloudJSONFacetSKGEquiv` and it does use the same pattern. So we are consistently unconsistent.
--
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
[GitHub] [solr] magibney edited a comment on pull request #513: SOLR-9376: [xml] and [json] RawValue DocTransformers should work in cloud mode
Posted by GitBox <gi...@apache.org>.
magibney edited a comment on pull request #513:
URL: https://github.com/apache/solr/pull/513#issuecomment-1016056378
Thanks for the feedback, @epugh! I believe the recent commits have incorporated your suggestions. I also just added an explicit/trivial test expressly for the `[xml]` DocTransformer (in 738b24e6). This test can be run against current `apache-solr/main` to demonstrate that the current behavior of the xml doc transformer is different from the json transformer (and different from what this PR implements/proposes). I'm going to add a comment about this to the jira issue shortly.
EDIT: [added comment to Jira issue](https://issues.apache.org/jira/browse/SOLR-9376?focusedCommentId=17478327#comment-17478327)
--
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
[GitHub] [solr] magibney commented on a change in pull request #513: SOLR-9376: [xml] and [json] RawValue DocTransformers should work in cloud mode
Posted by GitBox <gi...@apache.org>.
magibney commented on a change in pull request #513:
URL: https://github.com/apache/solr/pull/513#discussion_r787212604
##########
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:
added javadocs to clarify
--
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
[GitHub] [solr] epugh commented on pull request #513: SOLR-9376: [xml] and [json] RawValue DocTransformers should work in cloud mode
Posted by GitBox <gi...@apache.org>.
epugh commented on pull request #513:
URL: https://github.com/apache/solr/pull/513#issuecomment-1015954805
Some great work here!
--
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
[GitHub] [solr] magibney commented on a change in pull request #513: SOLR-9376: [xml] and [json] RawValue DocTransformers should work in cloud mode
Posted by GitBox <gi...@apache.org>.
magibney commented on a change in pull request #513:
URL: https://github.com/apache/solr/pull/513#discussion_r789162650
##########
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:
(I just pushed commits addressing all other feedback but this subclass/interface question -- oh, and did also add tests to stress the renaming issue, as I suggested immediately above)
wrt the subclass/interface question: on further reflection, unfortunately I think it might just make things more convoluted (and less flexible) to try to factor out the "renaming" logic, as opposed to (as currently implemented) delegating that responsibility to each class that implements `FieldRenamer` (at a minimum each DocTransformer needs to parse its own SolrParams to determine "source" field, and the GeoTransformer in some cases _doesn't_ rename a field (instead retrieving source values directly from docValues) -- and once you add hooks for both those cases, it's unclear that anything is really being simplified).
Unless you feel strongly about it, I'm also inclined to leave this as an interface, because I don't think there's any direct benefit (in terms of factoring out code) to making it a subclass, and although the subclass approach would clarify somewhat the main use of this type, it would also needlessly prevent users from extending existing `TransformerFactory` classes to add support for field renaming. This might be kind of an edge case, but absent a strong preference for strict subclass, I'll probably stick with the flexibility of an interface-based approach.
--
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
[GitHub] [solr] magibney edited a comment on pull request #513: SOLR-9376: [xml] and [json] RawValue DocTransformers should work in cloud mode
Posted by GitBox <gi...@apache.org>.
magibney edited a comment on pull request #513:
URL: https://github.com/apache/solr/pull/513#issuecomment-1010372541
>Can you explain why this didn't work in Cloud mode? I don't quite see what changed that makes it work in Cloud where it didn't prevously.
As Hoss mentions on this issue, these transformers were applied at the _replica_ level, at which point for cloud `wt=javabin` causes the "raw" serialization to (appropriately) not kick in. This issue only affects these "raw value" transformers because they have specifically to do with the serialization, whereas "child doc transformers", e.g. don't depend on serialization so it's not a problem for them to be applied at the replica level.
This PR basically moves the actual "raw value" _actions_ away from the replicas and applies it directly in-line, where (and when) serialization occurs. This I think is appropriate for what these transformers set out to do. I'm happy to go into more detail if it's helpful.
--
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
[GitHub] [solr] magibney commented on a change in pull request #513: SOLR-9376: [xml] and [json] RawValue DocTransformers should work in cloud mode
Posted by GitBox <gi...@apache.org>.
magibney commented on a change in pull request #513:
URL: https://github.com/apache/solr/pull/513#discussion_r783522121
##########
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 take your point. It does try to follow some of the de facto conventions that @hossman seems to have set out in some tests, which are some of the most robust I could find (or at least the best among those with which I'm familiar?) -- e.g. [TestCloudJSONFacetSKGEquiv](https://github.com/apache/solr/blob/c7abf3f32ca439b73fbe0ed5a3a6d70ecc411cbb/solr/core/src/test/org/apache/solr/search/facet/TestCloudJSONFacetSKGEquiv.java#L104).
They're not properly `final` (so all-caps does violate the broader java convention) -- but as used in the test class they're "final-_ish_" ... so idk. I'm fine to change it, though perhaps that change will be obviated by an altogether different approach to cloud vs. standalone?
--
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
[GitHub] [solr] magibney commented on a change in pull request #513: SOLR-9376: [xml] and [json] RawValue DocTransformers should work in cloud mode
Posted by GitBox <gi...@apache.org>.
magibney commented on a change in pull request #513:
URL: https://github.com/apache/solr/pull/513#discussion_r783524161
##########
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:
Yeah; `== null` was indeed an obvious first choice; but that doesn't work because it's being used as shorthand to check whether we're in the appropriate state to do special handling. And some of the "inappropriate" cases are signified by `null` explicitly passed, which would have matched on `null`. I could at least comment it or name the variable more clearly. Good point.
--
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
[GitHub] [solr] sonatype-lift[bot] commented on a change in pull request #513: SOLR-9376: [xml] and [json] RawValue DocTransformers should work in cloud mode
Posted by GitBox <gi...@apache.org>.
sonatype-lift[bot] commented on a change in pull request #513:
URL: https://github.com/apache/solr/pull/513#discussion_r782583430
##########
File path: solr/core/src/java/org/apache/solr/response/JSONWriter.java
##########
@@ -41,14 +43,15 @@ public JSONWriter(Writer writer, SolrQueryRequest req, SolrQueryResponse rsp) {
public JSONWriter(Writer writer, SolrQueryRequest req, SolrQueryResponse rsp,
String wrapperFunction, String namedListStyle) {
- super(writer, req, rsp);
+ super(writer, req, rsp, getRawFields(req, rsp, "json"));
this.wrapperFunction = wrapperFunction;
this.namedListStyle = namedListStyle;
+ final String wt = req.getParams().get(CommonParams.WT);
+ final ReturnFields topLevelReturnFields = rsp.getReturnFields();
Review comment:
*UnusedVariable:* The local variable 'topLevelReturnFields' is never read. [(details)](https://errorprone.info/bugpattern/UnusedVariable)
(at-me [in a reply](https://help.sonatype.com/lift/talking-to-lift) with `help` or `ignore`)
##########
File path: solr/core/src/java/org/apache/solr/response/JSONWriter.java
##########
@@ -41,14 +43,15 @@ public JSONWriter(Writer writer, SolrQueryRequest req, SolrQueryResponse rsp) {
public JSONWriter(Writer writer, SolrQueryRequest req, SolrQueryResponse rsp,
String wrapperFunction, String namedListStyle) {
- super(writer, req, rsp);
+ super(writer, req, rsp, getRawFields(req, rsp, "json"));
this.wrapperFunction = wrapperFunction;
this.namedListStyle = namedListStyle;
+ final String wt = req.getParams().get(CommonParams.WT);
Review comment:
*UnusedVariable:* The local variable 'wt' is never read. [(details)](https://errorprone.info/bugpattern/UnusedVariable)
(at-me [in a reply](https://help.sonatype.com/lift/talking-to-lift) with `help` or `ignore`)
--
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
[GitHub] [solr] magibney commented on a change in pull request #513: SOLR-9376: [xml] and [json] RawValue DocTransformers should work in cloud mode
Posted by GitBox <gi...@apache.org>.
magibney commented on a change in pull request #513:
URL: https://github.com/apache/solr/pull/513#discussion_r788337887
##########
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:
Fair; the way this is usually invoked is directly on a single top level `DocTransformers` instance (extends `DocTransformer`), which collects raw fields from each of its child `DocTransformer` instances ... hence the "collector"/"accumulator" pattern. I think I agree with you though ultimately -- esp. given that most (probably _all_) "leaf" `DocTransformer` instances, if they return anything, will return one field (wrapped as a singleton). Will change this.
--
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
[GitHub] [solr] alessandrobenedetti commented on pull request #513: SOLR-9376: [xml] and [json] RawValue DocTransformers should work in cloud mode
Posted by GitBox <gi...@apache.org>.
alessandrobenedetti commented on pull request #513:
URL: https://github.com/apache/solr/pull/513#issuecomment-1016436588
The pull request appears to be very well done, but it is huge and unfortunately, I don't have a massive experience with this area.
I have referred this PR to my colleague Elia Porciani who has good experience with Transformers, hopefully, this helps!
--
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
[GitHub] [solr] sonatype-lift[bot] commented on a change in pull request #513: SOLR-9376: [xml] and [json] RawValue DocTransformers should work in cloud mode
Posted by GitBox <gi...@apache.org>.
sonatype-lift[bot] commented on a change in pull request #513:
URL: https://github.com/apache/solr/pull/513#discussion_r787271807
##########
File path: solr/core/src/java/org/apache/solr/response/transform/TransformerFactory.java
##########
@@ -40,6 +41,45 @@ 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 {
+ /**
+ *
+ * @param field The destination field
Review comment:
*MissingSummary:* A summary fragment is required; consider using the value of the @return block as a summary fragment instead. [(details)](https://google.github.io/styleguide/javaguide.html#s7.2-summary-fragment)
(at-me [in a reply](https://help.sonatype.com/lift/talking-to-lift) with `help` or `ignore`)
--
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
[GitHub] [solr] epugh commented on pull request #513: SOLR-9376: [xml] and [json] RawValue DocTransformers should work in cloud mode
Posted by GitBox <gi...@apache.org>.
epugh commented on pull request #513:
URL: https://github.com/apache/solr/pull/513#issuecomment-1011490212
@magibney i just did my first Review in github with some comments... I'd love to hear your thoughts on my comments, and then I'd love to give it a test and commit!
--
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
[GitHub] [solr] epugh commented on a change in pull request #513: SOLR-9376: [xml] and [json] RawValue DocTransformers should work in cloud mode
Posted by GitBox <gi...@apache.org>.
epugh commented on a change in pull request #513:
URL: https://github.com/apache/solr/pull/513#discussion_r785301319
##########
File path: solr/core/src/test/org/apache/solr/cloud/TestRandomFlRTGCloud.java
##########
@@ -96,14 +105,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
- //
- // new RawFieldValueValidator("json", "eee_s", "my_json_field_alias"),
- // new RawFieldValueValidator("json", "fff_s"),
- // new RawFieldValueValidator("xml", "ggg_s", "my_xml_field_alias"),
- // new RawFieldValueValidator("xml", "hhh_s"),
- //
+
+ new RawFieldValueValidator("json", "eee_s", "my_json_field_alias"),
+ new RawFieldValueValidator("json", "fff_s"),
Review comment:
Is there a indent that needs removing? Love we rentables test!
--
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
[GitHub] [solr] epugh commented on a change in pull request #513: SOLR-9376: [xml] and [json] RawValue DocTransformers should work in cloud mode
Posted by GitBox <gi...@apache.org>.
epugh commented on a change in pull request #513:
URL: https://github.com/apache/solr/pull/513#discussion_r784072115
##########
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:
Awesome... thanks!
--
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
[GitHub] [solr] magibney merged pull request #513: SOLR-9376: [xml] and [json] RawValue DocTransformers should work in cloud mode
Posted by GitBox <gi...@apache.org>.
magibney merged pull request #513:
URL: https://github.com/apache/solr/pull/513
--
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
[GitHub] [solr] epugh commented on pull request #513: SOLR-9376: [xml] and [json] RawValue DocTransformers should work in cloud mode
Posted by GitBox <gi...@apache.org>.
epugh commented on pull request #513:
URL: https://github.com/apache/solr/pull/513#issuecomment-1013670916
I know this is really a bug fix PR, however if you wanted to make it easier to write new transformers like this, is there any other changes you would make? This looks great and i can test it on monday
--
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
[GitHub] [solr] magibney commented on a change in pull request #513: SOLR-9376: [xml] and [json] RawValue DocTransformers should work in cloud mode
Posted by GitBox <gi...@apache.org>.
magibney commented on a change in pull request #513:
URL: https://github.com/apache/solr/pull/513#discussion_r788335896
##########
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:
Yes, basically the fact that this class had been doing renaming _outside the context of "simple" renaming that corrected for interactions with other field renames_ was a latent problem (perhaps I could/should add a test to stress this issue -- though I might leave that for another issue?). I'm going to think about this, and the "`FieldRenamer` interface vs. "subclass of `TransformerFactory`" question -- perhaps there's a way to factor some of this logic out so it can live in once place and not be the responsibility of each `FieldRenamer` implementation. I'll report back.
--
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
[GitHub] [solr] magibney commented on a change in pull request #513: SOLR-9376: [xml] and [json] RawValue DocTransformers should work in cloud mode
Posted by GitBox <gi...@apache.org>.
magibney commented on a change in pull request #513:
URL: https://github.com/apache/solr/pull/513#discussion_r788339026
##########
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:
I considered this (and I guess I'm still considering it). Making this a subclass would clarify the intent, and the expense of some (albeit not particularly useful?) flexibility. I'll think about this in conjunction with the above feedback about the "rename checking" in the `[geo]` transformer.
--
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
[GitHub] [solr] magibney commented on a change in pull request #513: SOLR-9376: [xml] and [json] RawValue DocTransformers should work in cloud mode
Posted by GitBox <gi...@apache.org>.
magibney commented on a change in pull request #513:
URL: https://github.com/apache/solr/pull/513#discussion_r783515256
##########
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:
Heh; there isn't really a "common pattern" for doing solrcloud vs standalone -- if you can find a better example to pattern after I'm certainly open to alternatives. iiuc this is what [SOLR-11872](https://issues.apache.org/jira/browse/SOLR-11872) sets out to do. This would have been _far_ easier also if it weren't for the fact that we specifically need to exercise the non-javabin serialization formats, and inspect their output directly. So that's _part_ of what made this setup more complex.
--
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
[GitHub] [solr] epugh commented on a change in pull request #513: SOLR-9376: [xml] and [json] RawValue DocTransformers should work in cloud mode
Posted by GitBox <gi...@apache.org>.
epugh commented on a change in pull request #513:
URL: https://github.com/apache/solr/pull/513#discussion_r784073844
##########
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:
Honestly, I think that we might be bikeshedding here, and in lieu of any commonly accepted standard for these types of variable names (that is enforced by a linter!), this is fine. I looked at the `TestCloudJSONFacetSKGEquiv` and it does use the same pattern.
--
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
[GitHub] [solr] epugh commented on a change in pull request #513: SOLR-9376: [xml] and [json] RawValue DocTransformers should work in cloud mode
Posted by GitBox <gi...@apache.org>.
epugh commented on a change in pull request #513:
URL: https://github.com/apache/solr/pull/513#discussion_r785301498
##########
File path: solr/solrj/src/java/org/apache/solr/client/solrj/impl/XMLResponseParser.java
##########
@@ -480,5 +494,41 @@ protected SolrDocument readDocument( XMLStreamReader parser ) throws XMLStreamEx
}
}
+ /**
+ * Converts raw String input (should valid xml when wrapped in a root element) and converts it to the
Review comment:
“Should be”
--
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
[GitHub] [solr] dsmiley commented on a change in pull request #513: SOLR-9376: [xml] and [json] RawValue DocTransformers should work in cloud mode
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #513:
URL: https://github.com/apache/solr/pull/513#discussion_r789198038
##########
File path: solr/CHANGES-SOLR-9376.txt
##########
@@ -0,0 +1,21 @@
+TO BE DROPPED INTO CHANGES.txt (and possibly `solr-upgrade-notes.adoc`?)
+FOR WHATEVER VERSION THIS ULTIMATELY LANDS IN:
+
+Upgrade Notes
Review comment:
put in `major-changes-in-solr-9.adoc`; not its own file.
##########
File path: solr/core/src/java/org/apache/solr/response/transform/DocTransformer.java
##########
@@ -52,6 +53,21 @@ 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 returned
+ *
+ * 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.
+ *
+ * @return Collection containing field names to be written raw, or <code>null</code> if no field names should
+ * be written raw. Any collection returned collection need not be externally modifiable -- i.e.,
+ * {@link java.util.Collections#singleton(Object)} is acceptable.
+ */
+ public Collection<String> getRawFields() {
Review comment:
There doesn't seem to be a semantic difference in null vs an empty collection, and it would be clearer to return non-null so maybe let's just do that?
##########
File path: solr/core/src/java/org/apache/solr/response/transform/DocTransformers.java
##########
@@ -49,6 +50,18 @@ public String getName()
return str.toString();
}
+ @Override
+ public Collection<String> getRawFields() {
+ Collection<String> fields = new ArrayList<>(size());
Review comment:
this begs to be implemented using Java streams (including flatten). Just a thought.
--
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
[GitHub] [solr] magibney commented on pull request #513: SOLR-9376: [xml] and [json] RawValue DocTransformers should work in cloud mode
Posted by GitBox <gi...@apache.org>.
magibney commented on pull request #513:
URL: https://github.com/apache/solr/pull/513#issuecomment-1018123558
Merged in upstream `main`, and addressed all the latest suggestions -- including from Liftbot, who apparently doesn't like LinkedList :-). I think I fit the note into a reasonable place in `major-changes-in-solr-9.adoc`, but not 100% sure where the right place in that file would be. Thanks again for the review, @dsmiley! ... and please lmk if you notice anything else that could use attention.
--
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
[GitHub] [solr] sonatype-lift[bot] commented on a change in pull request #513: SOLR-9376: [xml] and [json] RawValue DocTransformers should work in cloud mode
Posted by GitBox <gi...@apache.org>.
sonatype-lift[bot] commented on a change in pull request #513:
URL: https://github.com/apache/solr/pull/513#discussion_r789235002
##########
File path: solr/core/src/java/org/apache/solr/search/SolrReturnFields.java
##########
@@ -168,32 +174,26 @@ private void parseFieldList(String[] fl, SolrQueryRequest req) {
return;
}
- NamedList<String> rename = new NamedList<>();
+ Deque<DeferredRenameEntry> deferredRenameAugmenters = new LinkedList<>();
Review comment:
*JdkObsolete:* It is very rare for LinkedList to out-perform ArrayList or ArrayDeque. Avoid it unless you're willing to invest a lot of time into benchmarking. Caveat: LinkedList supports null elements, but ArrayDeque does not. [(details)](https://errorprone.info/bugpattern/JdkObsolete)
(at-me [in a reply](https://help.sonatype.com/lift/talking-to-lift) with `help` or `ignore`)
##########
File path: solr/core/src/java/org/apache/solr/response/transform/TransformerFactory.java
##########
@@ -40,6 +41,62 @@ 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 {
+ // TODO: Behavior is undefined in the event of a "destination field" collision (e.g., a user maps two fields to
+ // the same "destination field", or maps a field to a top-level requested field). In the future, the easiest way
+ // to detect such a case would be by "failing fast" upon renaming to a field that already has an associated value,
+ // or support for this feature could be expressly added via a hypothetical
+ // `combined_field:[consolidate fl=field_1,field_2]` transformer.
+ /**
+ * Analogous to {@link TransformerFactory#create(String, SolrParams, SolrQueryRequest)}, but to be implemented
+ * by {@link TransformerFactory}s that produce {@link DocTransformer}s that may rename fields.
+ *
+ * @param field The destination field
+ * @param params Local params associated with this transformer (e.g., source field)
+ * @param req The current request
+ * @param renamedFields Maps source=>dest renamed fields. Implementations should check this first, updating
+ * their own "source" field(s) as necessary, and if renaming (not copying) fields, should
+ * also update this map with the implementations "own" introduced source=>dest field
+ * mapping
+ * @param reqFieldNames Set of explicitly requested field names; implementations should consult this set to
+ * determine whether it's appropriate to rename (vs. copy) a field (e.g.: <code>boolean
+ * copy = reqFieldNames != null && reqFieldNames.contains(sourceField)</code>)
+ * @return A transformer to be used in processing field values in returned documents.
+ */
+ DocTransformer create(String field, SolrParams params, SolrQueryRequest req, Map<String, String> renamedFields, Set<String> reqFieldNames);
+
+ /**
+ * @return <code>true</code> if implementations of this class may (even subtly) modify field values.
Review comment:
*MissingSummary:* A summary fragment is required; consider using the value of the @return block as a summary fragment instead. [(details)](https://google.github.io/styleguide/javaguide.html#s7.2-summary-fragment)
(at-me [in a reply](https://help.sonatype.com/lift/talking-to-lift) with `help` or `ignore`)
--
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
[GitHub] [solr] magibney commented on pull request #513: SOLR-9376: [xml] and [json] RawValue DocTransformers should work in cloud mode
Posted by GitBox <gi...@apache.org>.
magibney commented on pull request #513:
URL: https://github.com/apache/solr/pull/513#issuecomment-1018174184
[Test failure](https://github.com/apache/solr/actions/runs/1726746111/attempts/1) didn't reproduce for me (after removal of the `classMethod` test method specifier): `gradlew :solr:solrj:test --tests "org.apache.solr.client.solrj.impl.CloudHttp2SolrClientBuilderTest.classMethod" -Ptests.jvms=1 -Ptests.jvmargs=-XX:TieredStopAtLevel=1 -Ptests.seed=7B861A6BE1228F59 -Ptests.file.encoding=ISO-8859-1`
Re-ran jobs and tests passed.
--
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
[GitHub] [solr] magibney commented on pull request #513: SOLR-9376: [xml] and [json] RawValue DocTransformers should work in cloud mode
Posted by GitBox <gi...@apache.org>.
magibney commented on pull request #513:
URL: https://github.com/apache/solr/pull/513#issuecomment-1010372541
>Can you explain why this didn't work in Cloud mode? I don't quite see what changed that makes it work in Cloud where it didn't prevously.
As Hoss mentions on this issue, these transformers were applied at the _replica_ level, at which point for cloud `wt=javabin` causes the serialization to (appropriately) not kick in. This issue only affects these "raw value" transformers because they have specifically to do with the serialization, whereas "child doc transformers", e.g. don't depend on serialization so it's not a problem for them to be applied at the replica level.
This PR basically moves the actual "raw value" _actions_ away from the replicas and applies it directly in-line with where (and when) serialization occurs. This I think is appropriate for what these transformers set out to do. I'm happy to go into more detail if it's helpful.
--
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
[GitHub] [solr] magibney commented on pull request #513: SOLR-9376: [xml] and [json] RawValue DocTransformers should work in cloud mode
Posted by GitBox <gi...@apache.org>.
magibney commented on pull request #513:
URL: https://github.com/apache/solr/pull/513#issuecomment-1016056378
Thanks for the feedback, @epugh! I believe the recent commits have incorporated your suggestions. I also just added an explicit/trivial test expressly for the `[xml]` DocTransformer (in 738b24e6). This test can be run against current `apache-solr/main` to demonstrate that the current behavior of the xml doc transformer is different from the json transformer (and different from what this PR implements/proposes). I'm going to add a comment about this to the jira issue shortly.
--
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
[GitHub] [solr] magibney edited a comment on pull request #513: SOLR-9376: [xml] and [json] RawValue DocTransformers should work in cloud mode
Posted by GitBox <gi...@apache.org>.
magibney edited a comment on pull request #513:
URL: https://github.com/apache/solr/pull/513#issuecomment-1010337816
[This diff](https://github.com/apache/solr/compare/850b2977c1df9608d434563918e20ebe3168194b..a5ae3464aac01efc658a3b610b9d6e092ecf14fd?expand=1) of the most recent commits cuts down on some of the refactoring noise and gives a better sense of what the essence of the change is
--
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
[GitHub] [solr] magibney commented on pull request #513: SOLR-9376: [xml] and [json] RawValue DocTransformers should work in cloud mode
Posted by GitBox <gi...@apache.org>.
magibney commented on pull request #513:
URL: https://github.com/apache/solr/pull/513#issuecomment-1010337816
[This diff](https://github.com/apache/solr/compare/850b2977c1df9608d434563918e20ebe3168194b..a5ae3464aac01efc658a3b610b9d6e092ecf14fd?expand=1) of the most recent commit cuts down on some of the refactoring noise and gives a better sense of what the essence of the change is
--
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
[GitHub] [solr] magibney commented on pull request #513: SOLR-9376: [xml] and [json] RawValue DocTransformers should work in cloud mode
Posted by GitBox <gi...@apache.org>.
magibney commented on pull request #513:
URL: https://github.com/apache/solr/pull/513#issuecomment-1017106979
Thanks for the review, @dsmiley! I'll follow up on these suggestions shortly.
--
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