Skip to content
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

fix(client): delete transactionId field in middleware #7662

Merged
merged 6 commits into from Jun 15, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
@@ -0,0 +1 @@
!dev.db
Binary file not shown.
@@ -0,0 +1,12 @@
{
"name": "my-prisma-project",
"version": "1.0.0",
"description": "",
"main": "index.js",
"scripts": {
"test": "echo \"Error: no test specified\" && exit 1"
},
"keywords": [],
"author": "",
"license": "ISC"
}
@@ -0,0 +1,28 @@
datasource db {
provider = "sqlite"
url = "file:dev.db"
}

generator client {
provider = "prisma-client-js"
}

// / User model comment
model User {
id String @id @default(uuid())
email String @unique
// / name comment
name String?
posts Post[]
}

model Post {
id String @id @default(cuid())
createdAt DateTime @default(now())
updatedAt DateTime @updatedAt
published Boolean
title String
content String?
authorId String?
author User? @relation(fields: [authorId], references: [id])
}
@@ -0,0 +1,30 @@
import { getTestClient } from '../../../../utils/getTestClient'

describe('middleware and transaction', () => {
test('typeof next reponse', async () => {
const PrismaClient = await getTestClient()
const db = new PrismaClient()

let response
db.$use(async (params, next) => {
response = await next(params)

return response
})

await db.$transaction([
db.user.create({
data: {
email: 'test@test.com',
name: 'test',
},
}),
])

expect(response.email).toMatchInlineSnapshot(`test@test.com`)

await db.user.deleteMany()

db.$disconnect()
})
})
69 changes: 33 additions & 36 deletions src/packages/client/src/runtime/getPrismaClient.ts
Expand Up @@ -435,16 +435,16 @@ export function getPrismaClient(config: GetPrismaClientOptions): any {
* Hook a middleware into the client
* @param middleware to hook
*/
$use(middleware: QueryMiddleware)
$use(namespace: 'all', cb: QueryMiddleware)
$use(namespace: 'engine', cb: EngineMiddleware)
$use(
arg0: Namespace | QueryMiddleware,
arg1?: QueryMiddleware | EngineMiddleware,
$use<T>(middleware: QueryMiddleware<T>)
$use<T>(namespace: 'all', cb: QueryMiddleware<T>)
$use<T>(namespace: 'engine', cb: EngineMiddleware<T>)
$use<T>(
arg0: Namespace | QueryMiddleware<T>,
arg1?: QueryMiddleware | EngineMiddleware<T>,
) {
// TODO use a mixin and move this into MiddlewareHandler
if (typeof arg0 === 'function') {
this._middlewares.query.use(arg0)
this._middlewares.query.use(arg0 as QueryMiddleware)
} else if (arg0 === 'all') {
this._middlewares.query.use(arg1 as QueryMiddleware)
} else if (arg0 === 'engine') {
Expand Down Expand Up @@ -915,45 +915,42 @@ new PrismaClient({
* @param middlewareIndex
* @returns
*/
private _request(
private async _request(
Jolg42 marked this conversation as resolved.
Show resolved Hide resolved
internalParams: InternalRequestParams,
middlewareIndex = 0,
): Promise<any> {
try {
// in this recursion, we check for our terminating condition
const middleware = this._middlewares.query.get(middlewareIndex)
let index = -1
// async scope https://github.com/prisma/prisma/issues/3148
const resource = new AsyncResource('prisma-client-request')
// make sure that we don't leak extra properties to users
const params: QueryMiddlewareParams = {
args: internalParams.args,
dataPath: internalParams.dataPath,
runInTransaction: internalParams.runInTransaction,
action: internalParams.action,
model: internalParams.model,
}

// prepare recursive fn that will pipe params through middlewares
const consumer = (changedParams: QueryMiddlewareParams) => {
// if this `next` was called and there's some more middlewares
const nextMiddleware = this._middlewares.query.get(++index)

if (middleware) {
// make sure that we don't leak extra properties to users
const params: QueryMiddlewareParams = {
args: internalParams.args,
dataPath: internalParams.dataPath,
runInTransaction: internalParams.runInTransaction,
action: internalParams.action,
model: internalParams.model,
if (nextMiddleware) {
// we pass the modfied params down to the next one, & repeat
return nextMiddleware(changedParams, consumer)
}

return resource.runInAsyncScope(() => {
// call the middleware of the user & get their changes
return middleware(params, (changedParams) => {
// this middleware returns the value of the next one 🐛
return this._request(
{
...internalParams,
...changedParams,
},
++middlewareIndex,
) // recursion happens over here
})
})
const changedInternalParams = { ...internalParams, ...params }

// TODO remove this, because transactionId should be passed?
if (index > 0) delete changedInternalParams['transactionId']
Comment on lines +944 to +945
Copy link
Member

Choose a reason for hiding this comment

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

TODO for now?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes

Copy link
Member Author

Choose a reason for hiding this comment

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

We will need to remove this, because middlewares with transactions makes it not to be executed as a regular transaction (removes transactionId)

Copy link
Member

Choose a reason for hiding this comment

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

Can we make an issue out of this so we can remember and prioritize? Thanks.

Copy link
Member Author

@millsp millsp Jun 15, 2021

Choose a reason for hiding this comment

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

This is preventing the transactions from rolling back. If enabled, transactions do rollback, but introduce those bugs we've seen on studio. I'm keeping track of this here #6705 (comment)


// no middleware? then we just proceed with request execution
return this._executeRequest(changedInternalParams)
}

// they're finished, or there's none, then execute request
return resource.runInAsyncScope(() => {
return this._executeRequest(internalParams)
})
return resource.runInAsyncScope(() => consumer(params))
} catch (e) {
e.clientVersion = this._clientVersion

Expand Down