diff --git a/x-pack/plugin/eql/src/main/antlr/EqlBase.g4 b/x-pack/plugin/eql/src/main/antlr/EqlBase.g4 index ef52fd1bf3d..c38538f65e0 100644 --- a/x-pack/plugin/eql/src/main/antlr/EqlBase.g4 +++ b/x-pack/plugin/eql/src/main/antlr/EqlBase.g4 @@ -30,7 +30,7 @@ sequenceParams ; sequence - : SEQUENCE (by=joinKeys sequenceParams? | sequenceParams by=joinKeys?)? + : SEQUENCE (by=joinKeys sequenceParams? | sequenceParams disallowed=joinKeys?)? sequenceTerm sequenceTerm+ (UNTIL until=sequenceTerm)? ; diff --git a/x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/parser/EqlBaseParser.java b/x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/parser/EqlBaseParser.java index 624cda6dc9c..1a6e745fb44 100644 --- a/x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/parser/EqlBaseParser.java +++ b/x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/parser/EqlBaseParser.java @@ -393,6 +393,7 @@ class EqlBaseParser extends Parser { public static class SequenceContext extends ParserRuleContext { public JoinKeysContext by; + public JoinKeysContext disallowed; public SequenceTermContext until; public TerminalNode SEQUENCE() { return getToken(EqlBaseParser.SEQUENCE, 0); } public List sequenceTerm() { @@ -462,7 +463,7 @@ class EqlBaseParser extends Parser { if (_la==BY) { { setState(83); - ((SequenceContext)_localctx).by = joinKeys(); + ((SequenceContext)_localctx).disallowed = joinKeys(); } } diff --git a/x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/parser/LogicalPlanBuilder.java b/x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/parser/LogicalPlanBuilder.java index 0aba0f208d2..41910e352ee 100644 --- a/x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/parser/LogicalPlanBuilder.java +++ b/x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/parser/LogicalPlanBuilder.java @@ -112,8 +112,13 @@ public abstract class LogicalPlanBuilder extends ExpressionBuilder { @Override public Sequence visitSequence(SequenceContext ctx) { - List parentJoinKeys = visitJoinKeys(ctx.by); + Source source = source(ctx); + if (ctx.disallowed != null && ctx.sequenceParams() != null) { + throw new ParsingException(source, "Please specify sequence [by] before [with] not after"); + } + + List parentJoinKeys = visitJoinKeys(ctx.by); TimeValue maxSpan = visitSequenceParams(ctx.sequenceParams()); LogicalPlan until; @@ -122,7 +127,7 @@ public abstract class LogicalPlanBuilder extends ExpressionBuilder { until = visitSequenceTerm(ctx.until, parentJoinKeys); } else { // no until declared means the condition never gets executed and thus folds to false - until = new Filter(source(ctx), RELATION, new Literal(source(ctx), Boolean.FALSE, DataTypes.BOOLEAN)); + until = new Filter(source, RELATION, new Literal(source(ctx), Boolean.FALSE, DataTypes.BOOLEAN)); } int numberOfKeys = -1; @@ -145,7 +150,7 @@ public abstract class LogicalPlanBuilder extends ExpressionBuilder { } } - return new Sequence(source(ctx), queries, until, maxSpan); + return new Sequence(source, queries, until, maxSpan); } public KeyedFilter visitSequenceTerm(SequenceTermContext ctx, List joinKeys) { @@ -173,45 +178,40 @@ public abstract class LogicalPlanBuilder extends ExpressionBuilder { } String timeString = text(ctx.timeUnit().IDENTIFIER()); - TimeUnit timeUnit = TimeUnit.SECONDS; - if (timeString != null) { - switch (timeString) { - case "": - case "s": - case "sec": - case "secs": - case "second": - case "seconds": - timeUnit = TimeUnit.SECONDS; - break; - case "m": - case "min": - case "mins": - case "minute": - case "minutes": - timeUnit = TimeUnit.MINUTES; - break; - case "h": - case "hs": - case "hour": - case "hours": - timeUnit = TimeUnit.HOURS; - break; - case "d": - case "ds": - case "day": - case "days": - timeUnit = TimeUnit.DAYS; - break; - default: - throw new ParsingException(source(ctx.timeUnit().IDENTIFIER()), "Unrecognized time unit [{}]", timeString); - } + + if (timeString == null) { + throw new ParsingException(source(ctx.timeUnit()), "No time unit specified, did you mean [s] as in [{}s]?", text(ctx + .timeUnit())); + } + + TimeUnit timeUnit = null; + switch (timeString) { + case "ms": + timeUnit = TimeUnit.MILLISECONDS; + break; + case "s": + timeUnit = TimeUnit.SECONDS; + break; + case "m": + timeUnit = TimeUnit.MINUTES; + break; + case "h": + timeUnit = TimeUnit.HOURS; + break; + case "d": + timeUnit = TimeUnit.DAYS; + break; + default: + throw new ParsingException(source(ctx.timeUnit().IDENTIFIER()), + "Unrecognized time unit [{}] in [{}], please specify one of [ms, s, m, h, d]", + timeString, text(ctx.timeUnit())); } return new TimeValue(value, timeUnit); } else { - throw new ParsingException(source(numberCtx), "Decimal time interval [{}] not supported yet", text(numberCtx)); + throw new ParsingException(source(numberCtx), "Decimal time interval [{}] not supported; please use an positive integer", + text(numberCtx)); } } } \ No newline at end of file diff --git a/x-pack/plugin/eql/src/test/java/org/elasticsearch/xpack/eql/planner/QueryFolderFailTests.java b/x-pack/plugin/eql/src/test/java/org/elasticsearch/xpack/eql/planner/QueryFolderFailTests.java index 347b6b6b30b..bd56a9af6e3 100644 --- a/x-pack/plugin/eql/src/test/java/org/elasticsearch/xpack/eql/planner/QueryFolderFailTests.java +++ b/x-pack/plugin/eql/src/test/java/org/elasticsearch/xpack/eql/planner/QueryFolderFailTests.java @@ -20,9 +20,9 @@ public class QueryFolderFailTests extends AbstractQueryFolderTestCase { } private String errorParsing(String eql) { - ParsingException e = expectThrows(ParsingException.class, () -> plan(eql)); + Exception e = expectThrows(Exception.class, () -> plan(eql)); + assertTrue(e.getClass().getSimpleName().endsWith("ParsingException")); final String header = "line "; - assertTrue(e.getMessage().startsWith(header)); return e.getMessage().substring(header.length()); } @@ -187,4 +187,14 @@ public class QueryFolderFailTests extends AbstractQueryFolderTestCase { assertEquals("Found 1 problem\n" + "line 1:15: first argument of [wildcard(pid, '*.exe')] must be [string], found value [pid] type [long]", msg); } + + public void testSequenceWithBeforeBy() { + String msg = errorParsing("sequence with maxspan=1s by key [a where true] [b where true]"); + assertEquals("1:2: Please specify sequence [by] before [with] not after", msg); + } + + public void testSequenceWithNoTimeUnit() { + String msg = errorParsing("sequence with maxspan=30 [a where true] [b where true]"); + assertEquals("1:24: No time unit specified, did you mean [s] as in [30s]?", msg); + } } diff --git a/x-pack/plugin/eql/src/test/resources/queries-supported.eql b/x-pack/plugin/eql/src/test/resources/queries-supported.eql index 41d8c0f4a74..372760adc3f 100644 --- a/x-pack/plugin/eql/src/test/resources/queries-supported.eql +++ b/x-pack/plugin/eql/src/test/resources/queries-supported.eql @@ -546,7 +546,7 @@ sequence by unique_pid [network where true] ; -sequence by pid with maxspan=200 +sequence by pid with maxspan=200s [process where process_name == "*" ] [file where file_path == "*"] ; @@ -556,12 +556,12 @@ sequence by pid with maxspan=2s [file where file_path == "*"] ; -sequence by pid with maxspan=2sec +sequence by pid with maxspan=500ms [process where process_name == "*" ] [file where file_path == "*"] ; -sequence by pid with maxspan=2seconds +sequence by pid with maxspan=2h [process where process_name == "*" ] [file where file_path == "*"] ; \ No newline at end of file