alexedwards / scs

HTTP Session Management for Go
MIT License
2.02k stars 165 forks source link

Custom names for sessions tables #180

Open alexedwards opened 8 months ago

alexedwards commented 8 months ago

As per the comments on https://github.com/alexedwards/scs/pull/177, consider supporting custom names for the sessions tables where it makes sense.

doggedOwl commented 8 months ago

Table name is an implementation detail of the relational db stores so I think it's fine to implement it store by store like in the PR at #177.

For consistency it may be usefull to add it to all of stores backed by a relational db, more or less the equivalent of the prefix option for the KV stores.

Setting the tablename after init though (using SetSessionsTableName like in #177) does not seem right so maybe it would be better to have a new constructor: func NewWithOptions(pool *pgxpool.Pool, cleanupInterval time.Duration, tableName string) *PostgresStore

This again mirrors more or less the func NewWithOptions(client *api.Client, cleanupInterval time.Duration, prefix string) *ConsulStore for example.

alexedwards commented 8 months ago

I like the idea of a NewWithOptions() approach. Perhaps we could make Options a struct and pass it as a 2nd parameter to allow further options in the future without making breaking changes to the public API. So, for postgresstore for example:

type Options struct {
    CleanupInterval time.Duration
    TableName string
}

Which could then be used like:

postgresstore.NewWithOptions(db, postgresstore.Options{
    CleanupInterval: time.Minute,
    TableName: "mycustomsessions",
})

If any fields in Options are not provided (i.e. they contain the relevant zero value), then we can use a sensible default (i.e. cleanup interval of 5 minutes, table name of "sessions").

I think we could potentially roll this pattern out across all the different stores and deprecate the NewWithPrefix() and NewWithCleanupInterval() functions on the individual stores, leaving each store with two ways to initialize it: either New() (which uses the defaults) or NewWithOptions() which allows you to customize the behavior in whatever way makes sense for that particular store.

The only problem I can see right now is that the consulstore package already has a NewWithOptions() function with a different signature. So we could either:

a. Make a breaking change to consulstore. b. Not implement the Options struct pattern in consulstore. c. Think of an alternative name for NewWithOptions(). I'm blanking on a decent name. Any suggestions?

Kraysent commented 6 months ago

That would be a nice feature!

Maybe instead of whole separate constructor one might consider using Functional Options pattern. See pull request #190 for the prototype.

In this case, no backwards compatibility is broken: New(db) will fall back to the same names as now. If one wants to change the default name (and schema for that matter) of the table as well as column names, they can use nice syntax:

New(db,
  WithSessionTableName("schema.session_table"),
  WithExpiryColumnName("expiry_date"),
  WithTokenColumnName("token"),
  // data column will not change!
)

As a bonus, one may change cleanupInterval the same way (also without breaking backwards compatibility):

New(db, WithCleanupInterval(10*time.Minute))

It seems to me that such implementation would solve all the problems above and is applicable to any other store. Feel free to point at anything I might have forgotten!