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 4 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
5 changes: 3 additions & 2 deletions lib/chrono_model/adapter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -140,8 +140,9 @@ 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) }
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)}")
Copy link
Member

Choose a reason for hiding this comment

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

I would suggest this change for the moment, if you think it is the same thing (specs are green)

Suggested change
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_without_schema = quote_table_name(table.to_s.split('.').last)
on_temporal_schema { data_source_exists?(table_name_without_schema) } &&
on_history_schema { data_source_exists?(table_name_without_schema) }

end

# Reads the Gem metadata from the COMMENT set on the given PostgreSQL
Expand Down
67 changes: 67 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,53 @@

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