Skip to content

fix(tesseract): align bridge member SQL with JS proxy reference#10850

Merged
waralexrom merged 6 commits into
masterfrom
tesseract-bridge-skip-fixes
May 11, 2026
Merged

fix(tesseract): align bridge member SQL with JS proxy reference#10850
waralexrom merged 6 commits into
masterfrom
tesseract-bridge-skip-fixes

Conversation

@waralexrom
Copy link
Copy Markdown
Member

Summary

Fixes the bridge layer that drives MemberSql.compile_template_sql so it matches contextSymbolsProxyFrom/resolveSymbolsCall in CubeSymbols.ts. Closes the 12 regression cases that were sitting it.skip in the Tesseract bridge harness (recently introduced in #10838) — the bridge sweep now passes 80/80 with zero skips.

Changes

  • args_names parser — replaces the regex (function\s+\w+\(([A-Za-z0-9_,]*)|...) that choked on whitespace inside named arg lists, default values, rest spread, destructuring, and anonymous function expressions. New parser strips the async/function [*] [name] prefix with a tiny regex, then walks (...) with a balanced scanner so cases like (x = f()) are handled correctly. Token normalisation covers defaults (top-level =), rest (...), and {}/[] destructuring via identifier extraction.
  • SECURITY_CONTEXT filter — JS truthy short-circuitundefined / null / \"\" / 0 / NaN / false collapse to a 1 = 1 filter (and trip requiredFilter to throw), instead of being bound as real values. Array params now run through to_array().to_vec() plus a per-element scalar coercion that accepts strings, numbers, and booleans, so number arrays produce the expected IN (...) clause instead of erroring out as "Invalid param for security context". Drops the stray space from the array toString separator (.join(\", \").join(\",\")).
  • SECURITY_CONTEXT placeholders — lazy allocation — the leaf-proxy's toString used to pre-bake to_string_fn(allocated) at construction time, so every property access eagerly registered the value, double-pushing values, starting {sv:N} at 1 instead of 0, and making unsafeValue() look like it allocated a placeholder. Moved the allocation inside a make_vararg_function closure so registration happens only when the proxy is actually coerced via ${...}, matching String(allocateParam(paramValue)) in the JS reference. Each coercion gets its own placeholder.
  • Primitive return coercionconvert_to_string only handled JsString/null and then fell back to a struct-side toString, which errored as "Object is not the Struct object" when a member SQL function returned 42 or true. Added JsNumber/JsBoolean branches so primitives coerce to their JS string form ('42', '1.5', 'true', 'false').
  • f64 saturation — dropped the n as i64 shortcut in four spots; f64::Display already prints integers without a trailing .0 and matches JS String(n) for the cases the bridge cares about, while not saturating above i64::MAX.

waralexrom added 5 commits May 9, 2026 15:29
Replace the buggy regex (`function\s+\w+\(([A-Za-z0-9_,]*)|...`) that
choked on whitespace inside named function parameter lists, default
values, rest spread, destructuring, and anonymous function expressions.
The new parser strips the `async`/`function [*] [name]` prefix with a
small regex and then walks the parameter list with a balanced scanner,
so cases like `(x = f())` are handled correctly. Token normalisation
covers defaults (top-level `=`), rest (`...`), and `{}`/`[]`
destructuring via identifier extraction.

Unskip the previously known-bad cases in the bridge test suite and add
inline unit tests for the new helpers in `cubenativeutils`.
Apply the JS truthy short-circuit from `contextSymbolsProxyFrom` in the
bridge: undefined / null / "" / 0 / NaN / false collapse to a `1 = 1`
filter (and trip `requiredFilter` to throw), instead of being bound as
real values. Array params now go through `to_array().to_vec()` plus a
per-element scalar coercion that accepts strings, numbers, and booleans,
so number arrays produce the expected `IN (...)` clause instead of
erroring out as "Invalid param for security context".

Also drop the stray space from the array `toString` separator
(`.join(", ")` → `.join(",")`) to match the JS reference
(`paramValue.map(allocateParam).join(',')`).
The bridge's `security_context_to_string_fn` used to pre-bake the leaf's
toString output via `to_string_fn(allocated)` — every leaf-proxy
construction (i.e. every property access) eagerly registered the value
in `proxy_state`, even when the user only called `.filter()` or
`.unsafeValue()`. That double-pushed values, started `{sv:N}` at 1
instead of 0, and made `unsafeValue()` look like it allocated a
placeholder when the JS reference clearly does not.

Move the allocation inside a `make_vararg_function` closure so the
placeholder is only registered when the proxy is actually coerced via
`${...}`, matching `String(allocateParam(paramValue))` in
`contextSymbolsProxyFrom`. Each coercion gets its own placeholder, in
line with JS `toString` semantics. Array leaves now also use
`to_array().to_vec()` plus the existing `coerce_scalar_to_string`
helper, so non-string element types (numbers, booleans) work the same
way they do in the `.filter` path.

The bridge tests are reorganised to drop the now-stale "known
divergences from JS reference" block (the divergences are gone),
parametrise the falsy `.filter` / `.requiredFilter` matrix, and pin
two contract details that fall out of the lazy switch: empty-array
direct interpolation renders to `""`, and capturing a leaf proxy in a
local and coercing it twice now produces two distinct placeholders.
The multi-arg assertion is updated to match single-registration.
`convert_to_string` only handled JsString and null and then fell back to
calling `toString` as a struct method, which errored out as "Object is
not the Struct object" when a member SQL function returned a primitive
like `42` or `true`. The JS reference does no coercion at the bridge
boundary — the value flows through `resolveSymbolsCall` verbatim and the
template literal in the consumer does `String(value)`.

Add JsNumber and JsBoolean branches to `convert_to_string` so primitive
returns coerce to their JS string form (`'42'`, `'1.5'`, `'true'`,
`'false'`) and the bridge no longer rejects them. Update the bridge
result-shape tests accordingly: the two cases that pinned the error are
replaced with the JS-equivalent assertions (and broadened to cover both
integer/decimal and true/false).
Four spots — `convert_to_string`, `coerce_scalar_to_string`, and the
two SECURITY_CONTEXT scalar cascades — formatted whole f64 values via
`n as i64`. That saturates above `i64::MAX`, so `9e18` collapsed to
`"9223372036854775807"` instead of `"9000000000000000000"`. Drop the
special case: `f64::Display` already prints integers without a trailing
`.0` and matches JS `String(n)` for the cases the bridge cares about.
The fix collapses the four duplicated formatting blocks back to the
same one-liner, so future tweaks land in one place by default.
@waralexrom waralexrom requested a review from a team as a code owner May 9, 2026 14:33
@github-actions github-actions Bot added rust Pull requests that update Rust code javascript Pull requests that update Javascript code labels May 9, 2026
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 9, 2026

Claude finished @waralexrom's task in 2m 30s —— View job


Code Review: PR #10850 — fix(tesseract): align bridge member SQL with JS proxy reference

  • Gather context and read all changed files
  • Review Rust changes (neon_function.rs, object_handle.rs, member_sql.rs)
  • Review test changes (args-names, multi-arg, result-shape, security-context)
  • Evaluate code quality, security, performance, and test coverage
  • Post final review

Overall Assessment

This is a well-crafted PR that fixes 5 distinct categories of divergence between the Rust bridge and the JS reference implementation. The changes are logically decomposed into clean commits, thoroughly tested, and the PR description is exceptionally detailed. All 12 previously-skipped tests are now active and passing with the correct expected values.

Highlights

args_names parser rewrite — The new balanced-paren scanner + top-level split approach (neon_function.rs) is a significant improvement over the fragile regex. The decomposition into matching_paren, split_top_level, and strip_default is clean, each function has a single responsibility, and the Rust-side #[cfg(test)] unit tests provide solid coverage for edge cases (nested defaults, destructuring, rest params, generator functions).

Lazy placeholder allocation — The shift from eager to_string_fn(allocated) to a make_vararg_function closure in security_context_to_string_fn is the most impactful fix. This eliminates double-registration, starts {sv:N} indices at 0, and ensures unsafeValue() no longer accidentally registers placeholders. The "allocates a fresh placeholder on every coercion of the same leaf proxy" test is a great addition.

JS truthy short-circuit — The security_context_filter_fn now correctly maps all JS falsy values (undefined, null, "", 0, NaN, false) to ParamValue::None, matching if (paramValue) in contextSymbolsProxyFrom. The type-checking order (null/undefined → array → string → number → boolean) is correct and avoids coercion ambiguities.

Primitive return coercion — The convert_to_string fix in object_handle.rs adds NativeNumber/NativeBoolean branches before the struct fallback — simple but essential to prevent "Object is not the Struct object" errors on primitives.

f64 stringification — Dropping the n as i64 cast is correct; f64::Display handles integer formatting cleanly and avoids saturation at i64::MAX.

Issues & Suggestions

See inline comments for specifics. Summary of findings:

Severity Finding
Nit matching_paren / split_top_level don't handle string literals containing brackets — acceptable for V8's toString() output but worth documenting as a known limitation
Nit split_top_level can underflow depth on malformed input (stray closing bracket)
Low convert_to_string checks is_null() but not is_undefined() — could error on undefined handles if callers change
Nit Pre-existing typo secutity (in process_secutity_context_value, SecutityContextProps) — good opportunity to fix since all call sites are already touched
Suggestion Consider adding NaN to the it.each falsy-values test for completeness

Test Coverage

Test coverage is strong — all 12 previously-skipped cases are activated, and several new scenarios are added (numeric arrays, empty array leaf toString, fresh-placeholder-per-coercion, it.each parametric falsy tests). The Rust-side unit tests for the args parser are a nice touch that provides fast feedback without requiring the full native build.

No security concerns identified — the changes correctly maintain the separation between unsafeValue() (raw value, no placeholder) and filter()/toString (placeholder-registered values).

Verdict: Looks good. The nits above are non-blocking. Nice work closing all 12 regression cases with clean, well-structured fixes.

Comment thread rust/cube/cubenativeutils/src/wrappers/neon/object/neon_function.rs
Comment thread rust/cube/cubenativeutils/src/wrappers/neon/object/neon_function.rs
Comment thread rust/cube/cubenativeutils/src/wrappers/object_handle.rs
Comment thread packages/cubejs-backend-native/test/bridge/security-context.test.ts
Comment thread packages/cubejs-backend-native/test/bridge/security-context.test.ts
Pointers to specific identifiers from the legacy JS schema-compiler
(`contextSymbolsProxyFrom`, `allocateParam`, the `String(...)` snippet,
etc.) will rot once that code is fully deprecated. Rewrite the comments
to describe the bridge behavior on its own terms — the WHY (lazy
allocation, falsy short-circuit, primitive coercion vs. struct
fallback) stays without naming the legacy site.
@codecov
Copy link
Copy Markdown

codecov Bot commented May 9, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 78.86%. Comparing base (8d8d2a9) to head (53c1eec).
⚠️ Report is 3 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #10850      +/-   ##
==========================================
+ Coverage   78.84%   78.86%   +0.02%     
==========================================
  Files         470      470              
  Lines       92274    92289      +15     
  Branches     3433     3435       +2     
==========================================
+ Hits        72751    72788      +37     
+ Misses      19020    18999      -21     
+ Partials      503      502       -1     
Flag Coverage Δ
cube-backend 58.36% <ø> (+0.14%) ⬆️
cubesql 83.47% <ø> (+<0.01%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 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.

@waralexrom waralexrom merged commit d5da121 into master May 11, 2026
182 of 186 checks passed
@waralexrom waralexrom deleted the tesseract-bridge-skip-fixes branch May 11, 2026 16:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

javascript Pull requests that update Javascript code rust Pull requests that update Rust code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants