Add ::always_load to resources for eager loading.#847
Add ::always_load to resources for eager loading.#847mrloop wants to merge 7 commits intoJSONAPI-Resources:masterfrom
Conversation
|
Test suite passing for 5.0 and 4.2 but is failing on rails 4.1. 4.1 is no longer officially supported and isn't supported by railslts. No unpatched secuirty vunrebilities reported yet but it's just a matter on time. Will 4.1 support be dropped or do I need to make this PR 4.1 compatible to be considered for merging? |
See JSONAPI-Resources#485 for background.
if no resource for specified always_include presume it is an existing model with no associated resource and use the symbol.
only use related type includes when loading related resource to reduce the number of unused includes.
The previous recursive strategy for constructing the Active Record
includes argument could result in unused eager loads being performed.
The new strategy normalizes and merges the model includes and always
includes, then iterates over the model includes and normalizes and
merges their always includes. The model and always includes can both
be nested.
For example the recursive strategy gave the following includes for the
app I'm working on:
[{:raw_show=>[{:events=>[:raw_show, {:run=>[{:show=>[:events,
:photos]}]}, :master_allocations, {:venue_layout=>[:venue]}]},
:photos]}, :bookings, :run, :master_allocations, :venue_layout]
While the merge strategy results in:
[:run, :master_allocations, :venue_layout, :bookings,
{:raw_show=>[:events, :photos]}]
d9d69a9 to
e128840
Compare
|
@mrloop This seems to be in violation of the spec. From http://jsonapi.org/format/#fetching-includes:
Implies it's Ok, but it breaks a subsequent section:
I think we need to handle this as a |
|
Maybe the naming is a bit off and should be |
|
@mrloop I see. I was getting hung up on the name an assuming the feature would extend to including the resources, not just the eager loading. I should have looked at the code and I would have seen how it was used - sorry. A name change would definitely be appreciated, especially since I hope to add the |
Rename to better express the intent from `always_include` to `always_load` as in `preload` or `eager_load`
|
Have renamed |
|
@lgebhardt that's the renaming done and the documentation added. |
|
Wouldn't this make more sense as |
|
@NuckChorris Something like this would be useful at the resource layer when you are exposing a property that would otherwise lazy-load a related model. I wouldn't want it to always load this data directly on the model since it's use-case is only for serialization at the resource. |
|
Edit, I don't think the above is correct, going to spike something to get a better idea of what's needed. |
|
@mrloop what is the status of this PR? Did it work for you? We are being bitten by N+c queries loading records that need relations in their computed attributes. |
|
@Genkilabs it's stalled, using similar version on code in small production app at the moment, working on another larger app at the moment using it as well, hope to be able to iron out any issues when that's deployed and in use, looks ok in testing. |
Enables resources to be defined with
always_includesfor eager loading. e.g.Normalize and merge the model includes and always includes, iterate over the model includes and merge their always includes. The model and always includes can both be nested.
See #485