zenstackhq / zenstack

Fullstack TypeScript toolkit that enhances Prisma ORM with flexible Authorization layer for RBAC/ABAC/PBAC/ReBAC, offering auto-generated type-safe APIs and frontend hooks.
https://zenstack.dev
MIT License
2.07k stars 88 forks source link

regression: delegate models don't inherit abstract base properties #1588

Closed chunkerchunker closed 2 months ago

chunkerchunker commented 2 months ago

Description and expected behavior

The following schema compiles incorrectly in version 2.3.0 but worked correctly in previous versions:

abstract model Base {
    foo String
}

model Base2 {
    id String @id @default(cuid())
    type String

    @@delegate(type)
}

model Item extends Base2 {
    x String?
}

In version 2.3.0, model Item doesn't inherit the property foo.

Attached is a simple test case: issue-delegate-abstract-inheritance.test.ts.txt

Environment (please complete the following information):

ymc9 commented 2 months ago

Thanks for filing this @chunkerchunker ! I'll look into it and make a patch fix as needed.

ymc9 commented 2 months ago

Hi @chunkerchunker , in the case you provided, the abstract base model Base seems never used. Did you miss anything in it?

ruizehung commented 2 months ago

I faced similar issues after upgrading to v2.3.0. Here's my .zmodel definition

abstract model TrackCreateUpdateTime {
    createdAt DateTime @default(now())
    updatedAt DateTime @updatedAt
}

model InvitationBase extends TrackCreateUpdateTime {
    id           Int       @id @default(autoincrement())

    inviterId    String
    inviter      User      @relation(fields: [inviterId], references: [id], onDelete: Cascade)
    inviteeEmail String // Invitee's email

    type         String

    @@delegate(type)

    @@index([inviteeEmail])

    // Inviter can create, update, and delete their own invitations
    @@allow("all", auth() == inviter)
    // Invitee can read and update their own invitations
    @@allow("read, update, delete", inviteeEmail == auth().email)
}

model TeamMembershipInvitation extends InvitationBase {
    teamId String
    team   TeamBase           @relation(fields: [teamId], references: [id], onDelete: Cascade)

    role   TeamMembershipRole @deny("update", inviteeEmail == auth().email) // Invitee can't change their own role 
}

There's a drift in generated prisma schema using zenstack v2.2.4 compared to v2.3.0 without changing .zmodel files. The left is generated using zenstack v2.2.4. The right is zenstack v2.3.0

image

You can see that the createdAt and updatedAt field got removed from the generated prisma file. And my guess is that somehow TeamMembershipInvitation does inherit from TrackCreateUpdateTime

chunkerchunker commented 2 months ago

Hi @chunkerchunker , in the case you provided, the abstract base model Base seems never used. Did you miss anything in it?

Sorry! Base2 is supposed to inherit Base in my example, but adding that fixes the problem.

It turns out that the issue isn't what I thought. It seems that prior to 2.3.0, properties inherited from abstract base classes were propagated to delegates as well as any intermediate base classes. So, in the example, the generated prisma schema declares property foo on both Base2 and Item. In 2.3.0, the generated prisma schema only declares property foo on Base2. I assume that 2.3.0 is the correct behavior; it's just that the resulting schema migration was very confusing.

Bottom line, I think this is not a bug, but the change in behavior might confuse other people as well (including @ruizehung above).

ymc9 commented 2 months ago

Hi @chunkerchunker @ruizehung , thanks for providing your examples. Yes, this is indeed an "accidental fix". Fields inherited from abstract models should "stop" at the first non-abstract model. The field inheritance between two non-abstract models is represented via an internal relation between them, so the fields shouldn't be duplicated in the generated "physical" prisma schema. In previous versions, even though the duplicated fields are generated, they shouldn't be effective at runtime.

My apologies for the confusion caused by this.

chunkerchunker commented 2 months ago

Thanks @ymc9 ! Sorry for the false bug report :)