Skip to content

fix(SPD-23149): nil-safe bracket access in scope.js + nil-safe filters for sd_order_details sd_unit crash - #46

Open
sd-gh-bot wants to merge 1 commit into
masterfrom
fix/sd-order-details-nil-safe-scope-filters
Open

fix(SPD-23149): nil-safe bracket access in scope.js + nil-safe filters for sd_order_details sd_unit crash#46
sd-gh-bot wants to merge 1 commit into
masterfrom
fix/sd-order-details-nil-safe-scope-filters

Conversation

@sd-gh-bot

@sd-gh-bot sd-gh-bot commented Jun 16, 2026

Copy link
Copy Markdown

User description

Problem

Customer: Wingify/VWO (WS 3409, prod-india/IN cluster)
Workflow: 11250 — Order Form (Order Details dynamic table)
Symptom: sd_unit computed column crashes at document generation time, blocking all Order Form sends.

Error in cfexporter logs:

Dynamic Table sd_order_details computation failed for sd_unit key:
RenderError: Cannot read properties of undefined (reading 'length'), line:2

Root Causes Fixed

Bug 1 — src/scope.js propertyAccessSeq nil bracket-variable crash

When a computed formula uses bracket notation unit_mapping[self.sd_product] and self.sd_product resolves to undefined (e.g. the row is missing the field), the inner push() function in propertyAccessSeq calls name.length where name is undefined → TypeError crash.

Fix: coerce nil name to '' before the length check so undefined bracket keys are safely skipped.

Before:

function push () {
  if (name.length) seq.push(name)
  name = ''
}

After:

function push () {
  name = (name == null) ? '' : name  // nil-safety: bracket accessor can return undefined
  if (name.length) seq.push(name)
  name = ''
}

Bug 2 — filters.js nil-unsafe downcase, first, last, size, join

When any of these filters receive undefined (from a nil Liquid variable), they crash.

Fix: guard each filter to return safe empty value for nil input.

Tests

11 regression tests added:

  • test/scope.js — 2 tests: nil bracket-variable safety
  • test/filters.js — 5 tests: nil-safe guard for each affected filter
  • test/sd-custom/computeColumn-nil-safe.js — 4 end-to-end tests reproducing the exact Wingify crash scenario

Results: 863 passing (baseline was 852), 23 failing (all pre-existing, unchanged)

Template Fix Still Required (CSM action)

This PR fixes the engine crash. Workflow 11250 also needs its computed column formulas wrapped in computeColumn blocks so self is properly bound per-row (sd_unit, sd_ap, sd_cp).

Closes: Rootly Incident #3402 / SPD-23149


Generated description

Below is a concise technical summary of the changes proposed in this PR:
Prevent computed column execution from crashing by coalescing undefined bracket keys in scope.js's propertyAccessSeq and by shoring up nil-safe behavior for the filters used in dynamic tables. Exercise the computeColumn flows and filter expectations with regression tests that mirror the Wingify/VWO sd_unit failure.

TopicDetails
Nil-safe Filters Guard downcase, size, first, last, and join filters to return safe defaults for nil inputs and validate the behavior with Liquid render tests.
Modified files (2)
  • filters.js
  • test/filters.js
Latest Contributors(2)
UserCommitDate
neo@spotdraft.comfix(SPD-23149): nil-sa...June 16, 2026
parth-sd[SPD-23149] Add filter...July 05, 2024
Nil-safe Brackets Harden propertyAccessSeq bracket handling and computeColumn flows by coercing undefined bracket names to empty string so dynamic table rows missing fields no longer crash, and exercise the scenario through nil-specific scope and computeColumn tests.
Modified files (3)
  • src/scope.js
  • test/scope.js
  • test/sd-custom/computeColumn-nil-safe.js
Latest Contributors(2)
UserCommitDate
neo@spotdraft.comfix(SPD-23149): nil-sa...June 16, 2026
harttle@harttle.comchange: remove depreca...December 23, 2017
Review this PR on Baz | Customize your next review

Root cause of Wingify/VWO Order Details sd_unit computation crash:
- scope.js propertyAccessSeq push(): when bracket accessor resolves nil variable
  (e.g. unit_mapping[self.sd_product] where sd_product is missing from row),
  'name' is undefined; calling 'name.length' threw TypeError.
  Fix: coerce nil name to '' before length check.

- filters.js downcase/first/last/size/join: nil input caused crashes like
  'Cannot read properties of undefined (reading "toLowerCase")'.
  Fix: guard each filter to return safe empty value for nil input.

11 regression tests added (scope.js x2, filters.js x5, sd-custom x4).
863 passing, 23 pre-existing failures unchanged.

Resolves: WS 3409 / Workflow 11250 sd_order_details.sd_unit computation blocked.

Co-authored-by: Shreejit Nair <shreejit@spotdraft.com>
Comment thread filters.js
Comment on lines +49 to 53
downcase: v => (v == null ? '' : String(v).toLowerCase()),
escape: escape,
escape_once: str => escape(unescape(str)),
first: v => v[0],
first: v => (v == null ? undefined : v[0]),
floor: v => Math.floor(v),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These nil-safety guards repeat the same v == null ? <fallback> : <expression> pattern across downcase, first, join, last, and size, should we extract a shared nilSafe helper like const nilSafe = (fn, fallback) => v => (v == null ? fallback : fn(v))?

Severity

Want Baz to fix this for you? Activate Fixer

@baz-reviewer

baz-reviewer Bot commented Jun 16, 2026

Copy link
Copy Markdown

Spec Reviewer Report

🚫 1 missing requirement:

1. LiquidJS sum filter sums plain array values No new `sum` filter exists; only `sumArray` was added, so plain-array summing through the `sum` filter is not implemented (or not evidenced) in the provided diff.

Evidence:

 filters.js:30-58 (filters map defining no sum filter)

✅ 1 met requirement:

1. LiquidJS sum filter sums specified column in dynamic table array `sumArray` filter extracts the specified column across rows and reduces the values via `performOperations`, covering numeric/duration/currency dynamic-table data.

Evidence:

 filters.js:56-58 sumArray filter registration; filters.js:539-563 sumArray helper sums column values; test/filters.js:688-707 dynamic table column sum tests


Used resources:
Hash: 1ae480d | Ticket: Add sum filter in liquidjs to get the sum of a Dynamic Table Column or an Array | Checkout in Baz

To rerun the Spec Reviewer, comment "baz rerun spec review".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant