Merge pull request DeusData#1922 from CaptainMittens/fix/reject-out-of-scope-projection

fix(cypher): refuse a name that is not in scope instead of projecting a blank column

Author: DeusData

src/cypher/cypher.c

@@ -4894,6 +4894,164 @@ static int execute_single(cbm_store_t *store, cbm_query_t *q, const char *projec
 
 /* ── Main entry point ─────────────────────────────────────────── */
 
+/* ── Is every name a RETURN or WITH item uses actually in scope? ──
+ *
+ * An unbound variable resolves to NULL and renders as "" on purpose: an
+ * OPTIONAL MATCH target that found no row has to project a blank rather than
+ * drop the row. The cost of that convention is that a name the query NEVER
+ * carried through looks exactly the same on screen. A typo, or a variable a
+ * WITH dropped, then reads as "the graph holds no such data" instead of "your
+ * query named something that is not there" (#1919). Same failure shape as
+ * #373, and the same answer: say so out loud.
+ *
+ * The check runs on the parsed query, not on the run-time bindings, and that
+ * is the whole trick. It asks whether the query DECLARED the name — a
+ * different question from whether a row happened to bind it. So the OPTIONAL
+ * MATCH convention above is untouched: a target that did not match is still
+ * declared, still legal, and still projects "".
+ */
+
+/* Cap for one query's declared names. CYP_MAX_VARS is the binding cap for
+ * nodes alone; a pattern also names edges, so this is the roomier of the two.
+ * A query that overruns it skips the check rather than guessing — a wrong
+ * refusal costs the caller a working query, which is worse than the silence
+ * this guard removes. */
+enum { CYP_SCOPE_MAX_NAMES = 32 };
+
+static bool scope_holds(const char *const *names, int count, const char *want) {
+    for (int i = 0; i < count; i++) {
+        if (names[i] && strcmp(names[i], want) == 0) {
+            return true;
+        }
+    }
+    return false;
+}
+
+/* Every name the query's patterns declare, plus an UNWIND alias.
+ * Answers -1 when there are more names than the cap holds. */
+static int collect_declared_names(const cbm_query_t *q, const char **out, int cap) {
+    int n = 0;
+    for (int pi = 0; pi < q->pattern_count; pi++) {
+        const cbm_pattern_t *pat = &q->patterns[pi];
+        for (int ni = 0; ni < pat->node_count; ni++) {
+            const char *var = pat->nodes[ni].variable;
+            if (var && !scope_holds(out, n, var)) {
+                if (n >= cap) {
+                    return -1;
+                }
+                out[n++] = var;
+            }
+        }
+        for (int ri = 0; ri < pat->rel_count; ri++) {
+            const char *var = pat->rels[ri].variable;
+            if (var && !scope_holds(out, n, var)) {
+                if (n >= cap) {
+                    return -1;
+                }
+                out[n++] = var;
+            }
+        }
+    }
+    if (q->unwind_alias && !scope_holds(out, n, q->unwind_alias)) {
+        if (n >= cap) {
+            return -1;
+        }
+        out[n++] = q->unwind_alias;
+    }
+    return n;
+}
+
+/* What a WITH leaves behind: its alias where it made one, and the plain
+ * variable where it carried one through whole. `WITH f.name AS caller` leaves
+ * `caller` and nothing else — `f` is gone, which is the case #1919 is about. */
+static int collect_with_names(const cbm_return_clause_t *wc, const char **out, int cap) {
+    int n = 0;
+    for (int i = 0; i < wc->count; i++) {
+        const cbm_return_item_t *item = &wc->items[i];
+        const char *name = NULL;
+        if (item->alias) {
+            name = item->alias;
+        } else if (item->variable && !item->property && !item->func) {
+            name = item->variable;
+        }
+        if (name && !scope_holds(out, n, name)) {
+            if (n >= cap) {
+                return -1;
+            }
+            out[n++] = name;
+        }
+    }
+    return n;
+}
+
+/* Build the message. It names the variable and the clause, because an error
+ * that does not say WHICH name is wrong sends the reader back to guessing. */
+static char *scope_error(const char *var, const char *clause, const char *why) {
+    char buf[CBM_SZ_256];
+    snprintf(buf, sizeof(buf), "variable '%s' is not in scope for %s — %s", var, clause, why);
+    return heap_strdup(buf);
+}
+
+/* The variable this item really references, or NULL when it references none.
+ * Two items carry a placeholder in that field rather than a name the query
+ * declared: `count(*)` stores "*", and a CASE expression stores "CASE". Both
+ * would read as an unknown variable, so neither is checkable here. */
+static const char *scope_checkable_var(const cbm_return_item_t *item) {
+    if (item->kase || !item->variable) {
+        return NULL;
+    }
+    if (strcmp(item->variable, "*") == 0) {
+        return NULL;
+    }
+    return item->variable;
+}
+
+/* Answers NULL when the query is fine, or a heap message naming the first
+ * variable that is not in scope. Checks one query; the caller walks a UNION. */
+static char *check_projection_scope(const cbm_query_t *q) {
+    const char *declared[CYP_SCOPE_MAX_NAMES];
+    int declared_n = collect_declared_names(q, declared, CYP_SCOPE_MAX_NAMES);
+    if (declared_n < 0) {
+        return NULL; /* too many names to model — stay quiet rather than guess */
+    }
+
+    /* A WITH still reads the pattern variables. */
+    if (q->with_clause && !q->with_clause->star) {
+        for (int i = 0; i < q->with_clause->count; i++) {
+            const char *var = scope_checkable_var(&q->with_clause->items[i]);
+            if (var && !scope_holds(declared, declared_n, var)) {
+                return scope_error(var, "WITH", "no pattern in this query names it");
+            }
+        }
+    }
+
+    if (!q->ret || q->ret->star) {
+        return NULL;
+    }
+
+    /* A RETURN after a WITH reads only what the WITH left behind. */
+    const char *after_with[CYP_SCOPE_MAX_NAMES];
+    const char *const *scope = declared;
+    int scope_n = declared_n;
+    if (q->with_clause) {
+        scope_n = collect_with_names(q->with_clause, after_with, CYP_SCOPE_MAX_NAMES);
+        if (scope_n < 0) {
+            return NULL;
+        }
+        scope = after_with;
+    }
+
+    for (int i = 0; i < q->ret->count; i++) {
+        const char *var = scope_checkable_var(&q->ret->items[i]);
+        if (var && !scope_holds(scope, scope_n, var)) {
+            return scope_error(var, "RETURN",
+                               q->with_clause ? "the WITH clause did not carry it through"
+                                              : "no pattern in this query names it");
+        }
+    }
+    return NULL;
+}
+
 int cbm_cypher_execute(cbm_store_t *store, const char *query, const char *project, int max_rows,
                        cbm_cypher_result_t *out) {
     memset(out, 0, sizeof(*out));
@@ -4911,6 +5069,15 @@ int cbm_cypher_execute(cbm_store_t *store, const char *query, const char *projec
         return CBM_NOT_FOUND;
     }
 
+    for (const cbm_query_t *sq = q; sq; sq = sq->union_next) {
+        char *scope_err = check_projection_scope(sq);
+        if (scope_err) {
+            cbm_query_free(q);
+            out->error = scope_err;
+            return CBM_NOT_FOUND;
+        }
+    }
+
     result_builder_t rb = {0};
     if (execute_single(store, q, project, max_rows, &rb) < 0) {
         rb_free(&rb);

tests/test_cypher.c

@@ -2031,6 +2031,64 @@ TEST(cypher_exec_count_distinct_issue239) {
  * function like split(...) or list indexing [..]) must FAIL LOUDLY with a clear
  * "unsupported function" error rather than silently projecting an empty column
  * (which looks like a valid-but-blank result and hides the real problem). */
+/* issue #1919: a variable the WITH clause dropped must be REFUSED, not projected
+ * as an empty column. `g` does not survive `WITH f.name AS caller`, so naming it
+ * afterwards is a query fault. The old code answered NULL for the unbound name,
+ * rendered it "", and exited clean — a column of nothing that reads as "the graph
+ * holds no such data" rather than "your query named something out of scope".
+ * Same principle as #373: fail loudly instead of projecting a blank column. */
+TEST(cypher_rejects_projection_of_dropped_with_var_issue1919) {
+    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 RETURN caller, g.name", "test",
+        0, &r);
+    ASSERT_TRUE(rc != 0);
+    ASSERT_NOT_NULL(r.error);
+    /* The message has to name the offending variable — an error that does not
+     * say which name is wrong sends the reader back to guessing. */
+    ASSERT_TRUE(strstr(r.error, "g") != NULL);
+    ASSERT_TRUE(strstr(r.error, "scope") != NULL);
+    cbm_cypher_result_free(&r);
+    cbm_store_close(s);
+    PASS();
+}
+
+/* The guard must NOT reject an OPTIONAL MATCH target that simply did not match.
+ * `g` is a declared pattern variable there, so it stays in scope; it is merely
+ * unbound at run time, and projecting "" for it is the documented convention.
+ * This is the line between the two cases, and the reason the check reads the
+ * query's declared variables rather than the run-time bindings. */
+TEST(cypher_optional_match_target_still_allowed_issue1919) {
+    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 f.name, g.name", "test", 0,
+        &r);
+    ASSERT_EQ(rc, 0);
+    ASSERT_NULL(r.error);
+    ASSERT_TRUE(r.row_count > 0);
+    cbm_cypher_result_free(&r);
+    cbm_store_close(s);
+    PASS();
+}
+
+/* An alias the WITH created is in scope afterwards, and a name carried through
+ * unchanged is too. Both must keep working. */
+TEST(cypher_with_alias_stays_in_scope_issue1919) {
+    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 AS callee RETURN caller, callee.name",
+        "test", 0, &r);
+    ASSERT_EQ(rc, 0);
+    ASSERT_NULL(r.error);
+    ASSERT_TRUE(r.row_count > 0);
+    cbm_cypher_result_free(&r);
+    cbm_store_close(s);
+    PASS();
+}
+
 TEST(cypher_exec_unsupported_func_errors_issue373) {
     cbm_store_t *s = setup_cypher_store();
 
@@ -4126,6 +4184,9 @@ SUITE(cypher) {
     RUN_TEST(cypher_exec_label_alternation_issue242);
     RUN_TEST(cypher_exec_count_distinct_issue239);
     RUN_TEST(cypher_exec_unsupported_func_errors_issue373);
+    RUN_TEST(cypher_rejects_projection_of_dropped_with_var_issue1919);
+    RUN_TEST(cypher_optional_match_target_still_allowed_issue1919);
+    RUN_TEST(cypher_with_alias_stays_in_scope_issue1919);
     RUN_TEST(cypher_exec_unknown_func_return_errors);
     RUN_TEST(cypher_exec_inline_props);
     RUN_TEST(cypher_parse_where_starts_with);