Drop supports_feature? methods

[kbrock]

Do not merge until we talk

Overview

Change implementation of supports to not define methods.

• Use supports in more instances #21989
• Complete Transition from AvailabilityMixin to SupportsFeatureMixin #21179
• extract subclassess_supporting #21553 - avoiding &:supports_feature? calls
• transition supports_orderable? to support?(:order) #22902
• define supports :action #22908
• drop supports_ methods manageiq-automation_engine#542
• Drop supports_feature? and unsupported_reason_add manageiq-providers-amazon#843
• drop use of unsupported_reason_add manageiq-providers-autosde#244
• drop unsupported_reason_add manageiq-providers-azure#549
• drop use of unuspported_reason_add manageiq-providers-google#260
• drop use of unsupported_reason_add an supports_feature manageiq-providers-ibm_cloud#486
• drop unsupported_reason_add manageiq-providers-ibm_power_hmc#153
• supports_start? => supports?(:start) manageiq-providers-ibm_power_hmc#155
• drop unsupported_reason_add manageiq-providers-ibm_power_vc#103
• drop unsupported_reason_add manageiq-providers-kubernetes#516
• convert supports_metrics => supports? manageiq-providers-kubernetes#520
• drop unsupported_reason_add manageiq-providers-kubevirt#229
• drop use of unsupported_reason_add manageiq-providers-openshift#255
• drop unsupported_reason_add and supports_event? manageiq-providers-openstack#875
• drop use of unsupported_reason_add manageiq-providers-oracle_cloud#97
• drop use of unuspported_reason_add manageiq-providers-ovirt#659
• drop unsupported_reason_add manageiq-providers-red_hat_virtualization#43
• drop unsupported_reason_add manageiq-providers-vmware#901

(technically we do not need unsupported_reason_add to add this feature. But that initiative is also removing supports_feature - which is definitely needed)

Followup:

• Drop Supports' unsupports variable #22898 (review to understand what to sweat in this PR)

Before

1. supports :feature and supports_not :feature defines supports_{feature}?.
2. if the feature is not supported, supports_{feature}? calls add_unsupported_reason to store the reason.
3. supports?(feature) calls supports_{feature}? under the covers.
4. supports?(unknown_feature) detects when supports_{unknown_feature}? is not defined and directly calls add_unsupported_reason.

After

1. supports :feature and supports_not :feature stores the reasons in a class level hash.
2. if the feature is not supported, supports_{feature}? calls add_unsupported_reason to store the reason.
3. supports?(feature) uses class level hash to determine the reason. It leverages add_unsupported_reason the same as before.
4. supports?(unknown_feature) notices that the feature is not in the class level hash and directly calls add_unsupported_reason.

Highlights

Fix infinite stack trace for Supports Attribute

We have a feature called supports_attributes that introduces a virtual attribute supports_{feature} (note: no trailing question mark). Since this is a boolean attribute, Rails defines this method with a question mark, which conflicts with the method that supports :feature defines. (see before number 1)

So when the code tries to determine if the feature is defined (see before number 4), it sees the rails attribute definition and assumes it is always defined and calls the rails attribute accessors.

Rails 6.1 worked fine with this bug, but in rails 7, it causes an infinite stack trace. This PR removes this duplicate definition issue and fixes both rails 6.1 and 7.0

Dropping supports_feature? methods

The supports_{feature}? method is useful when using detect or select -- detect(&:supports_feature?), so we used it a few times in the ui-classic and in core. We have pruned these out but unfortunately we do loose this shortcut. I think subclasses_supporting has made those calls no longer necessary.

Determining if supports_feature? methods are used

Searching the code base for calls to supports_feature? is a little tricky since there are methods with the name supports_feature? defined, but they are not using the supports :feature mechanism. There is an initiative (linked at the top) to converge on the supports :feature mechanism, but that has been a background thread

Reducing the number of methods defined

I remember having a conversation with @chessbyte to cut down on the number of methods defined in classes, but I can't find an issue actually stating this goal. That was the main reason I had embarked on this journey some dozen PRs ago.

We have not completed to goal to converge all the supports type mechanisms.
But this PR completes the goal to reduce all the extra methods defined on the class.

We did reduce quite a few methods defined by #22578 - that no longer defines the methods for every base class. That PR definitely exposed the problem we are seeing now, but supports should not be going through the rails attributes code, so that is why I am suggesting this solution

[kbrock]

update:

• changed supports_order alias (a wip)
• fixed rubocops for hashrockets, extra space
• dropping unused COMMON_FEATURE constant and present? => presence

ok, supports_order? is being used, though I think it was introduced in #19186 / ManageIQ/manageiq-api#656

I'll see if that is used anywhere. Since these sometimes get exposed as columns,

WIP: possibly drop supports_order / validate_supports
WIP: want to discuss before merging (and run cross tests

[jrafanie]

FYI, I tried this PR with my rails 7 branch and things look good.

[Fryguy]

The only thing I can think of that I didn't see covered in any of the comments here (unless I missed it), is that we have a handful of virtual columns representing "supports", which are meant to be accessed over the API. As long as those still work, I'm good with this.

[kbrock]

@Fryguy

Thanks I changed Highlights / Supports Attribute to better highlight attribute_supports.
These supports_attributes are what caused this issue in the first place.

[kbrock]

update:

• dropped aliases to orderable? (PR in ui-classic to drop)
• dropped validate - not able to find this anywhere

update:

• no longer delete validate_order - looks like it may be called by order_#{action} from ui-classic

[kbrock]

update:

• removed the deletion of ServiceTemplate#orderable? (OrchestrationTemplate implements this and this is no longer a quick change - we can do this work in a separate PR if we even want to do this)

[kbrock]

update:

• simplified default_supports_reason
• no longer setting a default reason keeping false value. that way it can be localized later
• added specs that chain supports. This is a code pattern that we have been using more and more. These ensure the pattern works as designed.

[Fryguy]

My only concern here is that I think this needs to be made thread-safe. I think this is because we could lazy load classes at run-time in different threads (think 2 different requests coming that each need to lazy load things).

I know that lazy loading has bit us in the past with this - see the way the unsupported method is defined to use a Concurrent::Hash for this reason.

[kbrock]

This is the Rails pattern for defining class level configuration that inherits from parent classes and then inherit columns from parents and allow overriding.

• define class variable https://github.com/rails/rails/blob/7-0-stable/activerecord/lib/active_record/attributes.rb#L11
• append to class variable https://github.com/rails/rails/blob/7-0-stable/activerecord/lib/active_record/attributes.rb#L231
• use variable to set methods https://github.com/rails/rails/blob/7-0-stable/activerecord/lib/active_record/attributes.rb#L265

We use this same pattern for virtual_attributes:

• https://github.com/kbrock/activerecord-virtual_attributes/blob/master/lib/active_record/virtual_attributes.rb#L41
• https://github.com/kbrock/activerecord-virtual_attributes/blob/master/lib/active_record/virtual_attributes.rb#L62

Since these values are set at class load time, and Rails has a class loader mutex to protect the other loaders, I am pretty sure we are in the clear.

[kbrock]

supported_features, similar to rails attribute API, only modifies the class at load time and should be protected by the rails class loader mutex.

unsupported is constantly modified at runtime. While I am not happy with this race condition, this PR focuses on removing the extra methods that were causing the infinite backtraces. In the future we can hopefully drop unsupported and use supported_feature instead.

[kbrock]

I created #22898 as a place holder for the next phase

[Fryguy]

This code does a get->modify->set, and another thread can get in the middle. In the past, 2 threads could lazy-load 2 separate classes, particularly in the API.

only modifies the class at load time and should be protected by the rails class loader mutex.

@jrafanie Can you comment here? Not sure if this is now completely handled by zeitwerk or not, but this was a problem in the past and is why we had to wrap all of those class loading places in the special Rails handler, particularly in the API.

[jrafanie]

hmm, there is discussion of thread safety for code reloading. I'm wondering if we need to implement something similar to what we had previously for dev mode reloading only. I think autoloading in production mode is fine as long as it's after the application is initialized/rails loaded.

[kbrock]

This code does a get->modify->set, and another thread can get in the middle. In the past, 2 threads could lazy-load 2 separate classes, particularly in the API.

Yes, The 'get, modify, set' of unsupported[] is bad. This is the existing behavior.

The first commit's goal is to just consolidate the existing logic.
Rails uses this patter when defining methods: def field_name ; attr_reader(:field_name) ; end instead of putting the actual gook into the defined method. (not always, but generally)

  def supports(feature, &block)
    define_method("supports_#{feature}?") do
+     common_method(feature)
+   end
+ end
+
+   def common_method(feature) # sorry rubocop for indentation
      unsupported.delete(feature)
      if block
        instance_eval(&block) # gook (not for the class level)
      else
        unsupported.add(feature) # some kind of modify
      end
      !!unsupported[feature]
    end
- end

I can see if there is a way to make this fact more obvious.
But again, I wasn't as concerned about fixing the code knowing that #22898 will remove the clear ; modify ; get to a just plain get.

only modifies the class at load time
[...] this was a problem in the past and is why we had to wrap all of those class loading places in the special Rails handler

There have been issues when one class referenced another class at load time. virtual_delegates used to have this problem but we changed it to lazy reference the other class. has_many also has this problem, and why we use class_name=String rather than class references.

In our case, we are following the example from the attributes() api. It defines Symbols/Strings that are put into a Hash at load time and never changed after that.

[Fryguy]

Ok discussed with @kbrock and I was mistakenly thinking that the class_attribute would be shared, so that child classes would be pushing values and could clobber each other. However, what's actually happening is the parent class loads first, then each child creates a copy of the parent class' feature and then inserts their own into the copy.

So this is fine 👍

[kbrock]

update:

• rebase (no conflicts)
• merged check_supports into class level supports?

update:

• pulled check_supports back out of supports?. I had started to make this private and enhance, but in the end of the day, this method is going away once we merge Drop Supports' unsupports variable #22898

[kbrock]

cross repo test failures:

• automate (waiting for merge)
• api
• kubernetes (waiting for merge)
• ui-service

[kbrock]

update:

• rebased (to pull in agrare's change around managers - to turn kubernetes green)

[kbrock]

@Fryguy You have any concerns dropping the supports_feature? syntax from our code base?

[Fryguy]

Nope I love it

[kbrock]

update:

• rebase (fix merge conflicts)
• moved commit order a little

[miq-bot]

Checked commits kbrock/manageiq@15330bf~...181a03e with ruby 2.7.8, rubocop 1.56.3, haml-lint 0.51.0, and yamllint
5 files checked, 0 offenses detected
Everything looks fine. 🍪