AbstractController#internal_methods: ignore action_methods

[kamil-gwozdz]

This PR fixes AbstractController#internal_methods to exclude action_methods.

Motivation / Background

I tried enabling raise_on_missing_callback_actions and I found out that MyController#action_methods is missing a method that MyController actually defines. It only happens when a method is also defined by an ancestor abstract controller.

xref #48559 (comment)

Checklist

Before submitting the PR make sure the following are checked:

• This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
• Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
• Tests are added or updated if you fix a bug or add a feature.
• CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

[Edouard-chin]

Thanks, LGTM!

FYI, if you have time (and want to ofc) this issue #44867 is similar and after merging the change you made in this PR, it will be very easy to fix.

[zzak]

I had a look at this, and came up with some questions.

It seems like the path forward on #44867 would be to remove the monkey-patched action_methods? That passes the bug report test, but this test fails:

Failure:
FlashTest#test_additional_flash_types_are_not_listed_in_actions_set [/Users/zzak/code/rails/actionpack/test/controller/flash_test.rb:231]:
Expected #<Set: {"foo", "use_flash_and_keep_it", "use_flash_and_update_it", "use_flash_after_reset_session", "halt_and_redir", "std_action", "filter_halting_action", "redirect_with_alert", "redirect_with_notice", "render_with_flash_now_alert", "render_with_flash_now_notice", "redirect_with_other_flashes", "redirect_with_foo_flash", "set_flash", "set_flash_now", "attempt_to_use_flash_now", "use_flash"}> to not include "foo".

And the following test test_add_flash_type_to_subclasses also seems like it should fail but doesn't.

Does that make sense? Or am I just missing something obvious 🤔

[Edouard-chin]

Thanks for looking @zzak.

Regarding #44867, yes, just removing the overwritten action_methods was what I had in mind to fix the issue. But you are right that it wouldn't work as I was expecting. I assumed that the flash helper were defined as private methods on the controller, but it's not the case, it's public and therefore considered an action.
I don't really see a reason why it's defined as public though 🤔.

BTW, I'm also just realising that there is a PR open in #44868 for this issue

[carlosantoniodasilva]

Hey all, just popping in to say that based on a quick bisect, this may have caused one Devise test to choke on main:

Failure:
HelpersTest#test_resources_methods_are_not_controller_actions [/home/runner/work/devise/devise/test/controllers/internal_helpers_test.rb:54]:
Expected #<Set: {"_prefixes"}> to be empty.

I'm still digging, but wanted to mention it. This is the test:
https://github.com/heartcombo/devise/blob/f6e73e5b5c8f519f4be29ac9069c6ed8a2343ce4/test/controllers/internal_helpers_test.rb#L53-L55

And this is the _prefixes method overridden by DeviseController to support scoped views:
https://github.com/heartcombo/devise/blob/f6e73e5b5c8f519f4be29ac9069c6ed8a2343ce4/app/controllers/devise_controller.rb#L21-L34

All other controllers inherit from it, and if I'm understanding the change correctly, Devise should flag that controller as abstract! to make sure its public methods aren't considered action methods, but rather "internal" now (including _prefixes).

I'm guessing this wasn't a problem before because _prefixes is overridden, and thus public_instance_methods(true) would cause it to still be considered "internal" by the previous implementation, which seems to be what this is possibly fixing. Let me know if that sounds about right and/or you have any other thoughts, thanks.

[rafaelfranca]

That sounds right to me. DeviseController should be abstract

[zzak]

@carlosantoniodasilva Random question, but is that controller generated or just inherited inside apps?

[carlosantoniodasilva]

@rafaelfranca thanks, I'm giving that a shot and it does make the test pass, although it makes me wonder if it can cause any issue down the road with it inheriting from, say, ApplicationController, which is not abstract.

So we basically will have other controllers with something like this:

Devise::RegistrationsController # final controller/implementation (if not inherited by application itself)
DeviseController # abstract
ApplicationController # implementation
ActionController::Base # abstract

The logic that runs until controller.abstract? would stop at DeviseController here, meaning any potentially public method of ApplicationController no longer becomes part of action_methods. (not that I think people should have those there, but it illustrates a possible change in behavior by introducing an abstract class in the middle of the chain -- and people do weird things.)

@zzak that's a "base" controller which all other Devise controllers inherit from, it's not generated inside apps.

[rafaelfranca]

although it makes me wonder if it can cause any issue down the road with it inheriting from, say, ApplicationController, which is not abstract.

That is an excelent question and I think your interpretation is correct. Public methods in ApplicationController will not longer be part of action_methods. I'm not sure if that is common though, but it is possible this change will break those apps.

Any suggestion of what we should do here?

[Edouard-chin]

Sorry that I can’t check myself, I’m with limited network and no laptop for a few days. Would it be possible to make the resources method (_prefixes) private instead of public? Le mer. 13 sept. 2023 à 09:32, Rafael Mendonça França < ***@***.***> a écrit :

…

although it makes me wonder if it can cause any issue down the road with it inheriting from, say, ApplicationController, which is not abstract. That is an excelent question and I think your interpretation is correct. Public methods in ApplicationController will not longer be part of action_methods. I'm not sure if that is common though, but it is possible this change will break those apps. Any suggestion of what we should do here? — Reply to this email directly, view it on GitHub <#48699 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB567BTNSJPBLZTODOSFBW3X2HGXDANCNFSM6AAAAAA2DVJW2I> . You are receiving this because you commented.Message ID: ***@***.***>

[rafaelfranca]

I also didn't properly check, but I think Rails call it internally outside of the controller.

[carlosantoniodasilva]

Would it be possible to make the resources method (_prefixes) private instead of public?

On Devise, you mean? It's made public because Rails makes it public, but we can probably bet that's just one method that we happen to override, there may be others out there.

Any suggestion of what we should do here?

I am not sure, will try to think about it more... it's not common (maybe not even expected) to have "abstract -> non-abstract -> abstract -> non-abstract" inheritance. Generally speaking abstract is the last layer, or at least that's how it's interpreted by the implementation(s).

We could potentially have the implementation walk up the chain and not stopping at the first abstract class, but that might seem a bit odd. Or I could just have Devise also override internal_methods and add its own _prefixes to it, bit like I mentioned above, there may be other use-cases out there we don't know about.