[MazterQyou]

Check List

• Tests have been run in packages where changes have been made if available
• Linter has been run for changed code
• Tests for the changes have been added if not covered yet
• Docs have been added / updated if required

Description of Changes Made

This PR adds :name parameters to the SQL API REST endpoint: /v1/cubesql accepts params, whose values are parsed as SQL expressions and replace their placeholders in the query, and the new /v1/cubesql/params endpoint lists a query's parameters with the Cube type (number, string, time, boolean or unknown) each is expected to have.

Tests: cubesql unit tests for binding and type inference, a native execSql/describeSqlParams test, and API gateway tests for the new field and endpoint.

[mintlify]

Preview deployment for your docs. Learn more about Mintlify Previews.

Project	Status	Preview	Updated
cubed3	🟢 Ready	View Preview	Nov 5, 2026, 1:44 PM

💡 Tip: Enable Automations to automatically generate PRs for you.

[claude]

Claude finished @MazterQyou's task in 1m 50s —— View job

---

Review of #12612: looks good, 1 issue found (1 medium)

Details

• Gather context
• Review Rust parameter binding (parameters.rs)
• Review native bindings and API gateway
• Review docs and tests
• Resolve stale threads / post findings

Findings

• Medium: packages/cubejs-backend-native/src/node_export.rs:693. When a value fails to parse, redactedError keeps the raw parameter value. The sqlparser error quotes the bad token (for example found: 'secret'), and bound_sql_query returns None in that case, so nothing removes it. Details are in the inline comment.

Notes (no action needed)

• Security: treating values as SQL expressions doesn't give the caller any new power. They already control the whole query and need the data scope. Values can't contain subqueries or placeholders, and the total size is capped (MAX_BOUND_PARAMETERS_SIZE).
• Type inference: TypeInferrer::infer covers every DFExpr variant, so a new DataFusion variant will fail to compile instead of being skipped silently. The tests cover LIMIT/OFFSET, BETWEEN, IN, CASE, arithmetic with intervals, function signatures and casts.
• Behaviour change: a /v1/cubesql query that already contained :name without a matching param now fails with Missing value for parameter :name. That is expected.
• Docs: the new params field and the /v1/cubesql/params endpoint are documented in reference.mdx, and the scope table in index.mdx is updated. The endpoint isn't in core-data.yaml, but /v1/sql-filters isn't there either, so I didn't flag it.
• Comments: all within 3 lines, and each explains why rather than restating the code.
• Threads: the listing returned none of my own threads, so there was nothing to resolve or re-affirm. The one open thread is a CodeQL alert at gateway.ts:687, which I left alone.

[MazterQyou]

/bot-review

[codecov]

Codecov Report

❌ Patch coverage is 87.47885% with 74 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.17%. Comparing base (9a7337c) to head (f97d501).
⚠️ Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
rust/cubesql/cubesql/src/compile/parameters.rs	87.80%	70 Missing ⚠️
packages/cubejs-api-gateway/src/gateway.ts	85.71%	2 Missing ⚠️
packages/cubejs-api-gateway/src/sql-server.ts	0.00%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           master   #12612       +/-   ##
===========================================
+ Coverage   66.22%   83.17%   +16.95%     
===========================================
  Files         258      532      +274     
  Lines       22023   125365   +103342     
  Branches     5308     5311        +3     
===========================================
+ Hits        14585   104275    +89690     
- Misses       6640    20292    +13652     
  Partials      798      798               

Flag	Coverage Δ
cube-backend	66.23% <76.47%> (+<0.01%)	⬆️
cubesql	86.79% <87.80%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[claude]

Claude finished @MazterQyou's task in 3m 7s —— View job

---

Review of #12612: looks good. 0 high, 0 medium, 1 low, plus 1 prior thread still open.

Details

• Read diff and changed files
• Analyze code quality, security, performance, tests, docs, comments
• Reconcile prior review threads
• Post findings

Findings

• Low: packages/cubejs-backend-native/src/node_export.rs:356. The query logs (Load Request / Success / error) record only the SQL before binding (span_id.query_key at node_export.rs:365), so requests that differ only in params produce identical log entries. Posted inline.

Checked and no issue found

• Security: each parameter value is parsed as a single expression, and any subquery or placeholder inside it is rejected (parameters.rs:52-68, :129). It can't add statements or FROM clauses, and the caller already controls the whole query, so binding gives them nothing new. Total bound size is capped at 1 MiB. /v1/cubesql/params requires both the data and meta scopes. The CodeQL "missing rate limiting" alert applies equally to every existing route and isn't specific to this PR.
• Correctness:
  • Placeholders in LIMIT/OFFSET/FETCH work in both binding and describing.
  • DATE :d is turned into a CAST, and a placeholder left where no expression can stand is reported.
  • A parameter value that is an operator expression is wrapped in parentheses, so precedence is kept when it is printed.
  • bound_sql_query re-parses the query only on the error path, and only when redaction is on.
• Performance: with no params, bind_parameters costs one AST walk per query, which is negligible.
• Tests: binding and type inference have Rust unit tests. The native execSql/describeSqlParams path is tested end to end against the mocked load. The gateway tests cover the new field, the 400 on non-string values and the new endpoint.
• Docs: params is documented in reference.mdx and core-data.yaml. The new endpoint has its own section in reference.mdx and is in the scope table in index.mdx. core-data.yaml only lists /v1/load, /v1/cubesql and /v1/meta, so the new endpoint isn't missing from it. The docs requirement is met.
• Comments: all within 3 lines, and each explains why the code is there.

Review threads

The listing returned 1 of my own threads. I resolved 0 and re-affirmed 1:

• node_export.rs:698: when binding fails, redactedError can contain the parameter value, because the parser error quotes it. You replied that statement parse errors already leak this way and want a separate fix. That moves the fix out of this PR but doesn't make the concern go away, so the thread stays open for you to resolve if you accept the deferral.

[MazterQyou]

/bot-review

[claude]

Claude finished @MazterQyou's task in 2m 37s —— View job

---

LGTM: 0 high, 0 medium, 1 low. The only gap is a missing scope test for /v1/cubesql/params.

Review details

• Gather context and read the diff
• Review Rust parameter binding / type inference
• Review native bindings and API gateway
• Check docs and tests
• Reconcile prior review threads
• Post findings

Findings

Severity	Location	Issue
Low	packages/cubejs-api-gateway/test/index.test.ts:1792	No test checks that /v1/cubesql/params returns 403 when the token lacks either the data or the meta scope (gateway.ts:692-693).

Checked, no issue

• Binding (rust/cubesql/cubesql/src/compile/parameters.rs:148):
  • Each value is parsed as exactly one expression, and anything after it fails the expect_token(EOF) check, so 1; DROP TABLE t is rejected.
  • ValueValidator rejects subqueries and nested placeholders.
  • Placeholders in positions that aren't expressions, such as ESCAPE, are rejected by check_no_leftover_placeholders.
  • The 1 MiB total bounds how much repeated placeholders can grow the statement.
  • Values are spliced as AST nodes, never as strings, so there is no textual injection. Their identifiers and functions get the same planning and access checks as the rest of the query.
• Describe (parameters.rs:626):
  • Uses a stand-in UDF of type Null, so planning accepts the placeholders and types are read from their surroundings. Each name gets the first type found, with LIMIT/OFFSET/FETCH fixed from the syntax.
  • Planner messages show :name instead of the stand-in call.
  • Planning runs even when every type is already known, so an invalid query is always reported.
• Error classes (packages/cubejs-backend-native/src/describe_sql_params.rs:29): user-caused errors return { error }, which the gateway turns into a 400. Only Internal errors are thrown and become a 500.
• Docs: reference.mdx documents params and the new endpoint, and index.mdx lists its scopes. The OpenAPI spec covers only three endpoints, so leaving /v1/cubesql/params out of it matches the file. The docs requirement is met.
• Comments: all within 3 lines and load-bearing.
• Earlier human threads: the author's replies on redaction (node_export.rs:698) and CodeQL rate limiting (gateway.ts:687) are reasonable, and I'm not reopening them.

Review threads

The listing returned 1 of my own threads; I resolved 1 and re-affirmed 0. The resolved thread, on node_export.rs:356, asked for parameter values in query logs. The author explained that leaving them out is deliberate, so I withdrew it with a reply. I posted 1 new inline comment.

[claude]

Claude finished @MazterQyou's task in 2m 52s —— View job

---

PR #12612 review: approve once the scope comment is resolved. 0 high, 0 medium, 1 low.

Details

• Gather context (diff, changed files, PR conversation)
• Review Rust binding / type inference (parameters.rs)
• Review native + API gateway changes
• Check docs and tests
• Reconcile prior review threads
• Post findings

Findings

Severity	Location	Issue
Low	packages/cubejs-api-gateway/src/gateway.ts:692	The comment says running the query needs data and meta, but /v1/cubesql only checks data. A token with only data can run a query with params but gets a 403 from /v1/cubesql/params for the same query. Either drop the meta check or say in the comment why it's required.

What I checked

• Binding (parameters.rs):

  • Each value is parsed as exactly one expression.
  • Subqueries and placeholders inside values are rejected.
  • Compound values are wrapped in Nested, so they stay one operand when the statement is printed.
  • The total size cap counts every time a value is substituted, including repeats.
  • DATE :d is turned into CAST, and placeholders that can't be replaced (e.g. after ESCAPE) produce an error.
  • A value like 1; DROP TABLE t is rejected because the parser expects end of input. Values can't do anything the caller couldn't already write in the query.

• Type inference:

  • Planning uses a stand-in function of type Null, and placeholders after LIMIT/OFFSET/FETCH are always reported as numbers.
  • Covered and tested: comparisons, arithmetic with intervals, CASE, IN lists and subqueries, function signatures.
  • Planning errors show :name instead of the stand-in call.
  • This only runs DataFusion's initial planning, not the rewrite to a Cube query. So a query can pass /v1/cubesql/params and still fail when run. That seems an acceptable trade-off for a describe endpoint.

• Native/gateway:

  • params is checked by Joi in the gateway, then parsed in Rust as a HashMap<String, String>.
  • Internal errors from describeSqlParams are thrown and reach the error handler as a 500. All other errors come back as { error }, which the gateway answers with a 400.

• Points the author has already answered:

  • The rate-limit alert from CodeQL.
  • Values in parser errors not being redacted.
  • The query being logged before binding.

  I agree with the reasoning in each case and didn't raise them again.

• Docs: params is documented in reference.mdx and core-data.yaml, and the new endpoint has its own section plus a row in the scope table. The docs requirement is met.

• Tests:

  • Covered: Rust unit tests for binding errors and type inference, a native execSql/describeSqlParams round trip, and gateway tests for the new field, the new endpoint and both scopes.
  • Not covered: placeholders inside a CTE or subquery, and the documented case sensitivity (:Status vs status).

Review threads

The listing returned 1 thread of mine. I resolved it: it asked for tests of the data/meta scopes, and permissions.test.ts:146 now has them. None were re-affirmed.

I didn't run builds or tests in this round; the review is from reading the code.

[MazterQyou]

/bot-review

[claude]

Claude finished @MazterQyou's task in 2m 23s —— View job

---

Looks good to merge: 0 high, 0 medium, 1 low (docs warning).

Review details

• Read diff and changed files
• Review cubesql parameter binding and type inference
• Review API gateway and native bridge
• Check docs
• Review threads
• Post findings

Findings

Severity	Location	Issue
Low	docs-mintlify/reference/core-data-apis/rest-api/reference.mdx:590	The docs don't warn that params values are pasted into the query as SQL with no escaping. An app that passes end-user input straight through lets that user send an expression instead of a literal (e.g. status for :status makes the filter true for every row). Access policies still apply. Suggest a <Warning> to send untrusted input as a quoted string literal.

What I checked and found no problems with

• Binding (rust/cubesql/cubesql/src/compile/parameters.rs:148):
  • Each value is parsed as exactly one expression (it must end at EOF). Subqueries and placeholders inside a value are rejected.
  • Compound values are wrapped in parentheses, so precedence can't leak into the surrounding query.
  • Typed placeholders such as DATE :d become CASTs.
  • A placeholder left where no expression can go (ESCAPE :e) is an error instead of being silently dropped.
  • Total value size is capped per occurrence, so repeating one placeholder can't blow up the statement.
• Type inference: placeholders are planned as a Null-typed stand-in function (UDF), and each type is read from the planned expression around it. Error messages put :name back in place of the stand-in call.
• Gateway:
  • /v1/cubesql/params checks data + meta up front. That matches what /v1/cubesql ends up needing at planning, via ApiGateway.meta().
  • Joi rejects values that aren't strings. The test covers this (400).
• Native bridge: the params argument at index 9 matches the JS side. Error redaction also runs against the statement with parameters bound.
• Already answered by the author, not raised again: missing rate limiting (CodeQL), logging the query before binding, and parser errors quoting values.
• Docs: the params field, the new endpoint and its scope requirements are documented in rest-api/reference.mdx, rest-api/index.mdx and core-data.yaml.
• Tests: Rust unit tests cover binding and inference, a native execSql/describeSqlParams test checks the filters and limit the load receives, and the gateway tests cover routing and scopes.

Review threads

The listing returned 1 earlier thread of mine and I resolved it. That thread said /v1/cubesql/params shouldn't require meta. I withdrew it: the author showed /v1/cubesql needs meta too, and I replied on the thread to say so. None were re-affirmed. 1 new inline comment posted.

[claude]

Claude finished @MazterQyou's task in 2m 8s —— View job

---

LGTM: no new issues (0 high, 0 medium, 0 low)

Review details

• Gather context
• Read the diff and changed files
• Check docs coverage
• Handle review threads
• Post findings

Review threads

The listing returned 1 of my own threads. I resolved 1 (the params SQL-expression warning in reference.mdx, now covered by the <Warning> block). None were re-affirmed.

Notes

• Docs: params on /v1/cubesql and the new /v1/cubesql/params endpoint are documented in core-data.yaml, rest-api/reference.mdx and rest-api/index.mdx. The scope table matches the gateway's data + meta checks (gateway.ts:692-693).
• Binding (parameters.rs:148): values are parsed as standalone expressions and rejected if they contain a subquery or a placeholder. Non-trivial values are wrapped in Nested, so the printed statement keeps precedence ((SUM(x) * 2) + 1). Any placeholder left in a position that isn't an expression (ESCAPE :e) is rejected rather than silently passed through. The 1 MiB cap counts every occurrence, which covers a parameter repeated many times.
• Security: values are SQL by design, and the caller can already send arbitrary SQL in query, so params gives no new capability. The docs warning covers the case where a client splices untrusted input into a value. The decisions to leave the query key and span unbound and to defer redacting parser-quoted literals are already explained in the earlier threads. I agree with both.
• Describe path (parameters.rs:626): it plans through SqlToRel directly instead of QueryRouter::plan. Non-SELECT statements (SET, SHOW) therefore come back as a 400 with "Initial planning error" rather than being routed. That's acceptable for an endpoint meant to describe parameters.
• Error mapping (describe_sql_params.rs:29-35): user and planning errors return { error }, which the gateway turns into a 400. Internal errors are thrown and end up as a 500. The API gateway tests cover both.
• Tests: the Rust unit tests cover binding (including the error cases), type inference across operators, functions, CASE, IN, BETWEEN, interval arithmetic and LIMIT/OFFSET, plus a planning error. The native test checks that bound values reach the sqlApiLoad filters and limit. The gateway tests cover Joi validation of non-string values and the scope 403s.

[claude]

Claude finished @MazterQyou's task in 1m 57s —— View job

---

LGTM: no blocking issues (0 high, 0 medium, 0 low)

Review details

• Gather context (diff, changed files, PR conversation)
• Review cubesql parameters.rs
• Review native bindings and API gateway
• Check docs and tests
• Handle prior review threads
• Post findings

What I checked

• Binding (rust/cubesql/cubesql/src/compile/parameters.rs:148): each value is parsed as exactly one expression, with EOF enforced so 1; DROP TABLE t is rejected. Subqueries and placeholders are rejected inside values. Non-trivial values are wrapped in Nested, so :x::numeric with 1+1 prints as (1+1)::numeric and the value can't rebind to a neighbouring operator. DATE :d becomes a CAST. A placeholder in a position that can't hold an expression (ESCAPE :e) fails explicitly. The 1 MiB cap counts every use of a value, so a value repeated many times can't grow the statement past it. Values are spliced before planning, so security context and row-level checks apply to them as they do to the rest of the query.
• Describe (parameters.rs:626): the planner sees each placeholder as a call to a function that returns Null, so it types everything around it. LIMIT/OFFSET/FETCH are typed from the syntax before planning. Planning errors are rewritten to show :name again. Internal errors become 500s and user errors become 400s (describe_sql_params.rs:30).
• Gateway: Joi restricts params to string → string. The data + meta scope check on /v1/cubesql/params matches the author's explanation in the thread and is covered by permissions.test.ts.
• Docs: params, the new endpoint, its scopes and the "values are SQL, not data" warning are documented in docs-mintlify/reference/core-data-apis/rest-api/reference.mdx, index.mdx and core-data.yaml. The docs requirement is met.
• Comments: all are within 3 lines and explain why the code is the way it is, so nothing to flag.

Minor observations (not posted inline)

• PlanTypeInferrer passes Boolean as the expected type only for Filter. A bare JOIN … ON :flag is therefore reported as unknown, not boolean. It's not incorrect, just less precise.
• A placeholder inside a correlated or IN (SELECT …) subquery's own predicates only gets a type if the plan visitor descends into expression subqueries. The tests cover only an outer :g IN (SELECT …). Adding one case would pin this down.
• exec_sql binds after transport_service.meta(...) (node_export.rs:434-438). A bad params value still costs a meta fetch before it's rejected. This is cosmetic.

Prior threads

The listing returned 0 of my own threads, so I had nothing to resolve or re-affirm. No unresolved threads remain on the PR. The author already answered the CodeQL rate-limit alert and the redaction, logging and scope questions in-thread.

I did not run the cargo or Jest suites locally. This review is based on reading the code and tests.