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

Use strict mode in mysql #6069

Merged
merged 4 commits into from May 5, 2012

Conversation

Projects
None yet
9 participants
@mipearson
Contributor

mipearson commented Apr 30, 2012

MySQL, by default, opts to truncate or replace rather than 'fail fast' on incompatible data. This can lead to confusion, frustration and data loss.

This is inconsistent with two other popular database engines: PostgreSQL will error (since v 7.2) and sqlite will simply allow the original data (rather than modifying it).

Defaulting to STRICT_ALL_TABLES will ease error discovery and prevent new applications from depending on MySQL's quirks.

Original issue: #5988

@jeremyf

This comment has been minimized.

Show comment
Hide comment
@jeremyf

jeremyf Apr 30, 2012

Contributor

Without tests, it is likely that this would be merged. Also, I believe the two commits are not part of the same "concept".

(@jeremyf)

Contributor

jeremyf commented Apr 30, 2012

Without tests, it is likely that this would be merged. Also, I believe the two commits are not part of the same "concept".

(@jeremyf)

@carlosantoniodasilva

This comment has been minimized.

Show comment
Hide comment
@carlosantoniodasilva
Member

carlosantoniodasilva commented Apr 30, 2012

@mipearson

This comment has been minimized.

Show comment
Hide comment
@mipearson

mipearson Apr 30, 2012

Contributor

@jeremyf Correct, but I assumed that passing the AR tests would be sufficient. I need to get stuck into work now so I'll write the tests a bit later today. The schema.rb change was due to strict mode failing on that line, but I'm happy to split it.

Contributor

mipearson commented Apr 30, 2012

@jeremyf Correct, but I assumed that passing the AR tests would be sufficient. I need to get stuck into work now so I'll write the tests a bit later today. The schema.rb change was due to strict mode failing on that line, but I'm happy to split it.

@carlosantoniodasilva

This comment has been minimized.

Show comment
Hide comment
@carlosantoniodasilva

carlosantoniodasilva Apr 30, 2012

Member

@mipearson if the schema change was required to make the tests pass in strict mode, there's no need to split it. I'd just ask you to add a link to the docs that explains the reason for that in the commit where you do the change. Thanks!

Member

carlosantoniodasilva commented Apr 30, 2012

@mipearson if the schema change was required to make the tests pass in strict mode, there's no need to split it. I'd just ask you to add a link to the docs that explains the reason for that in the commit where you do the change. Thanks!

@jeremyf

This comment has been minimized.

Show comment
Hide comment
@jeremyf

jeremyf Apr 30, 2012

Contributor

I'll queue up a test run of this.

Contributor

jeremyf commented Apr 30, 2012

I'll queue up a test run of this.

@pda

This comment has been minimized.

Show comment
Hide comment
@pda

pda Apr 30, 2012

Contributor

Yes! MySQL default behaviour of silently mutating/truncating/dropping data and claiming success is awful; if we can save anybody from it at the framework level, we should.

Contributor

pda commented Apr 30, 2012

Yes! MySQL default behaviour of silently mutating/truncating/dropping data and claiming success is awful; if we can save anybody from it at the framework level, we should.

@jeremyf

This comment has been minimized.

Show comment
Hide comment
@jeremyf

jeremyf Apr 30, 2012

Contributor

mysql and mysql2 tests pass. Looks good!

cc @rafaelfranca

Contributor

jeremyf commented Apr 30, 2012

mysql and mysql2 tests pass. Looks good!

cc @rafaelfranca

@mipearson

This comment has been minimized.

Show comment
Hide comment
@mipearson

mipearson Apr 30, 2012

Contributor

Cheers. Getting some unit tests together for this now and I'm having a bit of trouble. I've put the test for mysql_adapter.rb in activerecord/test/cases/adapters/mysql/mysql_adapter_test.rb as follows:

      def test_mysql_in_strict_mode
        result = @connection.exec_query "SELECT @@SESSION.sql_mode"
        assert_equal [["STRICT_ALL_TABLES"]], result.rows
      end

However, I can't figure out where to put it for mysql2_adapter. Furthermore, there looks to be functionality in mysql2_adapter (eg, setting the encoding) that is untested.

Contributor

mipearson commented Apr 30, 2012

Cheers. Getting some unit tests together for this now and I'm having a bit of trouble. I've put the test for mysql_adapter.rb in activerecord/test/cases/adapters/mysql/mysql_adapter_test.rb as follows:

      def test_mysql_in_strict_mode
        result = @connection.exec_query "SELECT @@SESSION.sql_mode"
        assert_equal [["STRICT_ALL_TABLES"]], result.rows
      end

However, I can't figure out where to put it for mysql2_adapter. Furthermore, there looks to be functionality in mysql2_adapter (eg, setting the encoding) that is untested.

@mipearson

This comment has been minimized.

Show comment
Hide comment
@mipearson

mipearson Apr 30, 2012

Contributor

Oh, wait, you said "without tests, it is likely to be merged". I read that as 'unlikely' because I am sleepy.

Contributor

mipearson commented Apr 30, 2012

Oh, wait, you said "without tests, it is likely to be merged". I read that as 'unlikely' because I am sleepy.

@jeremy

This comment has been minimized.

Show comment
Hide comment
@jeremy

jeremy Apr 30, 2012

Member

👍

However, existing apps may rely on non-strict mode (perhaps accidentally) so being able to turn this off would help with Rails 4 upgrades. Perhaps add a strict config option, true by default?

Member

jeremy commented Apr 30, 2012

👍

However, existing apps may rely on non-strict mode (perhaps accidentally) so being able to turn this off would help with Rails 4 upgrades. Perhaps add a strict config option, true by default?

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Apr 30, 2012

Member

Agreed. I'm trying to set all my projects to strict mode and having a configuration for this in the Rails will be very handy. 👍

Member

rafaelfranca commented Apr 30, 2012

Agreed. I'm trying to set all my projects to strict mode and having a configuration for this in the Rails will be very handy. 👍

@josevalim

This comment has been minimized.

Show comment
Hide comment
@josevalim

josevalim Apr 30, 2012

Contributor

👍 for a config option. This option can be set by default in the new config/database.yml for mysql.

Contributor

josevalim commented Apr 30, 2012

👍 for a config option. This option can be set by default in the new config/database.yml for mysql.

@mipearson

This comment has been minimized.

Show comment
Hide comment
@mipearson

mipearson Apr 30, 2012

Contributor

Yeah, it's a good idea. I'd like it to default to on and to be disabled manually through database.yml. I'll see if I can get this + tests knocked out tonight.

Contributor

mipearson commented Apr 30, 2012

Yeah, it's a good idea. I'd like it to default to on and to be disabled manually through database.yml. I'll see if I can get this + tests knocked out tonight.

@mipearson

This comment has been minimized.

Show comment
Hide comment
@mipearson

mipearson May 5, 2012

Contributor

Config, tests done. Waiting on further feedback (cc @jeremyf / @josevalim)

Contributor

mipearson commented May 5, 2012

Config, tests done. Waiting on further feedback (cc @jeremyf / @josevalim)

@josevalim

This comment has been minimized.

Show comment
Hide comment
@josevalim
Contributor

josevalim commented May 5, 2012

@jeremy

View changes

Show outdated Hide outdated activerecord/lib/active_record/connection_adapters/mysql2_adapter.rb
@jeremy

View changes

Show outdated Hide outdated activerecord/lib/active_record/connection_adapters/mysql2_adapter.rb
t.text :preferences, :null => false, :default => ""
# MySQL does not allow default values for blobs. Fake it out with a
# big varchar below.
t.string :preferences, :null => false, :default => '', :limit => 1024

This comment has been minimized.

@jeremy

jeremy May 5, 2012

Member

Does changing this from text to varchar have side effects on other db tests?

@jeremy

jeremy May 5, 2012

Member

Does changing this from text to varchar have side effects on other db tests?

This comment has been minimized.

@mipearson

mipearson May 5, 2012

Contributor

No - if you trace the git blame you'll see this was originally added as an edge case test for people who wanted their store columns to be NOT NULL.

@mipearson

mipearson May 5, 2012

Contributor

No - if you trace the git blame you'll see this was originally added as an edge case test for people who wanted their store columns to be NOT NULL.

jeremy added a commit that referenced this pull request May 5, 2012

@jeremy jeremy merged commit 4d8bc1d into rails:master May 5, 2012

@mikhailov

This comment has been minimized.

Show comment
Hide comment
@mikhailov

mikhailov Jul 2, 2012

Contributor

a monkey-patched initializer to apply mysql strict mode to Rails 3.2:

class ActiveRecord::ConnectionAdapters::Mysql2Adapter

private
  alias_method :configure_connection_without_strict_mode, :configure_connection

  def configure_connection
    configure_connection_without_strict_mode
    strict_mode = "SQL_MODE='STRICT_ALL_TABLES'"
    execute("SET #{strict_mode}", :skip_logging)
  end
end
Contributor

mikhailov commented Jul 2, 2012

a monkey-patched initializer to apply mysql strict mode to Rails 3.2:

class ActiveRecord::ConnectionAdapters::Mysql2Adapter

private
  alias_method :configure_connection_without_strict_mode, :configure_connection

  def configure_connection
    configure_connection_without_strict_mode
    strict_mode = "SQL_MODE='STRICT_ALL_TABLES'"
    execute("SET #{strict_mode}", :skip_logging)
  end
end
@sodabrew

This comment has been minimized.

Show comment
Hide comment
@sodabrew

sodabrew Nov 27, 2012

Contributor

Hi, I see this was merged and is present in Rails 4. I'd like to suggest that this is a poor way to approach MySQL configuration, and should be replaced. There are quite a few possible values of SQL_MODE, and one that I think is really great is called TRADITIONAL (which includes STRICT_ALL_TABLES along with a few others).

See: http://dev.mysql.com/doc/refman/5.5/en/server-sql-mode.html#sqlmode_traditional

Way too many people have posted private method monkey patches around configure_connection that are about setting additional connection variables so, c'mon, if we're going to fix this, let's actually fix it. Rather than argue about which ones of these to give a specific keyword name to, we should just make it easy to specify the SET variables to send on each new connection from database.yml.

production:
  adapter: mysql2
  database: prod
  user: foo
  password: bar
  host: localhost
  encoding: utf8
  set:
    sql_mode: traditional

I'll open a new pull request with some code later on, but, hello and heads up.

Contributor

sodabrew commented Nov 27, 2012

Hi, I see this was merged and is present in Rails 4. I'd like to suggest that this is a poor way to approach MySQL configuration, and should be replaced. There are quite a few possible values of SQL_MODE, and one that I think is really great is called TRADITIONAL (which includes STRICT_ALL_TABLES along with a few others).

See: http://dev.mysql.com/doc/refman/5.5/en/server-sql-mode.html#sqlmode_traditional

Way too many people have posted private method monkey patches around configure_connection that are about setting additional connection variables so, c'mon, if we're going to fix this, let's actually fix it. Rather than argue about which ones of these to give a specific keyword name to, we should just make it easy to specify the SET variables to send on each new connection from database.yml.

production:
  adapter: mysql2
  database: prod
  user: foo
  password: bar
  host: localhost
  encoding: utf8
  set:
    sql_mode: traditional

I'll open a new pull request with some code later on, but, hello and heads up.

@jeremy

This comment has been minimized.

Show comment
Hide comment
@jeremy

jeremy Nov 27, 2012

Member

👍 to use TRADITIONAL sql mode and support an arbitrary settings: block in db config. Looking forward to your pull request, @sodabrew!

Member

jeremy commented Nov 27, 2012

👍 to use TRADITIONAL sql mode and support an arbitrary settings: block in db config. Looking forward to your pull request, @sodabrew!

@sodabrew

This comment has been minimized.

Show comment
Hide comment
@sodabrew

sodabrew Nov 28, 2012

Contributor

Pull request is live at #8346 -- I actually did not remove strict because it is also used for setting better column default value behavior that used to be very messy in the mysql drivers. The pull request includes a note about the potential overlap between strict and sql_mode.

Contributor

sodabrew commented Nov 28, 2012

Pull request is live at #8346 -- I actually did not remove strict because it is also used for setting better column default value behavior that used to be very messy in the mysql drivers. The pull request includes a note about the potential overlap between strict and sql_mode.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment