Fix fetch has_many embedded association after adding to it

[dylanahsmith]

cc @mgingras

Problem

The problem is shown well from the added regression test on a cache_has_many embed: :ids association

    item.associated_records.create!(name: 'foo')
    fetched_associated_records = item.fetch_associated_records
    assert_equal(item.associated_records.length, fetched_associated_records.length)

which results in the failure

  1) Failure:
NormalizedHasManyTest#test_fetch_association_after_adding_to_it [test/normalized_has_many_test.rb:252]:
Expected: 3
  Actual: 2

without the included fix to the code under test.

Solution

I looked at how Active Record distinguishes between loaded associations and unloaded associations with records added to it and found it checks for this with elsif !target.empty?. I decided to use a similar check, although used blank? so that we don't assume that target will continue to be initialized to [] for an unloaded association.

[mgingras]

This test seems to fail for Item.cache_has_many(:associated_records, embed: true), is that expected?

[dylanahsmith]

Good catch!

I was thinking it was only an issue with id embedding, but I think that is just because the symptoms of the problem were different for id embedding.

I've pushed a commit fixing this for cache_has_many embed: true

[dylanahsmith]

This test was failing because setup_has_many_children_and_grandchildren was creating records on an association without loading it, which messes with preloading checks. So calling prefetch_associations on records with a modified unloaded association like this may not be ideal, although I think a bigger refactor is needed to handle that complexity well. For instance, our check to see if an association has already been loaded/fetched assumes that it can just check the first record, but I don't think we want to make a similar assumption for associations being modified which are less likely to be homogeneous. I would also like to avoid adding too much overhead for the includes fetch option, which can make more assumptions of the state of the records since they aren't coming from application code.

[dylanahsmith]

Opened #453 to keep track of this issue.