Column aliases

[kbrock]

Context

virtual attributes:

In ActiveRecord, build_select (via arel_columns) converts select_values into sql. But it does not properly detect a virtual attribute as a field. So it doesn't properly convert the field to arel (arel_attribute) and it generated invalid sql.

We monkey patched select() to detect a virtual attribute field, convert it to arel, and put the arel into select_values. It now generates valid sql.

ManageIQ/manageiq custom attributes:

The custom attributes mixin defines fields as virtual attributes. Unfortunately these attribute names are not always sql friendly. Since virtual attributes does not properly escape the field's sql aliases in the select clause and it generated invalid sql. #BUG1

The custom attribute mixin added a custom alias to the arel so it generated valid sql. YAY. But BUG1 still kinda exists.

Before

Our monkey patched select() skipped arel with an alias, so it put the custom attribute name directly into select_values. This generated invalid sql. #BUG2

In many cases, build_select called arel_attribute anyway, so BUG2 was hidden and all worked.

Well, it worked until ManageIQ/manageiq#18822 introduced a custom from clause. build_select no longer called arel_attribute for the custom attribute and it started generating invalid sql again.

Also of note, ActiveRecord developers change the implementation of build_select a lot. Which makes sense since it never quite meets our needs either. But this also tells you that it would be dangerous to monkey patch it even though it feels like this is the right place to fix this problem.

After

1. Fix select() to escape the attribute name. This removes the need for custom attributes to add the alias to the arel in the first place. #BUG1
2. Fix select() to properly put arel with an alias into the select_values. #BUG2
3. refactor select() to be code climate friendly.

NOTE

Sql aliases do work in a SELECT clause but not in a WHERE or ORDER BY clause.
So if the attribute's arel contains an alias, it will work in a SELECT but not in a WHERE or ORDER BY statement.

To be honest, we should probably remove the custom alias for the custom attributes when defining the virtual attributes - but this PR is needed for that case as well.

We may want to look into modifying build_select and friends arel_columns.
We may want to look into the attribute aliasing mechanism used by includes().references().

[kbrock]

re code climate: that method has not been touched. ignoring

hmm. I read the code climate wrong. it was the select that they complained about. I fixed this problem.
And I added the #28 to remove the other code climate complaint (so there are now none in this file)

[miq-bot]

Checked commits kbrock/activerecord-virtual_attributes@33e966c~...8a34d40 with ruby 2.3.3, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🍰

[jrafanie]

@NickLaMuro can you 👀 the second commit... I'm not sure ...

@kbrock what are we giving up by it not working for order and where but will work with select? Does this break things, regress? I guess I'm not sure if this breaks things or just doesn't support aliases completely.

[kbrock]

@kbrock what are we giving up by it not working for order and where but will work with select? Does this break things, regress? I guess I'm not sure if this breaks things or just doesn't support aliases completely.

@jrafanie Yes, we really should not be using an alias there. You are right. But...

We currently use an alias in miq's custom attributes. So I'm putting in the specs to test the actual code (and failure) that we have in miq.

This PR quotes the column name, to fix the reason we had the alias in the first place. It also has a fix to allow aliases to work.

In the future, I would like to remove the aliases from custom attributes. But that has been in our code base for a while, and I'm trying not to scope creep this too much - postponing that change for later

[NickLaMuro]

Nick's 2cents™

High level, I gave this response Keenan in a DM about this a few days ago:

the point wasn't you shouldn't fix those issues, but instead that possibly you shouldn't fix those #rightNow™

And also linked to this image...

---

But if I recall correctly, this was specifically being targeted in response to issue when trying to enable inline_view? for all reports in our project:

https://github.com/kbrock/manageiq/blob/0e5652b26577d65dca2579543a2adee7b144798b/lib/rbac/filterer.rb#L276

Which I think is something easier solved by fixing for US (MIQ) first, and deciding what to do here the #rightWay™ when it is understood better. Specifically:

"I'm not a big fan of this approach as it will break use of this column for where and order clauses, but will still work in select clauses" (from second commit message)

Can this then not be fixed here and instead in MIQ for our specific use case? (see link above). If you yourself don't like this approach, I am having a hard time even starting to review this further.

I think the first commit, however, is completely fine and I think makes sense to be merged in on it's own since I don't see it hurting anything.

[kbrock]

"I'm not a big fan of this approach as it will break use of this column for where and order clauses, but will still work in select clauses" (from second commit message)
-- @kbrock

to clarify: I'm not a big fan of how we coded the alias in MIQ in the first place. (It was necessary at the time to avoid a bug. This bug was fixed in the first commit.

Can this then not be fixed here and instead in MIQ for our specific use case? (see link above). If you yourself don't like this approach, I am having a hard time even starting to review this further.
-- @NickLaMuro

We have 2 options:

1. Keep the second commit and leave the code in MIQ unchanged.
2. Drop the second commit and remove the alias from the MIQ custom attributes.

Looks like we both are leaning towards option 2?

[kbrock]

I want to keep this PR for prosperity case, oped #30 with just the first commit.

Thanks