Global configuration for :ignore_log_data option

[DmitryTsepelev]

This is the implementation of #109. The idea is that we store a list of models that allow overriding the global setting (i.e. where :ignore_log_data is not specified) and when global setting is changed - we start or stop ignoring the log_data column.

[palkan]

The current approach (model tracking and overriding) has one problem – it's not thread-safe.
And I don't think it's possible to make it thread-safe, unfortunately.

Looks like it's gonna be too complicated to patch this behaviour this way (via schema cache).

I have another idea: what if can add a default_scope which is only activated when the corresponding Logidze parameter (thread-safe) is true?

Smth like:

default_scope do
  with_log_data if Logidze.force_load_ignore_log_data
end

?

This way we cover the scenario: "ignore log data by default" + "load within a block".
And I don't think we need the opposite (at least for now): "enable by default" + "ignore within a block". Doesn't make a lot of sense to me.

[palkan]

Can't see where we add logidze config to Rails.

Let's just use Logidze.ignore_log_data_by_default = true (yeah, and rename the option).

[DmitryTsepelev]

✔️

[DmitryTsepelev]

The approach itself works fine (not all the tests are green now!), but the problem is that reload uses just self.class.find(self.id, options), so most of the specs fail without something like this. Moreover, when the column is ignored - Rails does not even define an attribute for it.

Since we're using default_scope I'm thinking about the opposite approach: no ignored_columns, default_scope excludes log_data from select by default and there is no need to hack reload

[palkan]

I'm thinking about the opposite approach: no ignored_columns, default_scope excludes log_data

I don't like the idea of replacing ignored_columns with the default scope: the first one is low-level, I'm afraid the latter one could have a lot of edge cases.

self.class.reset_column_information

This is not good for performance.

that reload uses just self.class.find(self.id, options),

Looks like it uses some scope-less version of .find, 'cause find itself uses default scopes.

So, the problem is only with reload; thus we can add reload_log_data call after reload if ignore_log_data is disabled (global parameter).

[DmitryTsepelev]

@palkan Done! I've patched unscoped instead of reload to make it a bit more readable, I've also had to redefine column_names, because Rails >=4 caches them based on ignored_columns and does not even save not listed attributes while updating the record

[palkan]

@DmitryTsepelev Looks good! Thanks!

@aderyabin Check please; any questions/suggestions?

[palkan]

@DmitryTsepelev Looks like Rails 4.2 can not do size / count with explicitly selected columns and we should use count(:all) (as I remember):

1) Logidze::Model Versioned associations when feature is enabled has_many returns not versioned association #size, due to AR implementaion
     Failure/Error: expect(very_old_article.comments.size).to eql(2)
     
     ActiveRecord::StatementInvalid:
       PG::UndefinedFunction: ERROR:  function count(integer, text, jsonb, integer, timestamp without time zone, timestamp without time zone, jsonb) does not exist
       LINE 1: SELECT COUNT(id, content, log_data, article_id, created_at, ...
                      ^
       HINT:  No function matches the given name and argument types. You might need to add explicit type casts.
       : SELECT COUNT(id, content, log_data, article_id, created_at, updated_at, log_data) FROM "comments" WHERE "comments"."article_id" = $1

Let's replace it with very_old_article.comments.to_a.size.

[DmitryTsepelev]

I bet our Rails 4 users still want to have working #size method :) I've patched with_log_data scope to call select("*") instead of select(column_names + ["log_data"]) when ignored_columns contains only "log_data" (which is the same thing as if Rails::VERSION::MAJOR == 4 because real ignored_columns were added in Rails 5). What do you think?