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 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/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"] 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 diff --git a/lib/active_record/connection_adapters/sqlserver/database_statements.rb b/lib/active_record/connection_adapters/sqlserver/database_statements.rb index daf635d63..48cd4b31f 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,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} 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 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/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) diff --git a/test/support/query_assertions.rb b/test/support/query_assertions.rb index 78dfce02d..017e570a5 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:\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