Migrations Using change() method: undefined method `each' for nil:NilClass

[sshaw]

Looks like the s-expression checking doesn't look for change, this causes a NoMethodError on the next line.
Additionally the rescue block returns nil, which results in another NoMethodError in the caller.

[jenseng]

one tricky thing about supporting change is that the create_trigger doesn't necessarily contain sufficient information to be reversible (i.e. if the migration replaces an existing trigger of the same name). there's a similar difficulty with a drop_trigger migration. i suppose we could add limited support for triggers in change migrations (would need a big caveat in the docs).

regardless though, hairtrigger should definitely be fixed so it doesn't blow up when it encounters a change

[sshaw]

Using up and down is fine I guess, you should mention this in the docs.

one tricky thing about supporting change is that the create_trigger doesn't necessarily contain sufficient
information to be reversible (i.e. if the migration replaces an existing trigger of the same name)

Not sure I understand you here, what does it need to know that it is missing?

there's a similar difficulty with a drop_trigger migration.

Yes, initially I thought I could just say:

# up
create_trigger.on(:users).after(:insert) do  "..." end

# down
drop_trigger.on(:users).after(:insert)

But I have to provide drop_trigger a name even if I didn't use one on create_trigger.
So... I did provide a name, but it seems that the sexp code is having problems with this:

class AddTriger < ActiveRecord::Migration
  def up
    create_trigger("bs").on(:users).after(:insert) do 
      "select 1"
    end
  end

  def down
    drop_trigger("bs", :users)
  end
end

Exception:

Error reading triggers in db/migrate/002_add_triger.rb: undefined method `[]' for nil:NilClass
rake aborted!
NoMethodError: undefined method `empty?' for nil:NilClass
/Users/sshaw/.rvm/gems/ruby-1.9.3-p448/gems/hairtrigger-0.2.6/lib/hair_trigger.rb:67:in `block in current_migrations'
/Users/sshaw/.rvm/gems/ruby-1.9.3-p448/gems/hairtrigger-0.2.6/lib/hair_trigger.rb:64:in `each'
/Users/sshaw/.rvm/gems/ruby-1.9.3-p448/gems/hairtrigger-0.2.6/lib/hair_trigger.rb:64:in `current_migrations'
/Users/sshaw/.rvm/gems/ruby-1.9.3-p448/gems/hairtrigger-0.2.6/lib/hair_trigger/schema_dumper.rb:15:in `triggers'
/Users/sshaw/.rvm/gems/ruby-1.9.3-p448/gems/hairtrigger-0.2.6/lib/hair_trigger/schema_dumper.rb:4:in `trailer_with_triggers'
/Users/sshaw/.rvm/gems/ruby-1.9.3-p448/gems/activerecord-4.0.0/lib/active_record/schema_dumper.rb:29:in `dump'
/Users/sshaw/.rvm/gems/ruby-1.9.3-p448/gems/activerecord-4.0.0/lib/active_record/schema_dumper.rb:21:in `dump'
/Users/sshaw/.rvm/gems/ruby-1.9.3-p448/gems/activerecord-4.0.0/lib/active_record/railties/databases.rake:244:in `block (4 levels) in <top (required)>'
/Users/sshaw/.rvm/gems/ruby-1.9.3-p448/gems/activerecord-4.0.0/lib/active_record/railties/databases.rake:243:in `open'
/Users/sshaw/.rvm/gems/ruby-1.9.3-p448/gems/activerecord-4.0.0/lib/active_record/railties/databases.rake:243:in `block (3 levels) in <top (required)>'
/Users/sshaw/.rvm/gems/ruby-1.9.3-p448/gems/activerecord-4.0.0/lib/active_record/railties/databases.rake:50:in `block (2 levels) in <top (required)>'
/Users/sshaw/.rvm/gems/ruby-1.9.3-p448/gems/activerecord-4.0.0/lib/active_record/railties/databases.rake:45:in `block (2 levels) in <top (required)>'

Why do you have to read the migration files?

Also, for triggers created "manually" (say via execute), the resulting schema.rb file contains the create trigger ... statements, but does not drop them first. This results in duplicate triggers errors when running test cases.

[jenseng]

Not sure I understand you here, what does it need to know that it is missing?

As currently implemented, create_trigger will automatically drop an existing trigger of the same name before creating a new one. So when rolling back a create_trigger migration, the correct behavior (delete vs. create a different trigger) would depend on the cumulative state of migrations before that one.

In hindsight, perhaps create_triggger should have been implemented differently, but the current behavior facilitates a lot of model trigger stuff.

But I have to provide drop_trigger a name even if I didn't use one on create_trigger.
So... I did provide a name, but it seems that the sexp code is having problems with this:

Yeah that example seems like it should work, I'll see what's up with that.

Why do you have to read the migration files?

Two reasons:

1. So that we can dump them db-agnostically in schema.rb (one of the main goals of HairTrigger). Basically we figure out what the current state of the database should be wrt triggers, and dump out the create_trigger calls. Otherwise they would just end up as execute "CREATE TRIGGER ..."
2. We also need to read the migrations for determining if/when to create a new model trigger migration. HairTrigger diffs the model triggers and the migration triggers to know what has changed.

Also, for triggers created "manually" (say via execute), the resulting schema.rb file contains the create trigger ... statements, but does not drop them first. This results in duplicate triggers errors when running test cases.

Isn't that the correct behavior though? Typically schema.rb would be applied to an empty database. In the same vein, schema.rb doesn't try to drop tables or indexes before it creates them (afaik).

[sshaw]

Also, for triggers created "manually" (say via execute), the resulting schema.rb file
contains the create trigger ... statements, but does not drop them first.

Isn't that the correct behavior though? Typically schema.rb would be applied to an empty database.
... schema.rb doesn't try to drop tables or indexes before it creates them (afaik).

By default the DB is recreated each time test are run, which makes sense unless you're using something like DatabaseCleaner.

schema.rb uses the force option when calling create_table to ensure the tables are dropped. It looks like this is done by create_trigger too, just not via the code that generates the raw SQL -which results in a dup trigger error.

As for the problem with create_trigger's name argument it looks like I can call name() instead.

Why do you have to read the migration files?

... Basically we figure out what the current state of the database should be wrt triggers,
and dump out the create_trigger calls.

Can't this be figured out via DB calls like show triggers in MySQL? Not trying to be critical here, just curious. This is a great gem!

[jenseng]

By default the DB is recreated each time test are run

Good point, I typically just do RAILS_ENV=test rake db:migrate (when needed), but I can see how the current behavior would be problematic if you use rake db:test:prepare.

Independent of this bug (or even HairTrigger), if you don't care about supporting other dbs, one thing you might consider is config.active_record.schema_format = :sql and rake db:test:clone_structure ... that'll make your migrations and test setup faster, since they won't have to build or apply schema.rb, respectively.

Can't this be figured out via DB calls like show triggers in MySQL?

Absolutely, in fact that's where the execute "CREATE TRIGGER..." in schema.rb comes from. But we're aiming for portability, so create_trigger calls are preferable so that it will work on any database. Trying to reverse engineer those from SHOW TRIGGERS (and the like) would get a little dicey (e.g. consider trigger groups).

My initial motivator in writing HairTrigger was so I could add triggers to canvas-lms, which at the time supported sqlite, mysql and postgres. If the migrations and schema.rb just had inferred raw CREATE TRIGGER calls from the model triggers, that would kill portability.

But yeah, regardless, HairTrigger should add the appropriate DROP TRIGGER IF EXISTS... calls before any raw CREATE TRIGGER calls it's going to do in schema.rb.

[jenseng]

@sshaw see my comments on #27 ... I'm having a hard time reproducing the dup triggers problem, so your input there would be appreciated