From 1435886104fad36b2f4c582f4988864ce1e8b662 Mon Sep 17 00:00:00 2001 From: John Nunemaker Date: Wed, 20 Sep 2017 10:33:10 -0400 Subject: [PATCH 1/4] Add failing test to demonstrate bug --- test/github/sql_test.rb | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/test/github/sql_test.rb b/test/github/sql_test.rb index 9493f44..3bdd5a0 100644 --- a/test/github/sql_test.rb +++ b/test/github/sql_test.rb @@ -213,4 +213,27 @@ def test_models ActiveRecord::Base.connection.execute("DROP TABLE IF EXISTS `repositories`") end end + + def test_add_doesnt_modify_timezone_if_early_return_invoked + begin + original_default_timezone = ActiveRecord::Base.default_timezone + refute_nil original_default_timezone + + ActiveRecord::Base.connection.execute("DROP TABLE IF EXISTS `repositories`") + ActiveRecord::Base.connection.execute <<-SQL + CREATE TABLE `repositories` ( + `id` int(11) NOT NULL AUTO_INCREMENT, + `name` varchar(255) DEFAULT NULL, + PRIMARY KEY (`id`) + ) ENGINE=InnoDB DEFAULT CHARSET=utf8; + SQL + + sql = GitHub::SQL.new("SELECT * FROM repositories WHERE id = ?", force_timezone: :local) + sql.add nil, id: 1 + + assert_equal original_default_timezone, ActiveRecord::Base.default_timezone + ensure + ActiveRecord::Base.connection.execute("DROP TABLE IF EXISTS `repositories`") + end + end end From e442e84a724e4dc8c9eeff4cd1f5a793c0b778d3 Mon Sep 17 00:00:00 2001 From: John Nunemaker Date: Wed, 20 Sep 2017 10:34:19 -0400 Subject: [PATCH 2/4] Fix bug in add ensure at the method level runs on every method call, even early returns like the return self if sql.blank? check. This means that timezone was being set to nil sometimes if force_timezone was used and the sql was blank (early return). This fixes that bug. --- lib/github/sql.rb | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/lib/github/sql.rb b/lib/github/sql.rb index 6e71c1c..7e6d278 100644 --- a/lib/github/sql.rb +++ b/lib/github/sql.rb @@ -167,16 +167,18 @@ def add(sql, extras = nil) query << " " unless query.empty? - if @force_tz - zone = ActiveRecord::Base.default_timezone - ActiveRecord::Base.default_timezone = @force_tz - end + begin + if @force_tz + zone = ActiveRecord::Base.default_timezone + ActiveRecord::Base.default_timezone = @force_tz + end - query << interpolate(sql.strip, extras) + query << interpolate(sql.strip, extras) - self - ensure - ActiveRecord::Base.default_timezone = zone if @force_tz + self + ensure + ActiveRecord::Base.default_timezone = zone if @force_tz + end end # Public: Add a chunk of SQL to the query, unless query generated so far is empty. From 99032a2f5d1cd2a47b57b4e48a06e43e8d761be9 Mon Sep 17 00:00:00 2001 From: John Nunemaker Date: Wed, 20 Sep 2017 10:36:32 -0400 Subject: [PATCH 3/4] Failing test to demonstrate bug in results --- test/github/sql_test.rb | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/test/github/sql_test.rb b/test/github/sql_test.rb index 3bdd5a0..0405cb6 100644 --- a/test/github/sql_test.rb +++ b/test/github/sql_test.rb @@ -233,6 +233,32 @@ def test_add_doesnt_modify_timezone_if_early_return_invoked assert_equal original_default_timezone, ActiveRecord::Base.default_timezone ensure + ActiveRecord::Base.default_timezone = original_default_timezone + ActiveRecord::Base.connection.execute("DROP TABLE IF EXISTS `repositories`") + end + end + + def test_results_doesnt_modify_timezone_if_early_return_invoked + begin + original_default_timezone = ActiveRecord::Base.default_timezone + refute_nil original_default_timezone + + ActiveRecord::Base.connection.execute("DROP TABLE IF EXISTS `repositories`") + ActiveRecord::Base.connection.execute <<-SQL + CREATE TABLE `repositories` ( + `id` int(11) NOT NULL AUTO_INCREMENT, + `name` varchar(255) DEFAULT NULL, + PRIMARY KEY (`id`) + ) ENGINE=InnoDB DEFAULT CHARSET=utf8; + SQL + + sql = GitHub::SQL.new("SELECT * FROM repositories LIMIT 1", force_timezone: :local) + sql.results + sql.results + + assert_equal original_default_timezone, ActiveRecord::Base.default_timezone + ensure + ActiveRecord::Base.default_timezone = original_default_timezone ActiveRecord::Base.connection.execute("DROP TABLE IF EXISTS `repositories`") end end From 6e800e7ae73121ce1a1b4d5005a7b982b7472fb6 Mon Sep 17 00:00:00 2001 From: John Nunemaker Date: Wed, 20 Sep 2017 10:37:41 -0400 Subject: [PATCH 4/4] Scope ensure to only run when timezone was actually modified See https://github.com/github/github-ds/commit/e442e84a724e4dc8c9eeff4cd1f5a793c0b778d3 for longer description of fix. --- lib/github/sql.rb | 50 ++++++++++++++++++++++++----------------------- 1 file changed, 26 insertions(+), 24 deletions(-) diff --git a/lib/github/sql.rb b/lib/github/sql.rb index 7e6d278..8640b24 100644 --- a/lib/github/sql.rb +++ b/lib/github/sql.rb @@ -272,38 +272,40 @@ def results return @results if defined? @results return [] if frozen? - if @force_tz - zone = ActiveRecord::Base.default_timezone - ActiveRecord::Base.default_timezone = @force_tz - end + begin + if @force_tz + zone = ActiveRecord::Base.default_timezone + ActiveRecord::Base.default_timezone = @force_tz + end - case query - when /\ADELETE/i - @affected_rows = connection.delete(query, "#{self.class.name} Delete") + case query + when /\ADELETE/i + @affected_rows = connection.delete(query, "#{self.class.name} Delete") - when /\AINSERT/i - @last_insert_id = connection.insert(query, "#{self.class.name} Insert") + when /\AINSERT/i + @last_insert_id = connection.insert(query, "#{self.class.name} Insert") - when /\AUPDATE/i - @affected_rows = connection.update(query, "#{self.class.name} Update") + when /\AUPDATE/i + @affected_rows = connection.update(query, "#{self.class.name} Update") - when /\ASELECT/i - # Why not execute or select_rows? Because select_all hits the query cache. - @hash_results = connection.select_all(query, "#{self.class.name} Select") - @results = @hash_results.map(&:values) + when /\ASELECT/i + # Why not execute or select_rows? Because select_all hits the query cache. + @hash_results = connection.select_all(query, "#{self.class.name} Select") + @results = @hash_results.map(&:values) - else - @results = connection.execute(query, "#{self.class.name} Execute").to_a - end + else + @results = connection.execute(query, "#{self.class.name} Execute").to_a + end - @results ||= [] + @results ||= [] - retrieve_found_row_count - freeze + retrieve_found_row_count + freeze - @results - ensure - ActiveRecord::Base.default_timezone = zone if @force_tz + @results + ensure + ActiveRecord::Base.default_timezone = zone if @force_tz + end end # Public: If the query is a SELECT, return an array of hashes instead of an array of arrays.