diff --git a/src/cypher/cypher.c b/src/cypher/cypher.c index 36c07ea4c..17840b04c 100644 --- a/src/cypher/cypher.c +++ b/src/cypher/cypher.c @@ -1992,6 +1992,13 @@ static int parse_post_where(parser_t *p, cbm_query_t *q, // NOLINT(misc-no-recur q->union_next = sub.query; sub.query = NULL; cbm_parse_free(&sub); + /* The branch after UNION was parsed by a SEPARATE parser over a slice + * of these tokens, so this parser's cursor never moved past the UNION + * keyword. That sub-parse now refuses to succeed with anything left + * over, so everything from here to the end is accounted for. Move the + * cursor to the end to say so, or cbm_parse's end-of-input check reads + * a fully parsed UNION query as unfinished. */ + p->pos = p->count; } return 0; } @@ -2055,6 +2062,36 @@ int cbm_parse(const cbm_token_t *tokens, int token_count, // NOLINT(misc-no-recu return CBM_NOT_FOUND; } + /* Every token must be consumed. The grammar accepts at most one WITH and + * treats RETURN as optional, so a query with a second WITH stage — or any + * typo after RETURN — used to stop parsing there and succeed anyway. The + * dropped tail took the filter and the RETURN with it, and the engine + * answered from the fragment it had parsed, using its default projection. + * That reported success and returned wrong rows, which is worse than a + * refusal because nothing tells the caller to look. Refuse instead. */ + if (peek(&p)->type != TOK_EOF) { + /* Only mention the one-WITH limit when a standalone WITH really is + * sitting in the part we could not read. Saying it every time points + * a reader at WITH when the problem is a typo. A WITH straight after + * STARTS is the STARTS WITH operator rather than a clause, the same + * guard parse_post_where uses. */ + const char *hint = ""; + for (int i = p.pos; i < p.count; i++) { + if (p.tokens[i].type == TOK_WITH && + (i == 0 || p.tokens[i - SKIP_ONE].type != TOK_STARTS)) { + hint = " Note that only one WITH clause is supported."; + break; + } + } + snprintf(p.error, sizeof(p.error), + "unexpected input at pos %d ('%s') — the query was not fully " + "parsed.%s", + peek(&p)->pos, peek(&p)->text ? peek(&p)->text : "", hint); + out->error = heap_strdup(p.error); + cbm_query_free(q); + return CBM_NOT_FOUND; + } + out->query = q; return 0; } diff --git a/tests/test_cypher.c b/tests/test_cypher.c index 7882be82f..b674da15d 100644 --- a/tests/test_cypher.c +++ b/tests/test_cypher.c @@ -209,6 +209,71 @@ TEST(cypher_parse_simple_node) { PASS(); } +/* Trailing input must be an error, never a silent drop. The parser used to + * stop at the first thing it did not understand and report success, so the + * engine answered from the fragment it had parsed. */ +TEST(cypher_parse_rejects_trailing_tokens) { + cbm_query_t *q = NULL; + char *err = NULL; + int rc = cbm_cypher_parse("MATCH (f:Function) RETURN f.name AS n BANANA SPLIT 99", &q, &err); + ASSERT_NEQ(rc, 0); + ASSERT_NOT_NULL(err); + ASSERT_NULL(q); + + /* The message must name what actually stopped the parse, and must not + * mention WITH: this query has no WITH in it anywhere. */ + ASSERT(strstr(err, "BANANA") != NULL); + ASSERT(strstr(err, "WITH") == NULL); + + free(err); + PASS(); +} + +/* Only one WITH is supported. A second one used to take the rest of the + * query with it — the filter and the RETURN both vanished, and every row + * came back unfiltered under the default projection. */ +TEST(cypher_parse_rejects_second_with_clause) { + cbm_query_t *q = NULL; + char *err = NULL; + int rc = cbm_cypher_parse("MATCH (f:Function) " + "OPTIONAL MATCH (a)-[:CALLS]->(f) " + "WITH f, count(a) AS calls " + "OPTIONAL MATCH (b)-[:USAGE]->(f) " + "WITH f, calls, count(b) AS usages " + "WHERE calls = 0 AND usages = 0 " + "RETURN f.name AS n", + &q, &err); + ASSERT_NEQ(rc, 0); + ASSERT_NOT_NULL(err); + ASSERT_NULL(q); + + /* Here the note earns its place. The parse stops at OPTIONAL, and the + * reason is the second WITH further along, which the reader cannot see + * from the stopping point alone. */ + ASSERT(strstr(err, "only one WITH clause is supported") != NULL); + + free(err); + PASS(); +} + +/* The guard must not reject a query that is simply finished. One WITH, a + * WHERE after it and a RETURN is the shape the grammar does support. */ +TEST(cypher_parse_accepts_single_with_clause) { + cbm_query_t *q = NULL; + char *err = NULL; + int rc = cbm_cypher_parse("MATCH (f:Function) " + "WITH f, f.name AS n " + "WHERE n = 'buildTree' " + "RETURN n", + &q, &err); + ASSERT_EQ(rc, 0); + ASSERT_NULL(err); + ASSERT_NOT_NULL(q); + + cbm_query_free(q); + PASS(); +} + TEST(cypher_parse_relationship_outbound) { cbm_query_t *q = NULL; char *err = NULL; @@ -4102,6 +4167,9 @@ SUITE(cypher) { RUN_TEST(cypher_lex_full_query); /* Parser */ RUN_TEST(cypher_parse_simple_node); + RUN_TEST(cypher_parse_rejects_trailing_tokens); + RUN_TEST(cypher_parse_rejects_second_with_clause); + RUN_TEST(cypher_parse_accepts_single_with_clause); RUN_TEST(cypher_parse_relationship_outbound); RUN_TEST(cypher_parse_relationship_inbound); RUN_TEST(cypher_parse_relationship_any);