Fix reader creation

[mrbrdo]

To me this seems not to break anything and fixes #569

Please note I did not update existing specs that explicitly test that translations have been created on read, which is now not the case so they are failing. However I do not see any reason for attr reader to build translations on parent.

[mrbrdo]

@shioyama could you take a look? You can update the failing few specs since they seem to be testing internal logic (which I think should not be tested, or said another way, the test should not depend on internal implementation, such as that reading an attr creates a translation in the db).

[shioyama]

Thanks @mrbrdo , really appreciate it. I'll have a look this weekend.

[mrbrdo]

@shioyama did you manage to take a look?

[mrbrdo]

@shioyama managed to check this yet?

[shioyama]

Hmm I think this is what the presence plugin is for. Also you have thrown away the **options here which will be a problem.

[shioyama]

It seems like github actions didn't run on this PR... not sure why 🤔

[shioyama]

Oh this: https://docs.github.com/en/actions/managing-workflow-runs/approving-workflow-runs-from-public-forks

[shioyama]

Sorry @mrbrdo I can't seem to approve github actions on this, but I tried changing some repo settings. Can you push a new commit? I'll try to kick CI so it runs. You're not actually getting test results, I'm pretty sure tests will fail on this change.

[shioyama]

Ok I don't know what's going on with actions but when I run this locally 4 tests fail:

Failures:

  1) Mobility::Backends::ActiveRecord::Table translations association cleaning up blank translations builds nil translations when reading but does not save them
     Failure/Error: expect(article.send(association_name).size).to eq(3)
     
       expected: 3
            got: 1
     
       (compared using ==)
     # ./spec/mobility/backends/active_record/table_spec.rb:105:in `block (5 levels) in <top (required)>'
     # ./spec/mobility/backends/active_record/table_spec.rb:104:in `block (4 levels) in <top (required)>'

  2) Mobility::Backends::ActiveRecord::Table with cache only fetches translation once per locale
     Failure/Error: find { |t| t.locale == locale }
     
       (#<ActiveRecord::Associations::CollectionProxy [#<Article::Translation id: nil, locale: "en", article_id: nil, title: "bar", subtitle: nil, content: nil, created_at: nil, updated_at: nil>]>).find(no args)
           expected: 2 times with any arguments
           received: 3 times
     # ./lib/mobility/backends/active_record/table.rb:302:in `in_locale'
     # ./lib/mobility/backends/active_record/table.rb:292:in `translation_for'
     # ./lib/mobility/backends/table.rb:140:in `translation_for'
     # ./lib/mobility/backends/table.rb:94:in `write'
     # ./spec/mobility/backends/active_record/table_spec.rb:55:in `block (4 levels) in <top (required)>'
     # ./spec/mobility/backends/active_record/table_spec.rb:50:in `block (3 levels) in <top (required)>'

  3) Mobility::Backends::ActiveRecord::Table with cache resets model translations cache when model is saved or reloaded
     Failure/Error: expect(article.instance_variable_get(:@__mobility_translations_cache).size).to eq(1)
     
     NoMethodError:
       undefined method `size' for nil:NilClass
     # ./spec/mobility/backends/active_record/table_spec.rb:67:in `block (4 levels) in <top (required)>'
     # ./spec/mobility/backends/active_record/table_spec.rb:65:in `block (3 levels) in <top (required)>'

  4) Mobility::Backends::ActiveRecord::Table Backend methods #read builds translation if no translation exists
     Failure/Error:
       expect {
         title_backend.read(:de)
       }.to change(subject.send(title_backend.association_name), :size).by(1)
     
       expected `Article::Translation::ActiveRecord_Associations_CollectionProxy#size` to have changed by 1, but was changed by 0
     # ./spec/mobility/backends/active_record/table_spec.rb:184:in `block (4 levels) in <top (required)>'

Finished in 14.68 seconds (files took 1.13 seconds to load)
1272 examples, 4 failures

Failed examples:

rspec ./spec/mobility/backends/active_record/table_spec.rb:93 # Mobility::Backends::ActiveRecord::Table translations association cleaning up blank translations builds nil translations when reading but does not save them
rspec ./spec/mobility/backends/active_record/table_spec.rb:46 # Mobility::Backends::ActiveRecord::Table with cache only fetches translation once per locale
rspec ./spec/mobility/backends/active_record/table_spec.rb:60 # Mobility::Backends::ActiveRecord::Table with cache resets model translations cache when model is saved or reloaded
rspec ./spec/mobility/backends/active_record/table_spec.rb:183 # Mobility::Backends::ActiveRecord::Table Backend methods #read builds translation if no translation exists

Randomized with seed 6552

[shioyama]

Those specs are not that important, they're basically testing implementation details.

The bigger problem is that this change kills the cache:

mobility/lib/mobility/backends/table.rb

Lines 134 to 141 in 7683433

	def translation_for(locale, **options)
	return super(locale, options) if options.delete(:cache) == false
	if cache.has_key?(locale)
	cache[locale]
	else
	cache[locale] = super(locale, **options)
	end
	end

The cache patches translation_for, but if that method is gone then in_locale will actually go to the joined translations on every read, instead of the cache.

There should be a way to improve that, but it would require a bit more work & thought.

[mrbrdo]

@shioyama yeah I mentioned those specs are testing implementation details.

Do you have a better suggestion to fix #572 then? This definitely did fix it. The issue that it creates empty translations causes other problems later, so it's something that should be fixed.

[shioyama]

The problem is the way caching is done, which depends on building translations and then caching those built translations.

And actually, the tests are mostly testing implementation details but not entirely.

  2) Mobility::Backends::ActiveRecord::Table with cache only fetches translation once per locale
     Failure/Error: find { |t| t.locale == locale }
     
       (#<ActiveRecord::Associations::CollectionProxy [#<Article::Translation id: nil, locale: "en", article_id: nil, title: "bar", subtitle: nil, content: nil, created_at: nil, updated_at: nil>]>).find(no args)
           expected: 2 times with any arguments
           received: 3 times
     # ./lib/mobility/backends/active_record/table.rb:302:in `in_locale'
     # ./lib/mobility/backends/active_record/table.rb:292:in `translation_for'
     # ./lib/mobility/backends/table.rb:140:in `translation_for'
     # ./lib/mobility/backends/table.rb:94:in `write'
     # ./spec/mobility/backends/active_record/table_spec.rb:55:in `block (4 levels) in <top (required)>'
     # ./spec/mobility/backends/active_record/table_spec.rb:50:in `block (3 levels) in <top (required)>'

This is a real issue, because the cache is basically not working. In this PR translation_for is no longer called on reads so the cache doesn't work.

I don't have a good solution unfortunately.

The problem is that "caching" as a concept is tricky across backends. I've never really been completely happy with the current implementation but it's definitely not as simple as just swapping out a couple lines. Probably a complete rethink would be necessary to really fix this issue.

[mrbrdo]

I agree. But there is a case to be made that correct functioning is more important than caching.
Not suggesting to merge as is, but as it stands I have to use this fix because otherwise I get a lot of issues due to those empty translations being created in DB. Like fallbacks issues and getting null values for stuff that never had null values and are expected to be_present in the app.

I think this issue is specific to table-based backends and pretty sure it could be solved in that context only while keeping caching working. Crucially the built empty translations should not be saved upon saving the parent, in some implementational way or another. This could even be an ActiveRecord-specific issue, as I don't remember if Sequel automatically saves associated records like that.

I think you will agree that even on a conceptual level, reading an attribute should never really append records to the DB (even though if this is done as a hook after the fact). In that way the AR table implementation is faulty. Potentially even race conditions exist where this would create several Translation records in the DB for the same model, and that would really be hell.

[shioyama]

I think this issue is specific to table-based backends and pretty sure it could be solved in that context only while keeping caching working.

I agree that it can, but I don't believe it's trivial to do within the confines of what is currently expected of the cache, if we want to keep the code relativley simple.

A solution would be to have some wrapper for the built translations which can handle the nil case without actually building an empty translation. Like you I would think that's not too hard but I tried improving the table cache a few times in the past and always ended up with a mess 😅

I think you will agree that even on a conceptual level, reading an attribute should never really append records to the DB (even though if this is done as a hook after the fact).

I agree. I would only note that nobody has raised this as an issue so far in the 5 years since Mobility has been released. Which, honestly, is a bit surprising 🤔 but that's the reason this hasn't been looked into further.

Anyway Globalize might be a reference point:

https://github.com/globalize/globalize/blob/484ed7ffec8931bf694004fa6486ed5c2a4a12f8/lib/globalize/active_record/instance_methods.rb#L129-L133

And build_if_missing is false when it is called from fetch_attribute:

https://github.com/globalize/globalize/blob/484ed7ffec8931bf694004fa6486ed5c2a4a12f8/lib/globalize/active_record/adapter.rb#L80-L87

Again though Globalize is not build the same way as Mobility.