Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cb68d16
Fixing XCOMMONS-2153: index set for siblings but used for nodes.
polx Mar 2, 2021
6bd2104
Comment to explain why we use this method of replacmeent.
polx Mar 2, 2021
1eaaa1e
Testing the replacement action.
polx Mar 3, 2021
c09c3b1
Reformatting and license insertion.
polx Mar 3, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions ...wiki-commons-tool-xar-integration-tests/src/test/java/org/xwiki/tool/xar/XARMojoTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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?");
}

@Test
Expand Down
8 changes: 8 additions & 0 deletions ...ol-xar/xwiki-commons-tool-xar-integration-tests/src/test/resources/transformedXml/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,14 @@
<action>INSERT_ATTACHMENT_CONTENT</action>
<content>src/main/resources/Blog/BlogBackground.png</content>
</transformation>

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

</transformations>
</configuration>
</plugin>
Expand Down
119 changes: 119 additions & 0 deletions ...tion-tests/src/test/resources/transformedXml/src/main/resources/Blog/BlogClass.xwikiclass
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,119 @@
<?xml version="1.0" encoding="UTF-8"?>
<!--
* See the NOTICE file distributed with this work for additional
* information regarding copyright ownership.
*
* This is free software; you can redistribute it and/or modify it
* under the terms of the GNU Lesser General Public License as
* published by the Free Software Foundation; either version 2.1 of
* the License, or (at your option) any later version.
*
* This software is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
* Lesser General Public License for more details.
*
* You should have received a copy of the GNU Lesser General Public
* License along with this software; if not, write to the Free
* Software Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
* 02110-1301 USA, or see the FSF site: http://www.fsf.org.
-->
<class>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. 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).
  2. 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:

  1. 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)
  2. 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:

  1. 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).
  2. 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.
  3. 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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

<name>Blog.BlogClass</name>
<customClass/>
<customMapping/>
<defaultViewSheet/>
<defaultEditSheet/>
<defaultWeb/>
<nameField/>
<validationScript/>
<blogType>
<cache>0</cache>
<customDisplay/>
<disabled>0</disabled>
<displayType>select</displayType>
<multiSelect>0</multiSelect>
<name>blogType</name>
<number>5</number>
<picker>0</picker>
<prettyName>Blog type</prettyName>
<relationalStorage>0</relationalStorage>
<separator> </separator>
<separators> ,|</separators>
<size>1</size>
<sort>none</sort>
<tooltip/>
<unmodifiable>0</unmodifiable>
<validationMessage/>
<validationRegExp/>
<values>local=Space blog (aggregates posts from its space only)|global=Global blog (aggregates posts from the entire wiki)</values>
<classType>com.xpn.xwiki.objects.classes.StaticListClass</classType>
</blogType>
<description>
<contenttype>FullyRenderedText</contenttype>
<customDisplay/>
<disabled>0</disabled>
<editor>---</editor>
<name>description</name>
<number>2</number>
<picker>0</picker>
<prettyName>Description</prettyName>
<rows>5</rows>
<size>60</size>
<tooltip/>
<unmodifiable>0</unmodifiable>
<validationMessage/>
<validationRegExp/>
<classType>com.xpn.xwiki.objects.classes.TextAreaClass</classType>
</description>
<displayType>
<cache>0</cache>
<customDisplay/>
<disabled>0</disabled>
<displayType>select</displayType>
<multiSelect>0</multiSelect>
<name>displayType</name>
<number>3</number>
<picker>0</picker>
<prettyName>Index display</prettyName>
<relationalStorage>0</relationalStorage>
<separator> </separator>
<separators> ,|</separators>
<size>1</size>
<sort>none</sort>
<tooltip/>
<unmodifiable>0</unmodifiable>
<validationMessage/>
<validationRegExp/>
<values>paginated=Paginated|weekly=Group posts weekly|monthly=Group posts monthly|all=Show all posts</values>
<classType>com.xpn.xwiki.objects.classes.StaticListClass</classType>
</displayType>
<itemsPerPage>
<customDisplay/>
<disabled>0</disabled>
<name>itemsPerPage</name>
<number>4</number>
<numberType>integer</numberType>
<prettyName>Items per page (only in the Paginated display mode)</prettyName>
<size>2</size>
<tooltip/>
<unmodifiable>0</unmodifiable>
<validationMessage/>
<validationRegExp/>
<classType>com.xpn.xwiki.objects.classes.NumberClass</classType>
</itemsPerPage>
<title>
<customDisplay/>
<disabled>0</disabled>
<name>title</name>
<number>1</number>
<picker>0</picker>
<prettyName>Blog title</prettyName>
<size>30</size>
<tooltip/>
<unmodifiable>0</unmodifiable>
<validationMessage/>
<validationRegExp/>
<classType>com.xpn.xwiki.objects.classes.StringClass</classType>
</title>
</class>
119 changes: 20 additions & 99 deletions ...r-integration-tests/src/test/resources/transformedXml/src/main/resources/Blog/WebHome.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,23 @@
<?xml version="1.0" encoding="UTF-8"?>
<!--
* See the NOTICE file distributed with this work for additional
* information regarding copyright ownership.
*
* This is free software; you can redistribute it and/or modify it
* under the terms of the GNU Lesser General Public License as
* published by the Free Software Foundation; either version 2.1 of
* the License, or (at your option) any later version.
*
* This software is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
* Lesser General Public License for more details.
*
* You should have received a copy of the GNU Lesser General Public
* License along with this software; if not, write to the Free
* Software Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
* 02110-1301 USA, or see the FSF site: http://www.fsf.org.
-->
<xwikidoc>
<web>Blog</web>
<name>WebHome</name>
Expand Down Expand Up @@ -32,105 +51,7 @@
<content></content>
</attachment>
<object>
<class>
<name>Blog.BlogClass</name>
<customClass/>
<customMapping/>
<defaultViewSheet/>
<defaultEditSheet/>
<defaultWeb/>
<nameField/>
<validationScript/>
<blogType>
<cache>0</cache>
<customDisplay/>
<disabled>0</disabled>
<displayType>select</displayType>
<multiSelect>0</multiSelect>
<name>blogType</name>
<number>5</number>
<picker>0</picker>
<prettyName>Blog type</prettyName>
<relationalStorage>0</relationalStorage>
<separator> </separator>
<separators> ,|</separators>
<size>1</size>
<sort>none</sort>
<tooltip/>
<unmodifiable>0</unmodifiable>
<validationMessage/>
<validationRegExp/>
<values>local=Space blog (aggregates posts from its space only)|global=Global blog (aggregates posts from the entire wiki)</values>
<classType>com.xpn.xwiki.objects.classes.StaticListClass</classType>
</blogType>
<description>
<contenttype>FullyRenderedText</contenttype>
<customDisplay/>
<disabled>0</disabled>
<editor>---</editor>
<name>description</name>
<number>2</number>
<picker>0</picker>
<prettyName>Description</prettyName>
<rows>5</rows>
<size>60</size>
<tooltip/>
<unmodifiable>0</unmodifiable>
<validationMessage/>
<validationRegExp/>
<classType>com.xpn.xwiki.objects.classes.TextAreaClass</classType>
</description>
<displayType>
<cache>0</cache>
<customDisplay/>
<disabled>0</disabled>
<displayType>select</displayType>
<multiSelect>0</multiSelect>
<name>displayType</name>
<number>3</number>
<picker>0</picker>
<prettyName>Index display</prettyName>
<relationalStorage>0</relationalStorage>
<separator> </separator>
<separators> ,|</separators>
<size>1</size>
<sort>none</sort>
<tooltip/>
<unmodifiable>0</unmodifiable>
<validationMessage/>
<validationRegExp/>
<values>paginated=Paginated|weekly=Group posts weekly|monthly=Group posts monthly|all=Show all posts</values>
<classType>com.xpn.xwiki.objects.classes.StaticListClass</classType>
</displayType>
<itemsPerPage>
<customDisplay/>
<disabled>0</disabled>
<name>itemsPerPage</name>
<number>4</number>
<numberType>integer</numberType>
<prettyName>Items per page (only in the Paginated display mode)</prettyName>
<size>2</size>
<tooltip/>
<unmodifiable>0</unmodifiable>
<validationMessage/>
<validationRegExp/>
<classType>com.xpn.xwiki.objects.classes.NumberClass</classType>
</itemsPerPage>
<title>
<customDisplay/>
<disabled>0</disabled>
<name>title</name>
<number>1</number>
<picker>0</picker>
<prettyName>Blog title</prettyName>
<size>30</size>
<tooltip/>
<unmodifiable>0</unmodifiable>
<validationMessage/>
<validationRegExp/>
<classType>com.xpn.xwiki.objects.classes.StringClass</classType>
</title>
</class>
<insertHereBlogXWikiClass/>
<name>Blog.WebHome</name>
<number>0</number>
<className>Blog.BlogClass</className>
Expand Down
6 changes: 5 additions & 1 deletion ...mons-tool-xar/xwiki-commons-tool-xar-plugin/src/main/java/org/xwiki/tool/xar/XARMojo.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,8 +221,12 @@ private void performTransformations() throws Exception
if (parent == null) {
document = xmlDocument;
} else {
// this is the method we use to replace the child and it seems to be
// the canonical way in dom4j (for which a manipulation on a collection
// is a manipulation of the XML tree: We manipulate the elements' list
// by setting the child at the position where it was found.
List<Element> siblings = parent.elements();
siblings.set(parent.indexOf(node), xmlElement);
siblings.set(siblings.indexOf(node), xmlElement);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
break;

Expand Down