feat(mysql): Ensure db connections always run in strict mode. #221
Conversation
| // Enforce sane defaults on every new connection. | ||
| // These *should* be set by the database by default, but it's nice | ||
| // to have an additional layer of protection here. | ||
| this._pool.on('connection', function(connection) { |
rfk
Nov 14, 2016
Author
Member
In theory the overhead of doing this won't be too bad, because we only do it when we create a new connection for the pool, and we then re-use that connection for many queries.
One downside of doing this by listening to an event though, is that we can't block until the query has been completed. So there'd probably be a race condition here, where calling code could use the connection in between the SELECT @@sql_mode and the SET SESSION sql_mode.
In theory the overhead of doing this won't be too bad, because we only do it when we create a new connection for the pool, and we then re-use that connection for many queries.
One downside of doing this by listening to an event though, is that we can't block until the query has been completed. So there'd probably be a race condition here, where calling code could use the connection in between the SELECT @@sql_mode and the SET SESSION sql_mode.
seanmonstar
Nov 14, 2016
Member
To prevent this race condition, this code could exist in the _getConnection method down below. It can then only resolve the connection once this query has completed, and done.
To prevent this race condition, this code could exist in the _getConnection method down below. It can then only resolve the connection once this query has completed, and done.
rfk
Nov 14, 2016
Author
Member
It wasn't obvious to me whether all the various helper methods used _getConnection though, it looks like e.g. _query() just directly takes a connection from the pool. Is _getConnection intended to replace the pool's default connection-getting logic uniformly?
It wasn't obvious to me whether all the various helper methods used _getConnection though, it looks like e.g. _query() just directly takes a connection from the pool. Is _getConnection intended to replace the pool's default connection-getting logic uniformly?
seanmonstar
Nov 14, 2016
Member
Oh interesting. It seems we could make use of _getConnection inside _query, and then yes, we can be sure the session is always set correctly.
Currently, it looks like _getConnection was written so that we can release the connection after using it, but that in most cases we just use pool.query which keeps the connection internal and thus we never worry about releasing it.
Oh interesting. It seems we could make use of _getConnection inside _query, and then yes, we can be sure the session is always set correctly.
Currently, it looks like _getConnection was written so that we can release the connection after using it, but that in most cases we just use pool.query which keeps the connection internal and thus we never worry about releasing it.
rfk
Nov 14, 2016
Author
Member
Also, ISTM that doing this logic in _getConnection would cause it to be run on every query, while using the pool event means it runs only when we create a new connection.
Also, ISTM that doing this logic in _getConnection would cause it to be run on every query, while using the pool event means it runs only when we create a new connection.
seanmonstar
Nov 15, 2016
Member
We could set a flag, like connection._fxa_strict = true, to only do it once per connection.
We could set a flag, like connection._fxa_strict = true, to only do it once per connection.
vladikoff
Nov 24, 2016
Contributor
👍
|
|
||
| function MysqlStore(options) { | ||
| if (options.charset && options.charset !== 'UTF8_UNICODE_CI') { | ||
| logger.warn('createDatabase', { charset: options.charset }); | ||
| if (options.charset && options.charset !== 'UTF8MB4_UNICODE_CI') { |
vladikoff
Nov 24, 2016
Contributor
nit: extract 'UTF8MB4_UNICODE_CI' into const REQUIRED_CHARSET = ...;
nit: extract 'UTF8MB4_UNICODE_CI' into const REQUIRED_CHARSET = ...;
|
@jrgm any other feedback? |
|
I've updated this based on discussion, and added some simple tests. @seanmonstar r? |
|
Looks clean to me, great! |
| 'STRICT_ALL_TABLES', | ||
| 'NO_ENGINE_SUBSTITUTION', | ||
| ]; | ||
| const REQUIRED_CHARSET = 'UTF8MB4_UNICODE_CI'; |
seanmonstar
Jan 5, 2017
Member
Yay moving to const.
Yay moving to const.
|
@jrgm per discussion in the meeting, the implications of strict mode are described here: http://dev.mysql.com/doc/refman/5.7/en/sql-mode.html#sql-mode-strict I don't see anything to worry about in there, and TBH if we are doing something that's affected by strict mode, I consider it a bug we need to uncover and fix ASAP. This works well in travis and my local testing, so I'm going to optimistically merge it and keep an eye on behaviour in latest. |
This was an idea I had to enforce mysql strict mode and utf8mb4 on all db connections. It's possible (and advisable!) to set these defaults in the MySQL server itself, but we can also enforce them here as an additional layer of protection.
No tests yet, because we don't have any test infra for mocking out MySQL in this repo.
@jrgm @vladikoff thoughts?