-
-
Notifications
You must be signed in to change notification settings - Fork 226
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
Add support for CASCADE in drop_view and update_view #218
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -75,11 +75,25 @@ def create_view(name, sql_definition) | |||||||||
# | ||||||||||
# @param name The name of the view to update | ||||||||||
# @param sql_definition The SQL schema for the updated view. | ||||||||||
# @param cascade Whether to drop and recreate dependent objects or not | ||||||||||
# | ||||||||||
# @return [void] | ||||||||||
def update_view(name, sql_definition) | ||||||||||
drop_view(name) | ||||||||||
def update_view(name, sql_definition, cascade=false) | ||||||||||
if cascade | ||||||||||
# Get existing views that could be dependent on this one. | ||||||||||
existing_views = views.reject{|v| v.name == name} | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
|
||||||||||
# Get indexes of existing materialized views | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
indexes = Indexes.new(connection: connection) | ||||||||||
view_indexes = existing_views.select(&:materialized).flat_map do |view| | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
indexes.on(view.name) | ||||||||||
end | ||||||||||
end | ||||||||||
|
||||||||||
drop_view(name, cascade) | ||||||||||
create_view(name, sql_definition) | ||||||||||
|
||||||||||
recreate_dropped_views(existing_views, views, view_indexes) if cascade | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
end | ||||||||||
|
||||||||||
# Replaces a view in the database using `CREATE OR REPLACE VIEW`. | ||||||||||
|
@@ -112,10 +126,11 @@ def replace_view(name, sql_definition) | |||||||||
# This is typically called in a migration via {Statements#drop_view}. | ||||||||||
# | ||||||||||
# @param name The name of the view to drop | ||||||||||
# @param cascade Whether to drop dependent objects or not | ||||||||||
# | ||||||||||
# @return [void] | ||||||||||
def drop_view(name) | ||||||||||
execute "DROP VIEW #{quote_table_name(name)};" | ||||||||||
def drop_view(name, cascade=false) | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
execute "DROP VIEW #{quote_table_name(name)}#{" CASCADE" if cascade};" | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Prefer single-quoted strings inside interpolations. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
end | ||||||||||
|
||||||||||
# Creates a materialized view in the database | ||||||||||
|
@@ -144,32 +159,47 @@ def create_materialized_view(name, sql_definition) | |||||||||
# | ||||||||||
# @param name The name of the view to update | ||||||||||
# @param sql_definition The SQL schema for the updated view. | ||||||||||
# @param cascade Whether to drop and recreate dependent objects or not | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
# | ||||||||||
# @raise [MaterializedViewsNotSupportedError] if the version of Postgres | ||||||||||
# in use does not support materialized views. | ||||||||||
# | ||||||||||
# @return [void] | ||||||||||
def update_materialized_view(name, sql_definition) | ||||||||||
def update_materialized_view(name, sql_definition, cascade=false) | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Surrounding space missing in default value assignment. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
raise_unless_materialized_views_supported | ||||||||||
|
||||||||||
if cascade | ||||||||||
# Get existing views that could be dependent on this one. | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
existing_views = views.reject{|v| v.name == name} | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Space missing to the left of {. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
|
||||||||||
# Get indexes of existing materialized views | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Trailing whitespace detected. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
indexes = Indexes.new(connection: connection) | ||||||||||
view_indexes = existing_views.select(&:materialized).flat_map do |view| | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Line is too long. [81/80] There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
indexes.on(view.name) | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
end | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
end | ||||||||||
|
||||||||||
IndexReapplication.new(connection: connection).on(name) do | ||||||||||
drop_materialized_view(name) | ||||||||||
drop_materialized_view(name, cascade) | ||||||||||
create_materialized_view(name, sql_definition) | ||||||||||
end | ||||||||||
|
||||||||||
recreate_dropped_views(existing_views, views, view_indexes) if cascade | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
end | ||||||||||
|
||||||||||
# Drops a materialized view in the database | ||||||||||
# | ||||||||||
# This is typically called in a migration via {Statements#update_view}. | ||||||||||
# | ||||||||||
# @param name The name of the materialized view to drop. | ||||||||||
# @param cascade Whether to drop dependent objects or not. | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
# @raise [MaterializedViewsNotSupportedError] if the version of Postgres | ||||||||||
# in use does not support materialized views. | ||||||||||
# | ||||||||||
# @return [void] | ||||||||||
def drop_materialized_view(name) | ||||||||||
def drop_materialized_view(name, cascade=false) | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Surrounding space missing in default value assignment. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
raise_unless_materialized_views_supported | ||||||||||
execute "DROP MATERIALIZED VIEW #{quote_table_name(name)};" | ||||||||||
execute "DROP MATERIALIZED VIEW #{quote_table_name(name)}#{" CASCADE" if cascade};" | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Prefer single-quoted strings inside interpolations. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
end | ||||||||||
|
||||||||||
# Refreshes a materialized view from its SQL schema. | ||||||||||
|
@@ -238,6 +268,24 @@ def refresh_dependencies_for(name) | |||||||||
connection, | ||||||||||
) | ||||||||||
end | ||||||||||
|
||||||||||
def recreate_dropped_views(old_views, current_views, indexes=[]) | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Surrounding space missing in default value assignment. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
index_reapplier = IndexReapplication.new(connection: connection) | ||||||||||
|
||||||||||
# Find any views that were lost | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
dropped_views = old_views.reject{|ov| current_views.any?{|cv| ov.name == cv.name}} | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Space missing to the left of {. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
# Recreate them | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
dropped_views.each do |view| | ||||||||||
if view.materialized | ||||||||||
create_materialized_view view.name, view.definition | ||||||||||
# Also recreate any indexes that were lost | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
lost_indexes = indexes.select{|index| index.object_name == view.name} | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Space missing to the left of {. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
lost_indexes.each{|index| index_reapplier.try_index_create index} | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Space missing to the left of {. |
||||||||||
else | ||||||||||
create_view view.name, view.definition | ||||||||||
end | ||||||||||
end | ||||||||||
end | ||||||||||
end | ||||||||||
end | ||||||||||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -45,6 +45,11 @@ def perform_scenic_inversion(method, args) | |
raise ActiveRecord::IrreversibleMigration, message | ||
end | ||
|
||
if method == :create_view && scenic_args.cascade | ||
message = "#{method} is not reversible if dependent objects were also dropped with CASCADE" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Line is too long. [99/80] |
||
raise ActiveRecord::IrreversibleMigration, message | ||
end | ||
|
||
[method, scenic_args.invert_version.to_a] | ||
end | ||
end | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -50,16 +50,18 @@ def create_view(name, version: nil, sql_definition: nil, materialized: false) | |||||
# `version` argument to {#create_view}. | ||||||
# @param materialized [Boolean] Set to true if dropping a meterialized view. | ||||||
# defaults to false. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
# @param cascade [Boolean] Set to true if dependent objects should also be | ||||||
# dropped. defaults to false. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
# @return The database response from executing the drop statement. | ||||||
# | ||||||
# @example Drop a view, rolling back to version 3 on rollback | ||||||
# drop_view(:users_who_recently_logged_in, revert_to_version: 3) | ||||||
# | ||||||
def drop_view(name, revert_to_version: nil, materialized: false) | ||||||
def drop_view(name, revert_to_version: nil, materialized: false, cascade: false) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unused method argument - revert_to_version. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unused method argument - revert_to_version. |
||||||
if materialized | ||||||
Scenic.database.drop_materialized_view(name) | ||||||
Scenic.database.drop_materialized_view(name, cascade) | ||||||
else | ||||||
Scenic.database.drop_view(name) | ||||||
Scenic.database.drop_view(name, cascade) | ||||||
end | ||||||
end | ||||||
|
||||||
|
@@ -77,12 +79,14 @@ def drop_view(name, revert_to_version: nil, materialized: false) | |||||
# `rake db rollback` | ||||||
# @param materialized [Boolean] True if updating a materialized view. | ||||||
# Defaults to false. | ||||||
# @param cascade [Boolean] Set to true if dependent objects should also be | ||||||
# dropped. defaults to false. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
# @return The database response from executing the create statement. | ||||||
# | ||||||
# @example | ||||||
# update_view :engagement_reports, version: 3, revert_to_version: 2 | ||||||
# | ||||||
def update_view(name, version: nil, sql_definition: nil, revert_to_version: nil, materialized: false) | ||||||
def update_view(name, version: nil, sql_definition: nil, revert_to_version: nil, materialized: false, cascade: false) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unused method argument - revert_to_version. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unused method argument - revert_to_version. |
||||||
if version.blank? && sql_definition.blank? | ||||||
raise( | ||||||
ArgumentError, | ||||||
|
@@ -100,9 +104,9 @@ def update_view(name, version: nil, sql_definition: nil, revert_to_version: nil, | |||||
sql_definition ||= definition(name, version) | ||||||
|
||||||
if materialized | ||||||
Scenic.database.update_materialized_view(name, sql_definition) | ||||||
Scenic.database.update_materialized_view(name, sql_definition, cascade) | ||||||
else | ||||||
Scenic.database.update_view(name, sql_definition) | ||||||
Scenic.database.update_view(name, sql_definition, cascade) | ||||||
end | ||||||
end | ||||||
|
||||||
|
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
@@ -0,0 +1,120 @@ | ||||
require "spec_helper" | ||||
|
||||
RSpec.shared_examples "a cascading migration" do |materialized| | ||||
it 'recreates the dependent view' do | ||||
views = Scenic::Adapters::Postgres::Views.new(connection) | ||||
run_migration(migration_for_create(materialized: materialized), :up) | ||||
expect { | ||||
run_migration(migration_for_update(materialized: materialized), :up) | ||||
}.to_not change { | ||||
views.all.length | ||||
} | ||||
end | ||||
|
||||
it 'recreates indexes on the dependent view' do | ||||
indexes = Scenic::Adapters::Postgres::Indexes.new(connection: connection) | ||||
run_migration(migration_for_create_materialized_dependent(materialized: materialized), :up) | ||||
run_migration(index_migration, :up) | ||||
expect { | ||||
run_migration(migration_for_update(materialized: materialized), :up) | ||||
}.to_not change { | ||||
indexes.on('dependent_greetings') | ||||
} | ||||
end | ||||
|
||||
it 'reverts' do | ||||
run_migration(migration_for_create(materialized: materialized), :up) | ||||
run_migration(migration_for_update(materialized: materialized), :up) | ||||
run_migration(migration_for_update(materialized: materialized), :down) | ||||
greeting = execute("SELECT * FROM dependent_greetings")[0]["greeting"] | ||||
expect(greeting).to eq 'hola' | ||||
end | ||||
end | ||||
|
||||
|
||||
describe "Dropping a view and its dependencies with cascade", :db do | ||||
around do |example| | ||||
with_view_definition :greetings, 1, "SELECT text 'hola' as greeting" do | ||||
with_view_definition :dependent_greetings, 1, "SELECT * from greetings" do | ||||
example.run | ||||
end | ||||
end | ||||
end | ||||
|
||||
it 'works' do | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping. |
||||
run_migration(migration_for_create, :up) | ||||
expect { | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Avoid using {...} for multi-line blocks. |
||||
run_migration(migration_for_drop, :up) | ||||
}.to_not raise_error | ||||
end | ||||
|
||||
describe 'as part of updating a view' do | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping. |
||||
around do |example| | ||||
with_view_definition :greetings, 2, "SELECT text 'good day' AS greeting" do | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Line is too long. [81/80] |
||||
example.run | ||||
end | ||||
end | ||||
|
||||
context 'with a non-materialized parent view' do | ||||
it_behaves_like "a cascading migration", false | ||||
end | ||||
|
||||
context 'with a materialized parent view' do | ||||
it_behaves_like "a cascading migration", true | ||||
end | ||||
end | ||||
|
||||
def migration_for_create(materialized: false) | ||||
eval <<-EOF | ||||
Class.new(migration_class) do | ||||
def change | ||||
create_view :greetings, materialized: #{materialized} | ||||
create_view :dependent_greetings | ||||
end | ||||
end | ||||
EOF | ||||
end | ||||
|
||||
def migration_for_create_materialized_dependent(materialized: false) | ||||
eval <<-EOF | ||||
Class.new(migration_class) do | ||||
def change | ||||
create_view :greetings, materialized: #{materialized} | ||||
create_view :dependent_greetings, materialized: true | ||||
end | ||||
end | ||||
EOF | ||||
end | ||||
|
||||
def migration_for_drop | ||||
Class.new(migration_class) do | ||||
def change | ||||
drop_view :greetings, revert_to_version: 1, cascade: true | ||||
end | ||||
end | ||||
end | ||||
|
||||
def migration_for_update(materialized: false) | ||||
eval <<-EOF | ||||
Class.new(migration_class) do | ||||
def change | ||||
update_view :greetings, | ||||
version: 2, | ||||
revert_to_version: 1, | ||||
cascade: true, | ||||
materialized: #{materialized} | ||||
end | ||||
end | ||||
EOF | ||||
end | ||||
|
||||
def index_migration | ||||
Class.new(migration_class) do | ||||
def change | ||||
add_index :dependent_greetings, :greeting | ||||
end | ||||
end | ||||
end | ||||
|
||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||
|
||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Extra blank line detected. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.