fix(policy): join base table when loading before-update entities for @@delegate sub-models

[ymc9]

Summary

Fixes #2595.

loadBeforeUpdateEntities built a plain SELECT <fields> FROM <SubModel> query to snapshot entity state before an update (for post-update / before() policy evaluation). When the model is a @@delegate sub-model, fields referenced via before() may live on the base table — Postgres rejects with "column does not exist".

• Qualify every selected column with its owning table (originModel ?? model) using a ReferenceNode instead of a bare ColumnNode
• LEFT JOIN any base tables that contribute inherited fields, joining on the shared id fields
• Mirrors the same originModel fix applied to cursor pagination in fix(orm): resolve delegate-inherited fields in cursor pagination #2591

Test plan

• New regression test tests/regression/test/issue-2595.test.ts covers the failing path: update on a @@delegate sub-model with a @@deny('post-update', before().<base-field> …) policy no longer errors
• Second test case verifies the deny rule fires correctly when before().status == 'Finalized'
• pnpm --filter @zenstackhq/plugin-policy exec tsc --noEmit passes

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes

  • Improved query handling for delegated and inherited fields when applying policies based on base model conditions.

• Tests

  • Added regression test for policy enforcement on delegate sub-model field updates.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: e9ca71ac-f314-4354-b53d-d6aba07fe12b

📥 Commits

Reviewing files that changed from the base of the PR and between fd8db57 and f06f927.

📒 Files selected for processing (2)

• packages/plugins/policy/src/policy-handler.ts
• tests/regression/test/issue-2595.test.ts

---

📝 Walkthrough

Walkthrough

Fixed loadBeforeUpdateEntities to correctly resolve base-model fields in delegate sub-models by constructing explicit table qualifications, joining to base models, and conditionally including joins in the generated SQL query.

Changes

Cohort / File(s)	Summary
Policy Handler Query Builder packages/plugins/policy/src/policy-handler.ts	Modified loadBeforeUpdateEntities to resolve each field's originModel, construct qualified column references with explicit table ownership, accumulate base model origins, and generate LeftJoin clauses mapping base model id fields to primary model id fields. Joins are only included when accumulated models exist.
Regression Test tests/regression/test/issue-2595.test.ts	Added test for issue #2595 covering delegate sub-model updates with before() policy rules. Tests both successful updates of delegate fields and policy rejection when status transitions violate post-update conditions on the base model.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 A delegate's path was twisted and crossed,
Fields from the base in translation were lost,
But now with explicit joins clear and bright,
The table qualification guides us right!
Base models joined, policies respect the view,
This rabbit applauds this fix—thumps for you! 🥕

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main change: fixing policy handler to join base tables when loading before-update entities for delegate sub-models.
Linked Issues check	✅ Passed	The PR directly addresses issue #2595 by implementing the required fix: qualifying columns with their owning tables via originModel resolution and LEFT JOINing base tables that contribute inherited/delegated fields.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to fixing the identified issue: modifying loadBeforeUpdateEntities logic and adding a regression test for the specific #2595 scenario.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/2595-delegate-before-update-base-fields

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[claude]

Claude finished @ymc9's task —— View job

---

Code Review Complete ✅

I've thoroughly reviewed the changes for issue #2595. This is a well-implemented fix that addresses a specific edge case with delegate models and before-update policies.

Review Checklist:

• Read and understand the changed files
• Analyze code quality and implementation approach
• Check for potential bugs or edge cases
• Review performance implications
• Assess security considerations
• Evaluate test coverage and quality
• Provide comprehensive feedback

Summary

The fix addresses a critical issue where loadBeforeUpdateEntities failed when accessing inherited fields from base tables in @@delegate sub-models via before() policy expressions. The solution correctly implements table joining and column qualification.

Code Quality & Implementation ✅

Strengths:

• Clean, targeted fix: Changes are minimal and focused on the specific problem
• Consistent with existing patterns: Mirrors the originModel approach used in cursor pagination (fix(orm): resolve delegate-inherited fields in cursor pagination #2591)
• Proper SQL generation: Uses ReferenceNode instead of bare ColumnNode for qualified column references
• Efficient JOIN strategy: Only joins base tables that actually contribute fields

Implementation Details (lines 590-618):

• Correctly identifies inherited fields via fieldDef?.originModel
• Uses Set to avoid duplicate base table joins
• Properly constructs LEFT JOIN conditions using shared ID fields
• Maintains backward compatibility for non-delegate models

Bug Analysis ✅

Issue Resolution:

• Root cause: Postgres rejected queries with SELECT base_field FROM SubModel when base_field exists only in the base table
• Solution: Qualify columns as SELECT base_table.base_field FROM SubModel LEFT JOIN BaseModel
• Edge cases handled: Works for models with mixed local/inherited fields

Performance Considerations ✅

Positive aspects:

• Minimal overhead: Only adds JOINs when inherited fields are actually accessed
• Efficient field filtering: Uses Set to deduplicate base models requiring JOINs
• No unnecessary work: Preserves existing fast path for non-delegate models

Potential concerns:

• LEFT JOIN adds query complexity, but this is unavoidable given the data model
• Performance impact should be minimal since this only affects the before-update snapshot query

Security Assessment ✅

No security concerns identified:

• No changes to policy evaluation logic
• No new user input handling
• SQL generation uses existing safe primitives (ReferenceNode, BinaryOperationNode)
• Maintains existing access controls

Test Coverage ✅

Excellent test coverage:

Test 1 (policy-handler.ts:590-628):

• Scenario: Update delegate sub-model with before() policy on base field
• Validation: Ensures no database errors during update
• Result: Confirms fix resolves the core issue

Test 2 (issue-2595.test.ts:36-49):

• Scenario: Policy correctly denies when before().status == 'Finalized'
• Validation: Ensures policy evaluation still works correctly
• Result: Confirms fix doesn't break policy logic

Test schema quality:

• Realistic delegate model setup (Transaction → Invoice)
• Practical policy using before() with base field
• Covers both success and denial scenarios

Minor Recommendations

1. Consider adding a comment at line 590 explaining why JOINs are needed for delegate models
2. Type safety: The implementation correctly handles the case where fieldDef?.originModel might be undefined

Verdict: LGTM 🚀

This is a high-quality fix that:

• ✅ Solves the reported issue completely
• ✅ Maintains existing functionality
• ✅ Includes comprehensive test coverage
• ✅ Follows established code patterns
• ✅ Has no security or performance concerns

The implementation demonstrates deep understanding of both the delegate model system and the policy evaluation pipeline. Ready for merge!

---