Skip to content

Commit

Permalink
Fix MySQL most_recent column
Browse files Browse the repository at this point in the history
  • Loading branch information
greysteil committed Apr 14, 2015
1 parent c7f1265 commit ed64413
Show file tree
Hide file tree
Showing 10 changed files with 101 additions and 16 deletions.
4 changes: 4 additions & 0 deletions lib/generators/statesman/generator_helpers.rb
Expand Up @@ -36,5 +36,9 @@ def mysql?
ActiveRecord::Base.configurations[Rails.env].
try(:[], "adapter").try(:match, /mysql/)
end

def database_supports_partial_indexes?
Statesman::Adapters::ActiveRecord.database_supports_partial_indexes?
end
end
end
Expand Up @@ -2,12 +2,18 @@ class AddConstraintsToMostRecentFor<%= migration_class_name %> < ActiveRecord::M
disable_ddl_transaction!

def up
<%- if database_supports_partial_indexes? -%>
add_index :<%= table_name %>, [:<%= parent_id %>, :most_recent], unique: true, where: "most_recent", name: "index_<%= table_name %>_parent_most_recent", algorithm: :concurrently
change_column_null :<%= table_name %>, :most_recent, false
<%- else -%>
add_index :<%= table_name %>, [:<%= parent_id %>, :most_recent], unique: true, name: "index_<%= table_name %>_parent_most_recent", algorithm: :concurrently
<%- end -%>
end

def down
remove_index :<%= table_name %>, name: "index_<%= table_name %>_parent_most_recent"
<%- if database_supports_partial_indexes? -%>
change_column_null :<%= table_name %>, :most_recent, true
<%- end -%>
end
end
13 changes: 10 additions & 3 deletions lib/generators/statesman/templates/create_migration.rb.erb
Expand Up @@ -5,11 +5,18 @@ class Create<%= migration_class_name %> < ActiveRecord::Migration
t.text :metadata<%= ", default: \"{}\"" unless mysql? %>
t.integer :sort_key, null: false
t.integer :<%= parent_id %>, null: false
t.boolean :most_recent, null: false
t.boolean :most_recent<%= ", null: false" if database_supports_partial_indexes? %>
t.timestamps null: false
end

add_index :<%= table_name %>, [:<%= parent_id %>, :sort_key], unique: true, name: "<%= index_name :parent_sort %>"
add_index :<%= table_name %>, [:<%= parent_id %>, :most_recent], unique: true, where: "most_recent", name: "<%= index_name :parent_most_recent %>"
add_index(:<%= table_name %>,
[:<%= parent_id %>, :sort_key],
unique: true,
name: "<%= index_name :parent_sort %>")
add_index(:<%= table_name %>,
[:<%= parent_id %>, :most_recent],
unique: true,
<%= "where: 'most_recent'," if database_supports_partial_indexes? %>
name: "<%= index_name :parent_most_recent %>")
end
end
22 changes: 21 additions & 1 deletion lib/statesman/adapters/active_record.rb
Expand Up @@ -8,6 +8,15 @@ class ActiveRecord

JSON_COLUMN_TYPES = %w(json jsonb).freeze

def self.database_supports_partial_indexes?
# Rails 3 doesn't implement `supports_partial_index?`
if ::ActiveRecord::Base.connection.respond_to?(:supports_partial_index?)
::ActiveRecord::Base.connection.supports_partial_index?
else
::ActiveRecord::Base.connection.adapter_name == 'PostgreSQL'
end
end

def initialize(transition_class, parent_model, observer, options = {})
serialized = serialized?(transition_class)
column_type = transition_class.columns_hash['metadata'].sql_type
Expand Down Expand Up @@ -78,7 +87,18 @@ def transitions_for_parent

def unset_old_most_recent
return unless most_recent_column?
transitions_for_parent.update_all(most_recent: false)
# Check whether the `most_recent` column allows null values. If it
# doesn't, set old records to `false`, otherwise, set them to `NULL`.
#
# Some conditioning here is required to support databases that don't
# support partial indexes. By doing the conditioning on the column,
# rather than Rails' opinion of whether the database supports partial
# indexes, we're robust to DBs later adding support for partial indexes.
if transition_class.columns_hash['most_recent'].null == false

This comment has been minimized.

Copy link
@davegudge

davegudge Jul 1, 2015

Contributor

@greysteil I'm using PostgreSQL and when attempting the migration steps outlined in the CHANGELOG, transition_class.columns_hash['most_recent'].null is returning true thus most_recent is updated to nil where as prior to this PR most_recent was updated to false.

When using a database that supports partial indexes, the most_recent not null constraint is added after this rake task via the execution of change_column_null :allocation_transitions, :most_recent, false

transitions_for_parent.update_all(most_recent: false)
else
transitions_for_parent.update_all(most_recent: nil)
end
end

def most_recent_column?
Expand Down
11 changes: 8 additions & 3 deletions lib/tasks/statesman.rake
Expand Up @@ -15,9 +15,14 @@ namespace :statesman do

parent_class.find_in_batches(batch_size: batch_size) do |models|
ActiveRecord::Base.transaction do
# Set all transitions' most_recent to FALSE
transition_class.where(parent_fk => models.map(&:id)).
update_all(most_recent: false)
if transition_class.columns_hash['most_recent'].null == false
# Set all transitions' most_recent to FALSE
transition_class.where(parent_fk => models.map(&:id)).
update_all(most_recent: false)
else
transition_class.where(parent_fk => models.map(&:id)).
update_all(most_recent: nil)
end

# Set current transition's most_recent to TRUE
ActiveRecord::Base.connection.execute %{
Expand Down
@@ -0,0 +1,11 @@
class AddConstraintsToMostRecentForBaconTransitions < ActiveRecord::Migration
disable_ddl_transaction!

def up
add_index :bacon_transitions, [:bacon_id, :most_recent], unique: true, name: "index_bacon_transitions_parent_most_recent", algorithm: :concurrently
end

def down
remove_index :bacon_transitions, name: "index_bacon_transitions_parent_most_recent"
end
end
Expand Up @@ -22,8 +22,13 @@
end

let(:fixture_file) do
File.read("spec/fixtures/add_constraints_to_most_recent_for_"\
"bacon_transitions.rb")
if Statesman::Adapters::ActiveRecord.database_supports_partial_indexes?
File.read("spec/fixtures/add_constraints_to_most_recent_for_"\
"bacon_transitions_with_partial_index.rb")
else
File.read("spec/fixtures/add_constraints_to_most_recent_for_"\
"bacon_transitions_without_partial_index.rb")
end
end

before do
Expand Down
14 changes: 13 additions & 1 deletion spec/statesman/adapters/active_record_spec.rb
Expand Up @@ -145,7 +145,7 @@
it "updates the previous transition's most_recent flag" do
expect { create }.
to change { previous_transition.reload.most_recent }.
from(true).to(false)
from(true).to be_falsey
end

context "and the parent model is updated in a callback" do
Expand All @@ -162,6 +162,18 @@
end
end
end

context "with two previous transitions" do
let!(:previous_transition) { adapter.create(from, to) }
let!(:another_previous_transition) { adapter.create(from, to) }
its(:most_recent) { is_expected.to eq(true) }

it "updates the previous transition's most_recent flag" do
expect { create }.
to change { another_previous_transition.reload.most_recent }.
from(true).to be_falsey
end
end
end

context "when the transition_class doesn't have a most_recent column" do
Expand Down
27 changes: 21 additions & 6 deletions spec/support/active_record.rb
Expand Up @@ -50,7 +50,6 @@ def change
t.string :to_state
t.integer :my_active_record_model_id
t.integer :sort_key
t.boolean :most_recent, default: true, null: false

# MySQL doesn't allow default values on text fields
if ActiveRecord::Base.connection.adapter_name == 'Mysql2'
Expand All @@ -59,17 +58,33 @@ def change
t.text :metadata, default: '{}'
end

if Statesman::Adapters::ActiveRecord.database_supports_partial_indexes?
t.boolean :most_recent, default: true, null: false
else
t.boolean :most_recent, default: true
end

t.timestamps null: false
end

add_index :my_active_record_model_transitions,
[:my_active_record_model_id, :sort_key],
unique: true, name: "sort_key_index"
add_index :my_active_record_model_transitions,
[:my_active_record_model_id, :most_recent],
unique: true, where: "most_recent",
name: "index_my_active_record_model_transitions_"\
"parent_most_recent"

if Statesman::Adapters::ActiveRecord.database_supports_partial_indexes?
add_index :my_active_record_model_transitions,
[:my_active_record_model_id, :most_recent],
unique: true,
where: "most_recent",
name: "index_my_active_record_model_transitions_"\
"parent_most_recent"
else
add_index :my_active_record_model_transitions,
[:my_active_record_model_id, :most_recent],
unique: true,
name: "index_my_active_record_model_transitions_"\
"parent_most_recent"
end
end
end
# rubocop:enable MethodLength
Expand Down

0 comments on commit ed64413

Please sign in to comment.