From 2162352540c8a42830ff51d6a9630255e9c81f15 Mon Sep 17 00:00:00 2001 From: ymc9 <104139426+ymc9@users.noreply.github.com> Date: Mon, 24 Jul 2023 14:29:18 +0800 Subject: [PATCH 1/3] refactor: perf improvement for nullable to-one nested read Use the "extendedWhereUnique" preview feature to avoid post-read policy check --- packages/runtime/package.json | 2 + .../src/enhancements/policy/policy-utils.ts | 130 ++++++++++-------- packages/runtime/src/index.ts | 1 + packages/runtime/src/version.ts | 29 ++++ .../src/plugins/prisma/schema-generator.ts | 18 ++- packages/schema/src/telemetry.ts | 2 +- packages/sdk/src/prisma.ts | 22 +-- packages/testtools/src/package.template.json | 4 +- pnpm-lock.yaml | 6 + tests/integration/test-run/package.json | 4 +- .../nextjs/test-project/package.json | 4 +- .../frameworks/trpc/test-project/package.json | 4 +- 12 files changed, 139 insertions(+), 87 deletions(-) diff --git a/packages/runtime/package.json b/packages/runtime/package.json index 25e5a8a26..68bbbcd34 100644 --- a/packages/runtime/package.json +++ b/packages/runtime/package.json @@ -55,6 +55,7 @@ "deepcopy": "^2.1.0", "lower-case-first": "^2.0.2", "pluralize": "^8.0.0", + "semver": "^7.3.8", "superjson": "^1.11.0", "tslib": "^2.4.1", "upper-case-first": "^2.0.2", @@ -71,6 +72,7 @@ "@types/jest": "^29.5.0", "@types/node": "^18.0.0", "@types/pluralize": "^0.0.29", + "@types/semver": "^7.3.13", "copyfiles": "^2.4.1", "rimraf": "^3.0.2", "typescript": "^4.9.3" diff --git a/packages/runtime/src/enhancements/policy/policy-utils.ts b/packages/runtime/src/enhancements/policy/policy-utils.ts index 20e790415..f9d0a23fb 100644 --- a/packages/runtime/src/enhancements/policy/policy-utils.ts +++ b/packages/runtime/src/enhancements/policy/policy-utils.ts @@ -23,7 +23,7 @@ import { PolicyOperationKind, PrismaWriteActionType, } from '../../types'; -import { getVersion } from '../../version'; +import { getPrismaVersion, getVersion } from '../../version'; import { getFields, resolveField } from '../model-meta'; import { NestedWriteVisitor, type NestedWriteVisitorContext } from '../nested-write-vistor'; import type { ModelMeta, PolicyDef, PolicyFunc, ZodSchemas } from '../types'; @@ -36,6 +36,11 @@ import { prismaClientUnknownRequestError, } from '../utils'; import { Logger } from './logger'; +import semver from 'semver'; + +// use Prisma version to detect if we can filter when nested-fetching to-one relation +const prismaVersion = getPrismaVersion(); +const supportNestedToOneFilter = prismaVersion ? semver.gte(prismaVersion, '4.8.0') : false; /** * Access policy enforcement utilities @@ -334,6 +339,7 @@ export class PolicyUtil { } const idFields = this.getIdFields(model); + for (const field of getModelFields(injectTarget)) { const fieldInfo = resolveField(this.modelMeta, model, field); if (!fieldInfo || !fieldInfo.isDataModel) { @@ -341,12 +347,17 @@ export class PolicyUtil { continue; } - if (fieldInfo.isArray) { + if ( + fieldInfo.isArray || + // if Prisma version is high enough to support filtering directly when + // fetching a nullable to-one relation, let's do it that way + // https://github.com/prisma/prisma/discussions/20350 + (supportNestedToOneFilter && fieldInfo.isOptional) + ) { if (typeof injectTarget[field] !== 'object') { injectTarget[field] = {}; } - // inject extra condition for to-many relation - + // inject extra condition for to-many or nullable to-one relation await this.injectAuthGuard(injectTarget[field], fieldInfo.type, 'read'); } else { // there's no way of injecting condition for to-one relation, so if there's @@ -361,9 +372,6 @@ export class PolicyUtil { } } } - - // recurse - await this.injectNestedReadConditions(fieldInfo.type, injectTarget[field]); } } @@ -373,69 +381,79 @@ export class PolicyUtil { * omitted. */ async postProcessForRead(data: any, model: string, args: any, operation: PolicyOperationKind) { - for (const entityData of enumerate(data)) { - if (typeof entityData !== 'object' || !entityData) { - continue; - } + await Promise.all( + enumerate(data).map((entityData) => this.postProcessSingleEntityForRead(entityData, model, args, operation)) + ); + } - // strip auxiliary fields - for (const auxField of AUXILIARY_FIELDS) { - if (auxField in entityData) { - delete entityData[auxField]; - } + private async postProcessSingleEntityForRead(data: any, model: string, args: any, operation: PolicyOperationKind) { + if (typeof data !== 'object' || !data) { + return; + } + + // strip auxiliary fields + for (const auxField of AUXILIARY_FIELDS) { + if (auxField in data) { + delete data[auxField]; } + } - const injectTarget = args.select ?? args.include; - if (!injectTarget) { + const injectTarget = args.select ?? args.include; + if (!injectTarget) { + return; + } + + // recurse into nested entities + for (const field of Object.keys(injectTarget)) { + const fieldData = data[field]; + if (typeof fieldData !== 'object' || !fieldData) { continue; } - // recurse into nested entities - for (const field of Object.keys(injectTarget)) { - const fieldData = entityData[field]; - if (typeof fieldData !== 'object' || !fieldData) { - continue; - } + const fieldInfo = resolveField(this.modelMeta, model, field); + if (fieldInfo) { + if ( + fieldInfo.isDataModel && + !fieldInfo.isArray && + // if Prisma version supports filtering nullable to-one relation, no need to further check + !(supportNestedToOneFilter && fieldInfo.isOptional) + ) { + // to-one relation data cannot be trimmed by injected guards, we have to + // post-check them + const ids = this.getEntityIds(fieldInfo.type, fieldData); + + if (Object.keys(ids).length !== 0) { + if (this.logger.enabled('info')) { + this.logger.info( + `Validating read of to-one relation: ${fieldInfo.type}#${formatObject(ids)}` + ); + } - const fieldInfo = resolveField(this.modelMeta, model, field); - if (fieldInfo) { - if (fieldInfo.isDataModel && !fieldInfo.isArray) { - // to-one relation data cannot be trimmed by injected guards, we have to - // post-check them - const ids = this.getEntityIds(fieldInfo.type, fieldData); - - if (Object.keys(ids).length !== 0) { - // if (this.logger.enabled('info')) { - // this.logger.info( - // `Validating read of to-one relation: ${fieldInfo.type}#${formatObject(ids)}` - // ); - // } - try { - await this.checkPolicyForFilter(fieldInfo.type, ids, operation, this.db); - } catch (err) { - if ( - isPrismaClientKnownRequestError(err) && - err.code === PrismaErrorCode.CONSTRAINED_FAILED - ) { - // denied by policy - if (fieldInfo.isOptional) { - // if the relation is optional, just nullify it - entityData[field] = null; - } else { - // otherwise reject - throw err; - } + try { + await this.checkPolicyForFilter(fieldInfo.type, ids, operation, this.db); + } catch (err) { + if ( + isPrismaClientKnownRequestError(err) && + err.code === PrismaErrorCode.CONSTRAINED_FAILED + ) { + // denied by policy + if (fieldInfo.isOptional) { + // if the relation is optional, just nullify it + data[field] = null; } else { - // unknown error + // otherwise reject throw err; } + } else { + // unknown error + throw err; } } } - - // recurse - await this.postProcessForRead(fieldData, fieldInfo.type, injectTarget[field], operation); } + + // recurse + await this.postProcessForRead(fieldData, fieldInfo.type, injectTarget[field], operation); } } } diff --git a/packages/runtime/src/index.ts b/packages/runtime/src/index.ts index 080d229a4..a964d72ed 100644 --- a/packages/runtime/src/index.ts +++ b/packages/runtime/src/index.ts @@ -3,3 +3,4 @@ export * from './enhancements'; export * from './error'; export * from './types'; export * from './validation'; +export * from './version'; diff --git a/packages/runtime/src/version.ts b/packages/runtime/src/version.ts index 854e5eab6..fb25b4b51 100644 --- a/packages/runtime/src/version.ts +++ b/packages/runtime/src/version.ts @@ -1,3 +1,5 @@ +import path from 'path'; + /* eslint-disable @typescript-eslint/no-var-requires */ export function getVersion() { try { @@ -11,3 +13,30 @@ export function getVersion() { } } } + +/** + * Gets installed Prisma version by first checking "@prisma/client" and if not available, + * "prisma". + */ +export function getPrismaVersion(): string | undefined { + try { + // eslint-disable-next-line @typescript-eslint/no-var-requires + return require('@prisma/client/package.json').version; + } catch { + try { + // eslint-disable-next-line @typescript-eslint/no-var-requires + return require('prisma/package.json').version; + } catch { + if (process.env.ZENSTACK_TEST === '1') { + // test environment + try { + return require(path.resolve('./node_modules/@prisma/client/package.json')).version; + } catch { + return undefined; + } + } + + return undefined; + } + } +} diff --git a/packages/schema/src/plugins/prisma/schema-generator.ts b/packages/schema/src/plugins/prisma/schema-generator.ts index c3f899219..4bd5b896c 100644 --- a/packages/schema/src/plugins/prisma/schema-generator.ts +++ b/packages/schema/src/plugins/prisma/schema-generator.ts @@ -251,8 +251,7 @@ export default class PrismaSchemaGenerator { const provider = generator.fields.find((f) => f.name === 'provider'); if (provider?.value === 'prisma-client-js') { const prismaVersion = getPrismaVersion(); - if (prismaVersion && semver.lt(prismaVersion, '4.7.0')) { - // insert interactiveTransactions preview feature + if (prismaVersion) { let previewFeatures = generator.fields.find((f) => f.name === 'previewFeatures'); if (!previewFeatures) { previewFeatures = { name: 'previewFeatures', value: [] }; @@ -261,8 +260,19 @@ export default class PrismaSchemaGenerator { if (!Array.isArray(previewFeatures.value)) { throw new PluginError(name, 'option "previewFeatures" must be an array'); } - if (!previewFeatures.value.includes('interactiveTransactions')) { - previewFeatures.value.push('interactiveTransactions'); + + if (semver.lt(prismaVersion, '4.7.0')) { + // interactiveTransactions feature is opt-in before 4.7.0 + if (!previewFeatures.value.includes('interactiveTransactions')) { + previewFeatures.value.push('interactiveTransactions'); + } + } + + if (semver.gte(prismaVersion, '4.8.0') && semver.lt(prismaVersion, '5.0.0')) { + // extendedWhereUnique feature is opt-in during [4.8.0, 5.0.0) + if (!previewFeatures.value.includes('extendedWhereUnique')) { + previewFeatures.value.push('extendedWhereUnique'); + } } } } diff --git a/packages/schema/src/telemetry.ts b/packages/schema/src/telemetry.ts index b07484629..5912044b1 100644 --- a/packages/schema/src/telemetry.ts +++ b/packages/schema/src/telemetry.ts @@ -1,4 +1,5 @@ import { createId } from '@paralleldrive/cuid2'; +import { getPrismaVersion } from '@zenstackhq/sdk'; import exitHook from 'async-exit-hook'; import { CommanderError } from 'commander'; import { init, Mixpanel } from 'mixpanel'; @@ -8,7 +9,6 @@ import sleep from 'sleep-promise'; import { CliError } from './cli/cli-error'; import { TELEMETRY_TRACKING_TOKEN } from './constants'; import { getVersion } from './utils/version-utils'; -import { getPrismaVersion } from '@zenstackhq/sdk'; /** * Telemetry events diff --git a/packages/sdk/src/prisma.ts b/packages/sdk/src/prisma.ts index 7130eafd7..81ce71f7a 100644 --- a/packages/sdk/src/prisma.ts +++ b/packages/sdk/src/prisma.ts @@ -1,11 +1,15 @@ import type { DMMF } from '@prisma/generator-helper'; import { getDMMF as getDMMF4 } from '@prisma/internals'; import { getDMMF as getDMMF5 } from '@prisma/internals-v5'; +import { getPrismaVersion } from '@zenstackhq/runtime'; import path from 'path'; import * as semver from 'semver'; import { GeneratorDecl, Model, Plugin, isGeneratorDecl, isPlugin } from './ast'; import { getLiteral } from './utils'; +// reexport +export { getPrismaVersion } from '@zenstackhq/runtime'; + /** * Given a ZModel and an import context directory, compute the import spec for the Prisma Client. */ @@ -65,24 +69,6 @@ function normalizePath(p: string) { return p ? p.split(path.sep).join(path.posix.sep) : p; } -/** - * Gets installed Prisma version by first checking "@prisma/client" and if not available, - * "prisma". - */ -export function getPrismaVersion(): string | undefined { - try { - // eslint-disable-next-line @typescript-eslint/no-var-requires - return require('@prisma/client/package.json').version; - } catch { - try { - // eslint-disable-next-line @typescript-eslint/no-var-requires - return require('prisma/package.json').version; - } catch { - return undefined; - } - } -} - export type GetDMMFOptions = { datamodel?: string; cwd?: string; diff --git a/packages/testtools/src/package.template.json b/packages/testtools/src/package.template.json index fc936c9a2..5d46d1d79 100644 --- a/packages/testtools/src/package.template.json +++ b/packages/testtools/src/package.template.json @@ -7,12 +7,12 @@ "author": "", "license": "ISC", "dependencies": { - "@prisma/client": "^4.0.0", + "@prisma/client": "^4.8.0", "@zenstackhq/runtime": "file:/packages/runtime/dist", "@zenstackhq/swr": "file:/packages/plugins/swr/dist", "@zenstackhq/trpc": "file:/packages/plugins/trpc/dist", "@zenstackhq/openapi": "file:/packages/plugins/openapi/dist", - "prisma": "^4.0.0", + "prisma": "^4.8.0", "typescript": "^4.9.3", "zenstack": "file:/packages/schema/dist", "zod": "3.21.1" diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 058d0f821..4c87fa7a4 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -375,6 +375,9 @@ importers: pluralize: specifier: ^8.0.0 version: 8.0.0 + semver: + specifier: ^7.3.8 + version: 7.5.3 superjson: specifier: ^1.11.0 version: 1.11.0 @@ -400,6 +403,9 @@ importers: '@types/pluralize': specifier: ^0.0.29 version: 0.0.29 + '@types/semver': + specifier: ^7.3.13 + version: 7.5.0 copyfiles: specifier: ^2.4.1 version: 2.4.1 diff --git a/tests/integration/test-run/package.json b/tests/integration/test-run/package.json index b4f3b0810..05aa037d5 100644 --- a/tests/integration/test-run/package.json +++ b/tests/integration/test-run/package.json @@ -10,9 +10,9 @@ "author": "", "license": "ISC", "dependencies": { - "@prisma/client": "^4.0.0", + "@prisma/client": "^4.8.0", "@zenstackhq/runtime": "file:../../../packages/runtime/dist", - "prisma": "^4.0.0", + "prisma": "^4.8.0", "react": "^18.2.0", "swr": "^1.3.0", "typescript": "^4.9.3", diff --git a/tests/integration/tests/frameworks/nextjs/test-project/package.json b/tests/integration/tests/frameworks/nextjs/test-project/package.json index e8c006202..b1727bb38 100644 --- a/tests/integration/tests/frameworks/nextjs/test-project/package.json +++ b/tests/integration/tests/frameworks/nextjs/test-project/package.json @@ -9,7 +9,7 @@ "lint": "next lint" }, "dependencies": { - "@prisma/client": "^4.0.0", + "@prisma/client": "^4.8.0", "@types/node": "18.11.18", "@types/react": "18.0.27", "@types/react-dom": "18.0.10", @@ -24,6 +24,6 @@ "zod": "3.21.1" }, "devDependencies": { - "prisma": "^4.0.0" + "prisma": "^4.8.0" } } diff --git a/tests/integration/tests/frameworks/trpc/test-project/package.json b/tests/integration/tests/frameworks/trpc/test-project/package.json index 7bb67cbff..fac627668 100644 --- a/tests/integration/tests/frameworks/trpc/test-project/package.json +++ b/tests/integration/tests/frameworks/trpc/test-project/package.json @@ -9,7 +9,7 @@ "lint": "next lint" }, "dependencies": { - "@prisma/client": "^4.0.0", + "@prisma/client": "^4.8.0", "@tanstack/react-query": "^4.22.4", "@trpc/client": "^10.34.0", "@trpc/next": "^10.34.0", @@ -27,7 +27,7 @@ "@zenstackhq/runtime": "../../../../../../../packages/runtime/dist" }, "devDependencies": { - "prisma": "^4.0.0", + "prisma": "^4.8.0", "zenstack": "../../../../../../../packages/schema/dist", "@zenstackhq/trpc": "../../../../../../../packages/plugins/trpc/dist" } From db43c1a54c62a2740e518062e0d0f63fab6f7989 Mon Sep 17 00:00:00 2001 From: ymc9 <104139426+ymc9@users.noreply.github.com> Date: Mon, 24 Jul 2023 14:34:51 +0800 Subject: [PATCH 2/3] fix --- .../src/enhancements/policy/policy-utils.ts | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/packages/runtime/src/enhancements/policy/policy-utils.ts b/packages/runtime/src/enhancements/policy/policy-utils.ts index f9d0a23fb..28250b0a3 100644 --- a/packages/runtime/src/enhancements/policy/policy-utils.ts +++ b/packages/runtime/src/enhancements/policy/policy-utils.ts @@ -38,10 +38,6 @@ import { import { Logger } from './logger'; import semver from 'semver'; -// use Prisma version to detect if we can filter when nested-fetching to-one relation -const prismaVersion = getPrismaVersion(); -const supportNestedToOneFilter = prismaVersion ? semver.gte(prismaVersion, '4.8.0') : false; - /** * Access policy enforcement utilities */ @@ -50,6 +46,8 @@ export class PolicyUtil { // @ts-ignore private readonly logger: Logger; + private supportNestedToOneFilter = false; + constructor( private readonly db: DbClientContract, private readonly modelMeta: ModelMeta, @@ -59,6 +57,10 @@ export class PolicyUtil { private readonly logPrismaQuery?: boolean ) { this.logger = new Logger(db); + + // use Prisma version to detect if we can filter when nested-fetching to-one relation + const prismaVersion = getPrismaVersion(); + this.supportNestedToOneFilter = prismaVersion ? semver.gte(prismaVersion, '4.8.0') : false; } /** @@ -352,7 +354,7 @@ export class PolicyUtil { // if Prisma version is high enough to support filtering directly when // fetching a nullable to-one relation, let's do it that way // https://github.com/prisma/prisma/discussions/20350 - (supportNestedToOneFilter && fieldInfo.isOptional) + (this.supportNestedToOneFilter && fieldInfo.isOptional) ) { if (typeof injectTarget[field] !== 'object') { injectTarget[field] = {}; @@ -416,7 +418,7 @@ export class PolicyUtil { fieldInfo.isDataModel && !fieldInfo.isArray && // if Prisma version supports filtering nullable to-one relation, no need to further check - !(supportNestedToOneFilter && fieldInfo.isOptional) + !(this.supportNestedToOneFilter && fieldInfo.isOptional) ) { // to-one relation data cannot be trimmed by injected guards, we have to // post-check them From bffa5e13ff84cd69292f838ae61c469d542cd5e0 Mon Sep 17 00:00:00 2001 From: ymc9 <104139426+ymc9@users.noreply.github.com> Date: Mon, 24 Jul 2023 15:10:25 +0800 Subject: [PATCH 3/3] fix injection recursion --- .../src/enhancements/policy/policy-utils.ts | 3 +++ .../with-policy/deep-nested.test.ts | 26 +++++++++---------- 2 files changed, 15 insertions(+), 14 deletions(-) diff --git a/packages/runtime/src/enhancements/policy/policy-utils.ts b/packages/runtime/src/enhancements/policy/policy-utils.ts index 28250b0a3..91d1cf78e 100644 --- a/packages/runtime/src/enhancements/policy/policy-utils.ts +++ b/packages/runtime/src/enhancements/policy/policy-utils.ts @@ -361,6 +361,9 @@ export class PolicyUtil { } // inject extra condition for to-many or nullable to-one relation await this.injectAuthGuard(injectTarget[field], fieldInfo.type, 'read'); + + // recurse + await this.injectNestedReadConditions(fieldInfo.type, injectTarget[field]); } else { // there's no way of injecting condition for to-one relation, so if there's // "select" clause we make sure 'id' fields are selected and check them against diff --git a/tests/integration/tests/enhancements/with-policy/deep-nested.test.ts b/tests/integration/tests/enhancements/with-policy/deep-nested.test.ts index 4b8013ecb..8bd5bd6cc 100644 --- a/tests/integration/tests/enhancements/with-policy/deep-nested.test.ts +++ b/tests/integration/tests/enhancements/with-policy/deep-nested.test.ts @@ -394,13 +394,12 @@ describe('With Policy:deep nested', () => { }, }); - // delete read-back reject: M4 @@deny('read', value == 200) - await expect( - db.m1.delete({ - where: { myId: '1' }, - include: { m2: { select: { m4: true } } }, - }) - ).toBeRejectedByPolicy(['result is not allowed to be read back']); + // delete read-back filtered: M4 @@deny('read', value == 200) + const r = await db.m1.delete({ + where: { myId: '1' }, + include: { m2: { select: { m4: true } } }, + }); + expect(r.m2.m4).toHaveLength(1); await expect(db.m4.findMany()).resolves.toHaveLength(0); @@ -418,12 +417,11 @@ describe('With Policy:deep nested', () => { }, }); - // delete read-back reject: M3 @@deny('read', value == 200) - await expect( - db.m1.delete({ - where: { myId: '2' }, - include: { m2: { select: { m3: { select: { id: true } } } } }, - }) - ).toBeRejectedByPolicy(); + // delete read-back filtered: M3 @@deny('read', value == 200) + const r1 = await db.m1.delete({ + where: { myId: '2' }, + include: { m2: { select: { m3: { select: { id: true } } } } }, + }); + expect(r1.m2.m3).toBeNull(); }); });