create_function and create_trigger aren't reversible if given a version parameter

[pjungwir]

Hello, thanks for this gem!

The create_function and create_trigger commands both accept a version argument, but when you roll back the migration you get this failure, because drop_function and drop_trigger don't know about that param:

ArgumentError: unknown keyword: version

(I'm not really sure why create_ commands would need a version other than 1, but I guess someone must be using it.)

I can see that you're already trying to make these commands reversible. Would you be interested in a PR that knows how to reverse creating functions & triggers when given a version? I guess it would still just DROP them.

[teoljungberg]

Hi!

I'm open for a PR fixing this, they should be reversible.

[pjungwir]

I'm just getting started with this, but when I run the tests on a fresh checkout (following the CONTRIBUTING.md instructions) I get a failure. Postgres adds the schema name to the table:

>> BUNDLE_GEMFILE=/home/paul/src/fx/gemfiles/rails40.gemfile bundle exec rake

Randomized with seed 4811
....................................................................F...........

Failures:

  1) Fx::Adapters::Postgres::Triggers.all returns `Trigger` objects
     Failure/Error:
                 expect(first.definition).to eq <<-EOS.strip_heredoc.strip
                   CREATE TRIGGER uppercase_users_name BEFORE INSERT ON users FOR EACH ROW EXECUTE PROCEDURE uppercase_users_name()
                 EOS

       expected: "CREATE TRIGGER uppercase_users_name BEFORE INSERT ON users FOR EACH ROW EXECUTE PROCEDURE uppercase_users_name()"
            got: "CREATE TRIGGER uppercase_users_name BEFORE INSERT ON public.users FOR EACH ROW EXECUTE PROCEDURE uppercase_users_name()"

       (compared using ==)
     # ./spec/fx/adapters/postgres/triggers_spec.rb:37:in `block (3 levels) in <module:Adapters>'
     # ./spec/spec_helper.rb:13:in `block (2 levels) in <top (required)>'

Finished in 0.33135 seconds (files took 0.57803 seconds to load)
80 examples, 1 failure

Failed examples:

rspec ./spec/fx/adapters/postgres/triggers_spec.rb:7 # Fx::Adapters::Postgres::Triggers.all returns `Trigger` objects

I take it you don't get that? I've tried against Postgres 9.3 and 9.6 (Ubuntu 16.04). It looks like the most recent build on Travis is from 2 years ago, so I'm curious if these tests are still passing for you.

Note that recent Postgres code will always schema-qualify the table:

  /*
   * In non-pretty mode, always schema-qualify the target table name for
   * safety.  In pretty mode, schema-qualify only if not visible.
   */
  appendStringInfo(&buf, " ON %s ",
           pretty ?
           generate_relation_name(trigrec->tgrelid, NIL) :
           generate_qualified_relation_name(trigrec->tgrelid));

Older code would guess whether it was needed though, based on schema visibility.

[teoljungberg]

Hi @pjungwir, sorry for taking so long to loop back to this. I haven't worked much on the project as it works flawlessly in the apps I work on a daily basis. So that there hasn't been any builds or work done is explained by that.

But yes, I do experience the same errors. As you said the default behavior seems to be to always add the qualified relation name, do you think it makes sense to update the test to reflect that? If so - how?

[teoljungberg]

Something like #19 should be sufficient. Let me know what you think.

[teoljungberg]

Echoing the comment I made in the PR:

https://github.com/teoljungberg/fx/pull/25#issuecomment-486963934