Skip to content

Fix #139: Handle non-ASCII characters correctly when annotating text #143

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

Merged
dwnusbaum merged 6 commits into from
Jan 4, 2019
Merged

Fix #139: Handle non-ASCII characters correctly when annotating text #143

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3506cfb
Handle non-ascii characters in annotations correctly
dwnusbaum Dec 4, 2018
cc93232
Update changelog
dwnusbaum Dec 5, 2018
e324c82
Simplify fix by removing charset and always using ASCII encoding
dwnusbaum Dec 11, 2018
0ff2f82
Restore comment
dwnusbaum Dec 11, 2018
ffb5045
Support non-BMP characters in annotated text correctly
dwnusbaum Dec 12, 2018
bee95aa
Improve comment explaining character handling in ColorConsoleAnnotator
dwnusbaum Dec 12, 2018
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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
============

* Your contribution here.
* [#139](https://github.com/jenkinsci/ansicolor-plugin/issues/139)/[#143](https://github.com/jenkinsci/ansicolor-plugin/pull/143): Prevent an `IndexOutOfBoundsException` from being thrown when two or more non-ASCII-compatible characters are present in colored text - [@dwnusbaum](https://github.com/dwnusbaum).

0.6.0 (11/14/2018)
============
Expand Down
23 changes: 14 additions & 9 deletions src/main/java/hudson/plugins/ansicolor/ColorConsoleAnnotator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,12 +50,10 @@ final class ColorConsoleAnnotator extends ConsoleAnnotator<Object> {
private static final long serialVersionUID = 1;

private final String colorMapName;
private final String charset;
Copy link
Member Author

@dwnusbaum dwnusbaum Dec 11, 2018

Choose a reason for hiding this comment

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

I guess this could cause issues if someone upgraded the plugin to the version with this fix and then downgraded to 0.6.0, but it's not clear to me what the lifecycle of a serialized ConsoleAnnotator would be to know for sure if it would matter.

Copy link
Member

@jglick jglick Jan 3, 2019

Choose a reason for hiding this comment

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

It is only serialized while a console page for a running build is being held open, so I do not think you need to care.


ColorConsoleAnnotator(String colorMapName, String charset) {
ColorConsoleAnnotator(String colorMapName) {
this.colorMapName = colorMapName;
this.charset = charset;
LOGGER.fine("creating annotator with colorMapName=" + colorMapName + " charset=" + charset);
LOGGER.fine("creating annotator with colorMapName=" + colorMapName);
}

@Override
Expand All @@ -70,12 +68,12 @@ class EmitterImpl implements AnsiAttributeElement.Emitter {
int lastPoint = -1; // multiple HTML tags may be emitted for one control sequence
@Override
public void emitHtml(String html) {
LOGGER.finest("emitting " + html + " @" + incoming.getCount());
LOGGER.log(Level.FINEST, "emitting {0} @{1}/{2}", new Object[] { html, incoming.getCount(), s.length() });
text.addMarkup(incoming.getCount(), html);
if (incoming.getCount() != lastPoint) {
lastPoint = incoming.getCount();
int hide = incoming.getCount() - outgoing.getCount() - adjustment;
LOGGER.finest("hiding " + hide + " @" + (outgoing.getCount() + adjustment));
LOGGER.log(Level.FINEST, "hiding {0} @{1}", new Object[] { hide, outgoing.getCount() + adjustment });
text.addMarkup(outgoing.getCount() + adjustment, outgoing.getCount() + adjustment + hide, "<!--", "-->");
adjustment += hide;
}
Expand All @@ -85,7 +83,14 @@ public void emitHtml(String html) {
CountingOutputStream incoming = new CountingOutputStream(new AnsiHtmlOutputStream(outgoing, colorMap, emitter));
emitter.incoming = incoming;
try {
byte[] data = s.getBytes(charset);
/*
* We only use AnsiHtmlOutputStream for its calls to Emitter.emitHtml when it encounters ANSI escape
* sequences. The output of the stream will be discarded. We encode the String using US_ASCII because
* all ANSI escape sequences can be represented in ASCII using a single byte, and any non-ASCII
* characters will be replaced with '?', which is good, since it means that byte offsets will match
Copy link

@vincentisambart vincentisambart Dec 12, 2018

Choose a reason for hiding this comment

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

Sorry for commenting out of nowhere, but will that work with non-BMP characters, like emojis?
In Java, "😀".length() is 2 but "😀".getBytes("US-ASCII").length seems to be 1.

Copy link
Member Author

@dwnusbaum dwnusbaum Dec 12, 2018

Choose a reason for hiding this comment

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

No, it wouldn't, but see #143 (comment) where my initial tests seemed to show that emojis are already mangled by the time they get to this method. I will double-check whether that is the case, but probably safer to switch to going char-by-char so that this would work for non-BMP characters assuming everything else worked as well.

Copy link
Member Author

@dwnusbaum dwnusbaum Dec 12, 2018

Choose a reason for hiding this comment

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

Ok, non-BMP characters were broken by the earlier method because markup text was getting inserted in between the two code units of a surrogate pair in some cases. After ffb5045 they should work correctly. Here's a screenshot of running the following pipeline with that commit:

ansiColor {
    echo '\033[94;1m[ INFO ] 😀😀Wooh\033[0m😀NotCoolored'
}

screen shot 2018-12-12 at 10 36 37

Thanks for the comment!

* character offsets in the emitter without any extra work.
*/
byte[] data = s.getBytes("US-ASCII");
for (int i = 0; i < data.length; i++) {
// Do not use write(byte[]) as offsets in incoming would not be accurate.
incoming.write(data[i]);
Expand All @@ -107,7 +112,7 @@ public ConsoleAnnotator<Object> newInstance(Object context) {
if (context instanceof Run) {
ColorizedAction action = ((Run) context).getAction(ColorizedAction.class);
if (action != null) {
return new ColorConsoleAnnotator(action.colorMapName, ((Run) context).getCharset().name());
return new ColorConsoleAnnotator(action.colorMapName);
}
} else if (Jenkins.get().getPlugin("workflow-api") != null && context instanceof FlowNode) {
FlowNode node = (FlowNode) context;
Expand All @@ -122,7 +127,7 @@ public ConsoleAnnotator<Object> newInstance(Object context) {
if (exec instanceof Run) {
ColorizedAction action = ((Run) exec).getAction(ColorizedAction.class);
if (action != null) {
return new ColorConsoleAnnotator(action.colorMapName, /* JEp-206 */ "UTF-8");
return new ColorConsoleAnnotator(action.colorMapName);
}
}
}
Expand Down
25 changes: 25 additions & 0 deletions src/test/java/hudson/plugins/ansicolor/AnsiColorBuildWrapperTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,4 +116,29 @@ public void evaluate() throws Throwable {
}
});
}

@Test
public void testNonAscii() throws Exception {
story.then(r -> {
FreeStyleProject p = r.createFreeStyleProject();
p.getBuildWrappersList().add(new AnsiColorBuildWrapper(null));
p.getBuildersList().add(new TestBuilder() {
@Override
public boolean perform(AbstractBuild<?, ?> build, Launcher launcher, BuildListener listener) throws InterruptedException, IOException {
listener.getLogger().println("\033[94;1m[ INFO ] Récupération du numéro de version de l'application\033[0m");
listener.getLogger().println("\033[94;1m[ INFO ] ビルドのコンソール出力を取得します。\033[0m");
return true;
}
});
FreeStyleBuild b = r.buildAndAssertSuccess(p);
StringWriter writer = new StringWriter();
b.getLogText().writeHtmlTo(0L, writer);
String html = writer.toString();
System.out.print(html);
assertThat(html.replaceAll("<!--.+?-->", ""),
allOf(
containsString("<span style=\"color: #4682B4;\"><b>[ INFO ] Récupération du numéro de version de l'application</b></span>"),
containsString("<span style=\"color: #4682B4;\"><b>[ INFO ] ビルドのコンソール出力を取得します。</b></span>")));
});
}
}