Skip to content

Honor database-config-specific default_timezone configuration value #1329

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: 8-0-stable
Choose a base branch
from

Conversation

taylorthurlow
Copy link

@taylorthurlow taylorthurlow commented May 9, 2025

ActiveRecord supports a database-configuration-level value for default_timezone, and this adapter is not honoring that setting, instead using the global ActiveRecord.default_timezone setting. This change should honor the configuration-specific setting when present and still retain prior behavior.

This PR is applied to the 8-0-stable branch, as there is a conflict in main, which appears to be superficial and only related to some hash formatting changes. Not sure what the desired process is here.


Some broader context

This all stems from the fact that we're using this SQLServer adapter as a means to connect to multiple legacy databases that are separate from the parent Rails application's main database. This is why setting the ActiveRecord.default_timezone value isn't a solution, I don't want it to affect either of the other two databases. The one database in question is unfortunately storing timestamps in a non-UTC time zone and we have no control over that, so we need to conform to that from an ActiveRecord perspective.

An additional complication is that the other MSSQL database is separate, and does not have this issue, so it can't be solved on an adapter level either. The configuration option this PR solves the issue of reading the values from the database without mangling the datetime value.

I am still unable to fix writes - writing to a datetime column with a Time instance does a UTC conversion before building the query, causing data to be written in UTC which is not desired. I would assume that writing this data should also honor the default_timezone value, but it only does so with the global setting, not the connection configuration level one.

I looked at ActiveRecord::ConnectionAdapters::SQLServer::Type::DateTime, which does serialization, but that seems to slot in after whenever the time zone conversion would happen.

So I eventually found ActiveRecord::Type::DateTime, which is just a wrapper around the ActiveModel equivalent with Internal::Timezone mixed in. That Timezone module's initialize method accepts a time zone argument which very clearly is meant to override the value of ActiveRecord.default_timezone, so I thought this was my silver bullet, but the only place I could find this could actually be used in the context of sqlserver-adapter was in the type map builder:

   m.register_type %r{\Adatetime} do |sql_type|
      precision = extract_precision(sql_type)
      if precision
        SQLServer::Type::DateTime2.new precision: precision
      else
        SQLServer::Type::DateTime.new(timezone: ActiveSupport::TimeZone["Pacific Time (US & Canada)"])
      end
    end

And I'm at a road block here because all of this type map setup is done without the context of an active database connection and therefore has no way obvious way to fetch a configuration object that could inform the time zone selection.


Sorry if that wasn't concise enough, in general it seems like sqlserver-adapter doesn't really support a connection-level timezone config, even though ActiveRecord itself does.

@taylorthurlow
Copy link
Author

Added some extra info after I spent some time poking around trying to figure out how to get writes to use the correct time zone, after the simple change in this PR got reads to work.

As-is it probably wouldn't make sense to merge as surely the feature does imply that it should work for both.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant