palkan / logidze

Database changes log for Rails
MIT License
1.59k stars 74 forks source link

Activerecord table_name_prefix or table_name_suffix are not honored #207

Closed cavi21 closed 2 years ago

cavi21 commented 2 years ago

Ruby Version: 2.7.2

Rails Version: 6.1.4.4

PostgreSQL Version: 10.1

Logidze Version: 1.2.0

What did you do?

In the project we have the table_name_prefix define in the application.rb so every table of the models are prefixed:

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

We are also using fx (0.6.2) in order to have everything we need in the schema.rb. So with this setup the logidze:install and logidze:model are not generating the the expected outputs.

What did you expect to happen?

On on both generators (logidze:install and logidze:model) we expected that the functions and triggers names generated include the project-prefix- on there filenames, for instance:

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

And on logidze:model also the trigger we expected to include the prefix (and the suffix) on the code generated, for example:

CREATE TRIGGER "logidze_on_project-prefix-some_table"
BEFORE UPDATE OR INSERT ON "project-prefix-some_table" FOR EACH ROW
WHEN (coalesce(current_setting('logidze.disabled', true), '') <> 'on')
#...

and on the migration file should also need to be applied, for example:

#...
      dir.down do
        execute "DROP TRIGGER IF EXISTS \"logidze_on_project-prefix-some_table\" on \"project-prefix-some_table\";"
      end
#...

What actually happened?

The functions and triggers did not honored the prefix, neither on the file names generated (in functions and triggers) or the sql code generated.

What we found so far

In regards to the generated file names, is related to howfx 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
      #...

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)

And about the generated code seems that similar approach to the proper_table_name() method is needed. For what I can see Activerecord migrations apply this prefix and suffix when the migration is run and not when is generated, but because here the trigger code is generating the sql we need to have them in mind when the generator is run.

I did not open a PR, because not sure what you prefer to do with this cases. Seems that the name of the generated files could be something related to fx, but for the generated code seems that should be handle by logidze, right?

Let me know what do you think! Thanks!

palkan commented 2 years ago

Seems that the name of the generated files could be something related to fx, but for the generated code seems that should be handle by logidze, right?

Right. We need to add, for example, a #full_table_name helper method which takes into account Active Record configuration and use it instead of the #table_name in the templates:

def full_table_name
  config = ActiveRecord::Base
  "#{config.table_name_prefix}#{table_name}#{config.table_name_suffix}"
end
cavi21 commented 2 years ago

Hi @palkan thanks for looking into this! If you're ok I can open a PR with what you propose, or not sure if you have another idea in mind? Anything let me know

palkan commented 2 years ago

Yeah, feel free to open a PR 👍