From 19268edb233679fc75f75c536f26159a740ccaca Mon Sep 17 00:00:00 2001 From: Aidan Haran Date: Sun, 18 Jan 2026 21:09:30 +0000 Subject: [PATCH 1/8] Introduce Parameter Object: QueryIntent Ref: https://github.com/rails/rails/pull/55897 --- .../sqlserver/database_statements.rb | 52 ++++++++++++++----- .../sqlserver/schema_statements.rb | 3 +- .../connection_adapters/sqlserver/showplan.rb | 4 +- 3 files changed, 42 insertions(+), 17 deletions(-) diff --git a/lib/active_record/connection_adapters/sqlserver/database_statements.rb b/lib/active_record/connection_adapters/sqlserver/database_statements.rb index daf635d63..0c1af7849 100644 --- a/lib/active_record/connection_adapters/sqlserver/database_statements.rb +++ b/lib/active_record/connection_adapters/sqlserver/database_statements.rb @@ -13,10 +13,12 @@ def write_query?(sql) # :nodoc: !READ_QUERY.match?(sql.b) end - def perform_query(raw_connection, sql, binds, type_casted_binds, prepare:, notification_payload:, batch:) - unless binds.nil? || binds.empty? - types, params = sp_executesql_types_and_parameters(binds) - sql = sp_executesql_sql(sql, types, params, notification_payload[:name]) + def perform_query(raw_connection, intent) + sql = intent.processed_sql + + unless intent.binds.nil? || intent.binds.empty? + types, params = sp_executesql_types_and_parameters(intent.binds) + sql = sp_executesql_sql(intent.processed_sql, types, params, intent.notification_payload[:name]) end id_insert_table_name = query_requires_identity_insert?(sql) @@ -30,8 +32,8 @@ def perform_query(raw_connection, sql, binds, type_casted_binds, prepare:, notif end verified! - notification_payload[:affected_rows] = affected_rows - notification_payload[:row_count] = result.count + intent.notification_payload[:affected_rows] = affected_rows + intent.notification_payload[:row_count] = result.count result end @@ -62,14 +64,34 @@ def internal_exec_sql_query(sql, conn) finish_statement_handle(handle) end - def exec_delete(sql, name = nil, binds = []) - sql = sql.dup << "; SELECT @@ROWCOUNT AS AffectedRows" - super + # Executes the delete statement and returns the number of rows affected. + def delete(arel, name = nil, binds = []) + intent = QueryIntent.new(arel: arel, name: name, binds: binds) + + # Compile Arel to get SQL + compile_arel_in_intent(intent) + + # Start of monkey-patch + sql = intent.processed_sql.present? ? intent.processed_sql : intent.raw_sql + intent.processed_sql = "#{sql}; SELECT @@ROWCOUNT AS AffectedRows" + # End of monkey-patch + + affected_rows(raw_execute(intent)) end - def exec_update(sql, name = nil, binds = []) - sql = sql.dup << "; SELECT @@ROWCOUNT AS AffectedRows" - super + # Executes the update statement and returns the number of rows affected. + def update(arel, name = nil, binds = []) + intent = QueryIntent.new(arel: arel, name: name, binds: binds) + + # Compile Arel to get SQL + compile_arel_in_intent(intent) + + # Start of monkey-patch + sql = intent.processed_sql.present? ? intent.processed_sql : intent.raw_sql + intent.processed_sql = "#{sql}; SELECT @@ROWCOUNT AS AffectedRows" + # End of monkey-patch + + affected_rows(raw_execute(intent)) end def begin_db_transaction @@ -237,9 +259,11 @@ def execute_procedure(proc_name, *variables) end.join(", ") sql = "EXEC #{proc_name} #{vars}".strip - log(sql, "Execute Procedure") do |notification_payload| + intent = QueryIntent.new(processed_sql: sql) + + log(intent, "Execute Procedure") do |notification_payload| with_raw_connection do |conn| - result = internal_raw_execute(sql, conn) + result = internal_raw_execute(intent.processed_sql, conn) verified! options = {as: :hash, cache_rows: true, timezone: ActiveRecord.default_timezone || :utc} diff --git a/lib/active_record/connection_adapters/sqlserver/schema_statements.rb b/lib/active_record/connection_adapters/sqlserver/schema_statements.rb index d5a28f438..ab6313f4b 100644 --- a/lib/active_record/connection_adapters/sqlserver/schema_statements.rb +++ b/lib/active_record/connection_adapters/sqlserver/schema_statements.rb @@ -33,7 +33,8 @@ def drop_table(*table_names, **options) def indexes(table_name) data = begin - select("EXEC sp_helpindex #{quote(table_name)}", "SCHEMA") + intent = QueryIntent.new(raw_sql: "EXEC sp_helpindex #{quote(table_name)}", name: "SCHEMA") + select(intent) rescue [] end diff --git a/lib/active_record/connection_adapters/sqlserver/showplan.rb b/lib/active_record/connection_adapters/sqlserver/showplan.rb index 30932ee72..9990fd7e9 100644 --- a/lib/active_record/connection_adapters/sqlserver/showplan.rb +++ b/lib/active_record/connection_adapters/sqlserver/showplan.rb @@ -30,8 +30,8 @@ def with_showplan_on end def set_showplan_option(enable = true) - sql = "SET #{showplan_option} #{enable ? "ON" : "OFF"}" - raw_execute(sql, "SCHEMA") + intent = QueryIntent.new(raw_sql: "SET #{showplan_option} #{enable ? "ON" : "OFF"}", name: "SCHEMA") + raw_execute(intent) rescue raise ActiveRecordError, "#{showplan_option} could not be turned #{enable ? "ON" : "OFF"}, perhaps you do not have SHOWPLAN permissions?" end From e896635d32ffa3a622884ff30f37c6a6a7383e6a Mon Sep 17 00:00:00 2001 From: Aidan Haran Date: Sun, 25 Jan 2026 15:43:55 +0000 Subject: [PATCH 2/8] Fix query cache for pinned connections in multi threaded transactional tests Ref: https://github.com/rails/rails/pull/55703 --- test/support/core_ext/query_cache.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/support/core_ext/query_cache.rb b/test/support/core_ext/query_cache.rb index 0679c42aa..f72db5c56 100644 --- a/test/support/core_ext/query_cache.rb +++ b/test/support/core_ext/query_cache.rb @@ -23,7 +23,7 @@ module SqlIgnoredCache def cache_sql(sql, name, binds) result = super - @query_cache.instance_variable_get(:@map).delete_if do |cache_key, _v| + query_cache.instance_variable_get(:@map).delete_if do |cache_key, _v| # Query cache key generated by `sql` or `[sql, binds]`, so need to retrieve `sql` for both cases. cache_key_sql = Array(cache_key).first Regexp.union(IGNORED_SQL).match?(cache_key_sql) From 11ce49d2b43e569e615a23caf4ea9ce228813fc0 Mon Sep 17 00:00:00 2001 From: Aidan Haran Date: Sun, 25 Jan 2026 16:13:26 +0000 Subject: [PATCH 3/8] Make ActiveRecord::Assertions::QueryAssertions method outputs consistent Ref: https://github.com/rails/rails/pull/55852 --- test/cases/coerced_tests.rb | 4 ++-- test/support/query_assertions.rb | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/test/cases/coerced_tests.rb b/test/cases/coerced_tests.rb index 0615ff254..b53273df9 100644 --- a/test/cases/coerced_tests.rb +++ b/test/cases/coerced_tests.rb @@ -2704,12 +2704,12 @@ def test_assert_queries_match_coerced error = assert_raises(Minitest::Assertion) { assert_queries_match(/ASC OFFSET 0 ROWS FETCH NEXT @0 ROWS ONLY/i, count: 2) { Post.first } } - assert_match(/1 instead of 2 queries/, error.message) + assert_match(/1 instead of 2 matching queries/, error.message) error = assert_raises(Minitest::Assertion) { assert_queries_match(/ASC OFFSET 0 ROWS FETCH NEXT @0 ROWS ONLY/i, count: 0) { Post.first } } - assert_match(/1 instead of 0 queries/, error.message) + assert_match(/1 instead of 0 matching queries/, error.message) end end end diff --git a/test/support/query_assertions.rb b/test/support/query_assertions.rb index 78dfce02d..5b47ec9df 100644 --- a/test/support/query_assertions.rb +++ b/test/support/query_assertions.rb @@ -14,9 +14,9 @@ def assert_queries_count(count = nil, include_schema: false, &block) # End of monkey-patch if count - assert_equal count, queries.size, "#{queries.size} instead of #{count} queries were executed. Queries: #{queries.join("\n\n")}" + assert_equal count, queries.size, "#{queries.size} instead of #{count} queries were executed#{queries.empty? ? '' : ". Queries:\n\n#{queries.join("\n\n")}"}" else - assert_operator queries.size, :>=, 1, "1 or more queries expected, but none were executed.#{"\nQueries:\n#{queries.join("\n")}" unless queries.empty?}" + assert_operator queries.size, :>=, 1, "1 or more queries expected, but none were executed" end result end From 31dba7b009bb980491c0f0d19ac71485a7131050 Mon Sep 17 00:00:00 2001 From: Aidan Haran Date: Sat, 7 Feb 2026 16:20:49 +0000 Subject: [PATCH 4/8] Run CI against commit --- Dockerfile.ci | 2 +- compose.ci.yaml | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Dockerfile.ci b/Dockerfile.ci index 74f636508..a2dc06155 100644 --- a/Dockerfile.ci +++ b/Dockerfile.ci @@ -9,6 +9,6 @@ WORKDIR $WORKDIR COPY . $WORKDIR -RUN RAILS_BRANCH=main bundle install --jobs `expr $(cat /proc/cpuinfo | grep -c "cpu cores") - 1` --retry 3 +RUN RAILS_COMMIT=65dc2163e3 bundle install --jobs `expr $(cat /proc/cpuinfo | grep -c "cpu cores") - 1` --retry 3 CMD ["sh"] diff --git a/compose.ci.yaml b/compose.ci.yaml index 8242db9b8..12fcae5ce 100644 --- a/compose.ci.yaml +++ b/compose.ci.yaml @@ -4,7 +4,7 @@ services: ci: environment: - ACTIVERECORD_UNITTEST_HOST=sqlserver - - RAILS_BRANCH=main + - RAILS_COMMIT=65dc2163e3 build: context: . dockerfile: Dockerfile.ci @@ -13,7 +13,7 @@ services: - "sqlserver" standardrb: environment: - - RAILS_BRANCH=main + - RAILS_COMMIT=65dc2163e3 build: context: . dockerfile: Dockerfile.ci From 030fb6cd6b1813a05d56c06c51a0e74c9b0b8f76 Mon Sep 17 00:00:00 2001 From: Aidan Haran Date: Sat, 7 Feb 2026 16:32:27 +0000 Subject: [PATCH 5/8] Upgrade minitest to match Rails --- Gemfile | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Gemfile b/Gemfile index cbb406565..deb73d543 100644 --- a/Gemfile +++ b/Gemfile @@ -11,7 +11,8 @@ gem "pg", "1.5.9" gem "sqlite3", ">= 2.1" gem "tzinfo-data", platforms: [:mingw, :mswin, :x64_mingw, :jruby] gem "benchmark-ips" -gem "minitest", ">= 5.15.0" +gem "minitest", "~> 6.0" +gem "minitest-mock" gem "msgpack", ">= 1.7.0" if ENV["RAILS_SOURCE"] From ccb2021e677e97ec066183fae1db77d96b532154 Mon Sep 17 00:00:00 2001 From: Aidan Haran Date: Sat, 7 Feb 2026 17:02:08 +0000 Subject: [PATCH 6/8] Need to include clearing query cache for delete/update --- .../sqlserver/database_statements.rb | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/lib/active_record/connection_adapters/sqlserver/database_statements.rb b/lib/active_record/connection_adapters/sqlserver/database_statements.rb index 0c1af7849..86f2924f4 100644 --- a/lib/active_record/connection_adapters/sqlserver/database_statements.rb +++ b/lib/active_record/connection_adapters/sqlserver/database_statements.rb @@ -66,30 +66,38 @@ def internal_exec_sql_query(sql, conn) # Executes the delete statement and returns the number of rows affected. def delete(arel, name = nil, binds = []) + # Clear query cache if the connection pool is configured to do so. + if pool.dirties_query_cache + ActiveRecord::Base.clear_query_caches_for_current_thread + end + intent = QueryIntent.new(arel: arel, name: name, binds: binds) # Compile Arel to get SQL compile_arel_in_intent(intent) - # Start of monkey-patch + # Add `SELECT @@ROWCOUNT` to the end of the SQL to get the number of affected rows. This is needed because SQL Server does not return the number of affected rows in the same way as other databases. sql = intent.processed_sql.present? ? intent.processed_sql : intent.raw_sql intent.processed_sql = "#{sql}; SELECT @@ROWCOUNT AS AffectedRows" - # End of monkey-patch affected_rows(raw_execute(intent)) end # Executes the update statement and returns the number of rows affected. def update(arel, name = nil, binds = []) + # Clear query cache if the connection pool is configured to do so. + if pool.dirties_query_cache + ActiveRecord::Base.clear_query_caches_for_current_thread + end + intent = QueryIntent.new(arel: arel, name: name, binds: binds) # Compile Arel to get SQL compile_arel_in_intent(intent) - # Start of monkey-patch + # Add `SELECT @@ROWCOUNT` to the end of the SQL to get the number of affected rows. This is needed because SQL Server does not return the number of affected rows in the same way as other databases. sql = intent.processed_sql.present? ? intent.processed_sql : intent.raw_sql intent.processed_sql = "#{sql}; SELECT @@ROWCOUNT AS AffectedRows" - # End of monkey-patch affected_rows(raw_execute(intent)) end From 7d65d793ff266d20a6be49ddd1100ddecf43ca9b Mon Sep 17 00:00:00 2001 From: Aidan Haran Date: Sun, 8 Feb 2026 17:40:05 +0000 Subject: [PATCH 7/8] Fix tests --- .../connection_adapters/sqlserver/database_statements.rb | 4 +++- test/support/query_assertions.rb | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/lib/active_record/connection_adapters/sqlserver/database_statements.rb b/lib/active_record/connection_adapters/sqlserver/database_statements.rb index 86f2924f4..48cd4b31f 100644 --- a/lib/active_record/connection_adapters/sqlserver/database_statements.rb +++ b/lib/active_record/connection_adapters/sqlserver/database_statements.rb @@ -78,6 +78,7 @@ def delete(arel, name = nil, binds = []) # Add `SELECT @@ROWCOUNT` to the end of the SQL to get the number of affected rows. This is needed because SQL Server does not return the number of affected rows in the same way as other databases. sql = intent.processed_sql.present? ? intent.processed_sql : intent.raw_sql + ensure_writes_are_allowed(sql) if write_query?(sql) intent.processed_sql = "#{sql}; SELECT @@ROWCOUNT AS AffectedRows" affected_rows(raw_execute(intent)) @@ -85,7 +86,7 @@ def delete(arel, name = nil, binds = []) # Executes the update statement and returns the number of rows affected. def update(arel, name = nil, binds = []) - # Clear query cache if the connection pool is configured to do so. + # Clear query cache if the connection pool is configured to do so. if pool.dirties_query_cache ActiveRecord::Base.clear_query_caches_for_current_thread end @@ -97,6 +98,7 @@ def update(arel, name = nil, binds = []) # Add `SELECT @@ROWCOUNT` to the end of the SQL to get the number of affected rows. This is needed because SQL Server does not return the number of affected rows in the same way as other databases. sql = intent.processed_sql.present? ? intent.processed_sql : intent.raw_sql + ensure_writes_are_allowed(sql) if write_query?(sql) intent.processed_sql = "#{sql}; SELECT @@ROWCOUNT AS AffectedRows" affected_rows(raw_execute(intent)) diff --git a/test/support/query_assertions.rb b/test/support/query_assertions.rb index 5b47ec9df..017e570a5 100644 --- a/test/support/query_assertions.rb +++ b/test/support/query_assertions.rb @@ -14,7 +14,7 @@ def assert_queries_count(count = nil, include_schema: false, &block) # End of monkey-patch if count - assert_equal count, queries.size, "#{queries.size} instead of #{count} queries were executed#{queries.empty? ? '' : ". Queries:\n\n#{queries.join("\n\n")}"}" + assert_equal count, queries.size, "#{queries.size} instead of #{count} queries were executed#{". Queries:\n\n#{queries.join("\n\n")}" unless queries.empty?}" else assert_operator queries.size, :>=, 1, "1 or more queries expected, but none were executed" end From e0f34c382bbbf0012a1d523c46b0e578e4fd7323 Mon Sep 17 00:00:00 2001 From: Aidan Haran Date: Sun, 8 Feb 2026 17:40:41 +0000 Subject: [PATCH 8/8] Created AGENTS.md file --- AGENTS.md | 188 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 188 insertions(+) create mode 100644 AGENTS.md diff --git a/AGENTS.md b/AGENTS.md new file mode 100644 index 000000000..c90721dc4 --- /dev/null +++ b/AGENTS.md @@ -0,0 +1,188 @@ +# AGENTS.md + +This file contains guidelines and commands for agentic coding agents working in the ActiveRecord SQL Server Adapter repository. + +## Build, Lint & Test Commands + +### Core Commands +```bash +# Install dependencies +bundle install + +# Run all tests +bundle exec rake test + +# Run specific test files +bundle exec ruby -Ilib:test test/cases/adapter_test_sqlserver.rb + +# Run tests with environment variables +ONLY_SQLSERVER=1 bundle exec rake test +ONLY_ACTIVERECORD=1 bundle exec rake test + +# Run specific tests using TEST_FILES environment variable +TEST_FILES=test/cases/adapter_test_sqlserver.rb,test/cases/schema_test_sqlserver.rb bundle exec rake test:dblib + +# Run specific ActiveRecord tests using TEST_FILES_AR +TEST_FILES_AR=test/cases/adapters/mysql2/schema_test.rb bundle exec rake test:dblib + +# Enable warnings during testing +WARNING=1 bundle exec rake test:dblib + +# Run performance profiling +bundle exec rake profile:dblib:[profile_case_name] +``` + +### Code Quality +```bash +# Run Standard Ruby formatter/linter +bundle exec standardrb + +# Run Standard with fix +bundle exec standardrb --fix + +# Check only (no modifications) +bundle exec standardrb --format progress +``` + +## Code Style Guidelines + +### General Standards +- **Ruby Version**: Target Ruby 3.2.0+ (uses frozen_string_literal: true) +- **Code Style**: Uses Standard Ruby formatter +- **Line Length**: 120 characters max +- **String Literals**: Double quotes preferred (enforced by RuboCop) +- **Encoding**: Always include `# frozen_string_literal: true` at top of files + +### Import/Require Organization +```ruby +# frozen_string_literal: true + +# External libraries first +require "tiny_tds" +require "base64" +require "active_record" + +# Internal libraries next, grouped by functionality +require "active_record/connection_adapters/sqlserver/version" +require "active_record/connection_adapters/sqlserver/type" +require "active_record/connection_adapters/sqlserver/database_limits" +# ... etc + +# Local requires last +require_relative "support/paths_sqlserver" +``` + +### Naming Conventions +- **Files**: snake_case with `_test_sqlserver.rb` suffix for test files +- **Classes**: PascalCase, test classes end with `SQLServer` suffix +- **Methods**: snake_case, use descriptive names +- **Constants**: SCREAMING_SNAKE_CASE +- **Modules**: PascalCase, logical grouping (e.g., `SQLServer::Type`) + +### Module Structure +```ruby +module ActiveRecord + module ConnectionAdapters + module SQLServer + module Type + class Boolean < ActiveRecord::Type::Boolean + def sqlserver_type + "bit" + end + end + end + end + end +end +``` + +### Testing Patterns +- **Test Files**: Follow pattern `*_test_sqlserver.rb` in `test/cases/` +- **Test Classes**: Inherit from `ActiveRecord::TestCase` +- **Fixtures**: Use `fixtures :table_name` when needed +- **Let Blocks**: Use `let` for test setup data +- **Assertions**: Use custom assertions from `ARTest::SQLServer::QueryAssertions` + +### Error Handling +- Use SQL Server-specific error classes from `ActiveRecord::ConnectionAdapters::SQLServer::Errors` +- Handle TinyTDS errors appropriately +- Log database errors with context + +### Type System +- All SQL Server types inherit from corresponding ActiveRecord types +- Implement `sqlserver_type` method for database type mapping +- Location: `lib/active_record/connection_adapters/sqlserver/type/` + +### Connection Management +- Use `ActiveRecord::Base.lease_connection` for connection access +- Handle connection pooling and timeout scenarios +- Support both dblib modes + +### Schema Statements +- Follow SQL Server-specific SQL syntax +- Use proper identifier quoting with square brackets: `[table_name]` +- Handle SQL Server's quirks around primary keys, indexes, and constraints + +## File Organization + +### Core Structure +``` +lib/ +├── active_record/ +│ └── connection_adapters/ +│ └── sqlserver/ # Main adapter implementation +│ ├── type/ # SQL Server-specific types +│ └── core_ext/ # ActiveRecord extensions +test/ +├── cases/ # Test files (*_test_sqlserver.rb) +├── support/ # Test utilities and helpers +└── migrations/ # Test migrations +``` + +### Module Dependencies +- Core adapter: `lib/active_record/connection_adapters/sqlserver_adapter.rb` +- Types: `lib/active_record/connection_adapters/sqlserver/type.rb` +- Core extensions: `lib/active_record/connection_adapters/sqlserver/core_ext/` + +## Development Notes + +### Environment Variables +- `ARCONN`: Set to "sqlserver" for testing +- `ARCONFIG`: Path to database configuration file +- `TEST_FILES`: Comma-separated test files to run +- `TEST_FILES_AR`: ActiveRecord test files to run +- `ONLY_SQLSERVER`: Run only SQL Server-specific tests +- `ONLY_ACTIVERECORD`: Run only ActiveRecord tests +- `WARNING`: Enable Ruby warnings during tests + +### Dependencies +- **tiny_tds**: SQL Server connectivity (v3.0+) +- **activerecord**: Rails ORM (v8.2.0.alpha) +- **mocha**: Mocking framework for tests +- **minitest**: Testing framework (v6.0+) + +### Database Configuration +- Tests use configuration from `test/config.yml` +- Supports SQL Server 2012 and upward +- Requires proper SQL Server connection setup + +## Testing Best Practices + +### Running Single Tests +```bash +# Run a single test file +bundle exec ruby -Ilib:test test/cases/adapter_test_sqlserver.rb + +# Run specific test method +bundle exec ruby -Ilib:test test/cases/adapter_test_sqlserver.rb -n test_method_name +``` + +### Test Environment Setup +- All test helpers are in `test/support/` +- Use `ARTest::SQLServer` module for test utilities +- Schema loaded via `support/load_schema_sqlserver.rb` + +### Test Data +- Use fixtures where appropriate +- Let blocks for test-specific data setup +- Clean database state between tests