Skip to content
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

Update in_sql.rb #103

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Update in_sql.rb #103

wants to merge 1 commit into from

Conversation

Sjolus
Copy link

@Sjolus Sjolus commented Jun 29, 2021

Adjusts according to suggestions in #43

This fixed our issues as well so I thought we'd PR it.

@kenhys
Copy link
Contributor

kenhys commented Jul 1, 2021

@Sjolus
Thanks!
Could you follow DCO instructions and add the appropriate test for this case?

Fixes issue fluent#43

Signed-off-by: Johan Sjölin <[email protected]>
@Sjolus
Copy link
Author

Sjolus commented Jul 1, 2021

@kenhys Looks like I've successfully done the signoff push required by DCO (My first commit was just an edit in the web ui, now pushed using SSH key)

In regards to setting up a test case I'm afraid I don't have the ruby skills to set it up. I can describe the scenario if that would help any future developer design a test but I'm a copy and paste solution man, not a ruby developer. Sorry! Feel free to close the PR if this is required for your practice.

@kenhys
Copy link
Contributor

kenhys commented Jul 2, 2021

It changes the behavior to the previous version, how about making it customizable? (something like this)

diff --git a/lib/fluent/plugin/in_sql.rb b/lib/fluent/plugin/in_sql.rb
index babec57..beaf4a3 100644
--- a/lib/fluent/plugin/in_sql.rb
+++ b/lib/fluent/plugin/in_sql.rb
@@ -51,6 +51,9 @@ module Fluent::Plugin
     desc 'limit of number of rows for each SQL(optional)'
     config_param :select_limit, :time, default: 500
 
+    desc 'fetch record in specified timezone'
+    config_param :time_zone, :enum, list: [:utc, :local], default: :utc
+
     class TableElement
       include Fluent::Configurable
 
@@ -192,6 +195,7 @@ module Fluent::Plugin
         socket: @socket,
         schema_search_path: @schema_search_path,
       }
+      ActiveRecord::Base.default_timezone = @time_zone
 
       # creates subclass of ActiveRecord::Base so that it can have different
       # database configuration from ActiveRecord::Base.

@kenhys
Copy link
Contributor

kenhys commented Jul 2, 2021

And test like this:

diff --git a/test/plugin/test_in_sql_with_custom_time.rb b/test/plugin/test_in_sql_with_custom_time.rb
index 5bb7e95..e784685 100644
--- a/test/plugin/test_in_sql_with_custom_time.rb
+++ b/test/plugin/test_in_sql_with_custom_time.rb
@@ -7,6 +7,7 @@ class SqlInputCustomTimeTest < Test::Unit::TestCase
   end
 
   def teardown
+    Message.destroy_all
   end
 
   CONFIG = %[
@@ -108,6 +109,29 @@ class SqlInputCustomTimeTest < Test::Unit::TestCase
     end
   end
 
+  sub_test_case "timezone" do
+    def test_utc
+      d = create_driver(CONFIG + "select_interval 1 time_zone utc")
+      Message.create!(message: "message 1", updated_at: '2021-07-01 15:00:16.100758000 +0900')
+      d.end_if do
+        d.record_count >= 1
+      end
+      d.run(timeout: 5)
+      assert_equal("2021-07-01 06:00:16.100758+0000", d.events.first.last["updated_at"])
+    end
+
+    def test_local
+      d = create_driver(CONFIG + "select_interval 1 time_zone local")
+      stub(ActiveRecord::Base).default_timezone { 'Tokyo' }
+      Message.create!(message: "message 1", updated_at: '2021-07-01 15:00:16.100758000 +0900')
+      d.end_if do
+        d.record_count >= 1
+      end
+      d.run(timeout: 5)
+      assert_equal("2021-07-01 15:00:16.100758+0900", d.events.first.last["updated_at"])
+    end
+  end
+
   class Message < ActiveRecord::Base
     self.table_name = "messages_custom_time"
   end

@expteam-interactiv
Copy link

expteam-interactiv commented Aug 26, 2021

This patch solves my problem, time fields were losing time zone information and mis-converted to GMT

@expteam-interactiv
Copy link

Adjusts according to suggestions in #43

This fixed our issues as well so I thought we'd PR it.

This fixes my issue too, time fields where mis-translated in GMT, loosing our time zone information

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.

3 participants