fix(policy): handle DefaultInsertValueNode in createManyAndReturn

[pkudinov]

Summary

Fixes #2460

createManyAndReturn with PolicyPlugin crashes with Invariant failed: expecting a ValueNode when rows have asymmetric optional fields (one row provides a field, another omits it).

Kysely pads missing columns with DefaultInsertValueNode. The fix handles this node type in PolicyHandler.unwrapCreateValueRow by treating it as null.

Changes

• packages/plugins/policy/src/policy-handler.ts — 4-line fix to handle DefaultInsertValueNode
• tests/regression/test/issue-2460.test.ts — regression test

Summary by CodeRabbit

• Bug Fixes

  • Default placeholders in create/value rows are now treated as null during processing, improving handling of optional fields in createManyAndReturn.

• Tests

  • Added a regression test to ensure createManyAndReturn handles asymmetric optional fields across rows without errors.

[coderabbitai]

📝 Walkthrough

Walkthrough

Adds handling for DefaultInsertValueNode in unwrapCreateValueRow, treating such nodes as null by inserting a ValueNode with null and continuing, preventing invariant failures when processing create rows with asymmetric optional fields under the PolicyPlugin.

Changes

Cohort / File(s)	Summary
Policy Handler Enhancement packages/plugins/policy/src/policy-handler.ts	Detects DefaultInsertValueNode in unwrapCreateValueRow and replaces it with a ValueNode containing null so missing/Default-padded columns are handled without raising invariants.
Regression Test tests/regression/test/issue-2460.test.ts	Adds a regression test that exercises createManyAndReturn with asymmetric optional fields to verify the PolicyPlugin no longer fails when some rows omit optional columns.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Poem

🐰 Hopping through rows where defaults once hid,
I swap them for nulls—now the batch's well-fed.
No more invariant cries in the night,
CreateMany returns with everything right. ✨

🚥 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 'fix(policy): handle DefaultInsertValueNode in createManyAndReturn' directly addresses the main change: adding handling for DefaultInsertValueNode in the policy handler, which fixes the issue with createManyAndReturn crashes.
Linked Issues check	✅ Passed	The changes successfully address all coding requirements from issue #2460: handling DefaultInsertValueNode in unwrapCreateValueRow to treat it as null, adding the 4-line fix to policy-handler.ts, and providing a regression test that reproduces and verifies the fix.
Out of Scope Changes check	✅ Passed	All changes are directly related to fixing issue #2460: the policy handler modification addresses the root cause, and the regression test validates the fix without introducing unrelated modifications.
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 unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

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.}

[coderabbitai]

🧹 Nitpick comments (1)

tests/regression/test/issue-2460.test.ts (1)

8-36: Always disconnect the test client in a finally block.

A failure before Line 36 skips $disconnect(), which can leak the DB connection and destabilize the regression suite.

♻️ Suggested change

-        const user = await db.user.create({ data: { role: 'admin' } });
-
-        const result = await db.$setAuth(user).item.createManyAndReturn({
-            data: [
-                { key: 'a', note: 'hello' },
-                { key: 'b' },
-            ],
-        });
-
-        expect(result).toHaveLength(2);
-        await db.$disconnect();
+        try {
+            const user = await db.user.create({ data: { role: 'admin' } });
+
+            const result = await db.$setAuth(user).item.createManyAndReturn({
+                data: [
+                    { key: 'a', note: 'hello' },
+                    { key: 'b' },
+                ],
+            });
+
+            expect(result).toHaveLength(2);
+        } finally {
+            await db.$disconnect();
+        }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/regression/test/issue-2460.test.ts` around lines 8 - 36, Ensure the
test always closes the DB connection by moving the db.$disconnect() call into a
finally block: after creating the client via createPolicyTestClient(...) and
performing the operations (user creation,
db.$setAuth(user).item.createManyAndReturn(...)), wrap the main test logic in
try { ... } and put await db.$disconnect() inside finally { ... } so that
db.$disconnect() runs even if an assertion or DB operation throws; reference the
existing createPolicyTestClient call and the db.$disconnect invocation when
making the change.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@tests/regression/test/issue-2460.test.ts`:
- Around line 8-36: Ensure the test always closes the DB connection by moving
the db.$disconnect() call into a finally block: after creating the client via
createPolicyTestClient(...) and performing the operations (user creation,
db.$setAuth(user).item.createManyAndReturn(...)), wrap the main test logic in
try { ... } and put await db.$disconnect() inside finally { ... } so that
db.$disconnect() runs even if an assertion or DB operation throws; reference the
existing createPolicyTestClient call and the db.$disconnect invocation when
making the change.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 436eae36-4b83-432f-8e18-b221fc69de55

📥 Commits

Reviewing files that changed from the base of the PR and between bd2b111 and 9076fe6.

📒 Files selected for processing (2)

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

[coderabbitai]

🧹 Nitpick comments (1)

tests/regression/test/issue-2460.test.ts (1)

35-35: Consider adding assertions on the actual returned values.

While the length check confirms the operation doesn't crash, asserting the actual field values would strengthen the regression test by verifying that the DefaultInsertValueNode is correctly treated as null:

♻️ Suggested enhancement

-        expect(result).toHaveLength(2);
+        expect(result).toHaveLength(2);
+        expect(result[0].note).toBe('hello');
+        expect(result[1].note).toBeNull();

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/regression/test/issue-2460.test.ts` at line 35, The test currently only
checks expect(result).toHaveLength(2); but should also assert the actual
returned values to verify DefaultInsertValueNode is treated as null; update the
test to assert the contents of result (for example, check each row/object fields
and that the entry corresponding to the DefaultInsertValueNode is null or has
the expected null-equivalent field) by adding expectations against result (e.g.,
expect(result[0].field).toBe(...), expect(result[1].field).toBeNull() or an
appropriate shape) so the regression verifies value correctness rather than just
length.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@tests/regression/test/issue-2460.test.ts`:
- Line 35: The test currently only checks expect(result).toHaveLength(2); but
should also assert the actual returned values to verify DefaultInsertValueNode
is treated as null; update the test to assert the contents of result (for
example, check each row/object fields and that the entry corresponding to the
DefaultInsertValueNode is null or has the expected null-equivalent field) by
adding expectations against result (e.g., expect(result[0].field).toBe(...),
expect(result[1].field).toBeNull() or an appropriate shape) so the regression
verifies value correctness rather than just length.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 84c427bc-f190-4d10-9e44-08bb0eeecca8

📥 Commits

Reviewing files that changed from the base of the PR and between 9076fe6 and 22c0ad1.

📒 Files selected for processing (2)

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

🚧 Files skipped from review as they are similar to previous changes (1)

• packages/plugins/policy/src/policy-handler.ts

[ymc9]

Thanks @pkudinov ! LGTM and it should resolve optional field issue.

For fields with default value and omitted during create, I think we need to back-fill default values during policy evaluation (so that policy rules can properly access them). I'll make a separate fix for that a bit later.

[ymc9]

@claude review

[claude]

Claude encountered an error —— View job

Failed with exit code 128

---

I'll analyze this and get back to you.