Skip to content

Commit d9cff37

Browse files
dfa1claude
andcommitted
fix(cli): scan-based filter parser, catch VortexException
The regex {@code ^(\w[\w.]*)\s*(!=|>=|<=|==|>|<|=)\s*(.+)$} tripped SonarCloud S5852 (polynomial-runtime regex). Replaced with a linear left-to-right scan for the first operator character ({@code !, =, >, <}), then 1- or 2-char operator slice + name validation. No engine surface, no backtracking hazard, faster on long inputs. CLI now also catches VortexException from the export path and returns ExitStatus.ERROR; previously a typo'd column dumped a stack trace instead of a clean non-zero exit. FilterCommandTest covers: usage error, missing file, invalid expr, all six operators, unknown-column error path, and a 10k-char parser performance guard. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
1 parent 976934b commit d9cff37

2 files changed

Lines changed: 170 additions & 18 deletions

File tree

cli/src/main/java/io/github/dfa1/vortex/cli/FilterCommand.java

Lines changed: 59 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -23,17 +23,9 @@
2323
import java.nio.file.Path;
2424
import java.util.Arrays;
2525
import java.util.List;
26-
import java.util.regex.Matcher;
27-
import java.util.regex.Pattern;
2826

2927
final class FilterCommand {
3028

31-
// Greedy {@code \w[\w.]*}: operator chars {@code (!, =, >, <)} cannot appear inside the
32-
// column-name character class, so no backtracking is required. A reluctant quantifier
33-
// here was a regex-DoS hazard (Sonar S5852) without any matching benefit.
34-
private static final Pattern EXPR = Pattern.compile(
35-
"^(\\w[\\w.]*)\\s*(!=|>=|<=|==|>|<|=)\\s*(.+)$");
36-
3729
private FilterCommand() {
3830
}
3931

@@ -62,31 +54,80 @@ static int run(String[] args) {
6254
CsvExporter.exportCsvFiltered(path, stdout, ExportOptions.defaults(), scanOptions, rowPred);
6355
stdout.flush();
6456
return ExitStatus.OK;
65-
} catch (IOException e) {
57+
} catch (IOException | io.github.dfa1.vortex.core.VortexException e) {
58+
// VortexException is unchecked but surfaces user-facing failures (e.g. unknown
59+
// column on a typo'd filter); catching it here keeps the CLI from dumping a
60+
// stack trace and lets shell pipelines branch on the exit code.
6661
System.err.println("error: " + e.getMessage());
6762
return ExitStatus.ERROR;
6863
}
6964
}
7065

7166
private static RowFilter parseFilter(String expr) {
72-
Matcher m = EXPR.matcher(expr.trim());
73-
if (!m.matches()) {
74-
throw new IllegalArgumentException("invalid filter expression: \"" + expr
75-
+ "\" expected: col op value (op: >, >=, <, <=, =, ==, !=)");
76-
}
77-
String col = m.group(1);
78-
Comparable<?> value = parseValue(m.group(3).trim());
79-
return switch (m.group(2)) {
67+
String trimmed = expr.trim();
68+
int opStart = indexOfOperator(trimmed);
69+
if (opStart <= 0) {
70+
throw invalid(expr);
71+
}
72+
int opEnd = operatorEnd(trimmed, opStart);
73+
String op = trimmed.substring(opStart, opEnd);
74+
String col = trimmed.substring(0, opStart).stripTrailing();
75+
String rawValue = trimmed.substring(opEnd).stripLeading();
76+
if (col.isEmpty() || !isValidColumnName(col) || rawValue.isEmpty()) {
77+
throw invalid(expr);
78+
}
79+
Comparable<?> value = parseValue(rawValue);
80+
return switch (op) {
8081
case ">" -> RowFilter.gt(col, value);
8182
case ">=" -> RowFilter.gte(col, value);
8283
case "<" -> RowFilter.lt(col, value);
8384
case "<=" -> RowFilter.lte(col, value);
8485
case "=", "==" -> RowFilter.eq(col, value);
8586
case "!=" -> RowFilter.neq(col, value);
86-
default -> throw new IllegalArgumentException("unknown operator: " + m.group(2));
87+
default -> throw new IllegalArgumentException("unknown operator: " + op);
8788
};
8889
}
8990

91+
/// Scan-based parsing replaces the previous regex
92+
/// {@code ^(\w[\w.]*)\s*(!=|>=|<=|==|>|<|=)\s*(.+)$} which Sonar flagged as a
93+
/// potential regex-DoS (S5852). Linear scan, no backtracking, no engine surface.
94+
private static int indexOfOperator(String s) {
95+
for (int i = 0; i < s.length(); i++) {
96+
char c = s.charAt(i);
97+
if (c == '!' || c == '=' || c == '>' || c == '<') {
98+
return i;
99+
}
100+
}
101+
return -1;
102+
}
103+
104+
/// Returns the index one past the operator: 2 chars for {@code !=, >=, <=, ==},
105+
/// 1 char for the single-character operators {@code >, <, =}.
106+
private static int operatorEnd(String s, int start) {
107+
if (start + 1 < s.length() && s.charAt(start + 1) == '=') {
108+
return start + 2;
109+
}
110+
return start + 1;
111+
}
112+
113+
private static boolean isValidColumnName(String s) {
114+
if (!Character.isLetterOrDigit(s.charAt(0)) && s.charAt(0) != '_') {
115+
return false;
116+
}
117+
for (int i = 1; i < s.length(); i++) {
118+
char c = s.charAt(i);
119+
if (!Character.isLetterOrDigit(c) && c != '_' && c != '.') {
120+
return false;
121+
}
122+
}
123+
return true;
124+
}
125+
126+
private static IllegalArgumentException invalid(String expr) {
127+
return new IllegalArgumentException("invalid filter expression: \"" + expr
128+
+ "\" expected: col op value (op: >, >=, <, <=, =, ==, !=)");
129+
}
130+
90131
private static Comparable<?> parseValue(String raw) {
91132
try {
92133
return Long.parseLong(raw);
Lines changed: 111 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,111 @@
1+
package io.github.dfa1.vortex.cli;
2+
3+
import org.junit.jupiter.api.BeforeEach;
4+
import org.junit.jupiter.api.Test;
5+
import org.junit.jupiter.api.io.TempDir;
6+
import org.junit.jupiter.params.ParameterizedTest;
7+
import org.junit.jupiter.params.provider.ValueSource;
8+
9+
import java.io.IOException;
10+
import java.nio.file.Path;
11+
12+
import static io.github.dfa1.vortex.cli.CliTestSupport.capture;
13+
import static io.github.dfa1.vortex.cli.CliTestSupport.writeSmallVortex;
14+
import static org.assertj.core.api.Assertions.assertThat;
15+
16+
class FilterCommandTest {
17+
18+
@TempDir
19+
Path tmp;
20+
21+
private Path file;
22+
23+
@BeforeEach
24+
void setUp() throws IOException {
25+
// 3-row Vortex file with one I64 column {@code id} = [1, 2, 3].
26+
file = writeSmallVortex(tmp, "filter.vortex");
27+
}
28+
29+
@Test
30+
void wrongArity_returnsUsageError() {
31+
// Given / When
32+
CliTestSupport.Captured got = capture(() -> FilterCommand.run(new String[]{"filter"}));
33+
34+
// Then
35+
assertThat(got.status()).isEqualTo(ExitStatus.USAGE_ERROR);
36+
assertThat(got.stderr()).contains("usage:");
37+
}
38+
39+
@Test
40+
void missingFile_returnsFileNotFound() {
41+
// Given
42+
Path missing = tmp.resolve("nope.vortex");
43+
44+
// When
45+
CliTestSupport.Captured got = capture(() ->
46+
FilterCommand.run(new String[]{"filter", missing.toString(), "id", ">", "0"}));
47+
48+
// Then
49+
assertThat(got.status()).isEqualTo(ExitStatus.FILE_NOT_FOUND);
50+
assertThat(got.stderr()).contains("file not found");
51+
}
52+
53+
@Test
54+
void invalidExpression_returnsUsageError() {
55+
// Given — file exists but expression has no operator
56+
// When
57+
CliTestSupport.Captured got = capture(() ->
58+
FilterCommand.run(new String[]{"filter", file.toString(), "id", "is", "1"}));
59+
60+
// Then
61+
assertThat(got.status()).isEqualTo(ExitStatus.USAGE_ERROR);
62+
assertThat(got.stderr()).contains("invalid filter expression");
63+
}
64+
65+
@ParameterizedTest
66+
@ValueSource(strings = {"id > 1", "id >= 2", "id < 3", "id <= 2", "id = 2", "id == 2", "id != 1"})
67+
void validExpression_returnsOkAndEmitsHeader(String filter) {
68+
// Given — 3-row file with id in [1, 2, 3] (see setUp)
69+
String[] parts = filter.split(" ");
70+
71+
// When
72+
CliTestSupport.Captured got = capture(() ->
73+
FilterCommand.run(new String[]{"filter", file.toString(), parts[0], parts[1], parts[2]}));
74+
75+
// Then — every accepted operator reaches OK and produces a CSV header line.
76+
assertThat(got.status()).isEqualTo(ExitStatus.OK);
77+
assertThat(got.stdout()).startsWith("id");
78+
}
79+
80+
@Test
81+
void unknownColumn_returnsErrorNotCrash() {
82+
// Given — parses cleanly, fails at column resolution. CLI must catch VortexException
83+
// and emit a stable error exit code rather than dumping the stack trace.
84+
// When
85+
CliTestSupport.Captured got = capture(() ->
86+
FilterCommand.run(new String[]{"filter", file.toString(), "nope", "=", "1"}));
87+
88+
// Then
89+
assertThat(got.status()).isEqualTo(ExitStatus.ERROR);
90+
assertThat(got.stderr()).contains("error:");
91+
}
92+
93+
@Test
94+
void operatorPrefixAttack_doesNotCauseBacktracking() {
95+
// Given — input designed to trigger polynomial backtracking under the previous regex
96+
// parser (10k-char run before an operator). The current scan-based parser is linear;
97+
// this test guards against re-introducing a regex with backtracking.
98+
String longName = "a".repeat(10_000);
99+
long start = System.nanoTime();
100+
101+
// When
102+
CliTestSupport.Captured got = capture(() ->
103+
FilterCommand.run(new String[]{"filter", file.toString(), longName, "=", "1"}));
104+
long elapsedMs = (System.nanoTime() - start) / 1_000_000;
105+
106+
// Then — parsing 10k chars must complete well under a second (linear scan).
107+
// The column won't exist; ERROR exit is expected, USAGE_ERROR is what we rule out.
108+
assertThat(elapsedMs).isLessThan(1000);
109+
assertThat(got.status()).isNotEqualTo(ExitStatus.USAGE_ERROR);
110+
}
111+
}

0 commit comments

Comments
 (0)