Allow order fields to be subqueries with order

[kbrock]

Fixes: ManageIQ/manageiq-ui-classic#5374

Overview

Virtual fields that reference a has_many relationship, often need to have a LIMIT and ORDER in the field definition.

When rails works with these fields in an ORDER BY clause, they sometimes add that field to the SELECT clause for Sql reasons. In that process, they munge the field and break things.

Before

We see this issue on the Vm explorer page. If we sort by compliance (i.e. last_compliance_status) it blows up:

VmOrTemplate.includes(:hardware => :disks).references('x').order(:last_compliance_status).first

ActiveRecord::StatementInvalid (PG::InvalidColumnReference: ERROR:  for SELECT DISTINCT, ORDER BY expressions must appear in select list)
LINE 1: ..."disks"."hardware_id" = "hardwares"."id" ORDER BY (SELECT  "...

After

I put in a Rails PR that avoids string munging and leverages Arel. In the mean time, this PR is a monkey patch to our codebase with the same code that fixes the problem in the short term.

[kbrock]

RE Rubocops: - I copy and pasted these bits from the rails code.
I am aiming to keep it as close to rails style and concepts as possible.

[Fryguy]

cc @jrafanie

[jrafanie]

@kbrock Can you make the first commit the current code in rails (include which rails version you're pulling from... I have no idea if it's changed in 5.1 -> 5.2 -> 6), and then apply your patch in the second commit... It should make this easier to dissect. I'm pretty lost.

[kbrock]

@jrafanie ok, updated. is this easier to figure out?

[jrafanie]

This comment should be updated to reflect the conditional you're doing here. We should explain that there's an existing ORDER clause so we want to retain any ASC/DESC.

[miq-bot]

Checked commits kbrock/manageiq@18f71ad~...e237d0a with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 5 offenses detected

lib/extensions/ar_order.rb

• ❗ - Line 10, Col 24 - Style/ClassCheck - Prefer Object#kind_of? over Object#is_a?.
• ❗ - Line 20, Col 31 - Style/MethodCallWithArgsParentheses - Use parentheses for method calls with arguments.
• ❗ - Line 32, Col 13 - Layout/BlockAlignment - } at 32, 12 is not aligned with orders.reject(&:blank?).map { |s| at 8, 26 or order_columns = orders.reject(&:blank?).map { |s| at 8, 10.
• ❗ - Line 32, Col 13 - Style/MultilineBlockChain - Avoid multi-line chains of blocks.
• ❗ - Line 8, Col 55 - Style/BlockDelimiters - Avoid using {...} for multi-line blocks.

[jrafanie]

Note, ignoring cops because it's rails code... we're trying to avoid making unrelated changes to the core rails code.

[jrafanie]

@kbrock did you test the UI suite with this PR?

[jrafanie]

LGTM, I'll merge when we get confirmation that UI classic test suite works with this change.

[kbrock]

@jrafanie ui classic passed

[simaishi]

Hammer backport details:

$ git log -1
commit 004033cb16b3dbbe0ed8fd7ad45936ae88d35ff2
Author: Joe Rafaniello <[email protected]>
Date:   Thu Jul 11 17:14:32 2019 -0400

    Merge pull request #18909 from kbrock/fix_order
    
    Allow order fields to be subqueries with order
    
    (cherry picked from commit 5d6eb830cf0faded6d54c5661cdee6834f16259f)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1738266

[kbrock]

Other note - it also addresses https://bugzilla.redhat.com/show_bug.cgi?id=1745660

[jrafanie]

@miq-bot add_label rails5.2