fix(policy): relation field resolution issue

[ymc9]

Summary by CodeRabbit

• Bug Fixes
  • Corrected evaluation of comparisons involving relation fields, ensuring filters and conditions work accurately across related records.
  • Improved consistency when matching related records by using the related model’s identifier fields.
  • Added safeguards to prevent rare errors when evaluating relations, improving reliability in complex policies and queries.

[coderabbitai]

Walkthrough

Adjusted normalization of binary expressions involving relation fields to derive related types via getFieldDefFromFieldRef, use the related model’s id fields for comparisons, and add invariants ensuring valid relation field definitions before appending the first related id field to the expression.

Changes

Cohort / File(s)	Summary
Policy expression normalization packages/runtime/src/plugins/policy/expression-transformer.ts	Revised operand normalization for relation-field comparisons to resolve related types via field definitions, use related model id fields instead of context model ids, and added invariant checks.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• feat(policy): support read filtering for update with "from" and delete with "using" #253 — Also modifies normalization logic for relation-field comparisons in the same file; closely related to id-field resolution behavior.

Poem

A hop and a skip through fields we go,
I sniff the right IDs—now I know! 🐇
Left or right, relations aligned,
First key found, neatly assigned.
With checks in place, I thump in cheer—
Transformations crisp, with logic clear! ✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The title "fix(policy): relation field resolution issue" is concise, uses conventional commit style, and accurately describes the primary change in the diff—a bug fix addressing relation field resolution in the policy expression transformer. It plainly communicates the area and nature of the change without unnecessary detail, so a reviewer or teammate can quickly understand the intent.
Docstring Coverage	✅ Passed	No functions found in the changes. Docstring coverage check skipped.

✨ Finishing touches

• 📝 Generate Docstrings

🧪 Generate unit tests

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix/field-resolve

---

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 analyzed the changes in this PR which fixes a relation field resolution issue in the policy expression transformer.

Todo List:

• Read and analyze the changed file
• Understand the context of the policy system
• Check for related test files and coverage
• Provide comprehensive review feedback

---

Summary

The fix addresses a bug in normalizeBinaryOperationOperands where the code was incorrectly using context.model to get ID fields instead of using the actual relation target model type.

Code Quality & Best Practices ✅

• Clean fix: The change correctly resolves the field definition first, then uses the relation's target type
• Consistent pattern: Both left and right operand handling now follow the same correct pattern
• Good error handling: Added proper assertions to ensure field definitions are found
• Maintains existing code style: Uses existing utilities and follows established patterns

Bug Fix Analysis ✅

Root Cause: When comparing relation fields (e.g., post.author == null), the code was incorrectly using the current model's ID fields instead of the relation target model's ID fields.

Before:

const idFields = requireIdFields(this.schema, context.model); // Wrong: uses current model

After:

const leftRelDef = this.getFieldDefFromFieldRef(expr.left, context.model);
const idFields = requireIdFields(this.schema, leftRelDef.type); // Correct: uses relation target model

Impact: This would have caused incorrect SQL generation when relation fields are used in policy expressions, potentially leading to runtime errors or incorrect policy evaluation.

Performance Considerations ✅

• Minimal overhead: The fix adds a single field lookup which is negligible
• No algorithmic changes: The core logic flow remains the same
• Maintains caching: No impact on existing query optimization

Security Concerns ✅

• Policy integrity: This fix ensures policies with relation field comparisons work correctly
• No new attack vectors: The change doesn't introduce any new security risks
• Input validation: Proper assertions ensure malformed expressions are caught early

Test Coverage 📝

Existing Coverage: The auth-equality tests in /packages/runtime/test/policy/auth-equality.test.ts demonstrate relation field comparisons like auth() == author work correctly.

Recommendation: Consider adding a specific regression test that covers the exact scenario this fixes - comparing relation fields directly (e.g., post.author == null or user.manager == otherUser).

Technical Details

The fix properly handles both directions of relation field comparisons:

1. Left operand relation field: relationField == null
2. Right operand relation field: null == relationField

The getFieldDefFromFieldRef method correctly extracts field definitions from both direct field references and this.field member expressions.

---

[Copilot]

Pull Request Overview

This PR fixes a relation field resolution issue in the policy expression transformer. The fix addresses incorrect ID field resolution when handling relation fields in expressions.

• Adds proper field definition lookup for relation fields before getting ID fields
• Replaces context.model with the actual relation field type for ID field resolution

---

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

packages/runtime/src/plugins/policy/expression-transformer.ts (1)

236-239: Mirror the left-side change; unify via helper

The right-side normalization mirrors the left. Refactor to the same helper to avoid drift.

-            const rightRelDef = this.getFieldDefFromFieldRef(expr.right, context.model);
-            invariant(rightRelDef, 'failed to get relation field definition');
-            const idFields = requireIdFields(this.schema, rightRelDef.type);
-            normalizedRight = this.makeOrAppendMember(normalizedRight, idFields[0]!);
+            const { relatedIdField } = this.requireRelatedIdFromRef(expr.right, context.model);
+            normalizedRight = this.makeOrAppendMember(normalizedRight, relatedIdField);

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f9baaaf and 626ad98.

📒 Files selected for processing (1)

• packages/runtime/src/plugins/policy/expression-transformer.ts (1 hunks)

🧰 Additional context used

📓 Path-based instructions (1)

{packages,samples,tests}/**

📄 CodeRabbit inference engine (CLAUDE.md)

Place packages only under packages/, samples/, or tests/

Files:

• packages/runtime/src/plugins/policy/expression-transformer.ts

🧬 Code graph analysis (1)

packages/runtime/src/plugins/policy/expression-transformer.ts (2)

packages/common-helpers/src/tiny-invariant.ts (1)

• invariant (4-14)

packages/runtime/src/client/query-utils.ts (1)

• requireIdFields (63-70)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)

• GitHub Check: build-test (20.x, postgresql)
• GitHub Check: build-test (20.x, sqlite)
• GitHub Check: claude-review

🔇 Additional comments (1)

packages/runtime/src/plugins/policy/expression-transformer.ts (1)

228-231: Use related model IDs; add helper and verify null-checks

Replace the current extraction with a small helper that returns the related id field (diff below).

-            const leftRelDef = this.getFieldDefFromFieldRef(expr.left, context.model);
-            invariant(leftRelDef, 'failed to get relation field definition');
-            const idFields = requireIdFields(this.schema, leftRelDef.type);
-            normalizedLeft = this.makeOrAppendMember(normalizedLeft, idFields[0]!);
+            const { relatedIdField } = this.requireRelatedIdFromRef(expr.left, context.model);
+            normalizedLeft = this.makeOrAppendMember(normalizedLeft, relatedIdField);

Add helper near getFieldDefFromFieldRef():

+    private requireRelatedIdFromRef(expr: Expression, model: GetModels<Schema>) {
+        const relDef = this.getFieldDefFromFieldRef(expr, model);
+        invariant(relDef?.relation, `failed to resolve relation field: ${JSON.stringify(expr)}`);
+        const idFields = requireIdFields(this.schema, relDef.type);
+        return { relatedType: relDef.type, relatedIdField: idFields[0]! };
+    }

Ran a repository grep for null comparisons; multiple matches exist (e.g. packages/runtime/src/plugins/policy/expression-transformer.ts:208–212, packages/runtime/src/client/crud/dialects/base-dialect.ts, packages/runtime/src/client/crud/operations/base.ts, client/result-processor.ts, and tests). Manually confirm null-comparisons against relation fields are only used for to-one relations to avoid multi-row scalar subquery errors.