graphiti-api / graphiti-rails

MIT License
54 stars 28 forks source link

First-class support for custom handlers #19

Closed wagenet closed 5 years ago

wagenet commented 5 years ago

In the current implementation, we register two GraphitiErrors handlers, a default one and an InvalidError one, this mimics the default GraphitiErrors behavior. These handlers will get hit in the event that the request's content type is in Graphiti::Rails.handled_exception_formats. By default, we only handle JSON:API requests since Rails' handling of these requests isn't compliant with the JSON:API spec. Users can also opt-in to handling for other content types, but this is disabled by default since it is changing the default exception handling behavior in the application.

The outcome of this is that exceptions are not always handled by the GraphitiErrors handlers. A user could choose to register a new handler with Graphiti::Rails.register_exception but this could lead to a confusing outcome as the handler will not be used when hitting a content type that isn't in Graphiti::Rails.handled_exception_formats.

One solution is to just tell users to use rescue_from. However, this has the downside of requiring users to correctly format the response, especially in the case of JSON:API. If we do recommend this solution, we should try to provide some first-class handlers to streamline the process. At the moment, I suggest waiting for some real world feedback to determine what sort of APIs we should provide.

richmolj commented 5 years ago

Also relevant to the Railtie vs Mixin issue. One use case: imagine globally rescuing a NotAuthorized error with 403 and generic message. But if you're in Admin::PostsController, you want a custom error message for this internal functionality, maybe with details on who to contact to get privileges. This is another advantage to registering errors on the controller instead of globally.

wagenet commented 5 years ago

@richmolj I think we may still have a slight disconnect. We're not really registering handlers globally from a developer perspective. The fact that we are using GraphitiErrors.register_exception behind the scenes is an implementation detail that the developer should not have to be concerned about. My concern is to have a default handler for JSON:API (and other types that a developer may opt-in to) and it so happens that a good way to do that is to register a handler with GraphitiErrors.

The Rails approach is to directly use rescue_from for exceptions that you want to handle. I'd like to do the same and not also require the developer to call a second API (e.g. register_exception for this to work correctly).

wagenet commented 5 years ago

To rewrite the code in the graphiti_errors_spec.rb, something like this:

  def render_graphiti_exception(exception, handler: nil,  **options)
    raise ArgumentError if handler && !options.empty?
    handler ||= GraphitiErrors::ExceptionHandler.new(options)
    handler.log(exception)
    json   = handler.error_payload(exception)
    status = handler.status_code(exception)
    render json: json, status: status
  end

  rescue_from(CustomStatusError) { |e| render_graphiti_exception(e, status: 301) }
  rescue_from(CustomTitleError) { |e| render_graphiti_exception(e, title: "My Title") }
  rescue_from(MessageTrueError) { |e| render_graphiti_exception(e, message: true) }
  rescue_from(MessageProcError) do |e|
    render_graphiti_exception(e, message: ->(e) { e.class.name.upcase })
  end
  rescue_from(MetaProcError) do |e|
    render_graphiti_exception(e, meta: ->(e) { {class_name: e.class.name.upcase} })
  end
  rescue_from(LogFalseError) { |e| render_graphiti_exception(e, log: false) }
  rescue_from(CustomHandlerError) do |e|
    render_graphiti_exception(e, handler: CustomErrorHandler)
  end

This seems to me to be the Rails approach, so the thing to do would basically be to make render_graphiti_exception built-in.

wagenet commented 5 years ago

Addressed by #22