New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changed partial rendering to allow collections which don't implement `#to_ary`. #25912

Merged
merged 4 commits into from Jul 26, 2016

Conversation

Projects
None yet
8 participants
@stevenharman
Contributor

stevenharman commented Jul 21, 2016

An optimization was introduced in 27f4ffd which will call #to_ary on the collection to prevent unnecessary queries for ActiveRecord scopes/relations. If the given collection did not respond to #to_ary, an empty collection was returned. That meant you couldn't use collections built from Enumerator nor Enumerable.

With this change, #collection_from_options will attempt the optimization, but fall back to passing along the given collection, as-is. This is an alternative to #22005 which will raise an exception if you attempt to use something like an Enumerator or Enumerable instance.

@rails-bot

This comment has been minimized.

rails-bot commented Jul 21, 2016

r? @schneems

(@rails-bot has picked a reviewer for you, use r? to override)

@rafaelfranca

View changes

actionview/test/template/render_test.rb Outdated
y.yield(Customer.new("david"))
y.yield(Customer.new("mary"))
end
assert_equal "Hello: davidHello: mary", @view.render(:partial => "test/customer", collection: customers)

This comment has been minimized.

@rafaelfranca

rafaelfranca Jul 21, 2016

Member

Ruby 1.9 hash syntax here.

This comment has been minimized.

@stevenharman

stevenharman Jul 21, 2016

Contributor

Oops! That's a hard one to break. I'll push a new commit momentarily.

@rafaelfranca

View changes

actionview/test/template/render_test.rb Outdated
y.yield(Customer.new("david"))
y.yield(Customer.new("mary"))
end
assert_equal "Hello: davidHello: mary", @view.render(:partial => "test/customer", :collection => customers)

This comment has been minimized.

@rafaelfranca

rafaelfranca Jul 21, 2016

Member

oh, actually we prefer the Ruby 1.9 version. Sorry for not being clear.

This comment has been minimized.

@stevenharman

stevenharman Jul 21, 2016

Contributor

Ah, very good. I knew >= 1.9 was the preferred syntax generally, but thought you were asking to follow the style used in the rest of the file.

Just pushed a commit to change over to key: :value.

@stevenharman stevenharman force-pushed the stevenharman:fix_render_partial_collection_to_allow_custom_collection branch Jul 21, 2016

stevenharman referenced this pull request Jul 25, 2016

Make collection and collection_from_object methods return an array
This transforms for instance scoped objects into arrays and avoid unneeded queries

[#5958 state:committed]
@rafaelfranca

This comment has been minimized.

Member

rafaelfranca commented Jul 25, 2016

Action view tests seems to be broken.

@stevenharman stevenharman force-pushed the stevenharman:fix_render_partial_collection_to_allow_custom_collection branch Jul 25, 2016

stevenharman added some commits Jul 21, 2016

Fix collection_from_options to allow Enumerators
An optimization was introduced in
27f4ffd
which tried to `#to_ary` the collection to prevent unnecessary queries
for ActiveRecord scopes/relations. If the given collection did not
respond to `#to_ary`, and empty collection was returned. That meant you
couldn't use collections built from `Enumerator` nor `Enumerable`.

With this change, `#collection_from_options` will attempt the
optimization, but fall back to passing along the given collection,
as-is.
Default to an empty collection if falsey given
This will ensure we attempt to render an empty collection, meaning we
don't actually render anything at all. Allowing `nil` or a falsey value
through results in calling `render_partial` rather than
`render_collection`, which isn't what we want.

@stevenharman stevenharman force-pushed the stevenharman:fix_render_partial_collection_to_allow_custom_collection branch to e4a4936 Jul 26, 2016

@stevenharman

This comment has been minimized.

Contributor

stevenharman commented Jul 26, 2016

I've fixed the broken ActiveView test. I'm happy to squash this all into a single commit if y'all are happy with it.

@spastorino

This comment has been minimized.

Member

spastorino commented Jul 26, 2016

Let's also change to_ary to to_a since we are explicitly converting it to an Array.

@spastorino

This comment has been minimized.

Member

spastorino commented Jul 26, 2016

And there's no need to check respond_to, this should be enough ...

collection = @options[:collection]
collection ? collection.to_a : []
@stevenharman

This comment has been minimized.

Contributor

stevenharman commented Jul 26, 2016

I wonder, will that have the desired consequence? More things implement to_a than to_ary. Enumerator, for example doesn't implement to_ary. Nor does Enumerator::Lazy, and the to_a would force evaluating the entire thing, no? Maybe that's OK because presumably you'd not be passing some sort of infinite collection here.

My understanding of the original optimization was to cause an ActiveRecord scope to evaluate and then prevent extra DB calls. In that case, the to_ary is probably well-suited because Array-like things should implement it. And less Array-like things (Enumerators, for example) won't.

@matthewd

This comment has been minimized.

Member

matthewd commented Jul 26, 2016

Yeah, the original intention was basically to pre-buffer the each, for the benefit of AR scopes. to_a will still do that, and will also work for anything else enumerable, which is what we want.

Otherwise we're likely to run into an issue with our need for size anyway.

As you note, we know it's not infinite, because we know we're about to call each, and collect the results until it terminates of its own accord. So this feels like the best balance of API to impose, IMO.

Use to_a to pre-buffer the collection
We can safely assume we're not dealing with an infinite collection as
we're about to call `each` on it and collect the results until it
terminates on its own. Given that, `to_a` is implemented by the normal
Array-like objects, and less Array-like objects like `Enumerator` and
`Enumerator::Lazy`.

@stevenharman stevenharman force-pushed the stevenharman:fix_render_partial_collection_to_allow_custom_collection branch to 87899cf Jul 26, 2016

@spastorino spastorino merged commit 49315e2 into rails:master Jul 26, 2016

2 checks passed

codeclimate Code Climate didn't find any new or fixed issues.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@stevenharman

This comment has been minimized.

Contributor

stevenharman commented Jul 26, 2016

Many thanks for the feedback and help, @spastorino and @rafaelfranca. If I might ask one more thing... would you be open to me back porting this to 5-0-stable and 4-2-stable? It is a bug fix, albeit is minor one.

spastorino added a commit that referenced this pull request Jul 26, 2016

Merge pull request #25912 from stevenharman/fix_render_partial_collec…
…tion_to_allow_custom_collection

Changed partial rendering to allow collections which don't implement `#to_ary`.
@spastorino

This comment has been minimized.

Member

spastorino commented Jul 26, 2016

I've backported to 5-0-stable, I don't think it worth backporting to 4-2-stable.

@stevenharman

This comment has been minimized.

Contributor

stevenharman commented Jul 26, 2016

I was mostly interested in 4-2-stable b/c we're still running 4.2.x, and that's where I discovered this issue. 😄

glebm added a commit to thredded/thredded that referenced this pull request Dec 26, 2016

Add `#to_a` to view models for Rails 5.0.1 compat
The Rails PR that introduced the backwards-incompatible change that
prompted this:
rails/rails#25912
rails/rails@87899cf

The Rails changelog entry is under 5.0.1.rc1:
https://github.com/rails/rails/blob/v5.0.1/actionview/CHANGELOG.md#rails-501rc1-december-01-2016

See also #508
@glebm

This comment has been minimized.

Contributor

glebm commented Dec 26, 2016

This breaks backwards compatibility for collections that implement to_ary but not to_a.

@matthewd

This comment has been minimized.

Member

matthewd commented Dec 26, 2016

@glebm Yeah, I think I'm okay with that. I'm not sure to_ary-without-to_a satisfies the expected contract of to_a/to_ary... and our use of to_ary wasn't documented.

Either one of those alone might be insufficient to justify a change on a stable branch, but combined, I don't think it's worth trying both methods.

Happy to revisit if this turns out to be a wider problem, though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment