TokensController and TokenInfoController inherit ActionController::Metal

[jasl]

No description provided.

[houndci-bot]

Use 2 spaces for indentation in an array, relative to the start of the line where the left bracket is.

[tute]

Thank you!

[tyler-eon]

There are no details as to why this change was made. Is there a particular reason for it? I ask because this breaks the ability to use Devise/Warden when invoking resource_owner_from_credentials, or any other actions which execute inside TokensController. Was this a conscious decision to break that functionality?

[igrep]

@kolorahl +1! I'm also interested in it.

[jasl]

@kolorahl @igrep
The original discussion is here

According to learn its git log, it was inherited Metal since 305710d, and changed to inheritApplicationController since 5056e9b with no reason given, so I guess it's just a missing.

Besides, I can't see where invoke resource_owner_from_credentials, could you point it?

[tyler-eon]

@jasl Looking at the conversation, I can definitely see why you guys got rid of the extra includes now, but I'm still not sure what the benefit of ActionController::Metal is over ActionController::Base. Was there some performance gain to be had, was there erroneous behavior not exhibited in the metal version?

As far as the invocation of resource_owner_from_credentials, I was following the wiki to do a curl command for a password grant. My config code was very simply:

resource_owner_from_credentials do
    current_user
end

When hitting the POST /oauth/tokens endpoint using the password grant credentials I received a Rails error that says there is no method or variable named current_user in Doorkeeper::TokensController. I can confirm this is the the case because Devise and Warden modify ActionController::Base and not ActionController::Metal, so all metal-derived classes do not receive the included helper functionality of Devise or Warden.

[jasl]

@kolorahl
Inheriting Metal is consider about performance right.

I will check what you point out, thank for you reporting.

[tyler-eon]

Sure thing. Let me know if you want me to put this in a bug ticket. I'll let you do a bit of investigation first, though.

[jasl]

@kolorahl
Yes, please

[tyler-eon]

Opened issue #504 in regards to this discussion.

[tute]

At the moment it made sense to me as a general rule of thumb, but now I consider it was my mistake, as I didn't have before/after performance tests to compare them. I'd love to have them, and know what's better, for now we don't know if inheriting form Metal is actually better or not (we didn't grab any data).

[jasl]

@tute
it's not about performance(although Metal is quicker by theory).

The problem is no comment told why TokenController inherit ApplicationController originally and changed to MetalController and back to ApplicationController again with some no necessary codes, these changes are not break tests.

According to git log and its business, I think it should inherit Metal, it's about design, not performance.

[jasl]

Actually spending two months really busy work, I have some free time now.
My next step is learning Doorkeeper's controllers, and thinking how to refactor.