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

Duplicate escaping of quotes in check constraint expressions (MySQL) #42424

Closed
Flixt opened this issue Jun 8, 2021 · 7 comments · Fixed by #42429
Closed

Duplicate escaping of quotes in check constraint expressions (MySQL) #42424

Flixt opened this issue Jun 8, 2021 · 7 comments · Fixed by #42429

Comments

@Flixt
Copy link
Contributor

Flixt commented Jun 8, 2021

Steps to reproduce

With a check constraint in the (MySQL) database and the Rails project set to use the schema.rb format I end up with duplicate escape sequences for quotes in the expression of the check constraint.

  1. Add a migration that adds a check constraints with an expression that includes quotes.
  2. Run the migration (it runs fine)
  3. Check the generated schema.rb
  4. The generated schema.rb contains duplicate escaping for the quotes in the check constraint expression
  5. Trying to load the schema rails db:schema:load leads to an SQL syntax error

Here is an executable version of the problem (I used MySQLVer 8.0.23 for osx10.15 on x86_64):

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  gem "activerecord", "~> 6.1.3"
  gem "mysql2"
end

require "active_record"
require "minitest/autorun"
require "logger"

ActiveRecord::Base.establish_connection(
  adapter: "mysql2",
  database: 'check_constraint_schema_dump_bug',
  charset: 'utfmb4',
  encoding: 'utf8mb4',
  collation: 'utf8mb4_unicode_ci',
  username: ENV.fetch("MYSQL_USERNAME") { "rails" },
  password: ENV.fetch("MYSQL_PASSWORD") { "" },
  host: ENV.fetch("MYSQL_HOST") { "127.0.0.1" },
  port: ENV.fetch("MYSQL_PORT") { 3306 }
)
ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Schema.define do
  create_table :posts, force: true do |t|
    t.string :title
    t.check_constraint "(title <> 'forbidden_title')"
  end
end

class BugTest < Minitest::Test
  def test_adapter_check_constraint_quoting
    expected = %q(`title` <> _utf8mb4'forbidden_title')

    assert_equal ActiveRecord::Base.connection.check_constraints('posts').first.expression, expected
  end

  def test_schema_dumper_not_including_wrong_quotes
    dumped_schema = ActiveRecord::SchemaDumper.dump(ActiveRecord::Base.connection, StringIO.new).string

    wrong = %q(t.check_constraint "`title` <> _utf8mb4\\\\'forbidden_title\\\\'")
    refute_includes dumped_schema, wrong
  end

  def test_schema_dumper_including_correct_quotes
    dumped_schema = ActiveRecord::SchemaDumper.dump(ActiveRecord::Base.connection, StringIO.new).string

    expected = %q(t.check_constraint "`title` <> _utf8mb4'forbidden_title'")
    assert_includes dumped_schema, expected
  end
end

Expected behavior

E.g. a migration like this:

create_table :posts, force: true do |t|
  t.string :title
  t.check_constraint "(title <> 'forbidden_title')"
end

should generate a schema like that:

create_table "posts", charset: "utf8mb4", force: :cascade do |t|
  t.string "title"
  t.check_constraint "`title` <> _utf8mb4'forbidden_title'"
end

Actual behavior

It generates a schema like this:

create_table "posts", charset: "utf8mb4", force: :cascade do |t|
  t.string "title"
  t.check_constraint "`title` <> _utf8mb4\\'forbidden_title\\'" # <-- Notice the slashes
end

System configuration

Rails version: 6.1.3.2

Ruby version: 2.6.3

Potential fix

I created a monkey patch which fixes the issue for my project locally:

module ActiveRecord
  module ConnectionAdapters
    class AbstractMysqlAdapter
      def check_constraints(table_name)
        if supports_check_constraints?
          scope = quoted_scope(table_name)

          chk_info = exec_query(<<~SQL, "SCHEMA")
            SELECT cc.constraint_name AS 'name',
                  cc.check_clause AS 'expression'
            FROM information_schema.check_constraints cc
            JOIN information_schema.table_constraints tc
            USING (constraint_schema, constraint_name)
            WHERE tc.table_schema = #{scope[:schema]}
              AND tc.table_name = #{scope[:name]}
              AND cc.constraint_schema = #{scope[:schema]}
          SQL

          chk_info.map do |row|
            options = {
              name: row["name"]
            }
            expression = row["expression"].gsub("\\'", "'") # <-- replace duplicate escaping
            expression = expression[1..-2] unless mariadb? # remove parentheses added by mysql
            CheckConstraintDefinition.new(table_name, expression, options)
          end
        else
          raise NotImplementedError
        end
      end
    end
  end
end

If this goes into the right direction I'd open up a pull request and write the corresponding tests.
Diff: https://github.com/rails/rails/compare/main...Flixt:fix-mysql-check-constraints-quoting?expand=1

@rafaelfranca
Copy link
Member

Yes. Please open the PR.

@Flixt
Copy link
Contributor Author

Flixt commented Jun 10, 2021

Just for documentation purposes, the problem here is similar to #41156.

@Flixt
Copy link
Contributor Author

Flixt commented Jun 17, 2021

The PR is ready to merge, would be cool if somebody can have a look :)

@brunodccarvalho
Copy link

Bump, what needs doing here? Are there any issues with the attached MR?

@skipkayhil
Copy link
Member

I went ahead and re opened the PR, I'll see if I can get somebody to take a look

@Flixt
Copy link
Contributor Author

Flixt commented Jul 26, 2023

In case somebody will have a look. I rebased the PR onto main.

@povilasjurcys
Copy link

Is there any progress on this? I'm facing the same issue I would be more than happy to see this fix merged

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

Successfully merging a pull request may close this issue.

6 participants