Skip to content

Commit

Permalink
Sequel datetime class consistency (#151)
Browse files Browse the repository at this point in the history
* timezones: absent instruction otherwise, interpret times as local

these tests SUCCEED when run on their own, but FAIL when run after any
instance of a plugin has been initialized with `jdbc_default_timezone`,
since doing so globally configures Sequel to use DateTime instead of
Time:

To validate, modify the `.ci/run.sh` execution line to be:
> ~~~
> bundle exec rspec spec --format documentation --example "without jdbc_default_timezone"
> ~~~

* timezones: consistently pre-load Sequel, force datetime_class=Time

By default, when a plugin is configured _without_ `jdbc_default_timezone`
timestamps are assumed to be in the same timezone as the Logstash host
machine, because they are parsed with Ruby's `Time#parse` which uses
local context.

However, when any one plugin declares `jdbc_default_timezone`, we load
Sequel's `named_timezones` extension, which has a side-effect of globally
changing `Sequel.datetime_class` to ruby's `DateTime`. The plugins that
are configured with `jdbc_default_timezone` have enough information to
apply the separately-provided offset, but the plugins that do _not_ have
a `jdbc_default_timezone` directive become broken and effectively
fail to parse the timestamps as they did when run on their own.

Sequel's `named_timezones` extension supports being used with `Time`
objects, and is noted to override `Sequel#datetime_class` on load only
for historic reasons.

We force Sequel to use ruby's `Time` class globally (enforcing its
default and preventing it from being changed). This is done inside
of a separately-required bootstrap, which allows us to ensure it is
loaded exacltly once.

* bump version & add changelog entry
  • Loading branch information
yaauie authored May 23, 2024
1 parent 48f8053 commit c0583ce
Show file tree
Hide file tree
Showing 5 changed files with 52 additions and 2 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
## 5.4.11
- Fixes an issue in which any one instance of a JDBC input plugin using `jdbc_default_timezone` changes the behaviour of plugin instances that do _not_ use `jdbc_default_timezone`, ensuring that timezone offsets remain consistent for each instance of the plugin _as configured_ [#151](https://github.com/logstash-plugins/logstash-integration-jdbc/pull/151)
- Fixes an exception that could occur while reloading `jdbc_static` databases when the underlying connection to the remote has been broken [#165](https://github.com/logstash-plugins/logstash-integration-jdbc/pull/165)

## 5.4.10
Expand Down
2 changes: 1 addition & 1 deletion lib/logstash/filters/jdbc/basic_database.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# encoding: utf-8
require "fileutils"
require "sequel"
require "logstash/plugin_mixins/jdbc/sequel_bootstrap"
require "sequel/adapters/jdbc"
require "java"
require "logstash/util/loggable"
Expand Down
3 changes: 2 additions & 1 deletion lib/logstash/plugin_mixins/jdbc/common.rb
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,8 @@ def load_driver
return @driver_impl if @driver_impl ||= nil

require "java"
require "sequel"

require_relative "sequel_bootstrap"
require "sequel/adapters/jdbc"

# execute all the driver loading related duties in a serial fashion to avoid
Expand Down
21 changes: 21 additions & 0 deletions lib/logstash/plugin_mixins/jdbc/sequel_bootstrap.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
# encoding: utf-8

require "sequel"

# prevent Sequel's datetime_class from being modified,
# and ensure behaviour is restored to the library's default
# if something else in the Ruby VM has already changed it.
Sequel.synchronize do
def Sequel.datetime_class=(klass)
# noop
end
def Sequel.datetime_class
::Time
end
end

# load the named_timezones extension, which will attempt to
# override the global Sequel::datetime_class; for safety,
# we reset it once more.
Sequel.extension(:named_timezones)
Sequel.datetime_class = ::Time
27 changes: 27 additions & 0 deletions spec/inputs/jdbc_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -601,6 +601,33 @@
# With no timezone set, no change should occur
expect(event.get("custom_time").time).to eq(Time.iso8601("2015-01-01T12:00:00Z"))
end

%w(
Etc/UTC
America/Los_Angeles
Europe/Berlin
Asia/Tokyo
).each do |local_timezone|
context "when host machine has timezone `#{local_timezone}`" do
around(:each) do |example|
begin
previous_tz = ENV['TZ']
ENV['TZ'] = local_timezone
example.call
ensure
ENV['TZ'] = previous_tz
end
end

let(:tz) { TZInfo::Timezone.get(local_timezone) }

it "converts the time using the machine's local timezone" do
plugin.run(queue)
event = queue.pop
expect(event.get("custom_time").time).to eq(Time.new(2015,1,1,12,0,0,tz))
end
end
end
end

context "when iteratively running plugin#run" do
Expand Down

0 comments on commit c0583ce

Please sign in to comment.