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

add user_id field in session table #72

Open
mo opened this issue Sep 17, 2017 · 18 comments
Open

add user_id field in session table #72

mo opened this issue Sep 17, 2017 · 18 comments

Comments

@mo
Copy link

mo commented Sep 17, 2017

When a user does a password reset / changes his password, it makes sense to drop all sessions for that user. Currently the user id is stored inside a json blob on the session, so it becomes very inefficient to query for all sessions that belong to a specific user.

In issue #53 ( #53 ), it was suggested that one way to workaround this issue is to, A) not use createDatabaseTable: true, B) add a user_id column, and C) right after login, update the session with the appropriate user_id.

However, this approach has the downside that "user_id" cannot be a "NOT NULL" column with a foreign key constraint pointing to the user table. This in turn would enable cascading deletes for sessions when a user is deleted.

      columnNames: {
        user_id: 'user_id',
        ...
     }

Do you agree that having such a column is a good idea, i.e. would you accept a PR to add one?

@chill117
Copy link
Owner

chill117 commented Sep 17, 2017

I don't like the idea of adding a user_id field to the sessions table. Most users of this module won't use it. I think there might be a way to do what you need without modifying the module. A few possibilities to explore:

  • Set the default value on your user_id field to NULL; so that the session can be inserted into the database table without an error. I found this stackoverflow post which could help.
  • Using a reference table (e.g user_sessions), but I am not sure if this could really work. I would have to try it.
  • Use a trigger to pull the user ID from the JSON object in the data field and set the user_id field on the session before it is inserted into the sessions table.

Hope that helps!

@mo
Copy link
Author

mo commented Sep 18, 2017

@chill117 What if the "user_id" property was optional and such a column was only created and set by express-mysql-session if it was defined in the columnNames dict?

@chill117
Copy link
Owner

chill117 commented Sep 20, 2017

@mo Maybe, but only if the other solutions are not workable. Even if a user of this module wanted to set a user_id column in their sessions table, maybe they don't want to use a foreign key? And how would the module know the name of their users table?

@chill117
Copy link
Owner

@mo How did this one end up?

@mo
Copy link
Author

mo commented Oct 11, 2017

If express-mysql-session creates the table and the optional columnNames.user_id is defined, then it should create a column field without a foreign key. This would make it possible for me to pre-create the session table WITH the foreign key constraint, and then I would pass "columnNames.user_id" to express-mysql-session ensuring that all rows will always have a correct value in the user_id field. This way, express-mysql-session doesn't have to know about the user table at all. Does this sound OK for a PR @chill117 ?

For now, I'm doing a second query that writes the user_id field and I have set my foreign key constraint to be in effect only when the value is NOT NULL. However, I really don't want to allow sessions with a NULL value so I would be more much comfortable if my database schema enforced this.

@chill117
Copy link
Owner

chill117 commented Oct 12, 2017

Please tell me see if I understand correctly. When a new option flag is true, then the following happens:

  • The createDatabaseTable function will add a user ID field to the sessions database table. This new field's name will also be configurable via the schema.columnNames option.
  • The set(session_id, data, cb) function will set the value of this new user ID field equal to whatever is passed via data.user_id.

At least one issue I see with this:

  • How to know what type to use for the user ID field on the sessions table if it is created by this module?

Instead of making this a one-off solution for user IDs, let's try a more general solution. A new option that defines keys in the session data object that should be written to distinct columns in each session row. For example:

var sessionStore = new MySQLStore({
    dataWithOwnColumns: [ 'user_id', 'some_other_field' ]
});

Where the database table looks like this:

CREATE TABLE IF NOT EXISTS `sessions` (
  `session_id` varchar(128) COLLATE utf8mb4_bin NOT NULL,
  `expires` int(11) unsigned NOT NULL,
  `data` text COLLATE utf8mb4_bin,
  `user_id` int(11) unsigned,
  `some_other_field` varchar(255),
  PRIMARY KEY (`session_id`)
) ENGINE=InnoDB

The module user (developer) would be responsible for creating their own sessions table with the correct columns.

Then whenever sessionStore.set(session_id, data, cb) is called, the columns for the new/existing session will be set from values provided in the data object.

@chill117
Copy link
Owner

Is there still interest in this issue?

@mo
Copy link
Author

mo commented Feb 12, 2018

I'm still interested in this feature.

The field that passport puts into "data" for me is called "user" but the database column I have is called "user_id". Ideally, I think it's good if the session storage does not mandate certain column names so it would be nice to find a solution that ensures this, even for "dataWithOwnColumns" columns.

Did you mean that dataWithOwnColumns could be combined with columns, like this:

{
    createDatabaseTable: false,
    schema: {
      tableName: 'my_session_table',

      // note that I changed this to "user" instead of "user_id" since it refers to the data property
      dataWithOwnColumns: [ 'user' ],

      columnNames: {
        session_id: 'session_id',
        expires: 'expires',
        data: 'data',
        user: 'user_id'
      }
    }
  }
}

I suppose another option would be to simply omit the "dataWithOwnColumns" member, and instead make a rule that says:

If there is any property KEY:VAL inside columnNames dict where KEY is not "session_id", "expires" or "data", then the session store will create a column called VAL and assume that the data for it is in data[KEY]

{
    createDatabaseTable: false,
    schema: {
      tableName: 'my_session_table',
      columnNames: {
        session_id: 'session_id',
        expires: 'expires',
        data: 'data',
        user: 'user_id'
      }
    }
  }
}

@dimabolgov
Copy link

Hey guys, this function is still needed. I would like to write to this new column user_uuid value (binary16).
Have you come to any decision?

@chill117
Copy link
Owner

chill117 commented Feb 7, 2019

It's still not clear to me how to proceed with this one. I don't want to introduce foot-guns with a feature that most users of this module won't need. But if you have some suggestion for how to implement this, I am willing to listen.

@bcmyguest
Copy link

bcmyguest commented May 3, 2019

Using a reference table (e.g user_sessions), but I am not sure if this could really work. I would have to try it.

This solution appears to work fine @demjanich

@HoldYourWaffle
Copy link
Contributor

I'm not sure I completely understand what @mo is suggesting here, but wouldn't an implementation of #91 fix the inefficiency problem?
I haven't used the JSON column type yet since it didn't exist the last time I did something with MySQL, but a quick google search tells me it allows for more efficient querying using something like JSON_EXTRACT.

@mo
Copy link
Author

mo commented May 4, 2019

The things I would like are:

  • fast queries (i.e no JSON lookup)
  • foreign key from session table pointing to user table
  • user_id on session should be NOT NULL

The two last points are important if you want your db schema to ensure that the data is consistent.


My proposed solution is; if express-mysql-session gets:

      columnNames: {
        ...
        user: 'user_id'
      }

...then it will insert the user id into a column called "user_id". This would make it possible to pre-create the session table with the foreign key / not null.

@mo
Copy link
Author

mo commented May 4, 2019

My first workaround attempt was to run "res.end()" so that express-mysql-session creates the session in the db. Then I used an UPDATE query to fill in the user_id column. One drawback of this was that user_id could not be NOT NULL and after running this in prod I had a few junk sessions lying around with user_id=NULL. When I investigated why, I noticed that about 1 times out of 50 the express-mysql-session INSERT query would be done after the UPDATE query due to a race condition between the two. To reproduce this bug I had to run my E2E testsuite hundreds of times consecutively. This is why db schemas that enforce consistency are useful.

A slightly better solution (still hacky but it works) is to do something like:

await db.query(
      'INSERT INTO your_session_table(session_id, expires, user_id) VALUES (?, ?, ?) ON DUPLICATE KEY UPDATE user_id = VALUES(user_id)',
      [req.session.id, sessionExpiresSeconds, user.user_id])

...because then it doesn't matter if the INSERT or the UPDATE happens first. However, the best solution would be to fix express-mysql-session so that it allows users to have a strong consistent db schema.

@madacol
Copy link

madacol commented Oct 22, 2019

I agree with HoldYourWaffle, this could be partially solved along with #91 by implementing an opt-in JSON data type for the data column. I say partially, because this still does not allow foreign key constraints for cascading deletes.

But has fast reads, no JSON lookups.
From MYSQL docs:

JSON documents stored in JSON columns are converted to an internal format that permits quick read access to document elements. When the server later must read a JSON value stored in this binary format, the value need not be parsed from a text representation. The binary format is structured to enable the server to look up subobjects or nested values directly by key or array index without reading all values before or after them in the document.

And for cascading deletes you could workaround with a trigger

@chill117
Copy link
Owner

chill117 commented Sep 6, 2021

I've recently rolled out a project that uses express with a postgreSQL database and a compatible express session store. I wanted to achieve the same result as @mo as discussed in this issue - i.e. to be able to clear all sessions for a single user. I was able to do this by using a reference table (user sessions) which has two columns:

  • User ID - with a foreign key on the user table's ID column w/ cascade on delete
  • Session ID - with a foreign key on the session table's ID column w/ cascade on delete

I insert a new row into this reference table whenever a user authenticates a new session. Whenever it is needed to logout the user from all their active sessions, a single SQL query can get all session IDs from the reference table and delete all matching sessions in the sessions table. This requires no changes to either the users table nor the sessions table.

@M-Scott-Lassiter
Copy link

For the record, this is functionality I was looking for as well and had to hand roll a solution. I disagree this wouldn't be useful for most users.

To be clear, I'm not saying we would have to set foreign keys, but the data is all already there in the session info. Just tap into that req.user object and tack it on in its own column. At that point, you can leave it up to the user to do whatever with the data.

And of course, we could make this an optional field/functionality as well if people really didn't want it.

@eMTyMe
Copy link

eMTyMe commented Feb 9, 2024

So, is there now a way to define new columns with their own data? Since in the README under Custom database table schema is stated:

This can be useful if you want to have extra columns (e.g. "user_id"), indexes, foreign keys, etc.

If so, how does it work? Can we do something like:

const storeOptions = {
  host: ...,
  port: ...,
  ...,
  createDatabaseTable: false,
  schema: {
    tableName: 'sessions',
    columnNames: {
      session_id: 'session_id',
      expires: 'expires',
      data: 'data',
      someCustomData: 'custom_data'
    }
  }
}

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

8 participants