Skip to content

Commit 19c287e

Browse files
[ES|QL] Correct line and column numbers of missing named parameters (#120852) (#121449)
* correct line and column numbers of missing named parameters
1 parent 53e8731 commit 19c287e

File tree

4 files changed

+71
-11
lines changed

4 files changed

+71
-11
lines changed

docs/changelog/120852.yaml

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
pr: 120852
2+
summary: Correct line and column numbers of missing named parameters
3+
area: ES|QL
4+
type: bug
5+
issues: []

x-pack/plugin/esql/qa/server/src/main/java/org/elasticsearch/xpack/esql/qa/rest/RestEsqlTestCase.java

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -773,6 +773,33 @@ public void testNamedParamsForIdentifierAndIdentifierPatterns() throws IOExcepti
773773
}
774774
}
775775

776+
public void testErrorMessageForMissingParams() throws IOException {
777+
ResponseException re = expectThrows(
778+
ResponseException.class,
779+
() -> runEsql(requestObjectBuilder().query("from idx | where x == ?n1").params("[]"))
780+
);
781+
assertThat(
782+
EntityUtils.toString(re.getResponse().getEntity()).replaceAll("\\\\\n\s+\\\\", ""),
783+
containsString("line 1:23: Unknown query parameter [n1]")
784+
);
785+
786+
re = expectThrows(
787+
ResponseException.class,
788+
() -> runEsql(requestObjectBuilder().query("from idx | where x == ?n1 and y == ?n2").params("[{\"n\" : \"v\"}]"))
789+
);
790+
assertThat(EntityUtils.toString(re.getResponse().getEntity()).replaceAll("\\\\\n\s+\\\\", ""), containsString("""
791+
line 1:23: Unknown query parameter [n1], did you mean [n]?; line 1:36: Unknown query parameter [n2], did you mean [n]?"""));
792+
793+
re = expectThrows(
794+
ResponseException.class,
795+
() -> runEsql(requestObjectBuilder().query("from idx | where x == ?n1 and y == ?n2").params("[{\"n1\" : \"v1\"}]"))
796+
);
797+
assertThat(
798+
EntityUtils.toString(re.getResponse().getEntity()).replaceAll("\\\\\n\s+\\\\", ""),
799+
containsString("line 1:36: Unknown query parameter [n2], did you mean [n1]")
800+
);
801+
}
802+
776803
public void testErrorMessageForLiteralDateMathOverflow() throws IOException {
777804
List<String> dateMathOverflowExpressions = List.of(
778805
"2147483647 day + 1 day",

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/LogicalPlanBuilder.java

Lines changed: 1 addition & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -108,17 +108,7 @@ protected LogicalPlan plan(ParseTree ctx) {
108108
if (errors.hasNext() == false) {
109109
return p;
110110
} else {
111-
StringBuilder message = new StringBuilder();
112-
int i = 0;
113-
114-
while (errors.hasNext()) {
115-
if (i > 0) {
116-
message.append("; ");
117-
}
118-
message.append(errors.next().getMessage());
119-
i++;
120-
}
121-
throw new ParsingException(message.toString());
111+
throw ParsingException.combineParsingExceptions(errors);
122112
}
123113
}
124114

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/ParsingException.java

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,8 @@
99
import org.elasticsearch.xpack.esql.EsqlClientException;
1010
import org.elasticsearch.xpack.esql.core.tree.Source;
1111

12+
import java.util.Iterator;
13+
1214
import static org.elasticsearch.common.logging.LoggerMessageFormat.format;
1315

1416
public class ParsingException extends EsqlClientException {
@@ -21,6 +23,10 @@ public ParsingException(String message, Exception cause, int line, int charPosit
2123
this.charPositionInLine = charPositionInLine + 1;
2224
}
2325

26+
/**
27+
* To be used only if the exception cannot be associated with a specific position in the query.
28+
* Error message will start with {@code line -1:-1:} instead of using specific location.
29+
*/
2430
public ParsingException(String message, Object... args) {
2531
this(Source.EMPTY, message, args);
2632
}
@@ -37,6 +43,38 @@ public ParsingException(Exception cause, Source source, String message, Object..
3743
this.charPositionInLine = source.source().getColumnNumber();
3844
}
3945

46+
private ParsingException(int line, int charPositionInLine, String message, Object... args) {
47+
super(message, args);
48+
this.line = line;
49+
this.charPositionInLine = charPositionInLine;
50+
}
51+
52+
/**
53+
* Combine multiple {@code ParsingException} into one, this is used by {@code LogicalPlanBuilder} to
54+
* consolidate multiple named parameters related {@code ParsingException}.
55+
*/
56+
public static ParsingException combineParsingExceptions(Iterator<ParsingException> parsingExceptions) {
57+
StringBuilder message = new StringBuilder();
58+
int i = 0;
59+
int line = -1;
60+
int charPositionInLine = -1;
61+
62+
while (parsingExceptions.hasNext()) {
63+
ParsingException e = parsingExceptions.next();
64+
if (i > 0) {
65+
message.append("; ");
66+
message.append(e.getMessage());
67+
} else {
68+
// line and column numbers are the associated with the first error
69+
line = e.getLineNumber();
70+
charPositionInLine = e.getColumnNumber();
71+
message.append(e.getErrorMessage());
72+
}
73+
i++;
74+
}
75+
return new ParsingException(line, charPositionInLine, message.toString());
76+
}
77+
4078
public int getLineNumber() {
4179
return line;
4280
}

0 commit comments

Comments
 (0)