Fixing XCOMMONS-2153: index set for siblings but used for nodes. #136

polx · 2021-03-02T08:29:30Z

This is just the fix for this bug.
I wish I could create a test but I need guidance to do so. I think the transformation could should be refactored out of XARMojo which has too much context to be tested.
The little fix is quite trivial though, which makes it ok, I believe.

mflorea · 2021-03-02T08:47:04Z

...commons-tool-xar/xwiki-commons-tool-xar-plugin/src/main/java/org/xwiki/tool/xar/XARMojo.java

@@ -222,7 +222,7 @@ private void performTransformations() throws Exception
                                document = xmlDocument;
                            } else {
                                List<Element> siblings = parent.elements();
-                                siblings.set(parent.indexOf(node), xmlElement);
+                                siblings.set(siblings.indexOf(node), xmlElement);


I would add a comment to explain that dom4j Node doesn't have a replaceChild method (like org.w3c.dom.Node) so we're forced to replace the element by index. Moreover, dom4j Element doesn't have a method to get all child nodes (including text or comment nodes) so the index we use must be from the list of child elements.

Thank you Marius. Done in below commit (6bd2104).

tmortagne · 2021-03-03T08:34:03Z

If you want to work on a test you should add it to xwiki-commons-tool-xar-integration-tests: the idea of those tests is that they actually run Maven on a test project and then do some checking on the result, the log, etc. See the various examples there and especially XARMojoTest#transformXML() which seems to be testing REPLACE already.

vmassol · 2021-03-03T08:52:50Z

Yes we need a test for sure. It can either be an integration tests as Thomas mentioned or simply a unit test just to verify the count, put in https://github.com/xwiki/xwiki-commons/tree/6bd21042c7adcde067d1786ccedcb0cd91de6002/xwiki-commons-tools/xwiki-commons-tool-xar/xwiki-commons-tool-xar-plugin/src/test/java/org/xwiki/tool/xar along with the other unit tests. I prefer a unit test for this personally. It seems easy to write with no mocks required at all (I looked quickly, I could be wrong).

polx · 2021-03-03T09:59:02Z

Test contributed. It was not there before.
I would love to have a unit test but there was none before so the start is not so easy.
@tmortagne : it would be useful to annotate XARMojo somewhere that tests are not found in this tree but in the neighbour project to avoid dumb questions like mine.
However, I love the integration tests, they are very powerful! (I wish I could debug them as well as the code they run inside the XARMojo... Not yet managed).

vmassol · 2021-03-03T10:13:58Z

...gration-tests/src/test/resources/transformedXml/src/main/resources/Blog/BlogClass.xwikiclass

@@ -0,0 +1,99 @@
+<class>


I would name the file *.xml since it contains XML.

Also would be good to add a license header (we need to add that to other files too).

Ok for the license header.
These files end-up into the XAR... (it could or should be avoided) thus they become pages if they are named .xml which would fail since there is no xwikidoc element. Ok to keep .xwikiclass?
(there's a lot more XML files out there which are not called .xml, e.g. the .plist on macs, or the rss files).

I don't remember the test but then it looks like these resource files are placed in the wrong location if they end up in the XAR when they shouldn't. Using a different suffix is not the right way IMO. In XWiki we use xml for xml files (it helps in the IDE, it helps visually know what the file is about, etc). I don't see any valid argument for not doing that honestly, especially since this file is a xwiki XML page which is supposed to end in .xml.

I've now checked the code and the doc at https://dev.xwiki.org/xwiki/bin/view/Community/XARPlugin#HTransformations. I think the doc is a bit wrong at https://dev.xwiki.org/xwiki/bin/view/Community/XARPlugin#HTransformations and the files that are used only as intermediary resources that are used for transformations should be excluded. If they're not needed to be processed (velocity doesn't need to run on them) they could also be put in a different directory such as src/main/xar. I think that's a better idea than the exclude.

they could also be put in a different directory such as src/main/xar. I think that's a better idea than the exclude.

Yes, I don't think exclusion is a good idea.

Very cool. Btw, the file name is entirely left to the developer. If you give the XARMojo a JPEG file as XML source for a REPLACE or INSERT_CHILD, it will (try to) parse it.

So we should open a new bug about the XARMojo so that it implicitly adds excludes for files that are not considered as pages, right? (we could even try to parse the document if xml and exclude it if is not with root xwikidoc once we find this method).

So we should open a new bug about the XARMojo so that it implicitly adds excludes for files that are not considered as pages, right? (we could even try to parse the document if xml and exclude it if is not with root xwikidoc once we find this method).

This is not the problem and there's no bug to open on XARMojo. It already only reads xml files. The problem is the usage of the XARMojo (ie the tests you're modifying). They must not mix files that are sources for the XAR from files that are sources for the transformations. It's just bad practice and asking for trouble to mix them. It's easy to see that. Imagine if you had named your file .xml, you'd see the issue immediately.

What is the part that you don't understand or don't agree with? I'm asking because we're turning in circles and I feel like I keep repeating myself over and over.

1.- this discussion should not be here...
2.- I don't agree with the fact that XARMojo should copy vm, html, png, properties or any other file since it does nothing with this
3.- I don't agree with your view that it is best practice to separate the folders between what's the input for the transformation (e.g. properties or vm) and what is not quite the input (e.g. the XML which is, in the absence of transformation, just copied).

Ok so let me redo all the analysis work from scratch (I didn't want to spend so much time on this since it was minor and simple for me):

the source for the XML files is target/classes (

xwiki-commons/xwiki-commons-tools/xwiki-commons-tool-xar/xwiki-commons-tool-xar-plugin/src/main/java/org/xwiki/tool/xar/XARMojo.java

Line 111 in 348e478

File sourceDir = new File(this.project.getBuild().getOutputDirectory());

). And not src/main/resources (except for package.xml but that's potentially an error IMO).

The transformation sources are taken from src/main/resources in the existing code and in your PR:
<transformation> <file>Blog/WebHome.xml</file> <xpath>/xwikidoc/content</xpath> <action>INSERT_TEXT</action> <charset>UTF-8</charset> <content>src/main/resources/Blog/WebHome.vm</content> </transformation>

Conclusions:

There's a problem since the maven resources plugin will copy all files from src/main/resources to target/classes while the transformation sources are taken from src/main/resources which is useless in this case (and prevents using filtering if needed BTW)

There's another problem since the maven resources plugin will copy all files from src/main/resources to target/classes including XML files named *.xml, which will then be included in the XAR by default (

xwiki-commons/xwiki-commons-tools/xwiki-commons-tool-xar/xwiki-commons-tool-xar-plugin/src/main/java/org/xwiki/tool/xar/AbstractXARMojo.java

Line 93 in 71bbc48

private static final String[] DEFAULT_INCLUDES = new String[] {"**/*.xml"};

).

Possible solutions:

Configure the maven resource plugin to exclude transformation files from being copied to target/classes. Note that this works unless we want to use the filtering feature of the maven resources plugin (not the case in this test).

Use a separate source directories for transformation files (src/main/transformations). This will make sure that the maven resource plugin will not copy them and that the XARMojo won't try to include them in the XAR.

Change the pom.xml of the test to not use src/main/resources for transformation content and instead fetch them from target/classes (since they've been copied, and they can benefit from filtering too). Configure the XARMojo plugin with includes/excludes to only include what's needed in the generated XAR.

Pros/cons:

Solution 1 is not very interesting IMO. No filtering and a bit complex since you need to hand pick the excludes.

Solution 2 is the best to me (unless we want filtering but right now we've never used that so it's pretty safe and even if needed if doesn't prevent using solution 1 or 3 in the rare cases where it'd be needed), because it's very simple and doesn't require dealing with hand-picked includes/excludes. So it's simpler and less prone to errors.

Solution 3 is good if we need filtering. But same as solution 1, it requires overriding the include/excludes and hand-pick them, which is more complex and error prone.

So I continue to think that solution 2 is the best (@tmortagne seems to think too according to #136 (comment)).

Now you said you don't like it but I don't recall seing any argument from you as to why? Which solution do you prefer and why?

Thanks

Hello @vmassol ,

I don't like your solution 2 because files that constitute other files should be aside of them. This makes sense, for example, when vm should include neighbour pages... It is disputable and the dispute is really a matter of "conceptual cleanliness vs tools current capabilities". That said, I must say that if you insist that copied files inside a xar are a problem, I guess it is indeed (as @tmortagne mentioned) the lesser evil.

I mentioned my wish for a coherent picture in this more generic post. And I really think we should not argue further on this pull-request as it was just trying to fix a bug in less than two lines while the debate is far broader and touches much older things (e.g. the transformation examples or best practices more than the tests).

paul

vmassol · 2021-03-03T10:15:47Z

@polx The test is ok as an integration test since we were not already testing the REPLACE use case. If we already had one then it would have been better as a unit test (the problem with integration tests is that they take several order of magnitude more time than a unit test so we need to not have more than needed).

tmortagne · 2021-03-03T10:21:22Z

However, I love the integration tests, they are very powerful! (I wish I could debug them as well as the code they run inside the XARMojo... Not yet managed).

I think the test project is built in a different JVM, but maybe this is configurable.

tmortagne · 2021-03-03T10:24:50Z

It was not there before.

Note that REPLACE being the default there was already 2 REPLACE transformations in that test project from what I understand (but they were replacing by a value and not XML elements).

vmassol · 2021-03-03T10:31:47Z

Note that REPLACE being the default there was already 2 REPLACE transformations in that test project from what I understand (but they were replacing by a value and not XML elements).

I mean in the pom.xml file I see only one entry for REPLACE:

xwiki-commons/xwiki-commons-tools/xwiki-commons-tool-xar/xwiki-commons-tool-xar-integration-tests/src/test/resources/transformedXml/pom.xml

Lines 64 to 69 in 1eaaa1e

    
           <transformation> 
        
             <file>Blog/WebHome.xml</file> 
        
             <action>REPLACE</action> 
        
             <xpath>/xwikidoc/object/insertHereBlogXWikiClass</xpath> 
        
             <xml>src/main/resources/Blog/BlogClass.xwikiclass</xml> 
        
           </transformation>

tmortagne · 2021-03-03T10:58:29Z

@vmassol is that a question ? Not very sure what is your point exactly.

polx · 2021-03-03T11:32:50Z

Hello @tmortagne , yes, there were tests for replaces by value before (which is triggered if you have a value element).
Now there is a test for replace by element (the element is parsed from the file).

I think the test project is built in a different JVM, but maybe this is configurable.

If I can read about this, this is helpful! (it would also make the running faster)

I'd have at least one extra feature where testing by integration likely the only solution (it would imply extracting the xml from dependencies, e.g. the BlogClass from its original place).

So is this branch ready to be merged?

tmortagne · 2021-03-03T11:42:37Z

So is this branch ready to be merged?

There seems to be one last unresolved comment from @vmassol (regarding the name of the xml file you introduced and the fact that it should have a license).

vmassol · 2021-03-03T12:29:37Z

...r/xwiki-commons-tool-xar-integration-tests/src/test/java/org/xwiki/tool/xar/XARMojoTest.java

@@ -211,6 +211,9 @@ void transformXML() throws Exception

        assertTrue(document.selectSingleNode("/xwikidoc/attachment/content") != null,
            "Insertion of attachment did not happen?");
+
+        assertTrue(document.selectSingleNode("//insertHereBlogXWikiClass") == null,
+                "Replacement of insertHereBlogXWikiClass did not happen?");


Indentation has 4 chars too much.

Is it normal to use "//" instead of the "/xwikidoc/..." used in the previous assert? Especially since it's a single node, it's probably more accurate to test for a precise location. WDYT?

"//" instead of the "/xwikidoc/..." used in the previous assert? Especially since it's a single node, it's probably more accurate to test for a precise location. WDYT?

Thus the test-verification is broader: it asserts that no element with that local-name are anywhere in the tree. So it makes it a more sure test!

Indentation has 4 chars too much.

fixed.

Thus the test-verification is broader: it asserts that no element with that local-name are anywhere in the tree. So it makes it a more sure test!

I disagree. Adding insertHereBlogXWikiClass in the wrong part of the XML is not correct and the test should verify that. The test must verify it's added in the right place.

You disagree with the fact that the verification is broader?
Or do you feel the test should verify that the replacement has happened by the existence of a particular element?

Your transformation is defined as:

<transformation> <file>Blog/WebHome.xml</file> <action>REPLACE</action> <xpath>/xwikidoc/object/insertHereBlogXWikiClass</xpath> <xml>src/main/resources/Blog/BlogClass.xwikiclass</xml> </transformation>

So AFAIU we need to verify that /xwikidoc/object/insertHereBlogXWikiClass is replaced and only that part of the XML. If another part is replaced then the code is clearly wrong.

Well, in the case of this example and example file (a test is defined by all its parts!) both are the same. Why should the test be minimal and ignore the nature of the input-data?

I guess it might be more elegant to write <xpath>//insertHereBlogXWikiClass</xpath> but showing by an automated test that both things are converging is actually showing that the machine works well. Another thing that this tests, is that the element has not been moved somewhere else (e.g. the insertion has taken place but the element is at some other place...).

vmassol · 2021-03-03T12:43:03Z

...gration-tests/src/test/resources/transformedXml/src/main/resources/Blog/BlogClass.xwikiclass

@@ -0,0 +1,99 @@
+<class>


I've now checked the code and the doc at https://dev.xwiki.org/xwiki/bin/view/Community/XARPlugin#HTransformations. I think the doc is a bit wrong at https://dev.xwiki.org/xwiki/bin/view/Community/XARPlugin#HTransformations and the files that are used only as intermediary resources that are used for transformations should be excluded. If they're not needed to be processed (velocity doesn't need to run on them) they could also be put in a different directory such as src/main/xar. I think that's a better idea than the exclude.

polx · 2021-03-03T13:45:44Z

think the doc is a bit wrong at https://dev.xwiki.org/xwiki/bin/view/Community/XARPlugin#HTransformations

can you be more precise? I don't see the error.

They could also be put in a different directory such as src/main/xar

This is the first comment about this practice... It's thinkable... but it is kind of late, I feel.

Second. Well... I'm sorry but the XML files within src/main/resources should be considered to be the xar content and you want to insert src/main/xar for things that are not going (directly) into the xar file? That seems definitely wrong. They could go to src/main/transform maybe.

But it would be far better if the xar plugin only takes the files which the importer will use... that is XML files. Isn't there a way to actually drop anything but XML without explicit excludes? This way you have one folder of interesting sources for a wiki/xar project. For example Translations.xml is aside of Translations.properties. Is this doable?

vmassol · 2021-03-03T14:48:50Z

I'm sorry but the XML files within src/main/resources should be considered to be the xar content and you want to insert src/main/xar for things that are not going (directly) into the xar file? That seems definitely wrong. They could go to src/main/transform maybe.

Why does it seem wrong? You even said "They could go to src/main/transform maybe" which seems to ack that you agree with the idea. If your only concern is the naming but not the concept then sure we can tune the name (I proposed "xar" to show that it contains resources for the xar plugin whereas "transform" doesn't remind of the xar plugin but i'm fine to use any directory name, what's important is the concept of separating template/transformation files from content that will end up in target/classes. I'm also fine with maven resource plugin configuration with excludes since what's important is that they don't go in target/classes (unless you wish to run velocity on them, in which cases, we'd need some exclude on the XAR plugin itself).

Fixing XCOMMONS-2153: index set for siblings but used for nodes.

cb68d16

mflorea reviewed Mar 2, 2021

View reviewed changes

Comment to explain why we use this method of replacmeent.

6bd2104

mflorea approved these changes Mar 3, 2021

View reviewed changes

Testing the replacement action.

1eaaa1e

vmassol reviewed Mar 3, 2021

View reviewed changes

vmassol requested changes Mar 3, 2021

View reviewed changes

Reformatting and license insertion.

c09c3b1

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixing XCOMMONS-2153: index set for siblings but used for nodes. #136

Fixing XCOMMONS-2153: index set for siblings but used for nodes. #136

polx commented Mar 2, 2021

mflorea Mar 2, 2021

polx Mar 2, 2021

tmortagne commented Mar 3, 2021 •

edited

Loading

vmassol commented Mar 3, 2021

polx commented Mar 3, 2021

vmassol Mar 3, 2021

polx Mar 3, 2021

vmassol Mar 3, 2021

vmassol Mar 3, 2021

tmortagne Mar 3, 2021

polx Mar 3, 2021

vmassol Mar 3, 2021

polx Mar 3, 2021

vmassol Mar 3, 2021

polx Mar 4, 2021

vmassol commented Mar 3, 2021

tmortagne commented Mar 3, 2021

tmortagne commented Mar 3, 2021 •

edited

Loading

vmassol commented Mar 3, 2021

tmortagne commented Mar 3, 2021 •

edited

Loading

polx commented Mar 3, 2021

tmortagne commented Mar 3, 2021

vmassol Mar 3, 2021

vmassol Mar 3, 2021

polx Mar 3, 2021

vmassol Mar 3, 2021

polx Mar 3, 2021

vmassol Mar 3, 2021

polx Mar 3, 2021

vmassol Mar 3, 2021

polx commented Mar 3, 2021

vmassol commented Mar 3, 2021

Fixing XCOMMONS-2153: index set for siblings but used for nodes. #136

Are you sure you want to change the base?

Fixing XCOMMONS-2153: index set for siblings but used for nodes. #136

Conversation

polx commented Mar 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmortagne commented Mar 3, 2021 • edited Loading

vmassol commented Mar 3, 2021

polx commented Mar 3, 2021

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

vmassol commented Mar 3, 2021

tmortagne commented Mar 3, 2021

tmortagne commented Mar 3, 2021 • edited Loading

vmassol commented Mar 3, 2021

tmortagne commented Mar 3, 2021 • edited Loading

polx commented Mar 3, 2021

tmortagne commented Mar 3, 2021

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

polx commented Mar 3, 2021

vmassol commented Mar 3, 2021

tmortagne commented Mar 3, 2021 •

edited

Loading

tmortagne commented Mar 3, 2021 •

edited

Loading

tmortagne commented Mar 3, 2021 •

edited

Loading