Fog::Collection doesn't fetch while converted by Kernel.Array method

[oakcask]

Hi.

I found out that Fog::Collection donsn't do lazy_load while being converted by Kernel.Array method that implicitly called by several Array methods.

For example, Array#concat converts its operand by Array() and Fog::Collection instance passed to Array#concat won't do lazy_load so an empty array will be concatinated.
This causes making flat_map (this internally uses Array#concat to gather the results) over Fog::Collections generate empty results.

It works well by manually calling all to load explicitly.

This is intentional? I think this behavior is bit confusable IMHO.

require "bundler/inline"

gemfile do
  source "https://rubygems.org"
  gem "fog-core"
  gem "minitest"
  gem "minitest-reporters"
end

require "securerandom"
require "minitest/unit"
require "minitest/autorun"

ENV["MINITEST_REPORTER"] ||= "SpecReporter"
Minitest::Reporters.use!

def simulate_fetch(n)
  n.times.map do
    { 'id' => SecureRandom.uuid, 'value' => Time.now.iso8601 }
  end
end

class Foo < Fog::Model
  identity :id
  attribute :value
end

class FooCollection < Fog::Collection
  model Foo

  def initialize(fetch_size)
    super()
    @fetch_size = fetch_size 
  end

  # Roughly same implementation with
  # https://github.com/fog/fog-aws/blob/e0d9ad4a1a78f46634c51e80583281b389c5212e/lib/fog/aws/models/storage/versions.rb#L12-L20
  # 
  # `lazy_load` will call `all`
  # https://github.com/fog/fog-core/blob/e359e66ddd81e7b0811bc0ff00b133722fb49ef8/lib/fog/core/collection.rb#L111
  def all(options = {})
    data = simulate_fetch @fetch_size
    load data
  end

  def new(attr = {})
    $count = $count.to_i + 1
    super
  end
end

class Test < MiniTest::Unit::TestCase
  def test_fetch
    a = FooCollection.new 1
    b = FooCollection.new 2
    assert_equal 1, a.size
    assert_equal 2, b.size
  end

  def test_concat
    a = FooCollection.new 1
    b = FooCollection.new 2

    result = a.concat b

    assert_equal 3, result.size
  end

  def test_flat_map
    a = FooCollection.new 1
    b = FooCollection.new 2

    result = [a, b].flat_map { |item| item }

    assert_equal 3, result.size
  end

  def test_flat_map_2
    a = FooCollection.new 1
    b = FooCollection.new 2

    result = [a, b].each(&:all).flat_map { |item| item }

    assert_equal 3, result.size
  end

  def setup
    puts ">> $count = #{$count}"
  end

  def teardown
    puts "<< $count = #{$count}"
  end
end

__END__
$ ruby test.rb 
MiniTest::Unit::TestCase is now Minitest::Test. From test.rb:52:in `<main>'
Started with run options --seed 36760

Test
>> $count = 
<< $count = 3
  test_fetch                                                      PASS (0.00s)
>> $count = 3
<< $count = 3
  test_flat_map                                                   FAIL (0.00s)
        Expected: 3
          Actual: 0
        test.rb:75:in `test_flat_map'

>> $count = 3
<< $count = 6
  test_flat_map_2                                                 PASS (0.00s)
>> $count = 6
<< $count = 7
  test_concat                                                     FAIL (0.00s)
        Expected: 3
          Actual: 1
        test.rb:66:in `test_concat'


Finished in 0.00171s
4 tests, 5 assertions, 2 failures, 0 errors, 0 skips

[geemus]

Thanks for the detailed report.

No, I do not think that is intended/expected. You can see here how we attempted at least to replace default Array behaviors with the lazy_load followed by doing the normal array behavior (which seems to work for many, but as you point out not all of the methods):

fog-core/lib/fog/core/collection.rb

Lines 12 to 34 in e359e66

	Array.public_instance_methods(false).each do |method|
	next if [:reject, :select, :slice, :clear, :inspect].include?(method.to_sym)
	class_eval <<-EOS, __FILE__, __LINE__
	def #{method}(*args)
	unless @loaded
	lazy_load
	end
	super
	end
	EOS
	end
	%w(reject select slice).each do |method|
	class_eval <<-EOS, __FILE__, __LINE__
	def #{method}(*args)
	unless @loaded
	lazy_load
	end
	data = super
	self.clone.clear.concat(data)
	end
	EOS
	end

It's also possible that this may have worked in earlier versions of ruby and implementation differences have introduced this. Just speculating here, but it seems surprising to me that we wouldn't have stumbled upon this earlier. Not that it matters too much one way or the other.

I'd certainly welcome ideas on how we might fix this discrepancy if you have thoughts.

[oakcask]

Thanks geemus.

Frankly, I think the proper solution is making Fog::Collection an Enumerable, not inheriting Array.
Because it seems Array() doesn't convert an Array (or subclass of Array) by invoking to_ary.

Most Array methods are provided by Enumerable so this will work well in most cases I suppose.
On the other hand, it might cause some breaking changes due to difference from Array.

How do you think?

[geemus]

It's an interesting idea for sure. I suspect that might in fact be a cleaner solution, but I am definitely worried about breaking changes. Given how much usage there already is for this (and how few reports there seem to have been of this problem) it might be more trouble than it is worth, even if this solution might be better. Does that make sense? What do you think?

[github-actions]

This issue has been marked inactive and will be closed if no further activity occurs.