add all features as a virtual column

[durandom]

@AparnaKarve @imtayadeway this is a more generalized approach to #15600

But

• I'm not sure how adding that many virtual columns to all models would impact performance
• can't we change the REST API to filter on methods?
• would dropping the ? be an backwards incompatiblity change?

[imtayadeway]

can't we change the REST API to filter on methods?

That might be bad, because some methods have side effects

would dropping the ? be an backwards incompatiblity change?

I think we originally dropped the ?s to make these look more like bona fide columns, and also to avoid potential confusion when reading the query string in the URL (which starts with one).

Incidentally, there are probably some deletions that could be made since I think (since we already added a few of these individually IIRC)

[durandom]

@imtayadeway so you are saying, yes drop the ? ?
also, would it make sense to add all supports features a columns?
If you think so, we should ping the performance guys if this change is ok

[imtayadeway]

@durandom yeah, I am 👍 to drop the ?, and with this in general if it doesn't adversely affect performance 😁

Grepping for existing virtual columns didn't turn up anything for me - perhaps @AparnaKarve will rememeber? EDIT: I may have imagined this. Please ignore!

[durandom]

@kbrock could you have a look wether adding all those virtual_columns is ok?

[miq-bot]

Checked commits durandom/manageiq@28bc801~...fbc9479 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 1 offense detected

app/models/mixins/supports_feature_mixin.rb

• ❗ - Line 134, Col 7 - Style/Next - Use next to skip iteration.

[AparnaKarve]

@durandom I do like this generic approach that you have taken here.
Hopefully performance is not an issue here.
But if it is, then I think we should stick to the approach of cherry-picking only those support_* methods that we absolutely need to be queryable by the API.

I assume that this is a growing list and that there will be more and more support_* methods that would be added down the line, so that is a factor to be considered as well.

[Fryguy]

virtual columns are really meant for exposing things to reporting. They are not really meant for exposing things to the API (even though the API piggybacks on virtual columns to expose more stuff). I am concerned this opens up a lot of things to reporting that users just don't care about. This feels more like it should be an OPTIONS specific thing in the API.

[durandom]

@imtayadeway can we change the filter code to treat all filters that begin with supports_ as queries to the feature mixin?

[kbrock]

virtual columns work best in the base class (or the root of the query) and ones that do not change in subclasses.

Having various subclasses override values in a column is a recipe for some serious issues.
But that may be more of a statement around the features api than virtual columns

[AparnaKarve]

@imtayadeway @durandom Considering all these above points, should we get #15600 merged, since it's a PR for a high priority BZ?

[blomquisg]

I think this comment from @Fryguy applies to #15600 as well, right? I mean, in either case we're talking about adding virtual columns for API and not reporting, it seems.

[Fryguy]

@blomquisg Yes

[durandom]

@imtayadeway reg. #15614 (comment) - is there a central place where we could intercept filters that start with supports_ ?

[imtayadeway]

@durandom it would be difficult given that it's backed by MiqExpression, which only deals with columns or virtual columns. This probably warrants a deeper discussion - it seems there are some growing pains to do with the idea of virtual columns as being for reporting, and the unfortunate fact that the API is leveraging them quite integrally while there is resistance to virtual columns growing to meet API needs. ISTM that if we can't do the latter there might be some serious work ahead.

[durandom]

This feels more like it should be an OPTIONS specific thing in the API.

@imtayadeway that sounds like not using MiqExpression and a query string, but some filter constraints passed in via headers? Not sure though.
So my naive approach would be to exclude supports_ from a MiqExpression and filter after all objects have been retrieved

[imtayadeway]

@durandom we could go down the options route if it pertains to things about providers in general

If you want to have a go at modifying the filterer, this would be a great place to start. I have some reservations though about filtering on things that aren't attributes - for instance, who would they be discoverable?

[durandom]

@imtayadeway are you saying, that we should stay with exposing selected supports_ features via virtual attributes?
I'm not really experienced with how the API and options etc work.
Maybe we should discuss this face to face and also question the use case of filtering on supports_ features in general.

[kbrock]

I am concerned that we are trying to expose data as if it were in the database, and in reality, it is stored in a class hierarchy. Seems these could be removed from the classes and put into the database somewhere? maybe as jsonb?

Also, do note that typically, we add things as virtual attributes and people tend to think that this is supported by sql. So they filter or sort on these columns. Since they are in ruby, we have to bring back all rows. It causes all sorts of hidden performance problems (the 30 minutes vs 5 seconds kind)

[miq-bot]

This pull request has been automatically closed because it has not been updated for at least 6 months.

Feel free to reopen this pull request if these changes are still valid.

Thank you for all your contributions!