Skip to content

Refactor response handling #53

@danschultzer

Description

@danschultzer

I'm not happy with how responses are currently handled. As an example here's how an error response is handled:

@spec authorize(Schema.t(), map(), keyword()) :: {:ok, binary()} | Response.error() | Response.redirect() | Response.native_redirect()
def authorize(resource_owner, request, config \\ []) do
case validate_response_type(request, config) do
{:error, :invalid_response_type} -> unsupported_response_type(resource_owner, request, config)
{:error, :missing_response_type} -> invalid_request(resource_owner, request, config)
{:ok, token_module} -> token_module.authorize(resource_owner, request, config)
end
end

defp unsupported_response_type(resource_owner, request, config),
do: handle_error_response(resource_owner, request, Error.unsupported_response_type(), config)

defp handle_error_response(resource_owner, request, error, config) do
resource_owner
|> Utils.prehandle_request(request, config)
|> Error.add_error(error)
|> Response.error_response(config)
end

@spec add_error({:ok, map()} | {:error, map()}, {:error, map(), atom()}) :: {:error, map()}
def add_error({:error, params}, _), do: {:error, params}
def add_error({:ok, params}, {:error, error, http_status}) do
{:error, Map.merge(params, %{error: error, error_http_status: http_status})}
end

@spec error_response({:error, map()}, keyword()) :: error() | redirect() | native_redirect()
def error_response({:error, %{error: error} = params}, config), do: build_response(params, error, config)

defp build_response(%{request: request} = params, payload, config) do
payload = add_state(payload, request)
case can_redirect?(params, config) do
true -> build_redirect_response(params, payload, config)
_ -> build_standard_response(params, payload)
end
end

From the above it's almost impossible to figure what's going on. The code is fragmented and you have to go back and fourth between several files to figure out what's going on. Also the response doesn't make much sense since it's a tuple with a map where an :error key value has been added. Not very helpful.

It should be a lot more compact an easy to read. Something along these lines, almost everything contained within this module:

  def authorize(resource_owner, request, config \\ []) do
    request
    |> validate_response_type(config)
    |> case do
      {:error, error}     -> response_type_validation_error({:error, error}, request, config)
      {:ok, token_module} -> token_authorize(token_module, resource_owner, request, config)
    end
  end

  defp response_type_validation_error({:error, error}, request, config) do
    # Return HTTP error code or build redirect response
  end

  defp token_authorize(Code, resource_owner, request, config), do: Code.authorize(resource_owner, request, config)

Metadata

Metadata

Assignees

No one assigned

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions