From 37c5c45e41194979aa3c97ba2375c0775af8ae36 Mon Sep 17 00:00:00 2001 From: Phil Date: Wed, 4 Mar 2020 15:37:32 -0800 Subject: [PATCH 1/6] Better check for whether or not a table is_chrono? The previous check didn't work properly because `data_source_sql` (which `data_source_exists?` uses internally) only qualifies the table_name to the current schema if a fully qualified name isn't passed. --- lib/chrono_model/adapter.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/chrono_model/adapter.rb b/lib/chrono_model/adapter.rb index 9c840539..bb71f36e 100644 --- a/lib/chrono_model/adapter.rb +++ b/lib/chrono_model/adapter.rb @@ -140,8 +140,8 @@ def on_schema(schema, recurse: :follow) # Returns true if the given name references a temporal table. # def is_chrono?(table) - on_temporal_schema { data_source_exists?(table) } && - on_history_schema { data_source_exists?(table) } + data_source_exists?("#{TEMPORAL_SCHEMA}.#{quote_table_name(table)}") && + data_source_exists?("#{HISTORY_SCHEMA}.#{quote_table_name(table)}") end # Reads the Gem metadata from the COMMENT set on the given PostgreSQL From e1dd13b54d5a7b2da48bdda6d63fedca5988bc28 Mon Sep 17 00:00:00 2001 From: Phil Date: Mon, 12 Sep 2022 11:11:12 -0700 Subject: [PATCH 2/6] Make sure to check table directly in case schema is passed in --- lib/chrono_model/adapter.rb | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/chrono_model/adapter.rb b/lib/chrono_model/adapter.rb index bb71f36e..14214734 100644 --- a/lib/chrono_model/adapter.rb +++ b/lib/chrono_model/adapter.rb @@ -140,8 +140,9 @@ def on_schema(schema, recurse: :follow) # Returns true if the given name references a temporal table. # def is_chrono?(table) - data_source_exists?("#{TEMPORAL_SCHEMA}.#{quote_table_name(table)}") && - data_source_exists?("#{HISTORY_SCHEMA}.#{quote_table_name(table)}") + base_table_name = table.to_s.split('.').last + data_source_exists?("#{TEMPORAL_SCHEMA}.#{quote_table_name(base_table_name)}") && + data_source_exists?("#{HISTORY_SCHEMA}.#{quote_table_name(base_table_name)}") end # Reads the Gem metadata from the COMMENT set on the given PostgreSQL From 85bb4577886b9f87d421b445f7247c8859d88c9d Mon Sep 17 00:00:00 2001 From: Phil Date: Mon, 12 Sep 2022 15:36:14 -0700 Subject: [PATCH 3/6] Added specs --- lib/chrono_model/adapter.rb | 2 +- spec/chrono_model/adapter/base_spec.rb | 65 ++++++++++++++++++++++++++ 2 files changed, 66 insertions(+), 1 deletion(-) diff --git a/lib/chrono_model/adapter.rb b/lib/chrono_model/adapter.rb index 14214734..a2e2e748 100644 --- a/lib/chrono_model/adapter.rb +++ b/lib/chrono_model/adapter.rb @@ -140,7 +140,7 @@ def on_schema(schema, recurse: :follow) # Returns true if the given name references a temporal table. # def is_chrono?(table) - base_table_name = table.to_s.split('.').last + base_table_name = table.to_s.split(".").last data_source_exists?("#{TEMPORAL_SCHEMA}.#{quote_table_name(base_table_name)}") && data_source_exists?("#{HISTORY_SCHEMA}.#{quote_table_name(base_table_name)}") end diff --git a/spec/chrono_model/adapter/base_spec.rb b/spec/chrono_model/adapter/base_spec.rb index f0eaaf85..3ff43460 100644 --- a/spec/chrono_model/adapter/base_spec.rb +++ b/spec/chrono_model/adapter/base_spec.rb @@ -134,6 +134,25 @@ it { expect(adapter.is_chrono?(table)).to be(false) } end + context "when passing a fully specified schema" do + before(:all) do + adapter.execute "BEGIN" + adapter.execute "CREATE SCHEMA test_schema" + + table "test_schema.#{table}" + adapter.execute "CREATE TABLE #{table} (id integer)" + end + + after(:all) do + table "test_table" + adapter.execute "ROLLBACK" + end + + it { expect { adapter.is_chrono?(table) }.to_not raise_error } + + it { expect(adapter.is_chrono?(table)).to be(false) } + end + context 'when schemas are not there yet' do before(:all) do adapter.execute 'BEGIN' @@ -150,5 +169,51 @@ it { expect(adapter.is_chrono?(table)).to be(false) } end + + context "within a different search_path" do + before(:all) do + schema_name = "schema_#{Random.uuid.tr("-", "_")}" + + adapter.execute "BEGIN" + adapter.execute "CREATE SCHEMA #{schema_name}" + adapter.execute "SET search_path TO #{schema_name}" + end + + after(:all) do + adapter.execute "ROLLBACK" + end + + with_temporal_table do + it { expect(adapter.is_chrono?(table)).to be(true) } + end + + with_plain_table do + it { expect(adapter.is_chrono?(table)).to be(false) } + end + end + + context "with a table in a different schema" do + before(:all) do + schema_name = "schema_#{Random.uuid.tr("-", "_")}" + + adapter.execute "BEGIN" + adapter.execute "CREATE SCHEMA #{schema_name}" + adapter.execute "SET search_path TO #{schema_name}" + + table "different_schema_table" + end + + after(:all) do + adapter.execute "ROLLBACK" + end + + with_temporal_table do + it { expect(adapter.is_chrono?(table)).to be(true) } + end + + with_plain_table do + it { expect(adapter.is_chrono?(table)).to be(false) } + end + end end end From 19905b5f1ed8b6988d0c8b61c6e86c9f224838e1 Mon Sep 17 00:00:00 2001 From: Phil Date: Mon, 12 Sep 2022 15:47:46 -0700 Subject: [PATCH 4/6] Fix spec so `table` is reset after the spec is run. --- spec/chrono_model/adapter/base_spec.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/spec/chrono_model/adapter/base_spec.rb b/spec/chrono_model/adapter/base_spec.rb index 3ff43460..55a50373 100644 --- a/spec/chrono_model/adapter/base_spec.rb +++ b/spec/chrono_model/adapter/base_spec.rb @@ -204,6 +204,8 @@ end after(:all) do + table "test_table" + adapter.execute "ROLLBACK" end From accaa5bf31fb57c81a0dbb6275d47612854359b1 Mon Sep 17 00:00:00 2001 From: Phil Date: Fri, 16 Sep 2022 12:05:23 -0700 Subject: [PATCH 5/6] Create spec for a table name with a period in it, and fixed issues around that. --- lib/chrono_model/adapter.rb | 14 ++++++++++--- lib/chrono_model/adapter/ddl.rb | 18 ++++++++++------- lib/chrono_model/adapter/indexes.rb | 27 ++++++++++++++++++++------ spec/chrono_model/adapter/base_spec.rb | 18 +++++++++++++++++ 4 files changed, 61 insertions(+), 16 deletions(-) diff --git a/lib/chrono_model/adapter.rb b/lib/chrono_model/adapter.rb index a2e2e748..4e391b05 100644 --- a/lib/chrono_model/adapter.rb +++ b/lib/chrono_model/adapter.rb @@ -140,9 +140,17 @@ def on_schema(schema, recurse: :follow) # Returns true if the given name references a temporal table. # def is_chrono?(table) - base_table_name = table.to_s.split(".").last - data_source_exists?("#{TEMPORAL_SCHEMA}.#{quote_table_name(base_table_name)}") && - data_source_exists?("#{HISTORY_SCHEMA}.#{quote_table_name(base_table_name)}") + table_name = if respond_to?(:extract_schema_qualified_name) + extract_schema_qualified_name(table).last + else + # AR 5 + tn = ActiveRecord::ConnectionAdapters::PostgreSQL::Utils.extract_schema_qualified_name(table.to_s) + tn.respond_to?(:identifier) ? tn.identifier : tn + end + table_name = "\"#{table_name}\"" if table_name[0] != '"' + + on_temporal_schema { data_source_exists?(table_name) } && + on_history_schema { data_source_exists?(table_name) } end # Reads the Gem metadata from the COMMENT set on the given PostgreSQL diff --git a/lib/chrono_model/adapter/ddl.rb b/lib/chrono_model/adapter/ddl.rb index 5bd21f4c..438cde62 100644 --- a/lib/chrono_model/adapter/ddl.rb +++ b/lib/chrono_model/adapter/ddl.rb @@ -78,8 +78,9 @@ def remove_history_validity_constraint(table, options = {}) # allow setting the PK to a specific value (think migration scenario). # def chrono_create_INSERT_trigger(table, pk, current, history, fields, values) + func_name = quote_identifier_name(prefix: "chronomodel_", table: table, suffix: "_insert") execute <<-SQL.strip_heredoc - CREATE OR REPLACE FUNCTION chronomodel_#{table}_insert() RETURNS TRIGGER AS $$ + CREATE OR REPLACE FUNCTION #{func_name}() RETURNS TRIGGER AS $$ BEGIN #{insert_sequence_sql(pk, current)} INTO #{current} ( #{pk}, #{fields} ) VALUES ( NEW.#{pk}, #{values} ); @@ -95,7 +96,7 @@ def chrono_create_INSERT_trigger(table, pk, current, history, fields, values) DROP TRIGGER IF EXISTS chronomodel_insert ON #{table}; CREATE TRIGGER chronomodel_insert INSTEAD OF INSERT ON #{table} - FOR EACH ROW EXECUTE PROCEDURE chronomodel_#{table}_insert(); + FOR EACH ROW EXECUTE PROCEDURE #{func_name}(); SQL end @@ -130,8 +131,9 @@ def chrono_create_UPDATE_trigger(table, pk, current, history, fields, values, op journal &= columns + func_name = quote_identifier_name(prefix: "chronomodel_", table: table, suffix: "_update") execute <<-SQL.strip_heredoc - CREATE OR REPLACE FUNCTION chronomodel_#{table}_update() RETURNS TRIGGER AS $$ + CREATE OR REPLACE FUNCTION #{func_name}() RETURNS TRIGGER AS $$ DECLARE _now timestamp; DECLARE _hid integer; DECLARE _old record; @@ -174,7 +176,7 @@ def chrono_create_UPDATE_trigger(table, pk, current, history, fields, values, op DROP TRIGGER IF EXISTS chronomodel_update ON #{table}; CREATE TRIGGER chronomodel_update INSTEAD OF UPDATE ON #{table} - FOR EACH ROW EXECUTE PROCEDURE chronomodel_#{table}_update(); + FOR EACH ROW EXECUTE PROCEDURE #{func_name}(); SQL end @@ -184,8 +186,9 @@ def chrono_create_UPDATE_trigger(table, pk, current, history, fields, values, op # DELETEd in the same transaction. # def chrono_create_DELETE_trigger(table, pk, current, history) + func_name = quote_identifier_name(prefix: "chronomodel_", table: table, suffix: "_delete") execute <<-SQL.strip_heredoc - CREATE OR REPLACE FUNCTION chronomodel_#{table}_delete() RETURNS TRIGGER AS $$ + CREATE OR REPLACE FUNCTION #{func_name}() RETURNS TRIGGER AS $$ DECLARE _now timestamp; BEGIN _now := timezone('UTC', now()); @@ -207,13 +210,14 @@ def chrono_create_DELETE_trigger(table, pk, current, history) DROP TRIGGER IF EXISTS chronomodel_delete ON #{table}; CREATE TRIGGER chronomodel_delete INSTEAD OF DELETE ON #{table} - FOR EACH ROW EXECUTE PROCEDURE chronomodel_#{table}_delete(); + FOR EACH ROW EXECUTE PROCEDURE #{func_name}(); SQL end def chrono_drop_trigger_functions_for(table_name) %w( insert update delete ).each do |func| - execute "DROP FUNCTION IF EXISTS chronomodel_#{table_name}_#{func}()" + func_name = quote_identifier_name(prefix: "chronomodel_", table: table_name, suffix: "_#{func}") + execute "DROP FUNCTION IF EXISTS #{func_name}()" end end diff --git a/lib/chrono_model/adapter/indexes.rb b/lib/chrono_model/adapter/indexes.rb index 26fb8858..8fac5408 100644 --- a/lib/chrono_model/adapter/indexes.rb +++ b/lib/chrono_model/adapter/indexes.rb @@ -70,7 +70,7 @@ def remove_timeline_consistency_constraint(table, options = {}) end def timeline_consistency_constraint_name(table) - "#{table}_timeline_consistency" + quote_identifier_name(table: table, suffix: "_timeline_consistency") end private @@ -79,9 +79,15 @@ def timeline_consistency_constraint_name(table) def chrono_create_history_indexes_for(table, p_pkey) add_temporal_indexes table, :validity, on_current_schema: true - execute "CREATE INDEX #{table}_inherit_pkey ON #{table} ( #{p_pkey} )" - execute "CREATE INDEX #{table}_recorded_at ON #{table} ( recorded_at )" - execute "CREATE INDEX #{table}_instance_history ON #{table} ( #{p_pkey}, recorded_at )" + { + "inherit_pkey" => p_pkey, + "recorded_at" => "recorded_at", + "instance_history" => "#{p_pkey}, recorded_at" + }.each do |suffix, columns| + index_name = quote_identifier_name(table: table, suffix: "_#{suffix}") + + execute "CREATE INDEX #{index_name} ON #{table} ( #{columns} )" + end end # Rename indexes on history schema @@ -153,7 +159,8 @@ def chrono_copy_indexes_to_history(table_name) # given range definition. # def temporal_index_names(table, range, options = {}) - prefix = options[:name].presence || "index_#{table}_temporal" + prefix = options[:name].presence || + quote_identifier_name(prefix: "index_", table: table, suffix: "_temporal") # When creating computed indexes # @@ -163,7 +170,7 @@ def temporal_index_names(table, range, options = {}) range = range.to_s.sub(/\W.*/, '') [range, "lower_#{range}", "upper_#{range}"].map do |suffix| - [prefix, 'on', suffix].join('_') + quote_identifier_name(table: prefix, suffix: "_on_#{suffix}") end end @@ -192,6 +199,14 @@ def chrono_alter_constraint(table_name, options) end end + def quote_identifier_name(prefix: "", table: "", suffix: "") + if table[0] == '"' && table[-1] == '"' + "\"#{prefix}#{table[1..-2]}#{suffix}\"" + else + "#{prefix}#{table}#{suffix}" + end + end + end diff --git a/spec/chrono_model/adapter/base_spec.rb b/spec/chrono_model/adapter/base_spec.rb index 55a50373..938114f9 100644 --- a/spec/chrono_model/adapter/base_spec.rb +++ b/spec/chrono_model/adapter/base_spec.rb @@ -217,5 +217,23 @@ it { expect(adapter.is_chrono?(table)).to be(false) } end end + + context "when the table has a period in the name" do + before(:all) do + table '"test.table"' + end + + after(:all) do + table "test_table" + end + + with_temporal_table do + it { expect(adapter.is_chrono?(table)).to be(true) } + end + + with_plain_table do + it { expect(adapter.is_chrono?(table)).to be(false) } + end + end end end From ea28153c9be19cd4d048f91c23df92f040aa3fd9 Mon Sep 17 00:00:00 2001 From: Phil Date: Fri, 16 Sep 2022 12:09:43 -0700 Subject: [PATCH 6/6] Fix for `Random.uuid` not available in older Ruby versions. --- spec/chrono_model/adapter/base_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/chrono_model/adapter/base_spec.rb b/spec/chrono_model/adapter/base_spec.rb index 938114f9..8d3309e1 100644 --- a/spec/chrono_model/adapter/base_spec.rb +++ b/spec/chrono_model/adapter/base_spec.rb @@ -172,7 +172,7 @@ context "within a different search_path" do before(:all) do - schema_name = "schema_#{Random.uuid.tr("-", "_")}" + schema_name = "schema_#{Random.rand(1000..10000)}" adapter.execute "BEGIN" adapter.execute "CREATE SCHEMA #{schema_name}" @@ -194,7 +194,7 @@ context "with a table in a different schema" do before(:all) do - schema_name = "schema_#{Random.uuid.tr("-", "_")}" + schema_name = "schema_#{Random.rand(1000..10000)}" adapter.execute "BEGIN" adapter.execute "CREATE SCHEMA #{schema_name}"