SOLR-15121: Move XSLT (tr param) to scripting contrib #2306

epugh · 2021-02-04T22:53:06Z

Description

Reduce security exposure for solr by moving the xslt to scripting.

Solution

Migrating code.

Tests

running existing tests.

Checklist

Please review the following and check all that apply:

[X ] I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
[ X] I have created a Jira issue and added the issue ID to my pull request title.
[ X] I have given Solr maintainers access to contribute to my PR branch. (optional but recommended)
[X ] I have developed this patch against the master branch.
[ X] I have run ./gradlew check.
[ X] I have added tests for my changes.
[ X] I have added documentation for the Ref Guide (for Solr changes only).

…c to xslt, so lets rework the test to reflect that

…it in other contribs. deals with some issues in finding files

epugh · 2021-02-04T22:53:26Z

I still have a broken test, and need to update docs.

solr/contrib/scripting/src/java/org/apache/solr/scripting/util/xslt/XSLTLoader.java

dsmiley

There used to be an XsltUpdateRequestHandler. Maybe we need to bring it back.
https://issues.apache.org/jira/browse/SOLR-2630
(not sure which JIRA it was removed in)
It would mean that use of "tr" would require a dedicated handler but I think that's fine not many people use this and they can register at /update/xslt.

solr/contrib/scripting/src/java/org/apache/solr/scripting/util/xslt/TransformerProvider.java

solr/contrib/scripting/src/java/org/apache/solr/scripting/response/package-info.java

dsmiley · 2021-02-06T06:20:34Z

solr/contrib/scripting/src/java/org/apache/solr/scripting/util/xslt/XSLTParams.java

+/**
+ * Parameters used across the XSLT related classes.
+ */
+public interface XSLTParams {


I see why you added this here but I don't think there's a role for XSLTParams. Just move this "tr" constant to the plugin that defines it for its internal use only. CommonParams was in SolrJ deliberately; This XSLTParams you added is not (and should not be).

dsmiley · 2021-02-06T06:24:49Z

solr/contrib/scripting/src/test/org/apache/solr/scripting/handler/XSLTOutputWriterTest.java

+ * See the related unit test OutputWriterTest.
+ *
+ */
+public class XSLTOutputWriterTest extends SolrTestCaseJ4 {


The formatting in this file is a bit off (indentation varies etc.). Lucene recently adopted the Google Java Format. I use an IntelliJ plugin for this. Can you please reformat this file with that? Even the built-in formatter is probably fine.

I found this blog post: http://www.practicesofmastery.com/post/eclipse-google-java-style-guide/ (since I use Eclipse)... Do we have a JIRA about moving towards this? Should I look to figure out how to add the Google Java Format to the default Eclipse setup? Or at least a message when you run ./gradlew eclipse that recommends adding this formatter to your setup?

Apparently there's an Eclipse plugin listed here: https://github.com/google/google-java-format
The Lucene side issue is here: https://issues.apache.org/jira/browse/LUCENE-9564
But you'll see in the comments were held off on Solr for a later date.

BTW if I'm mistaken and you moved an existing source file, then I recommend doing no formatting changes as it may fool git rename detection.

Your recent edits here look good BTW.

dsmiley · 2021-02-06T06:29:37Z

...ntrib/scripting/src/test/org/apache/solr/scripting/handler/XsltUpdateRequestHandlerTest.java

@@ -69,8 +77,8 @@ public void testUpdate() throws Exception
      "</random>";

    Map<String,String> args = new HashMap<>();
-    args.put(CommonParams.TR, "xsl-update-handler-test.xsl");
-
+    args.put(XSLTParams.TR, "xsl-update-handler-test.xsl");


In tests, you can just refer to the parameter directly: "tr". Yonik advised this practice once on the reasoning that the test catches API changes you might not have meant to make. Also, it's shorter :-)

dsmiley · 2021-02-06T06:35:38Z

solr/core/src/java/org/apache/solr/handler/UpdateRequestHandler.java

+    // Try to load the scripting contrib modules XSLTLoader, and if not, fall back to the XMLLoader.
+    // Enable the XSLTLoader by including the Scripting contrib module.
+    try {
+      final Class<?> clazz = Class.forName( "org.apache.solr.scripting.util.xslt.XSLTLoader" );


If the scripting contrib is added as a package, Solr core.jar will not be able to load it in this way, I think.

This was the part where I didn't like what I did... And was putting something up as a starter.. I really appreciate your and @uschindler input on this. Definitily lets find another approach!

dsmiley · 2021-02-06T07:08:07Z

solr/contrib/scripting/src/java/org/apache/solr/scripting/util/xslt/XSLTLoader.java

+import static org.apache.solr.common.params.CommonParams.NAME;
+
+
+public class XSLTLoader extends ContentStreamLoader {


Looks like you substantially copied XMLLoader? Sorry; let's find another approach.

solr/contrib/scripting/src/java/org/apache/solr/scripting/xslt/XSLTLoader.java

dsmiley · 2021-02-07T05:42:54Z

solr/contrib/scripting/src/java/org/apache/solr/scripting/XSLTResponseWriter.java

@@ -14,7 +14,7 @@
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */
-package org.apache.solr.scripting.response;
+package org.apache.solr.scripting;


why not the xslt package?

dsmiley · 2021-02-07T05:44:00Z

solr/contrib/scripting/src/test/org/apache/solr/scripting/handler/XSLTOutputWriterTest.java

+ * See the related unit test OutputWriterTest.
+ *
+ */
+public class XSLTOutputWriterTest extends SolrTestCaseJ4 {


Your recent edits here look good BTW.

…bling the "tr" parameter support

dsmiley · 2021-02-08T16:32:01Z

@epugh I see in your latest change that XMLLoader continues to know about XSLT stylesheets. Couldn't/shouldn't that go in the XsltRequestHandler that you added? I suppose you're getting somewhat conflicting feedback between me and @uschindler . My preference is that all XSLT stuff is in this new contrib. Uwe had suggested some trivial toggle/setting to the existing code. I think his latest feedback is basically the idea of adding an intercepting spot (in XMLLoader) so that it needn't have the XSLT code there but it could be added in by the contrib, say by subclassing XMLLoader. @uschindler does this make sense to you?

epugh · 2021-02-08T16:51:37Z

I think there is a bit of back and forth.. I added a boolean to XMLLoader constructor to decide if the tr parameter was acceptible. I was going to have it throw an exception from the UpdateRequestHandler if a tr was passed in via solr params, but allow it if it was from the XSLTUpdateRequestHandler...

I'm open to any approach. I wonder if the reason to keep the TransformerProvider.java in core is for people who write their own Solr plugins that use it, and not have to require the scripting module?

I'm going to push up a set of changes where all the paths work and tests pass momentarily.

…t match

uschindler · 2021-02-08T17:50:54Z

Yes, I tend to think of an interception point.

By default in core the interception point just returns the original Source instance. In the subclass it applies the transformation and returns a new Source instance.

uschindler · 2021-02-08T17:57:20Z

I have to again look at the code, to figure out the best interaction that is still performant and clean to implement. So we don't convert too often between different XML API representations.

I think the converter protected method should take a Source impl and return a new Source impl, in base class it just returns the parameter unmodified.

… XSLT in class names. I moved everything to XSLT* to match the XML* pattern.

epugh · 2021-02-08T18:54:41Z

@uschindler if you want to push up an example/make the change the way you are thinking, I'm 100% happy to have that! Your Java skillz are way beyond mine, and while I sort of understnad what you say, it may take you 20 minutes to get the change you want, and then I'll learn from you ;-)

epugh · 2021-02-08T18:55:09Z

I am testing out the idea of exporting in Solr XML, and then reimporting using a XSLT, which is something metnioned int he docs.. to see how well this works.

…e these demos wrok out of the box.

… xslt working on either the export or the import end of things.

…the scripting jar included. a bit icky that solrj uses the sample_techproducts_configs for unit testing...

epugh · 2021-02-09T22:07:34Z

Okay, I've done a bunch of tweaking (banging my head?) against the ref guide docs, and they are working, and I think all the tests are passing.

I don't like that the SolrJ tests depend on the sample_techproducts_configs directory, but I think adding some startup=lazy settings for the XSLT classes means that the links in the ref guide will work, and the solrj tests don't blow up on the missing xslt classes.

dsmiley · 2021-02-11T06:23:21Z

solr/docker/Docker-FAQ.md

@@ -150,7 +150,7 @@ rather than the "mycores" directory.
 Can I run ZooKeeper and Solr clusters under Docker?
 ---------------------------------------------------

-At the network level the ZooKeeper nodes need to be able to talk to eachother,


Feel free to just commit this typo stuff by itself directly (no PR needed)

dsmiley · 2021-02-11T06:28:06Z

.../contrib/scripting/src/test/org/apache/solr/scripting/xslt/XSLTUpdateRequestHandlerTest.java

+ * This tests the XSLTUpdateRequestHandler ability to work with XSLT stylesheet and xml content.
+ * </p>
+*/
+public class XSLTUpdateRequestHandlerTest extends SolrTestCaseJ4 {


BTW, I the most common naming convention is for acronyms to use camel case instead of all-caps. This has been a long trend over many years. I know why you changed it here -- to match the class it's testing, and so it's a net improvement over what existed. But arguably it's the class it's testing that should be renamed; not this.

I am down for making this change... XSLT -> Xslt for the files touched in this PR, and then maybe a seperate JIRA for XML --> Xml in the other classes?

Dedupe getTransformer logic.

solr/contrib/scripting/src/java/org/apache/solr/scripting/xslt/XSLTUpdateRequestHandler.java

epugh · 2021-02-13T13:09:11Z

Thanks for getting your hands dirty @dsmiley and making these changes, they LGTM.

uschindler · 2021-02-13T14:56:44Z

@uschindler if you want to push up an example/make the change the way you are thinking, I'm 100% happy to have that! Your Java skillz are way beyond mine, and while I sort of understnad what you say, it may take you 20 minutes to get the change you want, and then I'll learn from you ;-)

Sorry, my father died on Thursday. So excuse my ignorance! To me @dsmiley changes look fine. I will review later as some "long term specialist" 😉 on XML processing.

dsmiley · 2021-02-13T15:13:13Z

Sorry, my father died on Thursday.

Wow; so sorry to hear! I fear the same for my own Dad; I can't take his existence for granted so I'll be visiting him tomorrow.

I will review later as some "long term specialist" 😉 on XML processing.

You're not the only one; I'm a long-time XML fan :-) It seems the new kids know little about such things :-/ "What is this weird syntax... " as he/she goes off to then write a dozen lines that an XPath does in one.

I kept the all-caps XSLT despite my preferences because All of the XML related APIs we use are using all-caps for XML and thus it looked off to see it differently. But then... the user doesn't know that and sees XML in the config file so... Hmm.

Maybe the XSLT response writer should demand "tr" and thus be more consistent with the XSLT response writer? After all, you're choosing to send the response there so why would you send it there if it wasn't? I guess it gives you choice and the ability to be more similar to what we have today. Shrug.

We need some docs to show how to reference the XSLT update handler, or at the very least an upgrade note about this.

I hate the crappy caching; it's inferior to a SolrCache but alas... I hold myself back from feature-creep. This code was written forever-ago so I can appreciate it has excuses.

epugh · 2021-02-13T16:33:14Z

Sorry to hear that news @uschindler ....

I did a pass through the Ref Guide to document how to use the XSLT stuff, and more explicitly called out the sample .xsl files that we have. I can add more if needed.

So, if we aren't changing the name of the Java class in this PR, then I think we just need to update the Upgrade to Solr 9 page, and we're good to go?

dsmiley · 2021-02-13T17:18:01Z

So, if we aren't changing the name of the Java class in this PR, then I think we just need to update the Upgrade to Solr 9 page, and we're good to go?

+1

mikemccand · 2021-02-16T14:20:24Z

Sorry, my father died on Thursday.

So sorry to hear this @uschindler.

epugh@opensourceconnections.com added 9 commits February 4, 2021 09:53

light copyediting

0746293

document how to avoid shared memory issue

20e5d56

relocate xslt related classes into scripting contrib

3d584d8

we are really testing the replication of subdir, not anything specifi…

8751dad

…c to xslt, so lets rework the test to reflect that

relocating files to scripting and seperating out unit tests

1a749b6

relocate files under test-files/scripting/solr, similar to how we do …

15a9b76

…it in other contribs. deals with some issues in finding files

dont need xslt in here

8978945

moved tests to scripting

32deac8

provide a subdir for testin replication that isnt tied to xslt

9025c3b

epugh@opensourceconnections.com added 5 commits February 5, 2021 00:57

comment out the XSLT writer, and provide a link and some context

5088fc3

working through the tests to get them to pass

aac6bfc

fix javadoc error

e9af96a

here are two notional thoughts about how to enable the XSLTLoader

53f50ca

awkward check for a class existing, and it does, then use it

337b86d

epugh requested a review from dsmiley February 5, 2021 22:03

back out this old attempt at pluggablity

89de9ac

sonatype-lift bot reviewed Feb 5, 2021

View reviewed changes

solr/contrib/scripting/src/java/org/apache/solr/scripting/util/xslt/XSLTLoader.java Outdated Show resolved Hide resolved

dsmiley requested changes Feb 6, 2021

View reviewed changes

dsmiley reviewed Feb 6, 2021

View reviewed changes

epugh@opensourceconnections.com added 7 commits February 6, 2021 08:53

Remove the .util. package as not needed.

d8ba6af

Provide better intro to what this class does.

a80a1e3

better package info

431e0a5

Reformatting using the Google Java Format...

1e3164b

fix up pathing

e019a9f

use actual param name, not the variable to properly test api!

efd7334

start updating the docs, more effort needed.

72bcf7b

sonatype-lift bot reviewed Feb 6, 2021

View reviewed changes

solr/contrib/scripting/src/java/org/apache/solr/scripting/xslt/XSLTLoader.java Outdated Show resolved Hide resolved

dsmiley reviewed Feb 7, 2021

View reviewed changes

another round of moving files around, and an attempt at exposing/disa…

0331878

…bling the "tr" parameter support

Tests are passing, and package structure between src/java and src/tes…

1e71866

…t match

epugh@opensourceconnections.com added 2 commits February 8, 2021 13:14

Reduce white space changes

0233e84

Clean up references to paths, and deal with the mish mash of Xslt and…

f3e4cbd

… XSLT in class names. I moved everything to XSLT* to match the XML* pattern.

epugh@opensourceconnections.com added 4 commits February 8, 2021 15:40

restore and document why we still have xslt package

bccbf23

turns out the default setup includes the scripting jar, so we can hav…

8b3a35a

…e these demos wrok out of the box.

Round of edits to test the use of xslt in export and import, with the…

dddc0b4

… xslt working on either the export or the import end of things.

lazy lets us use these configs for the solrj tests, which don't have …

46f1b4e

…the scripting jar included. a bit icky that solrj uses the sample_techproducts_configs for unit testing...

dsmiley reviewed Feb 11, 2021

View reviewed changes

dsmiley added 3 commits February 13, 2021 00:44

Move XSLT processing out of XMLLoader

8ddb329

Move TransformerProvider.

751fa32

Dedupe getTransformer logic.

Remove unused

2b4a49d

sonatype-lift bot reviewed Feb 13, 2021

View reviewed changes

solr/contrib/scripting/src/java/org/apache/solr/scripting/xslt/XSLTUpdateRequestHandler.java Show resolved Hide resolved

epugh merged commit e6d9eaa into apache:master Feb 15, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SOLR-15121: Move XSLT (tr param) to scripting contrib #2306

SOLR-15121: Move XSLT (tr param) to scripting contrib #2306

epugh commented Feb 4, 2021

epugh commented Feb 4, 2021

dsmiley left a comment

dsmiley Feb 6, 2021

dsmiley Feb 6, 2021

epugh Feb 6, 2021

dsmiley Feb 7, 2021

dsmiley Feb 7, 2021

dsmiley Feb 6, 2021

dsmiley Feb 6, 2021

epugh Feb 6, 2021

dsmiley Feb 6, 2021

dsmiley Feb 7, 2021

dsmiley Feb 7, 2021

dsmiley commented Feb 8, 2021

epugh commented Feb 8, 2021

uschindler commented Feb 8, 2021 •

edited

uschindler commented Feb 8, 2021 •

edited

epugh commented Feb 8, 2021

epugh commented Feb 8, 2021

epugh commented Feb 9, 2021

dsmiley Feb 11, 2021

dsmiley Feb 11, 2021

epugh Feb 13, 2021

epugh commented Feb 13, 2021

uschindler commented Feb 13, 2021

dsmiley commented Feb 13, 2021

epugh commented Feb 13, 2021

dsmiley commented Feb 13, 2021

mikemccand commented Feb 16, 2021

		import static org.apache.solr.common.params.CommonParams.NAME;


		public class XSLTLoader extends ContentStreamLoader {

SOLR-15121: Move XSLT (tr param) to scripting contrib #2306

SOLR-15121: Move XSLT (tr param) to scripting contrib #2306

Conversation

epugh commented Feb 4, 2021

Description

Solution

Tests

Checklist

epugh commented Feb 4, 2021

dsmiley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsmiley commented Feb 8, 2021

epugh commented Feb 8, 2021

uschindler commented Feb 8, 2021 • edited

uschindler commented Feb 8, 2021 • edited

epugh commented Feb 8, 2021

epugh commented Feb 8, 2021

epugh commented Feb 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

epugh commented Feb 13, 2021

uschindler commented Feb 13, 2021

dsmiley commented Feb 13, 2021

epugh commented Feb 13, 2021

dsmiley commented Feb 13, 2021

mikemccand commented Feb 16, 2021

uschindler commented Feb 8, 2021 •

edited

uschindler commented Feb 8, 2021 •

edited