diff --git a/docs/changelog/120852.yaml b/docs/changelog/120852.yaml new file mode 100644 index 0000000000000..90a05aa860f3f --- /dev/null +++ b/docs/changelog/120852.yaml @@ -0,0 +1,5 @@ +pr: 120852 +summary: Correct line and column numbers of missing named parameters +area: ES|QL +type: bug +issues: [] diff --git a/x-pack/plugin/esql/qa/server/src/main/java/org/elasticsearch/xpack/esql/qa/rest/RestEsqlTestCase.java b/x-pack/plugin/esql/qa/server/src/main/java/org/elasticsearch/xpack/esql/qa/rest/RestEsqlTestCase.java index 66333421eeb75..88fc8a9a36312 100644 --- a/x-pack/plugin/esql/qa/server/src/main/java/org/elasticsearch/xpack/esql/qa/rest/RestEsqlTestCase.java +++ b/x-pack/plugin/esql/qa/server/src/main/java/org/elasticsearch/xpack/esql/qa/rest/RestEsqlTestCase.java @@ -773,6 +773,33 @@ public void testNamedParamsForIdentifierAndIdentifierPatterns() throws IOExcepti } } + public void testErrorMessageForMissingParams() throws IOException { + ResponseException re = expectThrows( + ResponseException.class, + () -> runEsql(requestObjectBuilder().query("from idx | where x == ?n1").params("[]")) + ); + assertThat( + EntityUtils.toString(re.getResponse().getEntity()).replaceAll("\\\\\n\s+\\\\", ""), + containsString("line 1:23: Unknown query parameter [n1]") + ); + + re = expectThrows( + ResponseException.class, + () -> runEsql(requestObjectBuilder().query("from idx | where x == ?n1 and y == ?n2").params("[{\"n\" : \"v\"}]")) + ); + assertThat(EntityUtils.toString(re.getResponse().getEntity()).replaceAll("\\\\\n\s+\\\\", ""), containsString(""" + line 1:23: Unknown query parameter [n1], did you mean [n]?; line 1:36: Unknown query parameter [n2], did you mean [n]?""")); + + re = expectThrows( + ResponseException.class, + () -> runEsql(requestObjectBuilder().query("from idx | where x == ?n1 and y == ?n2").params("[{\"n1\" : \"v1\"}]")) + ); + assertThat( + EntityUtils.toString(re.getResponse().getEntity()).replaceAll("\\\\\n\s+\\\\", ""), + containsString("line 1:36: Unknown query parameter [n2], did you mean [n1]") + ); + } + public void testErrorMessageForLiteralDateMathOverflow() throws IOException { List dateMathOverflowExpressions = List.of( "2147483647 day + 1 day", diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/LogicalPlanBuilder.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/LogicalPlanBuilder.java index ba8aaf6251c57..5ccdda5b1839b 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/LogicalPlanBuilder.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/LogicalPlanBuilder.java @@ -108,17 +108,7 @@ protected LogicalPlan plan(ParseTree ctx) { if (errors.hasNext() == false) { return p; } else { - StringBuilder message = new StringBuilder(); - int i = 0; - - while (errors.hasNext()) { - if (i > 0) { - message.append("; "); - } - message.append(errors.next().getMessage()); - i++; - } - throw new ParsingException(message.toString()); + throw ParsingException.combineParsingExceptions(errors); } } diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/ParsingException.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/ParsingException.java index c25ab92437bfc..119e96bbd865c 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/ParsingException.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/ParsingException.java @@ -9,6 +9,8 @@ import org.elasticsearch.xpack.esql.EsqlClientException; import org.elasticsearch.xpack.esql.core.tree.Source; +import java.util.Iterator; + import static org.elasticsearch.common.logging.LoggerMessageFormat.format; public class ParsingException extends EsqlClientException { @@ -21,6 +23,10 @@ public ParsingException(String message, Exception cause, int line, int charPosit this.charPositionInLine = charPositionInLine + 1; } + /** + * To be used only if the exception cannot be associated with a specific position in the query. + * Error message will start with {@code line -1:-1:} instead of using specific location. + */ public ParsingException(String message, Object... args) { this(Source.EMPTY, message, args); } @@ -37,6 +43,38 @@ public ParsingException(Exception cause, Source source, String message, Object.. this.charPositionInLine = source.source().getColumnNumber(); } + private ParsingException(int line, int charPositionInLine, String message, Object... args) { + super(message, args); + this.line = line; + this.charPositionInLine = charPositionInLine; + } + + /** + * Combine multiple {@code ParsingException} into one, this is used by {@code LogicalPlanBuilder} to + * consolidate multiple named parameters related {@code ParsingException}. + */ + public static ParsingException combineParsingExceptions(Iterator parsingExceptions) { + StringBuilder message = new StringBuilder(); + int i = 0; + int line = -1; + int charPositionInLine = -1; + + while (parsingExceptions.hasNext()) { + ParsingException e = parsingExceptions.next(); + if (i > 0) { + message.append("; "); + message.append(e.getMessage()); + } else { + // line and column numbers are the associated with the first error + line = e.getLineNumber(); + charPositionInLine = e.getColumnNumber(); + message.append(e.getErrorMessage()); + } + i++; + } + return new ParsingException(line, charPositionInLine, message.toString()); + } + public int getLineNumber() { return line; }