[WIP] update MiqReport::Search to include all columns #18176
Conversation
kbrock
force-pushed
the
rails-5-1-column-select
branch
from
301a56b to
7720d94
Compare
kbrock
force-pushed
the
rails-5-1-column-select
branch
2 times, most recently
from
0f86947 to
cf4c0e1
Compare
|
@kbrock I think I follow the change you make here but it's still unclear what exactly changed in rails 5.1 to break us. How did the foreign_key get included in the select in 5.0 and not 5.1? |
kbrock
changed the title
kbrock
force-pushed
the
rails-5-1-column-select
branch
from
cf4c0e1 to
799ef64
Compare
|
WIP status - I'm looking through rails searching for the actual issue This code is working and I like this change. |
When generating a query for a report, ensure that the columns necessary to join to other models are brought back. If a report needs additional columns for join models and the models are not part of the includes_for_find attribute, then ensure all columns including the join keys are in cols: attribute, and just the visible columns are in the col_order attribute
kbrock
force-pushed
the
rails-5-1-column-select
branch
from
799ef64 to
f9cacf8
Compare
|
Checked commit kbrock@f9cacf8 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 |
|
@kbrock where do we stand on this PR? I'm still using this commit on my rails 5.1 branch and I'm not sure if it's going away or will be replaced. |
There was a problem hiding this comment.
First off, sorry for the delayed review on this. Saw it come back in my inbox with LJ's comment and realized I never gave a proper review.
I think what you have here works for the bug that has been observed, but without knowing the true cause, I don't think it is wise to merge it until we have a better idea of what we are dealing with.
| @@ -92,7 +92,19 @@ def paged_view_search(options = {}) | |||
|
|
|||
| search_options = options.merge(:class => db, :conditions => conditions, :include_for_find => includes) | |||
| search_options.merge!(:limit => limit, :offset => offset, :order => order) if order | |||
| search_options[:extra_cols] = va_sql_cols if va_sql_cols.present? | |||
|
|
|||
| extra_cols = if includes | |||
There was a problem hiding this comment.
Kinda seems like this should be put in it's own method. va_sql_cols is just a memoized method as well, so you could toss it there, but I could see the argument that this should be it's own method and probably makes sense that it is.
There was a problem hiding this comment.
I am also confused why this is needed, which leads back to LJ's comment here:
The :extra_cols attribute on Rbac (**cough** **cough** nick is the original author **cough** **cough**) was meant to add additional columns and not remove ones that were already there (even if the default Arel.star was originally just assumed). So something seems borked in Rails if we need to start adding the foreign keys ourselves, or we broke something in manageiq along the way.
Is there possibly an additional .select in this report that is causing the issue? I also have some changes in #18353 that are affecting the results for :extra_cols that might be beneficial or detrimental to what you have here as well.
| extra_cols = if includes | ||
| includes.keys.map do |rel| | ||
| if (rel = db_class.reflect_on_association(rel)) | ||
| # adding primary_key is probably overkill. |
There was a problem hiding this comment.
You should always have the primary key just in case an association is needed on the model down the line, or you are immediately converting the results to a lower level data model. I think in the case of reports, we aren't doing that, so the use of relations might be necessary.
I would say remove this comment, just so there is no confusion.
|
@jrafanie yes, I am trying to figure out the issue here |
|
This pull request is not mergeable. Please rebase and repush. |
|
not going this route. modifying the way Core rails also fails when you |
Ok, backdrop: report.yml file.
Columns for the screen? Use
col_order.Associated models? Use
includes_for_find.The
foreign_keyneeded to bring back those associated models?That is tricky, I guess you could declare all the
col_orderagain in thecolsattribute.rails 5.0
We were bringing back data for all models in a single query (sometimes to our detriment).
We did not need the extra associated models'
foreign_keyids because we had all the associated models' data to create the associated AR records.rails 5.1, Before PR:
We were bring back associated models in additional queries, but the
foreign_keynecessary to bring them back was not present. This caused and exception / 500 error.rails 5.1, after PR:
We are now bringing back more columns, specifically the
foreign_key.The associated models able to be queried.
Concerns
It appears the rules as to what models are brought back by the primary query changed again. It used to require
includes(:model2), then it required 'includes(:model2).references(:model2)` in rails 4.x.Now there appears to be another rule.
There was a bug in 5.0 where has_many associations would be part of the primary query. Which brought back many many rows. So this could be the driver of the 5.1 change. But that is speculation
But what ever the reason, these associated
foreign_keyids should be brought back, for all versions of rails.