diff --git a/src/cypher/cypher.c b/src/cypher/cypher.c index 17840b04c..0654cea70 100644 --- a/src/cypher/cypher.c +++ b/src/cypher/cypher.c @@ -1802,8 +1802,16 @@ static int parse_return_or_with(parser_t *p, cbm_return_clause_t **out, bool is_ /* Projection is materialized per row into fixed-width stack arrays sized at * CBM_SZ_32 columns (execute_return_simple and its siblings). Bound the * parsed item count to that width so an over-wide RETURN is rejected here - * instead of writing past those arrays downstream. */ - if (r->count > CBM_SZ_32) { + * instead of writing past those arrays downstream. + * + * WITH is bounded tighter, by CYP_MAX_VARS. Every item a WITH projects + * becomes one variable of the binding that carries the rest of the query, + * and binding_t holds exactly CYP_MAX_VARS variables. A wider WITH used to + * parse, then lose every alias past the 16th in with_add_vbinding_var and + * answer with silently blank columns. Refuse it here, the same way an + * over-wide RETURN is refused, so the caller sees an error instead of a + * short or empty result. */ + if (r->count > (is_with ? CYP_MAX_VARS : CBM_SZ_32)) { free_return_clause(r); return CBM_NOT_FOUND; } @@ -4193,17 +4201,34 @@ static void execute_with_clause(cbm_query_t *q, binding_t **bindings_ptr, int *b /* Project RETURN * — all bound variable properties */ /* Collect all variable names from query patterns */ +/* Has this variable already been collected? A query may name the same variable + * in more than one pattern, and RETURN * must give it one set of columns. */ +static bool star_var_seen(const char **vars, int vc, const char *name) { + for (int i = 0; i < vc; i++) { + if (strcmp(vars[i], name) == 0) { + return true; + } + } + return false; +} + +/* Collect the variables a RETURN * projects, in the order the query names them + * and with no repeats. Without the repeat check, `MATCH (f) OPTIONAL MATCH + * (f)-[:CALLS]->(g)` names f in two patterns and f gets its four columns + * twice. */ static int collect_pattern_vars(cbm_query_t *q, const char **vars, int max_vars) { int vc = 0; for (int pi = 0; pi < q->pattern_count; pi++) { for (int ni = 0; ni < q->patterns[pi].node_count && vc < max_vars; ni++) { - if (q->patterns[pi].nodes[ni].variable) { - vars[vc++] = q->patterns[pi].nodes[ni].variable; + const char *var = q->patterns[pi].nodes[ni].variable; + if (var && !star_var_seen(vars, vc, var)) { + vars[vc++] = var; } } for (int ri = 0; ri < q->patterns[pi].rel_count && vc < max_vars; ri++) { - if (q->patterns[pi].rels[ri].variable) { - vars[vc++] = q->patterns[pi].rels[ri].variable; + const char *var = q->patterns[pi].rels[ri].variable; + if (var && !star_var_seen(vars, vc, var)) { + vars[vc++] = var; } } } @@ -4255,8 +4280,43 @@ static void project_star_row(binding_t *b, const char **vars, int vc, const char } } +/* RETURN * after a WITH. + * + * The pattern's variables are out of scope by this point — the WITH replaced + * them with the names it made. Each of those names holds one value, not a + * node, so each is ONE column rather than the four a node variable gets. + * + * Reading the pattern here instead is the fault this function exists to avoid: + * it named variables the bindings no longer hold, found nothing for every one + * of them, and answered a full result of empty strings with no error. */ +static void execute_return_star_after_with(cbm_query_t *q, binding_t *bindings, int bind_count, + int max_rows, result_builder_t *rb) { + cbm_return_clause_t *wc = q->with_clause; + char name_bufs[CYP_MAX_VARS][CBM_SZ_128]; + const char *cols[CYP_MAX_VARS]; + /* parse_return_or_with refuses a WITH wider than CYP_MAX_VARS, so this + * clamp cannot fire. It stays as the bound this function relies on. */ + int col_n = wc->count < CYP_MAX_VARS ? wc->count : CYP_MAX_VARS; + for (int i = 0; i < col_n; i++) { + cols[i] = resolve_item_alias(&wc->items[i], name_bufs[i], sizeof(name_bufs[i])); + } + rb_set_columns(rb, cols, col_n); + for (int bi = 0; bi < bind_count && rb->row_count < max_rows; bi++) { + const char *vals[CYP_MAX_VARS]; + for (int i = 0; i < col_n; i++) { + cbm_node_t *vn = binding_get(&bindings[bi], cols[i]); + vals[i] = vn && vn->name ? vn->name : ""; + } + rb_add_row(rb, vals); + } +} + static void execute_return_star(cbm_query_t *q, binding_t *bindings, int bind_count, int max_rows, result_builder_t *rb) { + if (q->with_clause) { + execute_return_star_after_with(q, bindings, bind_count, max_rows, rb); + return; + } const char *vars[CBM_SZ_32]; int vc = collect_pattern_vars(q, vars, CBM_SZ_32); build_star_columns(rb, vars, vc); diff --git a/tests/test_cypher.c b/tests/test_cypher.c index b674da15d..bf4770c02 100644 --- a/tests/test_cypher.c +++ b/tests/test_cypher.c @@ -2982,6 +2982,89 @@ TEST(cypher_exec_return_star) { PASS(); } +TEST(cypher_return_star_dedups_repeated_pattern_var) { + /* RETURN * collected its column variables from every pattern in turn and + * never deduped, so a variable named in two patterns got its four columns + * twice. Here f is named in the MATCH and again in the OPTIONAL MATCH, so + * eight columns is right and twelve is the fault. */ + cbm_store_t *s = setup_cypher_store(); + cbm_cypher_result_t r = {0}; + int rc = cbm_cypher_execute(s, "MATCH (f:Function) OPTIONAL MATCH (f)-[:CALLS]->(g) RETURN *", + "test", 0, &r); + ASSERT_EQ(rc, 0); + ASSERT_EQ(r.col_count, 8); + ASSERT_STR_EQ(r.columns[0], "f.name"); + ASSERT_STR_EQ(r.columns[4], "g.name"); + cbm_cypher_result_free(&r); + cbm_store_close(s); + PASS(); +} + +TEST(cypher_return_star_after_with_names_aliases) { + /* RETURN * built its columns from the query pattern, never from the + * bindings it was about to project. After a WITH the live scope is the + * aliases the WITH made, so the old code asked for f and g, found neither, + * and answered every value empty with no error. */ + cbm_store_t *s = setup_cypher_store(); + cbm_cypher_result_t r = {0}; + int rc = cbm_cypher_execute(s, + "MATCH (f:Function)-[:CALLS]->(g) " + "WITH f.name AS caller, g.name AS callee RETURN *", + "test", 0, &r); + ASSERT_EQ(rc, 0); + ASSERT_EQ(r.col_count, 2); + ASSERT_STR_EQ(r.columns[0], "caller"); + ASSERT_STR_EQ(r.columns[1], "callee"); + /* Three CALLS edges in the fixture. */ + ASSERT_EQ(r.row_count, 3); + for (int i = 0; i < r.row_count; i++) { + ASSERT_TRUE(r.rows[i][0][0] != '\0'); + ASSERT_TRUE(r.rows[i][1][0] != '\0'); + } + cbm_cypher_result_free(&r); + cbm_store_close(s); + PASS(); +} + +TEST(cypher_wide_with_refused_not_truncated) { + /* Every item a WITH projects becomes one variable of the binding that + * carries the rest of the query, and a binding holds CYP_MAX_VARS (16) of + * them. A 20-alias WITH used to parse, drop aliases 17 to 20 inside + * with_add_vbinding_var, and answer RETURN * with 16 columns and no error — + * a short result the caller could not tell from a complete one. It has to + * be refused at parse time instead. */ + char query[1024]; + int off = snprintf(query, sizeof(query), "MATCH (f:Function) WITH "); + for (int i = 0; i < 20; i++) { /* 20 > CYP_MAX_VARS (16) */ + off += snprintf(query + off, sizeof(query) - (size_t)off, "%sf.name AS c%d", i ? ", " : "", + i); + } + snprintf(query + off, sizeof(query) - (size_t)off, " RETURN *"); + + cbm_store_t *s = setup_cypher_store(); + cbm_cypher_result_t r = {0}; + int rc = cbm_cypher_execute(s, query, "test", 0, &r); + ASSERT_TRUE(rc != 0); /* refused, not silently narrowed to 16 columns */ + cbm_cypher_result_free(&r); + + /* The width just under the bound still works, so the guard rejects only + * what the binding genuinely cannot carry. */ + char ok_query[1024]; + off = snprintf(ok_query, sizeof(ok_query), "MATCH (f:Function) WITH "); + for (int i = 0; i < 16; i++) { + off += snprintf(ok_query + off, sizeof(ok_query) - (size_t)off, "%sf.name AS c%d", + i ? ", " : "", i); + } + snprintf(ok_query + off, sizeof(ok_query) - (size_t)off, " RETURN *"); + cbm_cypher_result_t r16 = {0}; + ASSERT_EQ(cbm_cypher_execute(s, ok_query, "test", 0, &r16), 0); + ASSERT_EQ(r16.col_count, 16); + cbm_cypher_result_free(&r16); + + cbm_store_close(s); + PASS(); +} + TEST(cypher_parse_neq) { cbm_query_t *q = NULL; char *err = NULL; @@ -4290,6 +4373,9 @@ SUITE(cypher) { RUN_TEST(cypher_exec_where_is_null); RUN_TEST(cypher_exec_where_is_not_null); RUN_TEST(cypher_exec_return_star); + RUN_TEST(cypher_return_star_dedups_repeated_pattern_var); + RUN_TEST(cypher_return_star_after_with_names_aliases); + RUN_TEST(cypher_wide_with_refused_not_truncated); RUN_TEST(cypher_parse_neq); RUN_TEST(cypher_parse_in); RUN_TEST(cypher_parse_is_null);