rails-sqlserver / activerecord-sqlserver-adapter

SQL Server Adapter For Rails
MIT License
972 stars 558 forks source link

Is add_check_constraint supported? #1140

Closed AnalyzePlatypus closed 10 months ago

AnalyzePlatypus commented 10 months ago

Issue

It appears that the migration method add_check_constraint doesn't actually do anything.

When added to a migration, the method executes successfully while silently not generating or executing any SQL. After sniffing around the codebase, it appears that the method is not implemented by this adaptor, but it isn't marked as unsupported either.

This adaptor should either:

  1. Add the supports_check_constraints? method and set it to false
  2. Implement the check constraint related Active Record methods.

Expected behavior

I tried adding a check constraint to my SQL Server database with add_check_constraint in this migration:

class AddJSONCheck < ActiveRecord::Migration[7.0]
  def change
    create_table :my_table do |t|
      t.json :metadata
    end

    # This is the important bit:
    add_check_constraint :my_table, "ISJSON(metadata)=1"
  end
end

This should generate and execute the following SQL statement:

ALTER TABLE my_table
ADD CONSTRAINT chk_rails_<identifier> CHECK (ISJSON(metadata)=1)

The database should then forbid non-JSON values from being inserted in the metadata column.

Actual behavior

After migrating, the database still allows non-JSON values.

After digging through the migration logs, it seems like the necessary SQL is never generated or executed.

The migration logs (captured with rails c --sandbox) show this:

> rails console --sandbox
Loading development environment in sandbox (Rails 7.0.8)
Any modifications you make will be rolled back on exit
> ActiveRecord::MigrationContext.new(Rails.root.join('db', 'migrate'), ActiveRecord::SchemaMigration).migrate
[REDACTED]/.rvm/gems/ruby-3.2.2/gems/activerecord-sqlserver-adapter-7.0.4.0/lib/active_record/connection_adapters/sqlserver_adapter.rb:111: warning: undefining the allocator of T_DATA class TinyTds::Result
  SQL (1.6ms)  USE [msdb]
  TRANSACTION (4.0ms)  BEGIN TRANSACTION
  ActiveRecord::SchemaMigration Pluck (0.9ms)  SELECT [schema_migrations].[version] FROM [schema_migrations] ORDER BY [schema_migrations].[version] ASC
Migrating to AddJSONCheck (20231119091451)
== 20231119091451 AddJSONCheck: migrating ===================================
-- add_check_constraint(:my_table, "ISJSON(metadata)=1")
   -> 0.0000s
== 20231119091451 AddJSONCheck: migrated (0.0026s) ==========================

  TRANSACTION (3.0ms)  SAVE TRANSACTION active_record_1
  ActiveRecord::SchemaMigration Create (6.2ms)  EXEC sp_executesql N'INSERT INTO [schema_migrations] ([version]) OUTPUT INSERTED.[version] VALUES (@0)', N'@0 nvarchar(4000)', @0 = N'20231119091451'  [["version", nil]]
  ActiveRecord::InternalMetadata Load (4.5ms)  EXEC sp_executesql N'SELECT [ar_internal_metadata].[key], [ar_internal_metadata].[value], [ar_internal_metadata].[created_at], [ar_internal_metadata].[updated_at] FROM [ar_internal_metadata] WHERE [ar_internal_metadata].[key] = @0 ORDER BY [ar_internal_metadata].[key] ASC OFFSET 0 ROWS FETCH NEXT @1 ROWS ONLY', N'@0 nvarchar(4000), @1 int', @0 = N'environment', @1 = 1  [["key", nil], ["LIMIT", nil]]

This seems to indicate that add_check_constraint hasn't generated or executed any SQL. This console line is also suspicious: -- add_check_constraint(:my_table, "ISJSON(metadata)=1") -> 0.0000s

The migration took less than a millisecond? My local roundtrip to the DB takes about 1.6ms, not less than 0.

How to reproduce

This failing test reproduces the issue. The test case should raise ActiveRecord::StatementInvalid, but instead the database accepts the invalid value.

If you uncomment the raw SQL in the migration, the test passes successfully.

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"
  gem "tiny_tds"
  gem "activerecord", "7.0.8"
  gem "activerecord-sqlserver-adapter", "7.0.4.0"
end

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

ActiveRecord::Base.establish_connection(
  adapter:  "sqlserver",
  timeout:  5000,
  pool:     100,
  encoding: "utf8",
  database: "msdb",
  username: "SA",
  password: "yourStrong(!)Password",
  host:     "localhost",
  port:     1433,
)
ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Schema.define do
  drop_table :bug_tests rescue nil

  create_table :bug_tests, force: true do |t|
    t.json :metadata
  end

  add_check_constraint :bug_tests, "ISJSON(metadata)=1"

  # Uncommenting this makes the test pass 
  # execute <<-SQL
  #     ALTER TABLE bug_tests
  #     ADD CONSTRAINT metadata_must_be_json CHECK (ISJSON(metadata, OBJECT)=1);
  # SQL
end

class BugTest < ActiveRecord::Base
end

class TestBugTest < Minitest::Test
  def test_count

    invalid_json = "blah!"
    expected_error_message = %Q(TinyTds::Error: The INSERT statement conflicted with the CHECK constraint "metadata_must_be_json". The conflict occurred in database "msdb", table "dbo.bug_tests", column 'metadata'.)

    error = assert_raises(ActiveRecord::StatementInvalid) {
        BugTest.create!(metadata: invalid_json)
    }

    assert_equal expected_error_message, error.message
  end
end

Details

% tsql -C
Compile-time settings (established with the "configure" script)
                            Version: freetds v1.4.6
             freetds.conf directory: /opt/homebrew/etc
     MS db-lib source compatibility: no
        Sybase binary compatibility: yes
                      Thread safety: yes
                      iconv library: yes
                        TDS version: 7.3
                              iODBC: no
                           unixodbc: yes
              SSPI "trusted" logins: no
                           Kerberos: yes
                            OpenSSL: yes
                             GnuTLS: no
                               MARS: yes

Workaround

For now, users can implement check constraints by writing raw SQL:

class MyMigration < ActiveRecord::Migration[7.0]
  def up    
    execute <<-SQL
      ALTER TABLE my_table
      ADD CONSTRAINT metadata_must_be_json CHECK (ISJSON(metadata, OBJECT)=1);
    SQL
  end

  def down
    execute <<-SQL
      ALTER TABLE my_table
      DROP CONSTRAINT metadata_must_be_json;
    SQL
  end
end
Michoels commented 10 months ago

Gosh, that was fast :)

aidanharan commented 10 months ago

Support for check constraints has been added to v7.0.6. Thanks for reporting @AnalyzePlatypus

AnalyzePlatypus commented 10 months ago

Oh wow, amazing! Thanks.