teoljungberg / fx

Versioned database functions and triggers for Rails
MIT License
763 stars 77 forks source link

Issue for when the table_name_prefix or table_name_suffix are used #81

Closed cavi21 closed 1 year ago

cavi21 commented 2 years ago

Hi! not sure if I'm missing something (maybe a config or similar) but when a project uses table_name_prefix (or table_name_suffix) on the application.rb, like:

module ProjectName
  class Application < Rails::Application
    # ...
    config.active_record.table_name_prefix = 'project-prefix-'
    # ...
  end
end

Seems that the generated functions or triggers should be aware of, in such the seems to need to generate the files like:

/functions/project-prefix-logidze_capture_exception_v01.sql
...
/triggers/project-prefix-logidze_on_some_table_v01.sql

And as metioned in the Issue #207 on the logidze gem this issue is related to how fx implemented the create_function method. It is though the method_missing defined in activerecord-6.1.4.4/lib/active_record/migration.rb:915 and so in there the method proper_table_name( ) is called, and in such when you have in the migration:

      #...
      dir.up do
        create_function :logidze_capture_exception, version: 1
      end
      #...

When the migration is run, the file that looks for is functions/project-prefix-logidze_capture_exception_v01.sql and the same happes with the triggers so if in the migration there is:

      #...
      dir.up do
        create_trigger :logidze_on_some_table, on: :some_table
      end
      #...

The file that looks is triggers/project-prefix-logidze_on_some_table_v01.sql and notice that the second argument is fine because the same proper_table_name( ) is called on them so the actual table name is referenced, in this case project-prefix-some_table

Not sure if this is something that you have in mind or do you want to address it. If so, what approach do you prefer? Thanks for all the work you put into this gem!

teoljungberg commented 2 years ago

I have never seen this usage before, it's an interesting one. If there are active-record helpers for this - we should totally use them. I'd entertain a PR that fixes this.

cavi21 commented 2 years ago

Hi @teoljungberg thanks for the reply! let me know if you want me to help in anyway!

teoljungberg commented 2 years ago

Feel free to take a stab at implementing this in a PR, that would be grand!

cavi21 commented 2 years ago

perfect @teoljungberg I'll look into it this weekend!

cavi21 commented 2 years ago

Hi @teoljungberg I just try to run a few quick checks, but seems that for some reason pry hangs, not sure is this happens to you as well or do you have any idea why that could it be? On other gems and projects is not happening. But in the context of fx it hangs whenever I call a binding.pry, and I saw that you have it as a dev dependency so I thought that maybe you face a similar issue at some point?

Edit: I found that the binding.pry call were being swallowed due to the silence_stream... called on the /spec/support/migration_helpers.rb.

  #...
  def run_migration(migration, directions)
    silence_stream(STDOUT) do
      Array.wrap(directions).each do |direction|
        migration.migrate(direction)
      end
    end
  end
  #...

In case anyone struggle with the same

cavi21 commented 2 years ago

I've opened #82 to address this issue

teoljungberg commented 2 years ago

Not that I recall, will review #82 during the week.

teoljungberg commented 1 year ago

As the PR is closed, I've closed this issue too.