Upgrade: Introduce Parameter Object: QueryIntent (#1380)

Author: aidanharan

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

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"]

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"]

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

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,44 @@ 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 = [])
+          # 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)
+
+          # 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))
         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 = [])
+          # 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)
+
+          # 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))
         end
 
         def begin_db_transaction
@@ -237,9 +269,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}
 
@@ -615,7 +649,7 @@ def joining_on_columns_with_uniqueness_constraints(columns_with_uniqueness_const
         # however, since we need to have "target." for the assignment, we also generate the CASE switch ourselves
         def build_sql_for_recording_timestamps_when_updating(insert:)
           insert.model.timestamp_attributes_for_update_in_model.filter_map do |column_name|
-            if insert.send(:touch_timestamp_attribute?, column_name)
+            if insert.send(:touch_timestamp_attribute?, column_name) && !column_name.to_sym.in?(insert.send(:insert_all).updatable_columns)
               "target.#{quote_column_name(column_name)}=CASE WHEN (#{insert.updatable_columns.map { |column| "(source.#{quote_column_name(column)} = target.#{quote_column_name(column)} OR (source.#{quote_column_name(column)} IS NULL AND target.#{quote_column_name(column)} IS NULL))" }.join(" AND ")}) THEN target.#{quote_column_name(column_name)} ELSE #{high_precision_current_timestamp} END,"
             end
           end.join

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

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

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

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)

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:\n\n#{queries.join("\n\n")}" unless queries.empty?}"
           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