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.

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

#2283 Escape special characters in subject #2284

Merged
merged 3 commits into from
May 23, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
56 changes: 48 additions & 8 deletions src/main/java/com/codeborne/selenide/logevents/SimpleReport.java
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@

/**
* A simple text report of Selenide actions performed during test run.
*
* <p>
* Class is thread-safe: the same instance of SimpleReport can be reused by different threads simultaneously.
*/
@ParametersAreNonnullByDefault
Expand Down Expand Up @@ -55,7 +55,7 @@ public void finish(String title) {
@Nonnull
@CheckReturnValue
String generateReport(String title, List<LogEvent> events) {
var eventsWithNestingLevel = computeNestingLevel(events);
var eventsWithNestingLevel = escapeSubjectAndComputeNestingLevel(events);

int firstColumnWidth = Math.max(maxLocatorLength(eventsWithNestingLevel), MIN_FIRST_COLUMN_WIDTH);
int secondColumnWidth = Math.min(maxSubjectLength(eventsWithNestingLevel), MAX_SECOND_COLUMN_WIDTH);
Expand All @@ -72,16 +72,26 @@ String generateReport(String title, List<LogEvent> events) {
return report.build();
}

private List<LogEventWithNestingLevel> computeNestingLevel(List<LogEvent> events) {
var stack = new ArrayDeque<LogEvent>();
private List<LogEventWithNestingLevel> escapeSubjectAndComputeNestingLevel(List<LogEvent> events) {
var stack = new ArrayDeque<LogEventWithNestingLevel.SimpleLogEvent>();
var result = new ArrayList<LogEventWithNestingLevel>(events.size());
for (LogEvent event : events) {
while (!stack.isEmpty() && stack.peekFirst().getEndTime() <= event.getStartTime())
var eventWithEscapedSubject = new LogEventWithNestingLevel.SimpleLogEvent(
event.getElement(),
escape(event.getSubject()),
Copy link
Member

Choose a reason for hiding this comment

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

At a first glance, the good moment to escape characters is not the construction of SimpleLogEvent, but when we render SimpleLogEvent. In other words, when we generate the resulting report. Then we could escape all fields, not only subject.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But we need to know the max size of a cell of the report table. If the calculation was made before the escape was applied then the final max size may be incorrect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since element and subject are just strings, I believe, that they are the only place where special characters can appear. So, it's enough to escape them, not all the report line/row.

But, I also didn't find a case when the element could contain some special characters (only some synthetic example), that's why I left it as is, but it can be easily fixed.

event.getStatus(),
event.getDuration(),
event.getStartTime(),
event.getEndTime(),
event.getError()
);

while (!stack.isEmpty() && stack.peekFirst().getEndTime() <= eventWithEscapedSubject.getStartTime())
stack.removeFirst();

result.add(new LogEventWithNestingLevel(stack.size(), event));
result.add(new LogEventWithNestingLevel(stack.size(), eventWithEscapedSubject));

stack.addFirst(event);
stack.addFirst(eventWithEscapedSubject);
}

return result;
Expand Down Expand Up @@ -208,6 +218,36 @@ private static void checkThatSlf4jIsConfigured() {
}

@Desugar
private record LogEventWithNestingLevel(int nestingLevel, LogEvent event) {
private record LogEventWithNestingLevel(int nestingLevel, SimpleLogEvent event) {

@Desugar
private record SimpleLogEvent(
String getElement,
String getSubject,
EventStatus getStatus,
long getDuration,
long getStartTime,
long getEndTime,
Throwable getError
) implements LogEvent {
}
}

private static String escape(String text) {
var builder = new StringBuilder((int) (text.length() * 1.5));
for (int i = 0; i < text.length(); i++) {
var symbol = text.charAt(i);

switch (symbol) {
case '\t' -> builder.append("\\t");
case '\r' -> builder.append("\\r");
case '\n' -> builder.append("\\n");
case '\f' -> builder.append("\\f");
case '\b' -> builder.append("\\b");
default -> builder.append(symbol);
Copy link
Member

Choose a reason for hiding this comment

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

@Au6ojlut Maybe we should also escape all invisible characters below 32 (space)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree, I'll fix it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

}
}

return builder.toString();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,18 @@ void reportWithTooLongValue() throws IOException {
assertThat(report).isEqualTo(sample("/simple-report-test2.txt"));
}

@Test
void reportWithSubjectWithSpecialChars() throws IOException {
String report = new SimpleReport().generateReport("escapeSpecialCharactersInSubject", asList(
new Log("open", "https://some.com/", PASS, 0, millisToNanos(100)),
new Log("shouldHave", "text \"with\n\t\f\bspecial\n\r\"characters", PASS,
millisToNanos(100), millisToNanos(200)),
new Log("#loginButton", "click", FAIL, millisToNanos(200), millisToNanos(300))
));

assertThat(report).isEqualTo(sample("/simple-report-test6.txt"));
}

@Test
void reportWithLongSelectors() throws IOException {
String report = new SimpleReport().generateReport("userCanUseVeryLongSelectors", asList(
Expand Down
8 changes: 8 additions & 0 deletions src/test/resources/simple-report-test6.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
Report for escapeSpecialCharactersInSubject
+----------------------+------------------------------------------+------------+------------+
| Element | Subject | Status | ms. |
+----------------------+------------------------------------------+------------+------------+
| open | https://some.com/ | PASS | 100 |
| shouldHave | text "with\n\t\f\bspecial\n\r"characters | PASS | 100 |
| #loginButton | click | FAIL | 100 |
+----------------------+------------------------------------------+------------+------------+