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

Possibility to skip some models #45

Closed
igeligel opened this issue Nov 22, 2023 · 23 comments
Closed

Possibility to skip some models #45

igeligel opened this issue Nov 22, 2023 · 23 comments

Comments

@igeligel
Copy link

Would like to have an option to disable the linter for some models.

Proposal:

/* prisma-case-format-disable */

model Account {
  id                String  @id @default(cuid())
  userId            String 
  type              String
  provider          String
  providerAccountId String  
  refreshToken      String?
  accessToken       String? 
  expiresAt         Int?    
  tokenType         String? 
  scope             String?
  idToken           String?
  sessionState      String?

  user User @relation(fields: [userId], references: [id], onDelete: Cascade)

  @@unique([provider, providerAccountId])
}
/* prisma-case-format-enable */

I came across this issue when wanting to use next-auth

@iiian
Copy link
Owner

iiian commented Nov 22, 2023

Hey @igeligel, thanks for the suggestion. What is next-auth, is that part of the nextjs ecosystem? If you don't mind me asking, what is the existing problem that you're facing, which these annotations would solve?

@dgellow
Copy link

dgellow commented Nov 26, 2023

Hi @iiian,

you're correct next-auth is part of the next.js ecosystem. It is a package that makes it fairly simple to setup authentication for a website. It supports various providers, and Prisma is one of them. You can check the provider documentation here, the expected schema has strict requirements for casing, some fields of the generated Prisma client are expected to be snake_case (such as token_type, expires_at), while other need to be camelCase (providerAccountId, userId).

We faced the same issue as @igeligel, I've myself been bitten by the casing while cleaning up our DB schema. I now would like to rely on a tool such as prisma-case-format as a linter to ensure we use consistent names. But that would require to either be able to disable prisma-case-format for next-auth's models, or a way to specify (via comments?) the case to use for a specific column.

@iiian
Copy link
Owner

iiian commented Nov 26, 2023

Got it, thanks for motivating the need--the problem makes sense to me now. Just commenting, seems like the disable feature is useful, and simultaneously the same could be achieved if just overrides for syntax expectations were allowed:

// prisma-case-format:override
// --field-case=camel
// --map-field-case=snake
model GotchaNextAuthModel {
  // now fields are forced to format as camel, with snake @map anno's
  tokenType String @map('token_type')
  expiresAt String @map('expires_at')
}
// prisma-case-format:end-override

That way, prisma-case-format can enforce the case-convention deviation.

@iiian
Copy link
Owner

iiian commented Nov 26, 2023

I like comments as the trigger, but we'll probably end up wanting to do a commandline arg as well, I'll have to think on that.

Ultimately, I think I hope to support this feature by the end of the year--is that soon enough?

@dgellow
Copy link

dgellow commented Nov 26, 2023

The override syntax is a neat idea, that would make it nicer to handle models that require a specific casing.

Looking at next-auth models, I would say that Account is the most problematic one, because it expects a mix of casing, that makes it a good test case to try out different strategies.

An idea would be to support override comments on the same line, in addition to an override block like the one you suggested.

// 👇 set a default case for the model, for both the Prisma client and columns @map()  
//
// prisma-case-format:override(field-case=camel,map-field-case=snake)
model Account {
  id                 String  @id @default(cuid())
  userId             String
  type               String
  provider           String
  providerAccountId  String
                                        // 👇 override some columns, only --field-case
  refresh_token      String?  @db.Text  // prisma-case-format:override(field-case=snake)
  access_token       String?  @db.Text  // prisma-case-format:override(field-case=snake)
  expires_at         Int?               // prisma-case-format:override(field-case=snake)
  token_type         String?            // prisma-case-format:override(field-case=snake)
  
  scope              String?
  id_token           String?  @db.Text  // prisma-case-format:override(field-case=snake)
  session_state      String?

  user User @relation(fields: [userId], references: [id], onDelete: Cascade)

  @@unique([provider, providerAccountId])
}
// prisma-case-format:end-override

Here the logic would be straight forward:

  1. if a line starts with prisma-case-format:override, start an override block
  2. if an inlined comment starts with prisma-case-format:override, override only for that specific line
  3. if a line starts with prisma-case-format:end-override, stop the override block

I don't think something more complicated such as nested override blocks would make too much sense here, that wouldn't be too readable.

What do you think? I slightly changed your override syntax to make it simpler to use inlined, but that's just a suggestion and my personal preference 😄.

Ultimately, I think I hope to support this feature by the end of the year--is that soon enough?

Oh for sure, yes!

@iiian
Copy link
Owner

iiian commented Nov 26, 2023

Yep sounds good to me -- I guess we'll just have to say "it has to be the first part of a comment", because we're seizing control of the inline comment, which might clash with other functionality put forth by prisma or other 3rd parties (why I'm generally opposed to adding control flags directly within the file, but in this case it seemed harmless as far as we were going with disable stuff.

I'm almost wondering if there's not value in just creating a --has-next-auth flag, that applies a pre-defined set of case-conventions.

Are there any comments pushing back against @vercel doing this to y'all? Seems arbitrary and poorly published

@dgellow
Copy link

dgellow commented Nov 26, 2023

I had to look it up because I wasn't sure but it doesn't seem to be an official project made by Vercel: https://opencollective.com/nextauth. The package name sounds official but it is made by the community at large.

I'm myself not too familiar with Next.js and its community, I couldn't tell you if this schema weirdness is something people push back against or if it seen as normal. I literally learned about next-auth 2 days ago when doing changes to tables used by a Next.js application and broke signin/signup after renaming some specific columns used by next-auth's Github provider (they aren't documented anywhere on next-auth website or GitHub repo, I only found out about it thanks to Prisma having really good error messages). So, not really a fan :)

That's why I've been looking for a linter/auto-formatter.

because we're seizing control of the inline comment, which might clash with other functionality put forth by prisma or other 3rd parties

Yes I agree, that's a fair point.

@dgellow
Copy link

dgellow commented Nov 26, 2023

I found this comment:

Thanks, but this looks correct to me. NextAuth.js provided properties are camelCase while the ones based on the OAuth standard are following snake_case

@iiian
Copy link
Owner

iiian commented Nov 26, 2023

I'll start with the inline // prisma-case-format:disable ... // prisma-case-format:disable-end feature, but I think an .prisma-case-format file could be a reasonable introduction:

# alternative to specifying cmd line args
defaults:
  table: pascal,plural
  field: camel
  enum:  pascal
  mapTable: snake
  mapField: snake
  mapEnum:  snake
uses_next_auth: true # optional: apply default conventions for "next-auth" models
overrides:
  ModelX:
    defaults: # specify overridden defaults for this scope
      table: snake,singular
      mapTable: pascal
    fields: # apply element-wise overrides
      # alternate short-hand; note this works in model-wise defaults & global defaults
      userId: field=pascal;mapField=pascal,plural
  other_model: disable # shut it off entirely  

@iiian
Copy link
Owner

iiian commented Nov 27, 2023

Okay, so I ended up not going for the //prisma-case-format:disable comments at all, since its manageable from the .prisma-case-format file.

The next PR will contain updates for the README.md, but here's an example of the syntax:

# file: .prisma-case-format
uses_next_auth: false
default: ...
override: #edited -- not overrides!
  Account:
    default: 'table=pascal; mapTable=pascal;'
    field:
      id:                'field=camel; mapField=camel'
      userId:            'field=camel; mapField=camel'
      type:              'field=camel; mapField=camel'
      provider:          'field=camel; mapField=camel'
      providerAccountId: 'field=camel; mapField=camel'
      refresh_token:     'field=snake; mapField=snake'
      access_token:      'field=snake; mapField=snake'
      expires_at:        'field=snake; mapField=snake'
      token_type:        'field=snake; mapField=snake'
      scope:             'field=snake; mapField=snake'
      id_token:          'field=snake; mapField=snake'
      session_state:     'field=snake; mapField=snake'
      user:              'field=snake; mapField=snake'
  Session:
    default: 'table=pascal; mapTable=pascal; field=camel; mapField=camel'
  User:
    default: 'table=pascal; mapTable=pascal; field=camel; mapField=camel'
  VerificationToken:
    default: 'table=pascal; mapTable=pascal; field=camel; mapField=camel'

note that the above example is strictly equivalent to

# file: .prisma-case-format
uses_next_auth: true

or to specifying the commandline argument --uses-next-auth.

@iiian
Copy link
Owner

iiian commented Nov 27, 2023

prisma-case-format@2.1.0-beta.0 is available for download, can you give it a spin and let me know if it looks okay?

@igeligel
Copy link
Author

Hey @iiian,
This looks great! Will try out today! I might also add some documentation around it if not existing already. Thanks so much for doing these changes so quickly!

@dgellow
Copy link

dgellow commented Nov 27, 2023

That was fast! Thanks so much, I will try it out right away :)

@dgellow
Copy link

dgellow commented Nov 27, 2023

Ok, I tried the beta version.

Here is the diff I get after running prisma-case-format -f services/shared/prisma/schema.prisma -c services/shared/prisma/.prisma-case-format:

--- a/services/shared/prisma/schema.prisma
+++ b/services/shared/prisma/schema.prisma
@@ -12,24 +12,24 @@ generator client {
 /// Table required by Next-Auth
 model Account {
   /// Columns required by Next-Auth
-  id                       String  @id @default(cuid())
-  userId                   String  @map("user_id")
-  user                     User    @relation(fields: [userId], references: [id], onDelete: Cascade)
-  type                     String
-  provider                 String
-  providerAccountId        String  @map("provider_account_id")
-  refresh_token_expires_in Int?
-  refresh_token            String? @db.Text
-  access_token             String? @db.Text
-  expires_at               Int?
-  token_type               String?
-  scope                    String?
-  id_token                 String? @db.Text
-  session_state            String?
+  id                    String  @id @default(cuid())
+  userId                String  @map("user_id")
+  user                  User    @relation(fields: [userId], references: [id], onDelete: Cascade)
+  type                  String
+  provider              String
+  providerAccountId     String  @map("provider_account_id")
+  refreshTokenExpiresIn Int?    @map("refresh_token_expires_in")
+  refreshToken          String? @map("refresh_token") @db.Text
+  accessToken           String? @map("access_token") @db.Text
+  expiresAt             Int?    @map("expires_at")
+  tokenType             String? @map("token_type")
+  scope                 String?
+  idToken               String? @map("id_token") @db.Text
+  sessionState          String? @map("session_state")

My config file:

# file: .prisma-case-format
default: 'table=pascal; mapTable=snake,plural; field=camel; mapField=snake;'
overrides:
  Account:
    default: 'table=pascal; mapTable=pascal;'
    field:
      id: 'field=camel; mapField=camel'
      userId: 'field=camel; mapField=camel'
      type: 'field=camel; mapField=camel'
      provider: 'field=camel; mapField=camel'
      providerAccountId: 'field=camel; mapField=camel'
      refresh_token_expires_in: 'field=snake; mapField=snake'
      refresh_token: 'field=snake; mapField=snake'
      access_token: 'field=snake; mapField=snake'
      expires: 'field=snake; mapField=snake'
      token_type: 'field=snake; mapField=snake'
      scope: 'field=snake; mapField=snake'
      id_token: 'field=snake; mapField=snake'
      session_state: 'field=snake; mapField=snake'
      user: 'field=snake; mapField=snake'
  Session:
    default: 'table=pascal; mapTable=pascal; field=camel; mapField=camel'
  User:
    default: 'table=pascal; mapTable=pascal; field=camel; mapField=camel'
  VerificationToken:
    default: 'table=pascal; mapTable=pascal; field=camel; mapField=camel'

Looking at fields such as session_state, id_token, token_type, the snake case overrides don't seem to be applied.

I also noticed an issue for fields mapping to a DB column with a different name (not just a different case):

@@ -73,121 +73,121 @@ model User {

 /// Table required by Next-Auth
 model VerificationToken {
   /// Columns required by Next-Auth
   identifier String
   token      String   @unique
-  expires    DateTime @map("expires_at") @db.Timestamptz(3)
+  expires    DateTime @db.Timestamptz(3)

   @@unique([identifier, token])
   @@map("verification_tokens")
 }

Here the DB column is expires_at, and the field next-auth expects is expires, but prisma-case-format removes the @map.

@iiian
Copy link
Owner

iiian commented Nov 27, 2023

Did you try it with --uses-next-auth as well? I swear I tested that earlier and it worked. In any case, I'll check if this was a regression, thanks for feedback

@iiian
Copy link
Owner

iiian commented Nov 27, 2023

Okay, bizarre. It seems to be working when you use --uses-next-auth, but not the equivalent config I provided above.

@iiian
Copy link
Owner

iiian commented Nov 27, 2023

😄 it's override not overrides. Should it be either? To make matters worse, I told you it was overrides, so sorry about that.

Anyway, renaming it to override should fix the problem, but even simpler is just specifying --uses-next-auth or setting uses_next_auth: true in the .prisma-case-format file.

@igeligel
Copy link
Author

igeligel commented Dec 4, 2023

Btw. tried and it works! Thanks so much!

@iiian
Copy link
Owner

iiian commented Dec 4, 2023

Great suggestion, I think somebody else had previously requested this feature but didn't do a great job of explaining it

@dgellow
Copy link

dgellow commented Dec 5, 2023

Thanks @iiian for the quick implementation 🙏. And updates to the readme are clear and easy to follow, that's appreciated!

@igeligel
Copy link
Author

Some bug report:

 npm run prisma-case-format

> web@0.0.1 prisma-case-format
se=snake

E:\programming\get-ready-app\apps\web\node_modules\prisma-case-format\bin\convention-store.js:270
                return this.children[(0, change_case_1.snakeCase)(next)]._recurse(k, rest);
                                                                         ^

TypeError: Cannot read properties of undefined (reading '_recurse')
    at ConventionStore._recurse (E:\programming\get-ready-app\apps\web\node_modules\prisma-case-format\bin\convention-store.js:270:74)
    at ConventionStore._recurse (E:\programming\get-ready-app\apps\web\node_modules\prisma-case-format\bin\convention-store.js:249:40)
    at ConventionStore.isDisabled (E:\programming\get-ready-app\apps\web\node_modules\prisma-case-format\bin\convention-store.js:213:27)
    at ConventionTransformer.reshapeModelFields (E:\programming\get-ready-app\apps\web\node_modules\prisma-case-format\bin\convention-transformer.js:209:31)
    at ConventionTransformer.migrateCaseConventions (E:\programming\get-ready-app\apps\web\node_modules\prisma-case-format\bin\convention-transformer.js:46:67)
    at E:\programming\get-ready-app\apps\web\node_modules\prisma-case-format\bin\cli.js:126:87
    at Generator.next (<anonymous>)
    at E:\programming\get-ready-app\apps\web\node_modules\prisma-case-format\bin\cli.js:9:71
    at new Promise (<anonymous>)
    at __awaiter (E:\programming\get-ready-app\apps\web\node_modules\prisma-case-format\bin\cli.js:5:12)

Node.js v20.8.1
npm ERR! Lifecycle script `prisma-case-format` failed with error:
npm ERR! Error: command failed
npm ERR!   in workspace: web@0.0.1
npm ERR!   at location: E:\programming\get-ready-app\apps\web

config:

# file: .prisma-case-format
uses_next_auth: true

schema:

model Account {
  id                 String    @id @default(cuid())
  userId             String    @map("user_id")
  providerType       String    @map("provider_type")
  providerId         String    @map("provider_id")
  providerAccountId  String
  refreshToken       String?   @map("refresh_token")
  accessToken        String?   @map("access_token")
  accessTokenExpires DateTime? @map("access_token_expires")
  createdAt          DateTime  @default(now()) @map("created_at")
  updatedAt          DateTime  @updatedAt @map("updated_at")
  user               User      @relation(fields: [userId], references: [id])

  @@unique([providerId, providerAccountId])
  @@map("account")
}

If the @map parameter already existed on the schema definition, the error will be thrown. I think most of the people will not get into this state. Just wanted to highlight this!

@iiian
Copy link
Owner

iiian commented Dec 20, 2023

Thanks @igeligel ! I was actually trying to replicate this.

@iiian
Copy link
Owner

iiian commented Dec 24, 2023

Where is providerType String @map("provider_type") coming from? Is that a real next-auth thing or are we just trying to see what we can break (which would be appreciated, btw)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants