Hi devs,
I’d like to change the signature of 2 public methods of org.xwiki.rendering.wikimodel.WikiPageUtil (in xwiki-rendering-wikimodel), breaking binary backward compatibility:
// Now
public static boolean isValidXmlNameStartChar(char ch, boolean colonEnabled)
public static boolean isValidXmlNameChar(char ch, boolean colonEnabled)
// Proposed
public static boolean isValidXmlNameStartChar(int codePoint, boolean colonEnabled)
public static boolean isValidXmlNameChar(int codePoint, boolean colonEnabled)
The reason is that these 2 methods implement XML 1.0’s NameStartChar/NameChar productions (see https://www.w3.org/TR/xml/#NT-NameStartChar), and those productions include the supplementary range #x10000-#xEFFFF, which simply cannot be expressed with a char argument since a char stops at 0xFFFF. The code even tries to test that range today, with a (ch >= 0x10000 && ch <= 0xEFFFF) alternative that can never be true — SonarQube reports it as dead code, which is how I ran into this (see [Misc] Fix SonarQube issues by dropping a comparison that can never hold by claude[bot] · Pull Request #425 · xwiki/xwiki-rendering · GitHub, which for now just drops that dead alternative and leaves a comment saying the range needs an int to be supported).
So today isValidXmlName("𠀀foo", false) returns false even though 𠀀foo (U+20000, a CJK Extension B ideograph) is a perfectly valid XML name: the code point arrives as 2 surrogates and neither of them is a NameStartChar. Here is a test that demonstrates it — the first 2 methods fail on current master and pass with the proposed change, and the 3 others are there to show that nothing else moves:
class WikiPageUtilTest
{
/**
* U+20000 is a CJK Ideograph Extension B, i.e. a supplementary code point inside the
* {@code #x10000-#xEFFFF} range that the {@code NameStartChar} production allows.
*/
private static final String SUPPLEMENTARY_NAME_CHAR = "𠀀";
@Test
void isValidXmlNameWithSupplementaryStartCharacter()
{
assertTrue(WikiPageUtil.isValidXmlName(SUPPLEMENTARY_NAME_CHAR + "name", false));
}
@Test
void isValidXmlNameWithSupplementaryCharacter()
{
assertTrue(WikiPageUtil.isValidXmlName("name" + SUPPLEMENTARY_NAME_CHAR, false));
}
@Test
void isValidXmlNameWithUnpairedSurrogate()
{
assertFalse(WikiPageUtil.isValidXmlName("\uD840name", false));
assertFalse(WikiPageUtil.isValidXmlName("name\uD840", false));
}
@Test
void isValidXmlNameWithAsciiName()
{
assertTrue(WikiPageUtil.isValidXmlName("name", false));
assertTrue(WikiPageUtil.isValidXmlName("_name-1.2", false));
assertFalse(WikiPageUtil.isValidXmlName("-name", false));
assertFalse(WikiPageUtil.isValidXmlName("1name", false));
assertFalse(WikiPageUtil.isValidXmlName("", false));
assertFalse(WikiPageUtil.isValidXmlName(null, false));
}
@Test
void isValidXmlNameWithColon()
{
assertTrue(WikiPageUtil.isValidXmlName("ns:name", true));
assertFalse(WikiPageUtil.isValidXmlName("ns:name", false));
}
}
On master (there is no WikiPageUtilTest today, so this would also be the first test for that class):
[ERROR] Tests run: 5, Failures: 2, Errors: 0, Skipped: 0 -- in org.xwiki.rendering.wikimodel.WikiPageUtilTest
[ERROR] WikiPageUtilTest.isValidXmlNameWithSupplementaryStartCharacter:43 expected: <true> but was: <false>
[ERROR] WikiPageUtilTest.isValidXmlNameWithSupplementaryCharacter:49 expected: <true> but was: <false>
Note that with an int code point the existing range check becomes meaningful again without touching it, and the only other change needed is that isValidXmlName(String, boolean) (whose signature doesn’t change) iterates over code points instead of chars:
for (int i = 0; i < len;) {
int codePoint = tagName.codePointAt(i);
if (i == 0) {
valid = isValidXmlNameStartChar(codePoint, colonEnabled);
} else {
valid = isValidXmlNameChar(codePoint, colonEnabled);
}
if (!valid) {
break;
}
i += Character.charCount(codePoint);
}
With that, the module builds green with -Plegacy,quality (138 existing tests + the 5 new ones, Checkstyle and JaCoCo passing). It also makes the class self-consistent, since the neighbouring isValidXmlChar(int ch) already takes an int.
About the backward compatibility break:
- Source compatibility is preserved: a
charwidens to anintimplicitly, so any existing code callingisValidXmlNameStartChar('a', true)still compiles unchanged. - Binary compatibility is broken: the method descriptor changes from
(CZ)Zto(IZ)Z, so an extension compiled against 18.7.x or earlier and not recompiled would get aNoSuchMethodError. Revapi reports it and we’d need an ignore:
[ERROR] java.method.parameterTypeChanged: parameter boolean org.xwiki.rendering.wikimodel.WikiPageUtil::isValidXmlNameChar(===int===, boolean): The type of the parameter changed from 'char' to 'int'.
[ERROR] java.method.parameterTypeChanged: parameter boolean org.xwiki.rendering.wikimodel.WikiPageUtil::isValidXmlNameStartChar(===int===, boolean): The type of the parameter changed from 'char' to 'int'.
I think the break is acceptable because these 2 methods look unused outside of WikiPageUtil itself:
- In
xwiki-rendering, the only callers are insideWikiPageUtil(fromisValidXmlName(String, boolean)). - No usage in
xwiki-platformnor inxwiki-commons. - A GitHub code search for
isValidXmlName org:xwiki-contribreturns 0 hits. - A global GitHub code search for
isValidXmlNameStartCharonly finds this class and its WikiModel ancestor (xamde/wikimodel), no third-party caller.
The alternative would be to deprecate the char versions and add int overloads, but I don’t like it much: both overloads can coexist, so existing sources would silently keep binding to the deprecated char one, which is exactly the one that gives the wrong answer for supplementary code points. If you’d rather be safe though, we can go the usual route and re-add the char versions in xwiki-rendering-legacy-wikimodel.
I’ll create an XRENDERING issue and use @since 18.8.0RC1 if this is accepted.
WDYT?
Here’s my +1 to break backward compat here. I’m also fine with the legacy option. Feels lots of work for little but we can go this way too.
Thanks