-
-
Notifications
You must be signed in to change notification settings - Fork 240
Add support for CASCADE in drop_view and update_view #218
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
Changes from 1 commit
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.drop_while{|v| v.name != name}.drop(1) | ||||||||||
|
|
||||||||||
| # Get indexes of existing materialized views | ||||||||||
|
Contributor
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| | ||||||||||
|
Contributor
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 | ||||||||||
|
Contributor
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) | ||||||||||
|
Contributor
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.
Contributor
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,16 +159,17 @@ 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 | ||||||||||
|
Contributor
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.
Contributor
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 | ||||||||||
|
|
||||||||||
| IndexReapplication.new(connection: connection).on(name) do | ||||||||||
| drop_materialized_view(name) | ||||||||||
| drop_materialized_view(name, cascade) | ||||||||||
| create_materialized_view(name, sql_definition) | ||||||||||
| end | ||||||||||
| end | ||||||||||
|
|
@@ -163,13 +179,14 @@ def update_materialized_view(name, sql_definition) | |||||||||
| # 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. | ||||||||||
|
Contributor
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.
Contributor
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.
Contributor
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 +255,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.
Contributor
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 | ||||||||||
|
Contributor
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 {.
Contributor
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 | ||||||||||
|
Contributor
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 | ||||||||||
|
Contributor
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 {.
Contributor
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. | ||||||
|
Contributor
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. | ||||||
|
Contributor
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. | ||||||
|
Contributor
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,99 @@ | ||||
| require "spec_helper" | ||||
|
|
||||
| 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 | ||||
|
|
||||
| it 'recreates the dependent 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. |
||||
| views = Scenic::Adapters::Postgres::Views.new(connection) | ||||
| 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. Parenthesize the param `change { |
||||
| run_migration(migration_for_update, :up) | ||||
| }.to_not change { | ||||
| views.all.length | ||||
| } | ||||
| end | ||||
|
|
||||
| it 'recreates indexes on the dependent 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. |
||||
| indexes = Scenic::Adapters::Postgres::Indexes.new(connection: connection) | ||||
| run_migration(migration_for_create_materialized_dependent, :up) | ||||
| run_migration(index_migration, :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. Parenthesize the param `change { |
||||
| run_migration(migration_for_update, :up) | ||||
| }.to_not change { | ||||
| indexes.on('dependent_greetings') | ||||
|
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. |
||||
| } | ||||
| end | ||||
|
|
||||
| it 'reverts' 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) | ||||
| run_migration(migration_for_update, :up) | ||||
| run_migration(migration_for_update, :down) | ||||
| greeting = execute("SELECT * FROM dependent_greetings")[0]["greeting"] | ||||
| expect(greeting).to eq 'hola' | ||||
|
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. |
||||
| end | ||||
| end | ||||
|
|
||||
| def migration_for_create | ||||
| Class.new(migration_class) do | ||||
| def change | ||||
| create_view :greetings | ||||
| create_view :dependent_greetings | ||||
| end | ||||
| end | ||||
| end | ||||
|
|
||||
| def migration_for_create_materialized_dependent | ||||
| Class.new(migration_class) do | ||||
| def change | ||||
| create_view :greetings | ||||
| create_view :dependent_greetings, materialized: true | ||||
| end | ||||
| end | ||||
| 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 | ||||
| Class.new(migration_class) do | ||||
| def change | ||||
| update_view :greetings, version: 2, revert_to_version: 1, cascade: true | ||||
| end | ||||
| end | ||||
| end | ||||
|
|
||||
| def index_migration | ||||
| Class.new(migration_class) do | ||||
| def change | ||||
| add_index :dependent_greetings, :greeting | ||||
| end | ||||
| end | ||||
| end | ||||
|
|
||||
|
Contributor
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.
Contributor
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 | ||||
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -52,23 +52,4 @@ def change | |||
| end | ||||
| end | ||||
|
|
||||
|
Contributor
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
|
||||
| def migration_class | ||||
| if Rails::VERSION::MAJOR >= 5 | ||||
| ::ActiveRecord::Migration[5.0] | ||||
| else | ||||
| ::ActiveRecord::Migration | ||||
| end | ||||
| end | ||||
|
|
||||
| def run_migration(migration, directions) | ||||
| silence_stream(STDOUT) do | ||||
| Array.wrap(directions).each do |direction| | ||||
| migration.migrate(direction) | ||||
| end | ||||
| end | ||||
| end | ||||
|
|
||||
| def execute(sql) | ||||
| ActiveRecord::Base.connection.execute(sql) | ||||
| end | ||||
| end | ||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -37,6 +37,13 @@ | |
| expect { recorder.revert { recorder.drop_view(*args) } } | ||
| .to raise_error(ActiveRecord::IrreversibleMigration) | ||
| end | ||
|
|
||
| it "raises when reverting with cascade set" do | ||
| args = [:users, { cascade: true, revert_to_version: 3 }] | ||
|
|
||
| expect { recorder.revert { recorder.drop_view(*args) } } | ||
| .to raise_error(ActiveRecord::IrreversibleMigration) | ||
|
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. Place the . on the previous line, together with the method call receiver. |
||
| end | ||
| end | ||
|
|
||
| describe "#update_view" do | ||
|
|
||
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.