Skip to content

fix(schema-compiler): invalidate Jinja render cache when imported macro file changes#10818

Open
igorlukanin wants to merge 1 commit intomasterfrom
igor/cub-2357-dimension-lag-in-playground-after-adding-to-macro-file
Open

fix(schema-compiler): invalidate Jinja render cache when imported macro file changes#10818
igorlukanin wants to merge 1 commit intomasterfrom
igor/cub-2357-dimension-lag-in-playground-after-adding-to-macro-file

Conversation

@igorlukanin
Copy link
Copy Markdown
Member

Summary

  • The per-file Jinja render cache in `DataSchemaCompiler.transpileJinjaFile()` was keyed on the importing file's content alone. When a YAML cube file imported a Jinja macro file via `{% import 'macros.yml' as macros %}`, edits to the macro file did not change the importer's content hash, so the cached rendered output was reused and new macro-defined dimensions never reached the YAML compiler.
  • This change folds a fingerprint of all files containing `{% macro %}` definitions into the cache key. Macro-file edits now invalidate cached renders of all cube files (over-invalidating slightly, but macro edits are rare). Cube-file edits still hit the cache for unchanged sibling files.
  • Approach is intentionally simple: no Jinja AST traversal, no per-file dependency graph. Trade-off documented in the JSDoc on `computeJinjaMacrosFingerprint`.

Test plan

  • Added integration test `packages/cubejs-schema-compiler/test/integration/postgres/jinja-macro-cache.test.ts` with two cases: bug regression (macro edit invalidates importer cache) and cache effectiveness (cube-file edit does not invalidate sibling caches).
  • Verified test ReferenceError: self is not defined - Issue with whatwg-fetch when using the core lib with Node.js #1 fails on master baseline and passes with the fix.
  • Manually verified end-to-end with a Cube + DuckDB `docker-compose` setup using a freshly built `cubejs/cube:dev` image: macro file edits propagate to `/meta` within ~5s without touching the importing cube file.
  • CI must pass.

@igorlukanin igorlukanin requested a review from a team as a code owner May 5, 2026 13:27
@github-actions github-actions Bot added the javascript Pull requests that update Javascript code label May 5, 2026
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 5, 2026

Claude finished @igorlukanin's task in 2m 20s —— View job


Code Review: fix(schema-compiler): invalidate Jinja render cache when imported macro file changes

  • Gather context and read changed files
  • Review DataSchemaCompiler.ts changes
  • Review new test file
  • Post comprehensive review

Verdict: This is a clean, well-scoped fix. The approach is sound, the trade-offs are clearly documented, and the tests cover both the bug regression and cache effectiveness.

Summary

The bug: transpileJinjaFile() cached rendered output keyed only on the importing file's content hash. When a cube file used {% import 'macros.yml' as macros %}, edits to macros.yml didn't change the importer's content, so stale cached output was served.

The fix: Compute an MD5 fingerprint of all files containing {% macro %} definitions and fold it into the per-file cache key. Any macro-file edit now invalidates all Jinja render caches.

What works well

  1. Regex design/{%[-+]?\s*macro\s/ correctly handles Jinja whitespace control characters (-, +), variable spacing, and avoids false positives on macros or macrofunction thanks to the trailing \s.

  2. Fingerprint computation — Sorting by fileName before hashing ensures deterministic output. Using \0 separators between name and content prevents concatenation collisions. The early return for zero macro files avoids unnecessary hash allocation.

  3. Minimal blast radius — The change touches only the cache key construction and adds one static method. No changes to the rendering pipeline, template loading, or Jinja evaluation. The jinjaMacrosFingerprint is an optional field on TranspileOptions with a || '' fallback, so all existing call sites (like readModuleFile at line 1042) are unaffected.

  4. Intentional over-invalidation — Choosing to hash all macro files together rather than building per-file dependency graphs is the right call. Macro edits are rare, and the simpler approach avoids the maintenance burden and bugs that come with Jinja AST traversal or import tracking. Well-documented in the JSDoc.

Test quality

The two test cases cover the critical scenarios:

  • Regression test: Macro file change is detected even when the importing cube file is byte-identical — this is the exact bug from CUB-2357.
  • Cache effectiveness: Editing one cube file doesn't evict cached renders of sibling cube files (verified via sharedCache.size).

The isNativeSupported() gate (suitexdescribe) correctly skips the tests in environments without native support.

Minor observations

  1. One-liner nit (inline comment posted): The | separator in the cache key hash works fine but \0 would be more consistent with the fingerprint computation's separator style.

  2. No concerns with the second transpile phase (transpilePhase at line 345) — it only processes JS files after the first phase, so it correctly doesn't need the Jinja macro fingerprint.

  3. readModuleFile call (line 1042) doesn't pass jinjaMacrosFingerprint, which is correct — node_modules files aren't Jinja templates and the || '' fallback in transpileJinjaFile handles the undefined case.

Overall: well-reasoned fix with good test coverage.

Comment on lines +836 to +839
const cacheKey = crypto.createHash('md5')
.update(file.content)
.update('|')
.update(options.jinjaMacrosFingerprint || '')
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: The | separator works fine here since the fingerprint is always hex or empty. For consistency with computeJinjaMacrosFingerprint (which uses \0 separators), you could use \0 here too — but this is purely stylistic and doesn't affect correctness.

Suggested change
const cacheKey = crypto.createHash('md5')
.update(file.content)
.update('|')
.update(options.jinjaMacrosFingerprint || '')
const cacheKey = crypto.createHash('md5')
.update(file.content)
.update('\0')
.update(options.jinjaMacrosFingerprint || '')
.digest('hex');

this.loadJinjaTemplates(jinjaTemplatedFiles);
}

const jinjaMacrosFingerprint = DataSchemaCompiler.computeJinjaMacrosFingerprint(jinjaTemplatedFiles);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Good placement — this runs once per compile cycle (after templates are loaded into minijinja's store) and the result is shared across all file transpilations in the phase. The cost is proportional to macro-file count, which is typically very small.

@codecov
Copy link
Copy Markdown

codecov Bot commented May 5, 2026

Codecov Report

❌ Patch coverage is 83.33333% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 58.24%. Comparing base (87f4dc1) to head (da7f1e6).
⚠️ Report is 3 commits behind head on master.

Files with missing lines Patch % Lines
...schema-compiler/src/compiler/DataSchemaCompiler.ts 83.33% 2 Missing and 1 partial ⚠️

❗ There is a different number of reports uploaded between BASE (87f4dc1) and HEAD (da7f1e6). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (87f4dc1) HEAD (da7f1e6)
cubesql 1 0
Additional details and impacted files
@@             Coverage Diff             @@
##           master   #10818       +/-   ##
===========================================
- Coverage   78.79%   58.24%   -20.56%     
===========================================
  Files         470      216      -254     
  Lines       92135    16914    -75221     
  Branches     3424     3428        +4     
===========================================
- Hits        72597     9851    -62746     
+ Misses      19035     6561    -12474     
+ Partials      503      502        -1     
Flag Coverage Δ
cube-backend 58.24% <83.33%> (+0.15%) ⬆️
cubesql ?

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.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant