search

kleydon / prisma-session-store

Express session store for Prisma
MIT License
116 stars 18 forks source link

only one id column if dbRecordIdIsSessionId is true #40

Closed andreasgerner closed 3 years ago

andreasgerner commented 3 years ago

Currently, if I set dbRecordIdIsSessionId: true, the Session Model needs an "id" and a "sid"-column which store exactly the same values. Wouldn't it be simpler to be able to change the Session model to remove the id-column when dbRecordIdIsSessionId: true is set?

So instead of:

model Session {
  id        String   @id
  sid       String   @unique
  data      String
  expiresAt DateTime @db.Timestamptz(6)
}

The following would do the trick:

model Session {
  sid       String   @id
  data      String
  expiresAt DateTime @db.Timestamptz(6)
}
wSedlacek commented 3 years ago

What are we attempting to solve here? Consumption ergonomics or...?

wSedlacek commented 3 years ago

For implementing support for both of these interfaces we would need to know at this point which interface we were using. https://github.com/kleydon/prisma-session-store/blob/8941f85564128bfb2cb1238aebe7670ddb1a3c55/src/lib/prisma-session-store.ts#L377

Afaik Prisma throws if you provide it additional fields it was not expecting, so we couldn't just provide the ID and have Prisma ignore it. The differences in the generated Prisma client afaik would be purely type information which is all transpiled away at runtime. So afaik there would be only two ways to determine the shape of the model at runtime (a) Make a request to the database to get an instance of the model and compare it (requires at least one instance of the model to be present) (b) Use reflected metadata emitted by TypeScript (which would require projects to be configured to emit that metadata)

The only other solution I can think of to know the model is to make a Prisma generator which I have thought about but requires additional research and development.

So while the ergonomics for the consumption of this library would improve with that change, it may not be feasible to support that interface at this time unless there is something I am overlooking.

andreasgerner commented 3 years ago

I tried some ways of finding a solution and there are several problems:

So, maybe introducing a new option would be a good start, but that will result in a big mess of ts-ignore because the compiler still uses the model with the Id-Column.

Other suggestions?

wSedlacek commented 3 years ago

There is still the question of why? Is it so necessary that we drop the id column?

I mean I get that it isn't normalized when using the SID as the id column but ultimately this table is used for relatively short lived sessions and shouldn't have any foreign keys. So is it really so important that it follows the absolute best SQL practices if it makes the runtime code more complicated or the library less type safe?

andreasgerner commented 3 years ago

Oh god my bad, I totally forgot the checkPeriod-option and just wanted to reduce storage size. With that option in mind, I think that feature is not needed.

wSedlacek commented 3 years ago

Oh god my bad, I totally forgot the checkPeriod-option and just wanted to reduce storage size. With that option in mind, I think that feature is not needed.

All good. If a single string column is the limiting factor on our database then you probably have other issues to address.

Thank you for the suggestion and enjoy using the library.