Skip to content
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,13 @@
*/
package org.xwiki.tool.xar;

import static org.twdata.maven.mojoexecutor.MojoExecutor.configuration;
Copy link
Member

Choose a reason for hiding this comment

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

Import static are supposed to be at the end according to XWiki codestyle (I also doubt this change in needed for what this PR is about, so better avoid it in general).

Copy link
Author

Choose a reason for hiding this comment

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

The eclipse formatter moved it. I needed to add this to the end in eclipse. I'll see about adding that to the section on setting up formatting in Eclipse.

image

Copy link
Member

@tmortagne tmortagne Jan 28, 2025

Choose a reason for hiding this comment

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

You might want to take a look at the various Eclipse related configurations on https://github.com/xwiki/xwiki-commons/tree/master/xwiki-commons-tools/xwiki-commons-tool-verification-resources/src/main/resources (and in this specific case import eclipse.importorder). But in general you should avoid auto format.

import static org.twdata.maven.mojoexecutor.MojoExecutor.element;
import static org.twdata.maven.mojoexecutor.MojoExecutor.executeMojo;
import static org.twdata.maven.mojoexecutor.MojoExecutor.executionEnvironment;
import static org.twdata.maven.mojoexecutor.MojoExecutor.goal;
import static org.twdata.maven.mojoexecutor.MojoExecutor.name;

import java.io.File;
import java.io.IOException;
import java.util.ArrayList;
Expand All @@ -44,13 +51,6 @@
import org.codehaus.plexus.components.io.resources.PlexusIoFileResourceCollection;
import org.codehaus.plexus.components.io.resources.PlexusIoResource;

import static org.twdata.maven.mojoexecutor.MojoExecutor.configuration;
import static org.twdata.maven.mojoexecutor.MojoExecutor.element;
import static org.twdata.maven.mojoexecutor.MojoExecutor.executeMojo;
import static org.twdata.maven.mojoexecutor.MojoExecutor.executionEnvironment;
import static org.twdata.maven.mojoexecutor.MojoExecutor.goal;
import static org.twdata.maven.mojoexecutor.MojoExecutor.name;

/**
* Common code for the Verify and Format Mojos.
*
Expand All @@ -73,6 +73,8 @@ public abstract class AbstractVerifyMojo extends AbstractXARMojo

private static final Pattern TRANSLATION_PATTERN = Pattern.compile("(.*)\\..*\\.xml");

private static final String UNIX_STYLE_FILE_SEPARATOR = "/";

/**
* If true then don't check if the packaging is XAR before running mojos.
*/
Expand Down Expand Up @@ -150,8 +152,8 @@ public abstract class AbstractVerifyMojo extends AbstractXARMojo

/**
* Explicitly define a list of pages (it's a regex) that are technical pages but that should be visible (not
* hidden). For example, home pages of applications. These pages must have their default langue set to empty so
* that a search in a given language doesn't return them as they're not content.
* hidden). For example, home pages of applications. These pages must have their default langue set to empty so that
* a search in a given language doesn't return them as they're not content.
*
* @since 12.3RC1
*/
Expand Down Expand Up @@ -243,15 +245,16 @@ protected Collection<File> getXARXMLFiles() throws MojoFailureException
/**
* Guess the {@code &lt;defaultLanguage&gt;} value to use for the passed file using the following algorithm:
* <ul>
* <li>If the page name matches one of the regexes defined by the user as a translated page, then check that
* the default language is {@link #defaultLanguage}.</li>
* <li>Otherwise, if the page name matches one of the regexes defined by the user as a content page, then check
* that the default language is {@link #defaultLanguage}.</li>
* <li>Otherwise, if there's no other translation of the file then check that the default language is empty
* since it's a technical page.</li>
* <li>Otherwise, if there are other translations ("(prefix).(language).xml" format) then check that the default
* language should is {@link #defaultLanguage}</li>
* <li>If the page name matches one of the regexes defined by the user as a translated page, then check that the
* default language is {@link #defaultLanguage}.</li>
* <li>Otherwise, if the page name matches one of the regexes defined by the user as a content page, then check that
* the default language is {@link #defaultLanguage}.</li>
* <li>Otherwise, if there's no other translation of the file then check that the default language is empty since
* it's a technical page.</li>
* <li>Otherwise, if there are other translations ("(prefix).(language).xml" format) then check that the default
* language should is {@link #defaultLanguage}</li>
* </ul>
*
* @param file the XML file for which to guess the default language that it should have
* @param xwikiXmlFiles the list of all XML files that is used to check for translations of the passed XML file
* @return the default language as a string (e.g. "en" for English or "" for an empty default language)
Expand Down Expand Up @@ -306,9 +309,14 @@ protected boolean isVisibleTechnicalPage(String filePath)

private boolean isMatchingPage(String filePath, List<Pattern> patterns)
{
String tranformedFilePath = filePath;
// we use the unix file.seperator even if we're currently running on windows.
if (!File.separator.equals(UNIX_STYLE_FILE_SEPARATOR)) {
tranformedFilePath = filePath.replaceAll(Pattern.quote(File.separator), UNIX_STYLE_FILE_SEPARATOR);
}
if (patterns != null) {
for (Pattern pattern : patterns) {
if (pattern.matcher(filePath).matches()) {
if (pattern.matcher(tranformedFilePath).matches()) {
return true;
}
}
Expand Down Expand Up @@ -347,25 +355,11 @@ protected void executeLicenseGoal(String goal) throws MojoExecutionException
throw new MojoExecutionException("License plugin could not be found in <pluginManagement>");
}

executeMojo(
licensePlugin,
goal(goal),
configuration(
element(name("licenseSets"),
element(name("licenseSet"),
element(name("header"), "license.txt"),
element(name("headerDefinitions"),
element(name("headerDefinition"), "license-xml-definition.xml")),
element(name("includes"),
element(name("include"), "src/main/resources/**/*.xml"))
)
)
),
executionEnvironment(
this.project,
this.mavenSession,
this.pluginManager
)
);
executeMojo(licensePlugin, goal(goal),
configuration(element(name("licenseSets"),
element(name("licenseSet"), element(name("header"), "license.txt"),
element(name("headerDefinitions"), element(name("headerDefinition"), "license-xml-definition.xml")),
element(name("includes"), element(name("include"), "src/main/resources/**/*.xml"))))),
executionEnvironment(this.project, this.mavenSession, this.pluginManager));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,23 +19,26 @@
*/
package org.xwiki.tool.xar;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.xwiki.tool.xar.internal.XMLUtils.getSAXReader;

import java.io.ByteArrayOutputStream;
import java.io.File;
import java.io.InputStream;
import java.nio.charset.Charset;
import java.util.Arrays;
import java.util.Collections;
import java.util.List;
import java.util.Locale;
import java.util.regex.Pattern;

import org.apache.commons.io.IOUtils;
import org.dom4j.Document;
import org.dom4j.io.OutputFormat;
import org.dom4j.io.SAXReader;
import org.junit.jupiter.api.Test;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.xwiki.tool.xar.internal.XMLUtils.getSAXReader;

/**
* Unit tests for {@link FormatMojo}.
*
Expand All @@ -51,9 +54,7 @@ void defaultLanguageForDefaultDocumentWhenTranslation()
mojo.defaultLanguage = "en";

File file = new File("Some/Space/Document.xml");
List<File> files = Arrays.asList(
new File("Some/Space/Document.xml"),
new File("Some/Space/Document.fr.xml"));
List<File> files = Arrays.asList(new File("Some/Space/Document.xml"), new File("Some/Space/Document.fr.xml"));

assertEquals(Locale.ENGLISH, mojo.guessDefaultLocale(file, files));
}
Expand All @@ -79,7 +80,7 @@ void defaultLanguageForMatchingContentPage()
mojo.contentPages = Arrays.asList(".*/Document\\.xml");
mojo.initializePatterns();

File file = new File("Some/Space/Document.xml");
File file = new File("Some" + File.separator + "Space" + File.separator + "Document.xml");

assertEquals(Locale.ENGLISH, mojo.guessDefaultLocale(file, Collections.emptyList()));
}
Expand Down Expand Up @@ -118,9 +119,7 @@ void defaultLanguageForDocumentWhenNoTranslationButFileWithSameNameInOtherSpace(

File file = new File("Space1/Document.xml");
// Simulate a page with the same name and with a translation but in a different space.
List<File> files = Arrays.asList(
new File("Space2/Document.xml"),
new File("Space2/Document.fr.xml"));
List<File> files = Arrays.asList(new File("Space2/Document.xml"), new File("Space2/Document.fr.xml"));

assertEquals(Locale.ROOT, mojo.guessDefaultLocale(file, files));
}
Expand All @@ -132,8 +131,13 @@ void defaultLanguageForDocumentWhenNoTranslationButFileWithSameNameInOtherSpace(
void formatSpecialContentFailingWithXercesFromJDK8() throws Exception
{
SAXReader reader = getSAXReader();
reader.setEncoding("UTF-8");
InputStream is = Thread.currentThread().getContextClassLoader().getResourceAsStream("XWikiSyntaxLinks.it.xml");
String expectedContent = IOUtils.toString(is, "UTF-8");
// github clients may automatically convert the file to use windows line returns.
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, might be better to deal with this kind of problems with https://github.com/xwiki/xwiki-commons/blob/master/.gitattributes.

Copy link
Author

Choose a reason for hiding this comment

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

I dug in on this. It seemed intuitively like that might be a change with larger impacts.

I tried setting the line.seperator on the format object of the writer to the native to see that would be an easy way to get them to match, but it did not. It is because the comment nodes of the parsed version have new lines in them which don't match the windows format.

So I looked up if the SAX parser is misbehaving. It is not. The XML spec says: "To simplify the tasks of applications, the XML processor MUST behave as if it normalized all line breaks in external parsed entities (including the document entity) on input, before parsing, by translating all of the following to a single #xA character:"
https://www.w3.org/TR/xml11/#sec-line-ends

That convinced me. I do think simply making all xml conform to using Unix style line returns is the right thing to do. The parser is right to strip out carriage returns. I believe using the EOL=LF in the .gitattributes will get clients to forcibly normalize this both locally and when checking in.

if (!System.lineSeparator().equals("\n")) {
expectedContent = expectedContent.replaceAll(Pattern.quote(System.lineSeparator()), "\n");
}

is = Thread.currentThread().getContextClassLoader().getResourceAsStream("XWikiSyntaxLinks.it.xml");
Document domdoc = reader.read(is);
Expand All @@ -146,7 +150,28 @@ void formatSpecialContentFailingWithXercesFromJDK8() throws Exception
writer.setVersion("1.1");
writer.write(domdoc);
writer.close();

assertEquals(expectedContent, baos.toString());
String actual = baos.toString(Charset.forName("UTF-8"));

int offset = -1;
if (!expectedContent.equals(actual)) {
for (int i = 0; i < Math.min(((String) expectedContent).length(), ((String) actual).length()); i++) {
char c1 = expectedContent.charAt(i);
char c2 = actual.charAt(i);
if (c1 != c2) {
offset = i;
break;
}
}
String result = "";
if (offset != -1) {
result += "Offset of first difference: " + offset + " expected char: " + expectedContent.charAt(offset)
+ " (" + ((int) expectedContent.charAt(offset)) + ")" + " actual char: " + actual.charAt(offset)
+ " (" + ((int) actual.charAt(offset)) + ")" + System.lineSeparator();
}
result += "Expected:" + System.lineSeparator() + expectedContent + System.lineSeparator()
+ System.lineSeparator() + " Does not match actual: " + System.lineSeparator() + actual
+ System.lineSeparator() + System.lineSeparator();
assertTrue(false, result);
}
}
}