BZ 1238179 - VM Utilization screen generating charts throws internal server error after Rails 4 #3394
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| @@ -445,7 +445,7 @@ def self.search(options = {}) | |||
| def self.method_with_scope(ar_scope, options) | |||
| if ar_scope == VmdbDatabaseConnection | |||
| ar_scope.all | |||
| elsif ar_scope < ActsAsArModel | |||
| elsif ar_scope < ActsAsArModel || ar_scope.respond_to?(:instances_are_derived?) && ar_scope.instances_are_derived? | |||
There was a problem hiding this comment.
I would add parentheses to make the complex condition clearer:
elsif ar_scope < ActsAsArModel || (ar_scope.respond_to?(:instances_are_derived?) && ar_scope.instances_are_derived?)
There was a problem hiding this comment.
Good suggestion. I just made the change.
…> true) the same as classes that derive from ActsAsArModel and call custom find method in brace search. Also added a test that exposes an ActiveRecord bug where a query like "Tagging.includes(:taggable => {}).where('bogus_table.column = 1').references(:bogus_table => {}).to_a" raises an error. The AR error is preventing this bug from being fixed.
https://bugzilla.redhat.com/show_bug.cgi?id=1238179
…ng a query where a polymorphic reflection is specified in "includes" and ""references" are used in the query. It will check all of the included associations to see if any are polymorphic and if so avoid calling "references" on the scope. This change will still eager load any polymorphic associations that are included in the query. https://bugzilla.redhat.com/show_bug.cgi?id=1238179
gtanzillo
force-pushed
the
rails-4-error-generating-vm-utilization-screen
branch
from
9304fd5
to
be5eec6
Compare
…cludes with a polymorphic association and references pending until issue is addressed in ActiveRecord. https://bugzilla.redhat.com/show_bug.cgi?id=1238179
|
Checked commits gtanzillo@8615ffe .. gtanzillo@432d7c2 with rubocop 0.32.0 |
gtanzillo
changed the title
|
@matthewd Please review when you have some time. Thx. |
|
I'm able to verify that with the above changes in place, the chart screens now work |
matthewd
added a commit
that referenced
this pull request
Jul 21, 2015
…tilization-screen BZ 1238179 - VM Utilization screen generating charts throws internal server error after Rails 4
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Treat classes that have "derived" instances (instances_are_derived? => true) the same as classes that derive from ActsAsArModel and call custom find method in brace search. Also added a test that exposes an ActiveRecord bug where a query like "Tagging.includes(:taggable => {}).where('bogus_table.column = 1').references(:bogus_table => {}).to_a" raises an error. The AR error is preventing this bug from being fixed. See (https://github.com/rails/rails/compare/master...gtanzillo:includes_references_with_polymorphic?w=1)
https://bugzilla.redhat.com/show_bug.cgi?id=1238179