-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DRY out AuthorizationsController#render_success #1706
base: main
Are you sure you want to change the base?
Conversation
In our own deployment, we have some custom exception handling around the `create`` method. By calling the more "public" `create` method in `render_success`, we get to have a _single_ code path that also supports `skip_authorization?` and `can_authorize_response?`. The code on L35 is identical to L16. That suggests identical intent, but not always. This might be coincidental, in which case, the pull request should be declined.
@@ -32,7 +32,7 @@ def destroy | |||
|
|||
def render_success | |||
if skip_authorization? || can_authorize_response? | |||
redirect_or_render(authorize_response) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we better extract redirect_or_render(authorize_response)
into a method like respond_with_authorization
or similar to avoid action calling inside another methods/actions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly, but redirect_or_render
is also another private
in the same controller, does not live anywhere else and calls controller-owned methods ... I don't have any better ideas for where a public method would get defined? Calling create
seems public enough behavior?
For our purposes, we created our own AuthorizationsController
class that inherits from Doorkeeper
, modified the routes accordingly and then overrode create
:
module Ours
class AuthorizationsController < Doorkeeper::AuthorizationsController
# Override app/controllers/doorkeeper/authorizations_controller.rb
# Override the #create method
# To catch and render errors for the ActiveRecord validation
def create
super # <= This is why we're proposing this pull request
rescue ActiveRecord::RecordInvalid => exception
case exception.message
when /Validation failed: Tenant has already been taken/
pre_auth.error = Ours::Errors::DuplicateAuthorization
redirect_or_render(pre_auth.error_response)
else
raise exception
end
end
end
end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nbulaj Any more thoughts on this? We've been running this on a private branch for 3 weeks and haven't encountered any adverse side effects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure @kmayer what is the problem with the private methods which is allowed to be overridden in Ruby . I still don't find it a good choice to call #create
action inside another method in the controller (like any other actions)
In our own deployment, we have some custom exception handling around the
create
method. By calling the more "public"create
method inrender_success
, we get to have a single code path that also supportsskip_authorization?
andcan_authorize_response?
. The code on L35 is identical to L16. That suggests identical intent, but not always. This might be coincidental, in which case, the pull request should be declined.