Skip to content
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

Complication with default_status #48

Open
jasonkarns opened this issue May 25, 2022 · 1 comment
Open

Complication with default_status #48

jasonkarns opened this issue May 25, 2022 · 1 comment

Comments

@jasonkarns
Copy link
Contributor

I've run into a complication with the way handler's ::default_status is used.

Presumably, the "proper" way for a custom handler to derive and expose status codes is to override #status_code. Within this method, I was expecting to be able to:

  1. return @status code if set (which would presumably mean the :status option was explicitly passed to register_exception
  2. derive the status code from the exception class if not set.
  3. fallback to the handler's ::default_status if the exception class didn't define its own status code

However, this line is causing issues:

status = options[:status] ||= handler.default_status

Because the handler's default_status is "forced" into the options hash, it is impossible for a custom handler to differentiate between an explicit status provided to register_exception from the handler's own default status used in place of the :status option.

I don't think I understand the rationale well enough to offer a PR with an alternative. (https://en.wikipedia.org/wiki/Wikipedia_talk:Chesterton%27s_fence) So discussing it here instead 😄

To work around this, I presently have some pretty hacky code:

class CustomHandler < RescueRegistry::ExceptionHandler
  # sentinel to distinguish explicit :status option from this default
  def self.default_status
    9000
  end

  # Let the exception tell us which status code it maps to
  def status_code
    # respect explicit :status option if provided
    return super if status < self.class.default_status

    # otherwise, derive from exception
    exception.status.presence || 500
  end

I'm hoping that it would be possible to eliminate the status = options[:status] ||= handler.default_status and instead have ExceptionHandler's inherited implementation of status_code be @status.presence || self.class.default_status? That way subclasses which override status_code could distinguish an explicit :status (b/c @status would be non-nil); and still invoke super to get the default value if they wish?

I can open a PR to this effect unless there's some other known logic depending on the current behavior.

@wagenet
Copy link
Owner

wagenet commented Jun 27, 2022

Apologies for the delayed reply here.

First off, let me note that I'm a big fan of Chesterton so I approve of him being referenced :)

Unfortunately, it's been a while since I worked on this project so I'm not 100% sure that even I remember all of my rationale. It also looks like I didn't specifically test this, so that's less than ideal :/

One option might be to allow default_status to take an argument of the current exception, though I guess that might technically be a breaking change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants