Feature/remove eager loading #238
Conversation
| @@ -65,6 +65,7 @@ def database_records | |||
| @model_class.where(nil).merge(override_scope) | |||
| elsif @model_class.respond_to?(:where) && @model_class.respond_to?(:joins) | |||
| if mergeable_conditions? | |||
| puts conditions | |||
There was a problem hiding this comment.
You'll want to remove the outputs in this file :)
|
Could we write some more tests to verify the correct sql is being generated? This is a more complex PR and may have some edge cases I may not be aware of. Going to pull it down and review it more thoroughly. |
|
Would you prefer to reduce the amount of code and depend directly from this gem? https://github.com/elabs/ar_outer_joins Is an old gem with just 15 stars so I preferred to include the code directly but we can eventually remove it and add the dependency. what do you think? |
|
Configurable includes In some scenarios I'd like to be able to specify per model what gets included to reduce further sql requests I know I'm going to make when looking at a resources related models. |
|
Ah look somebody has already suggested this #228 |
|
You can specify what to include already with |
|
@coorasse but I'm loading the resource via |
|
Ok, your just getting a scope from load_and_authorize_resource, thanks @coorasse |
|
Hello @Senjai. I think this PR brings an important improvement and, at the same time, change in cancancan. I already use the following changes in different projects and I had no problems, even in complex ones (https://github.com/coorasse/Airesis/tree/master/app/cancan). I really think using |
|
+1 |
|
@coorasse https://github.com/nerde/left_join seems a little newer. Do you want to use that instead? |
|
@coorasse Want to get on a hangout at some point for this one? |
…o feature/remove_eager_loading # Conflicts: # lib/cancan/model_adapters/active_record_3_adapter.rb
|
+1 |
|
Once the following PR is merged into Rails, it should be possible to just do a |
|
Any news on this feature ? |
|
I think this will fall in the next Major version (3.0) where I plan to deprecate also AR4. Therefore this PR will be simplified by a lot and rely on left_outer_join by AR5 |
| require "cancan/model_adapters/ar_outer_joins/join" | ||
|
|
||
| module ArOuterJoins | ||
| def outer_joins(*joins) |
There was a problem hiding this comment.
In Rails 5 we got .left_joins. The duplicate table in this regard was fixed in rails/rails#30410.
Maybe rename this to left_joins and only use if it Rails version is below Rails 5.2?
|
What's the status of v3.0? |
|
Closed in favor of #508 |
Using code from @elabs and continuing on what I wrote here: #171 (comment) that is my suggestion.
Note that I fixed it only for AR4