Rails 8 proper fixes! by amitsuroliya · Pull Request #4 · punchh/odbc_adapter

amitsuroliya · 2025-12-11T06:35:06Z

No description provided.

Copilot

Pull request overview

This PR aims to improve Rails 8 compatibility and add workarounds/performance tweaks for specific ODBC backends (notably Databricks/Snowflake), including handling an ODBC “null byte” error path.

Changes:

Reworks adapter connection initialization and adds Rails migration-check skipping integration.
Overrides schema introspection and SQL/type/quoting behavior for Databricks/Spark-style compatibility.
Adds aggressive null-byte sanitization and error handling in SQL execution paths.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 16 comments.

Show a summary per file

File	Description
odbc_adapter.gemspec	Converts gemspec to a stub-style spec with hard-coded file list and metadata.
lib/odbc_adapter/schema_statements.rb	Overrides `tables`/`columns` to return empty arrays; adds Databricks-specific `type_to_sql` behavior.
lib/odbc_adapter/railtie.rb	Adds a Railtie that monkey-patches migration pending checks.
lib/odbc_adapter/rails_integration.rb	Adds an additional Rails monkey-patch file for migration checks.
lib/odbc_adapter/quoting.rb	Changes quoting to always use backticks and adds class-method quoting helpers.
lib/odbc_adapter/database_statements.rb	Adds null-byte sanitization + extensive error handling in `execute`; adds Rails 8 compatibility hooks for binds and `internal_exec_query`.
lib/odbc_adapter.rb	Requires the new Railtie (with LoadError rescue).
lib/active_record/connection_adapters/odbc_adapter.rb	Changes connection factory behavior and adapter initialization; adds another migration-check monkey-patch.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-02T12:59:11Z

+      # For string types, ignore the limit and use STRING without length
+      # This is required for Databricks/Spark SQL compatibility
+      if type == :string || type.to_s == 'string'
+        return "STRING"
      end
+      # Call super for all other types
+      super
    end



type_to_sql now returns STRING for every :string column regardless of the connected DBMS. This is Databricks/Spark-specific and will produce invalid DDL for many ODBC targets (e.g., Postgres, MySQL) where STRING is not a valid type. Please scope this to a Databricks/Spark adapter variant or a config flag, and defer to super otherwise.

Suggested change

# For string types, ignore the limit and use STRING without length

# This is required for Databricks/Spark SQL compatibility

if type == :string || type.to_s == 'string'

return "STRING"

end

# Call super for all other types

super

end

# For string types on Databricks/Spark, ignore the limit and use STRING without length.

# For all other DBMSs, fall back to the default implementation.

if (type == :string || type.to_s == "string") && databricks_or_spark_dbms?

return "STRING"

end

super

end

private

# Detect whether the current ODBC connection targets Databricks or Spark.

# This relies on database_metadata.dbms_name, if available.

def databricks_or_spark_dbms?

return false unless respond_to?(:database_metadata) && database_metadata

return false unless database_metadata.respond_to?(:dbms_name)

name = database_metadata.dbms_name

return false if name.nil?

downcased = name.to_s.downcase

downcased.include?("databricks") || downcased.include?("spark")

end

Copilot · 2026-02-02T12:59:12Z

+    # Returns a quoted form of the table name.
+    # Override to use backticks for Databricks/Spark SQL compatibility
+    def quote_table_name(name)
+      # Use backticks for Databricks/Spark SQL identifiers
+      # Escape backticks by doubling them
+      %Q(`#{name.to_s.gsub('`', '``')}`)
+    end


quote_table_name now always uses backticks as well, which is not portable across ODBC targets and diverges from the adapter’s previous identifier_quote_char-based behavior. Please make this conditional (e.g., only for Databricks/Spark) or restore metadata-based quoting for general use.

Copilot · 2026-02-02T12:59:12Z


        database_metadata = ::ODBCAdapter::DatabaseMetadata.new(connection)
-        database_metadata.adapter_class.new(connection, logger, config, database_metadata)
+        [connection, logger, config, database_metadata]


ActiveRecord::Base.odbc_connection should return an ODBCAdapter instance (as it did previously). It now returns an array [connection, logger, config, database_metadata], which will break establish_connection because ActiveRecord expects an adapter object responding to the connection adapter API.

Suggested change

[connection, logger, config, database_metadata]

ActiveRecord::ConnectionAdapters::ODBCAdapter.new(connection, logger, config, database_metadata)

Copilot · 2026-02-02T12:59:13Z

+# Handles Rails-specific patches and workarounds for ODBC adapter compatibility
+
+# Skip migration checks for ODBC connections
+# The ODBC driver has a bug where it incorrectly detects null bytes in clean strings
+# This prevents Rails from checking pending migrations on startup for ODBC connections
+
+if defined?(ActiveRecord::ConnectionAdapters::ODBCAdapter) && defined?(ActiveRecord::Migration::CheckPending)
+  # Override the CheckPending middleware to skip for ODBC connections
+  ActiveRecord::Migration::CheckPending.class_eval do
+    alias_method :original_call, :call unless method_defined?(:original_call)
+
+    def call(env)
+      # Always wrap the original call in error handling to catch null byte errors
+      # This handles cases where ODBC connections might not be detected upfront
+      begin
+        # Check if primary connection is ODBC adapter
+        if ActiveRecord::Base.connection.is_a?(ActiveRecord::ConnectionAdapters::ODBCAdapter)
+          # Skip migration checks for ODBC adapters due to driver bug
+          @app.call(env)
+        else
+          # Try original call, but catch null byte errors
+          original_call(env)
+        end
+      rescue ArgumentError => e
+        if e.message.include?("null byte")
+          # If we get a null byte error, it's likely the ODBC driver bug
+          # Skip the migration check and continue
+          if defined?(Rails) && Rails.logger
+            Rails.logger.warn("Skipping migration check due to null byte error (ODBC driver bug): #{e.message}")
+          end
+          @app.call(env)
+        else
+          # Re-raise if it's a different ArgumentError
+          raise
+        end
+      rescue => e
+        # For any other error during migration check, log and continue
+        # This prevents the application from crashing due to migration check issues
+        if defined?(Rails) && Rails.logger
+          Rails.logger.warn("Skipping migration check due to error: #{e.class} - #{e.message}")
+        end
+        @app.call(env)
+      end
+    end
+  end
+end
+


lib/odbc_adapter/rails_integration.rb is added but is not required from anywhere in the codebase (and is also missing from the gemspec’s s.files list). This looks like dead code that duplicates the Railtie/adapter monkey-patch; please either wire it up intentionally (single source of truth) or remove it to avoid confusion.

Suggested change

# Handles Rails-specific patches and workarounds for ODBC adapter compatibility

# Skip migration checks for ODBC connections

# The ODBC driver has a bug where it incorrectly detects null bytes in clean strings

# This prevents Rails from checking pending migrations on startup for ODBC connections

if defined?(ActiveRecord::ConnectionAdapters::ODBCAdapter) && defined?(ActiveRecord::Migration::CheckPending)

# Override the CheckPending middleware to skip for ODBC connections

ActiveRecord::Migration::CheckPending.class_eval do

alias_method :original_call, :call unless method_defined?(:original_call)

def call(env)

# Always wrap the original call in error handling to catch null byte errors

# This handles cases where ODBC connections might not be detected upfront

begin

# Check if primary connection is ODBC adapter

if ActiveRecord::Base.connection.is_a?(ActiveRecord::ConnectionAdapters::ODBCAdapter)

# Skip migration checks for ODBC adapters due to driver bug

@app.call(env)

else

# Try original call, but catch null byte errors

original_call(env)

end

rescue ArgumentError => e

if e.message.include?("null byte")

# If we get a null byte error, it's likely the ODBC driver bug

# Skip the migration check and continue

if defined?(Rails) && Rails.logger

Rails.logger.warn("Skipping migration check due to null byte error (ODBC driver bug): #{e.message}")

end

@app.call(env)

else

# Re-raise if it's a different ArgumentError

raise

end

rescue => e

# For any other error during migration check, log and continue

# This prevents the application from crashing due to migration check issues

if defined?(Rails) && Rails.logger

Rails.logger.warn("Skipping migration check due to error: #{e.class} - #{e.message}")

end

@app.call(env)

end

end

end

end

# NOTE: Runtime Rails integration (including any required monkey-patches)

# is handled by the main Railtie/adapter integration. This file is kept as

# a no-op shim to avoid duplicating that logic and to prevent confusion

# about alternate integration entry points.

Copilot · 2026-02-02T12:59:13Z

+  s.bindir = "exe".freeze
+  s.date = "2025-12-10"
+  s.email = ["oss@localytics.com".freeze]
+  s.files = [".gitignore".freeze, ".rubocop.yml".freeze, ".travis.yml".freeze, "Gemfile".freeze, "LICENSE".freeze, "README.md".freeze, "Rakefile".freeze, "bin/ci-setup".freeze, "bin/console".freeze, "bin/setup".freeze, "lib/active_record/connection_adapters/odbc_adapter.rb".freeze, "lib/odbc_adapter.rb".freeze, "lib/odbc_adapter/adapters/mysql_odbc_adapter.rb".freeze, "lib/odbc_adapter/adapters/null_odbc_adapter.rb".freeze, "lib/odbc_adapter/adapters/postgresql_odbc_adapter.rb".freeze, "lib/odbc_adapter/column.rb".freeze, "lib/odbc_adapter/column_metadata.rb".freeze, "lib/odbc_adapter/database_limits.rb".freeze, "lib/odbc_adapter/database_metadata.rb".freeze, "lib/odbc_adapter/database_statements.rb".freeze, "lib/odbc_adapter/error.rb".freeze, "lib/odbc_adapter/quoting.rb".freeze, "lib/odbc_adapter/registry.rb".freeze, "lib/odbc_adapter/schema_statements.rb".freeze, "lib/odbc_adapter/version.rb".freeze, "odbc_adapter.gemspec".freeze]


The gemspec now hard-codes s.files but it does not include newly added files like lib/odbc_adapter/railtie.rb (and lib/odbc_adapter/rails_integration.rb if kept). As a result, the packaged gem will not ship these files and require_relative 'odbc_adapter/railtie' will fail for users. Prefer generating files via git ls-files (as before) or add the missing paths to s.files.

Suggested change

s.files = [".gitignore".freeze, ".rubocop.yml".freeze, ".travis.yml".freeze, "Gemfile".freeze, "LICENSE".freeze, "README.md".freeze, "Rakefile".freeze, "bin/ci-setup".freeze, "bin/console".freeze, "bin/setup".freeze, "lib/active_record/connection_adapters/odbc_adapter.rb".freeze, "lib/odbc_adapter.rb".freeze, "lib/odbc_adapter/adapters/mysql_odbc_adapter.rb".freeze, "lib/odbc_adapter/adapters/null_odbc_adapter.rb".freeze, "lib/odbc_adapter/adapters/postgresql_odbc_adapter.rb".freeze, "lib/odbc_adapter/column.rb".freeze, "lib/odbc_adapter/column_metadata.rb".freeze, "lib/odbc_adapter/database_limits.rb".freeze, "lib/odbc_adapter/database_metadata.rb".freeze, "lib/odbc_adapter/database_statements.rb".freeze, "lib/odbc_adapter/error.rb".freeze, "lib/odbc_adapter/quoting.rb".freeze, "lib/odbc_adapter/registry.rb".freeze, "lib/odbc_adapter/schema_statements.rb".freeze, "lib/odbc_adapter/version.rb".freeze, "odbc_adapter.gemspec".freeze]

s.files = [".gitignore".freeze, ".rubocop.yml".freeze, ".travis.yml".freeze, "Gemfile".freeze, "LICENSE".freeze, "README.md".freeze, "Rakefile".freeze, "bin/ci-setup".freeze, "bin/console".freeze, "bin/setup".freeze, "lib/active_record/connection_adapters/odbc_adapter.rb".freeze, "lib/odbc_adapter.rb".freeze, "lib/odbc_adapter/adapters/mysql_odbc_adapter.rb".freeze, "lib/odbc_adapter/adapters/null_odbc_adapter.rb".freeze, "lib/odbc_adapter/adapters/postgresql_odbc_adapter.rb".freeze, "lib/odbc_adapter/column.rb".freeze, "lib/odbc_adapter/column_metadata.rb".freeze, "lib/odbc_adapter/database_limits.rb".freeze, "lib/odbc_adapter/database_metadata.rb".freeze, "lib/odbc_adapter/database_statements.rb".freeze, "lib/odbc_adapter/error.rb".freeze, "lib/odbc_adapter/quoting.rb".freeze, "lib/odbc_adapter/registry.rb".freeze, "lib/odbc_adapter/schema_statements.rb".freeze, "lib/odbc_adapter/version.rb".freeze, "lib/odbc_adapter/railtie.rb".freeze, "lib/odbc_adapter/rails_integration.rb".freeze, "odbc_adapter.gemspec".freeze]

Copilot · 2026-02-02T12:59:15Z

+      def initialize(connection)
+        connection, logger, config, database_metadata = ActiveRecord::Base.odbc_connection(connection)


ODBCAdapter#initialize now takes a single argument and calls ActiveRecord::Base.odbc_connection(connection) internally. In ActiveRecord, adapter instances are typically constructed by *_connection(config) passing (connection, logger, config) into initialize; changing the initializer signature like this is very likely incompatible with Rails’ adapter instantiation. Please align with the expected initializer contract and keep connection creation inside odbc_connection (returning the adapter instance).

Suggested change

def initialize(connection)

connection, logger, config, database_metadata = ActiveRecord::Base.odbc_connection(connection)

def initialize(connection, logger, config, database_metadata = nil)

Copilot · 2026-02-02T12:59:15Z

+# Rails integration: Skip migration checks for ODBC connections
+# The ODBC driver has a bug where it incorrectly detects null bytes in clean strings
+if defined?(Rails) && Rails.application
+  Rails.application.config.to_prepare do
+    if defined?(ActiveRecord::Migration::CheckPending)
+      ActiveRecord::Migration::CheckPending.class_eval do
+        alias_method :original_call, :call unless method_defined?(:original_call)


This file adds a Rails migration-check patch at the bottom, but the PR also introduces similar overrides in lib/odbc_adapter/railtie.rb and lib/odbc_adapter/rails_integration.rb. Duplicating the same monkey-patch in multiple load paths makes ordering issues and future maintenance harder. Please consolidate the override into a single integration point (preferably the Railtie).

Copilot · 2026-02-02T12:59:16Z

+          rescue => e
+            Rails.logger.warn("Skipping migration check due to error: #{e.class} - #{e.message}") if Rails.logger
+            @app.call(env)
+          end


The new call override rescues all exceptions (rescue => e) and then continues booting the app by calling @app.call(env). This can hide legitimate migration-check failures (including real pending migrations) and lead to runtime errors later. Consider only rescuing the specific ArgumentError/"null byte" case (and re-raising everything else).

Copilot · 2026-02-02T12:59:16Z

+              rescue => e
+                # For any other error during migration check, log and continue
+                Rails.logger.warn("Skipping migration check due to error: #{e.class} - #{e.message}") if Rails.logger
+                @app.call(env)
+              end


The Railtie’s CheckPending#call override rescues all exceptions (rescue => e) and continues request processing, which can mask real issues unrelated to the reported ODBC null-byte bug (including genuinely pending migrations). It would be safer to limit the rescue to the known ArgumentError/"null byte" case and re-raise other exceptions.

Copilot · 2026-02-02T12:59:16Z

+          # Call ODBC - wrap in begin/rescue to handle any remaining null byte issues
+          # Special handling for schema_migrations CREATE TABLE due to ODBC driver false positive bug
+          is_schema_migrations_create = odbc_sql.include?("schema_migrations") && odbc_sql.match?(/CREATE\s+TABLE/i)
+
+          begin
+            @connection.do(odbc_sql, *odbc_binds)
+          rescue ArgumentError => e


There are no tests covering the new null-byte sanitization/false-positive handling paths (e.g., execute rescuing ArgumentError and the special schema_migrations workaround). Given the amount of new behavior in a critical execution path, please add focused tests that exercise queries/binds containing "\0" and verify the adapter’s behavior.

amitsuroliya added 2 commits December 11, 2025 01:33

Rails 8 proper fixes!

72fc2e9

More fixes!

f793149

pankajkumar1104 requested a review from Copilot February 2, 2026 12:52

Copilot started reviewing on behalf of pankajkumar1104 February 2, 2026 12:52 View session

Copilot AI reviewed Feb 2, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rails 8 proper fixes!#4

Rails 8 proper fixes!#4
amitsuroliya wants to merge 2 commits into
masterfrom
rails8_fixes

amitsuroliya commented Dec 11, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Feb 2, 2026

Uh oh!

Copilot AI Feb 2, 2026

Uh oh!

Copilot AI Feb 2, 2026

Uh oh!

Copilot AI Feb 2, 2026

Uh oh!

Copilot AI Feb 2, 2026

Uh oh!

Copilot AI Feb 2, 2026

Uh oh!

Copilot AI Feb 2, 2026

Uh oh!

Copilot AI Feb 2, 2026

Uh oh!

Copilot AI Feb 2, 2026

Uh oh!

Copilot AI Feb 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

-      # For string types, ignore the limit and use STRING without length
-      # This is required for Databricks/Spark SQL compatibility
-      if type == :string || type.to_s == 'string'
-        return "STRING"
-      end
-      # Call super for all other types
-      super
-    end
+      # For string types on Databricks/Spark, ignore the limit and use STRING without length.
+      # For all other DBMSs, fall back to the default implementation.
+      if (type == :string || type.to_s == "string") && databricks_or_spark_dbms?
+        return "STRING"
+      end
+      super
+    end
+    private
+    # Detect whether the current ODBC connection targets Databricks or Spark.
+    # This relies on database_metadata.dbms_name, if available.
+    def databricks_or_spark_dbms?
+      return false unless respond_to?(:database_metadata) && database_metadata
+      return false unless database_metadata.respond_to?(:dbms_name)
+      name = database_metadata.dbms_name
+      return false if name.nil?
+      downcased = name.to_s.downcase
+      downcased.include?("databricks") || downcased.include?("spark")
+    end

	[connection, logger, config, database_metadata]
	ActiveRecord::ConnectionAdapters::ODBCAdapter.new(connection, logger, config, database_metadata)

		def initialize(connection)
		connection, logger, config, database_metadata = ActiveRecord::Base.odbc_connection(connection)

	def initialize(connection)
	connection, logger, config, database_metadata = ActiveRecord::Base.odbc_connection(connection)
	def initialize(connection, logger, config, database_metadata = nil)

Conversation

amitsuroliya commented Dec 11, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 2, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants