Skip to content

fix(zod): "strip" mode causes create payload fields to be accidentally dropped #1747

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Sep 27, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/runtime/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,10 @@
"./models": {
"types": "./models.d.ts"
},
"./zod-utils": {
"types": "./zod-utils.d.ts",
"default": "./zod-utils.js"
},
"./package.json": {
"default": "./package.json"
}
Expand Down
121 changes: 121 additions & 0 deletions packages/runtime/src/zod-utils.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,121 @@
/* eslint-disable @typescript-eslint/no-explicit-any */
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Avoid disabling ESLint rules and eliminate any types

Disabling @typescript-eslint/no-explicit-any might hide potential type safety issues. It's recommended to avoid using any and instead provide explicit type definitions to ensure full type safety and maintainability.

import { z as Z } from 'zod';

/**
* A smarter version of `z.union` that decide which candidate to use based on how few unrecognized keys it has.
*
* The helper is used to deal with ambiguity in union generated for Prisma inputs when the zod schemas are configured
* to run in "strip" object parsing mode. Since "strip" automatically drops unrecognized keys, it may result in
* accidentally matching a less-ideal schema candidate.
*
* The helper uses a custom schema to find the candidate that results in the fewest unrecognized keys when parsing the data.
*/
export function smartUnion(z: typeof Z, candidates: Z.ZodSchema[]) {
// strip `z.lazy`
const processedCandidates = candidates.map((candidate) => unwrapLazy(z, candidate));

if (processedCandidates.some((c) => !(c instanceof z.ZodObject || c instanceof z.ZodArray))) {
// fall back to plain union if not all candidates are objects or arrays
return z.union(candidates as any);
}

let resultData: any;

return z
.custom((data) => {
if (Array.isArray(data)) {
const { data: result, success } = smartArrayUnion(
z,
processedCandidates.filter((c) => c instanceof z.ZodArray),
data
);
if (success) {
resultData = result;
}
return success;
} else {
const { data: result, success } = smartObjectUnion(
z,
processedCandidates.filter((c) => c instanceof z.ZodObject),
data
);
if (success) {
resultData = result;
}
return success;
}
})
.transform(() => {
// return the parsed data
return resultData;
});
Comment on lines +22 to +51
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Prevent mutating external state within Zod schemas

The use of an external variable resultData that's modified inside the .custom() method and then accessed in .transform() can lead to unexpected behaviors, especially in concurrent scenarios. Modifying external state inside schema parsing functions may cause race conditions or data inconsistency.

Consider refactoring to eliminate external state mutation by directly returning the parsed data in the .transform() method.

Here's a suggested refactor:

 export function smartUnion(z: typeof Z, candidates: Z.ZodSchema[]) {
     // ... [code omitted for brevity]
 
-    let resultData: any;
 
     return z
         .custom((data) => {
-            if (Array.isArray(data)) {
-                const { data: result, success } = smartArrayUnion(
+            let parseResult;
+            if (Array.isArray(data)) {
+                const result = smartArrayUnion(
                     z,
                     processedCandidates.filter((c) => c instanceof z.ZodArray),
                     data
                 );
-                if (success) {
-                    resultData = result;
+                if (result.success) {
+                    parseResult = result.data;
                     return true;
                 }
                 return false;
             } else {
-                const { data: result, success } = smartObjectUnion(
+                const result = smartObjectUnion(
                     z,
                     processedCandidates.filter((c) => c instanceof z.ZodObject),
                     data
                 );
-                if (success) {
-                    resultData = result;
+                if (result.success) {
+                    parseResult = result.data;
                     return true;
                 }
                 return false;
             }
         })
         .transform(() => {
-            // return the parsed data
-            return resultData;
+            return parseResult;
         });
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
let resultData: any;
return z
.custom((data) => {
if (Array.isArray(data)) {
const { data: result, success } = smartArrayUnion(
z,
processedCandidates.filter((c) => c instanceof z.ZodArray),
data
);
if (success) {
resultData = result;
}
return success;
} else {
const { data: result, success } = smartObjectUnion(
z,
processedCandidates.filter((c) => c instanceof z.ZodObject),
data
);
if (success) {
resultData = result;
}
return success;
}
})
.transform(() => {
// return the parsed data
return resultData;
});
return z
.custom((data) => {
let parseResult;
if (Array.isArray(data)) {
const result = smartArrayUnion(
z,
processedCandidates.filter((c) => c instanceof z.ZodArray),
data
);
if (result.success) {
parseResult = result.data;
return true;
}
return false;
} else {
const result = smartObjectUnion(
z,
processedCandidates.filter((c) => c instanceof z.ZodObject),
data
);
if (result.success) {
parseResult = result.data;
return true;
}
return false;
}
})
.transform(() => {
return parseResult;
});

}

function smartArrayUnion(z: typeof Z, candidates: Array<Z.ZodArray<Z.ZodObject<Z.ZodRawShape>>>, data: any) {
if (candidates.length === 0) {
return { data: undefined, success: false };
}

if (!Array.isArray(data)) {
return { data: undefined, success: false };
}

if (data.length === 0) {
return { data, success: true };
}

// use the first element to identify the candidate schema to use
const item = data[0];
const itemSchema = identifyCandidate(
z,
candidates.map((candidate) => candidate.element),
item
);

// find the matching schema and re-parse the data
const schema = candidates.find((candidate) => candidate.element === itemSchema);
return schema!.safeParse(data);
Comment on lines +76 to +77
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Handle undefined schema to prevent runtime errors

In smartArrayUnion, the schema obtained from .find() may be undefined if no matching candidate is found. Using schema! assumes it's always defined, which can lead to runtime errors.

Add a check to ensure schema is defined before proceeding:

 const schema = candidates.find((candidate) => candidate.element === itemSchema);
+if (!schema) {
+    return { data: undefined, success: false };
+}
 return schema.safeParse(data);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const schema = candidates.find((candidate) => candidate.element === itemSchema);
return schema!.safeParse(data);
const schema = candidates.find((candidate) => candidate.element === itemSchema);
if (!schema) {
return { data: undefined, success: false };
}
return schema.safeParse(data);

}

function smartObjectUnion(z: typeof Z, candidates: Z.ZodObject<Z.ZodRawShape>[], data: any) {
if (candidates.length === 0) {
return { data: undefined, success: false };
}
const schema = identifyCandidate(z, candidates, data);
return schema.safeParse(data);
}

function identifyCandidate(
z: typeof Z,
candidates: Array<Z.ZodObject<Z.ZodRawShape> | Z.ZodLazy<Z.ZodObject<Z.ZodRawShape>>>,
data: any
) {
const strictResults = candidates.map((candidate) => {
// make sure to strip `z.lazy` before parsing
const unwrapped = unwrapLazy(z, candidate);
return {
schema: candidate,
// force object schema to run in strict mode to capture unrecognized keys
result: unwrapped.strict().safeParse(data),
};
});

// find the schema with the fewest unrecognized keys
const { schema } = strictResults.sort((a, b) => {
const aCount = countUnrecognizedKeys(a.result.error?.issues ?? []);
const bCount = countUnrecognizedKeys(b.result.error?.issues ?? []);
return aCount - bCount;
})[0];
return schema;
}
Comment on lines +93 to +110
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Ensure consistency between unwrapped and returned schemas

In identifyCandidate, you unwrapped candidate to unwrapped for parsing but return the original candidate in schema, which may still be a ZodLazy. This inconsistency can cause issues if the returned schema is used without unwrapping.

Modify the return object to include the unwrapped schema:

 return {
-    schema: candidate,
+    schema: unwrapped,
     result: unwrapped.strict().safeParse(data),
 };

This change ensures that the schema used for parsing and the one returned are the same, preventing potential errors when the schema is used later.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const strictResults = candidates.map((candidate) => {
// make sure to strip `z.lazy` before parsing
const unwrapped = unwrapLazy(z, candidate);
return {
schema: candidate,
// force object schema to run in strict mode to capture unrecognized keys
result: unwrapped.strict().safeParse(data),
};
});
// find the schema with the fewest unrecognized keys
const { schema } = strictResults.sort((a, b) => {
const aCount = countUnrecognizedKeys(a.result.error?.issues ?? []);
const bCount = countUnrecognizedKeys(b.result.error?.issues ?? []);
return aCount - bCount;
})[0];
return schema;
}
const strictResults = candidates.map((candidate) => {
// make sure to strip `z.lazy` before parsing
const unwrapped = unwrapLazy(z, candidate);
return {
schema: unwrapped,
// force object schema to run in strict mode to capture unrecognized keys
result: unwrapped.strict().safeParse(data),
};
});
// find the schema with the fewest unrecognized keys
const { schema } = strictResults.sort((a, b) => {
const aCount = countUnrecognizedKeys(a.result.error?.issues ?? []);
const bCount = countUnrecognizedKeys(b.result.error?.issues ?? []);
return aCount - bCount;
})[0];
return schema;
}


function countUnrecognizedKeys(issues: Z.ZodIssue[]) {
return issues
.filter((issue) => issue.code === 'unrecognized_keys')
.map((issue) => issue.keys.length)
.reduce((a, b) => a + b, 0);
}

function unwrapLazy<T extends Z.ZodSchema>(z: typeof Z, schema: T | Z.ZodLazy<T>): T {
return schema instanceof z.ZodLazy ? schema.schema : schema;
}
109 changes: 109 additions & 0 deletions packages/runtime/tests/zod/smart-union.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,109 @@
import { z } from 'zod';
import { smartUnion } from '../../src/zod-utils';

describe('Zod smart union', () => {
it('should work with scalar union', () => {
const schema = smartUnion(z, [z.string(), z.number()]);
expect(schema.safeParse('test')).toMatchObject({ success: true, data: 'test' });
expect(schema.safeParse(1)).toMatchObject({ success: true, data: 1 });
expect(schema.safeParse(true)).toMatchObject({ success: false });
});

it('should work with non-ambiguous object union', () => {
const schema = smartUnion(z, [z.object({ a: z.string() }), z.object({ b: z.number() }).strict()]);
expect(schema.safeParse({ a: 'test' })).toMatchObject({ success: true, data: { a: 'test' } });
expect(schema.safeParse({ b: 1 })).toMatchObject({ success: true, data: { b: 1 } });
expect(schema.safeParse({ a: 'test', b: 1 })).toMatchObject({ success: true });
expect(schema.safeParse({ b: 1, c: 'test' })).toMatchObject({ success: false });
expect(schema.safeParse({ c: 'test' })).toMatchObject({ success: false });
});

it('should work with ambiguous object union', () => {
const schema = smartUnion(z, [
z.object({ a: z.string(), b: z.number() }),
z.object({ a: z.string(), c: z.boolean() }),
]);
expect(schema.safeParse({ a: 'test', b: 1 })).toMatchObject({ success: true, data: { a: 'test', b: 1 } });
expect(schema.safeParse({ a: 'test', c: true })).toMatchObject({ success: true, data: { a: 'test', c: true } });
expect(schema.safeParse({ a: 'test', b: 1, z: 'z' })).toMatchObject({
success: true,
data: { a: 'test', b: 1 },
});
expect(schema.safeParse({ a: 'test', c: true, z: 'z' })).toMatchObject({
success: true,
data: { a: 'test', c: true },
});
expect(schema.safeParse({ c: 'test' })).toMatchObject({ success: false });
});

it('should work with non-ambiguous array union', () => {
const schema = smartUnion(z, [
z.object({ a: z.string() }).array(),
z.object({ b: z.number() }).strict().array(),
]);

expect(schema.safeParse([{ a: 'test' }])).toMatchObject({ success: true, data: [{ a: 'test' }] });
expect(schema.safeParse([{ a: 'test' }, { a: 'test1' }])).toMatchObject({
success: true,
data: [{ a: 'test' }, { a: 'test1' }],
});

expect(schema.safeParse([{ b: 1 }])).toMatchObject({ success: true, data: [{ b: 1 }] });
expect(schema.safeParse([{ a: 'test', b: 1 }])).toMatchObject({ success: true });
expect(schema.safeParse([{ b: 1, c: 'test' }])).toMatchObject({ success: false });
expect(schema.safeParse([{ c: 'test' }])).toMatchObject({ success: false });

// all items must match the same candidate
expect(schema.safeParse([{ a: 'test' }, { b: 1 }])).toMatchObject({ success: false });
});

it('should work with ambiguous array union', () => {
const schema = smartUnion(z, [
z.object({ a: z.string(), b: z.number() }).array(),
z.object({ a: z.string(), c: z.boolean() }).array(),
]);

expect(schema.safeParse([{ a: 'test', b: 1 }])).toMatchObject({ success: true, data: [{ a: 'test', b: 1 }] });
expect(schema.safeParse([{ a: 'test', c: true }])).toMatchObject({
success: true,
data: [{ a: 'test', c: true }],
});
expect(schema.safeParse([{ a: 'test', b: 1, z: 'z' }])).toMatchObject({
success: true,
data: [{ a: 'test', b: 1 }],
});
expect(schema.safeParse([{ a: 'test', c: true, z: 'z' }])).toMatchObject({
success: true,
data: [{ a: 'test', c: true }],
});
expect(schema.safeParse([{ c: 'test' }])).toMatchObject({ success: false });

// all items must match the same candidate
expect(schema.safeParse([{ a: 'test' }, { c: true }])).toMatchObject({ success: false });
});

it('should work with lazy schemas', () => {
const schema = smartUnion(z, [
z.lazy(() => z.object({ a: z.string(), b: z.number() })),
z.lazy(() => z.object({ a: z.string(), c: z.boolean() })),
]);
expect(schema.safeParse({ a: 'test', b: 1 })).toMatchObject({ success: true, data: { a: 'test', b: 1 } });
expect(schema.safeParse({ a: 'test', c: true })).toMatchObject({ success: true, data: { a: 'test', c: true } });
expect(schema.safeParse({ a: 'test', b: 1, z: 'z' })).toMatchObject({
success: true,
data: { a: 'test', b: 1 },
});
});

it('should work with mixed object and array unions', () => {
const schema = smartUnion(z, [
z.object({ a: z.string() }).strict(),
z.object({ b: z.number() }).strict().array(),
]);

expect(schema.safeParse({ a: 'test' })).toMatchObject({ success: true, data: { a: 'test' } });
expect(schema.safeParse([{ b: 1 }])).toMatchObject({ success: true, data: [{ b: 1 }] });
expect(schema.safeParse({ a: 'test', b: 1 })).toMatchObject({ success: false });
expect(schema.safeParse([{ a: 'test' }])).toMatchObject({ success: false });
});
});
29 changes: 18 additions & 11 deletions packages/schema/src/plugins/zod/generator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,13 @@ import { upperCaseFirst } from 'upper-case-first';
import { name } from '.';
import { getDefaultOutputFolder } from '../plugin-utils';
import Transformer from './transformer';
import { ObjectMode } from './types';
import { makeFieldSchema, makeValidationRefinements } from './utils/schema-gen';

export class ZodSchemaGenerator {
private readonly sourceFiles: SourceFile[] = [];
private readonly globalOptions: PluginGlobalOptions;
private readonly mode: ObjectMode;

constructor(
private readonly model: Model,
Expand All @@ -39,6 +41,19 @@ export class ZodSchemaGenerator {
throw new Error('Global options are required');
}
this.globalOptions = globalOptions;

// options validation
if (
this.options.mode &&
(typeof this.options.mode !== 'string' || !['strip', 'strict', 'passthrough'].includes(this.options.mode))
) {
throw new PluginError(
name,
`Invalid mode option: "${this.options.mode}". Must be one of 'strip', 'strict', or 'passthrough'.`
);
}

this.mode = (this.options.mode ?? 'strict') as ObjectMode;
}

async generate() {
Expand All @@ -55,17 +70,6 @@ export class ZodSchemaGenerator {
ensureEmptyDir(output);
Transformer.setOutputPath(output);

// options validation
if (
this.options.mode &&
(typeof this.options.mode !== 'string' || !['strip', 'strict', 'passthrough'].includes(this.options.mode))
) {
throw new PluginError(
name,
`Invalid mode option: "${this.options.mode}". Must be one of 'strip', 'strict', or 'passthrough'.`
);
}

// calculate the models to be excluded
const excludeModels = this.getExcludedModels();

Expand Down Expand Up @@ -120,6 +124,7 @@ export class ZodSchemaGenerator {
project: this.project,
inputObjectTypes,
zmodel: this.model,
mode: this.mode,
});
await transformer.generateInputSchemas(this.options, this.model);
this.sourceFiles.push(...transformer.sourceFiles);
Expand Down Expand Up @@ -215,6 +220,7 @@ export class ZodSchemaGenerator {
project: this.project,
inputObjectTypes: [],
zmodel: this.model,
mode: this.mode,
});
await transformer.generateEnumSchemas();
this.sourceFiles.push(...transformer.sourceFiles);
Expand Down Expand Up @@ -243,6 +249,7 @@ export class ZodSchemaGenerator {
project: this.project,
inputObjectTypes,
zmodel: this.model,
mode: this.mode,
});
const moduleName = transformer.generateObjectSchema(generateUnchecked, this.options);
moduleNames.push(moduleName);
Expand Down
Loading
Loading