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

Better check for whether or not a table is_chrono? #98

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions lib/chrono_model/adapter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -140,8 +140,17 @@ 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) }
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
Expand Down
18 changes: 11 additions & 7 deletions lib/chrono_model/adapter/ddl.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we have this as a method?

Also with a couple of constants like FUNCTION_PREFIX = 'chronomodel' and FUNCTION_SEPARATOR = '_'

This method should work like function_name(table, 'insert') and return chronomodel_table_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} );
Expand All @@ -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

Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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

Expand All @@ -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());
Expand All @@ -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

Expand Down
27 changes: 21 additions & 6 deletions lib/chrono_model/adapter/indexes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
#
Expand All @@ -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

Expand Down Expand Up @@ -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

Expand Down
85 changes: 85 additions & 0 deletions spec/chrono_model/adapter/base_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,25 @@
it { expect(adapter.is_chrono?(table)).to be(false) }
end

context "when passing a fully specified schema" do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the failing spec, but to speed up things I need to better understand what it is the use case that this change is addressing.

Any chance to fork https://github.com/diowa/ruby3-rails7-bootstrap-heroku/tree/chronomodel and add a commit with the minimum amount of code needed to understand the impact of this issue on an existing application?

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'
Expand All @@ -150,5 +169,71 @@

it { expect(adapter.is_chrono?(table)).to be(false) }
end

context "within a different search_path" do
before(:all) do
schema_name = "schema_#{Random.rand(1000..10000)}"

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.rand(1000..10000)}"

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
table "test_table"

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 "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