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

Enforce exception handler uniquness #2166

Closed
ahopkins opened this issue Jun 16, 2021 · 10 comments · Fixed by #2537
Closed

Enforce exception handler uniquness #2166

ahopkins opened this issue Jun 16, 2021 · 10 comments · Fixed by #2537

Comments

@ahopkins
Copy link
Member

ahopkins commented Jun 16, 2021

  1. You should not be able to register the same exception more than once, or at least not on the same App/Blueprint.
  2. Handlers should only be fetched in relation to the BP or App context of the matched route. This effectively means that some exceptions (NotFound could only be registered app level).

Originally posted by @ahopkins in #2121 (comment)

@stale
Copy link

stale bot commented Oct 2, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If this is incorrect, please respond with an update. Thank you for your contributions.

@prryplatypus
Copy link
Member

Hello!

Regarding point 2, there is currently useful usecases to register exceptions like NotFound at a BP level. For example, you may have an API blueprint where you want all responses to be returned as JSON; but want them to be returned as HTML everywhere else.

I believe currently it is possible to do this and, if I understand correctly, point 2 would remove the ability to do so.

@ahopkins
Copy link
Member Author

ahopkins commented Oct 3, 2021

I guess it could work if you manually raised NotFound. But if the route does not exist, there is no way for Sanic to know which blueprint (and therefore which handler) to apply.

@prryplatypus
Copy link
Member

prryplatypus commented Oct 3, 2021

What if the blueprint has a URL prefix? I'd expect the blueprint's error handler to try handling any errors within that URL prefix, if no child handles them before. 🤔

@ahopkins
Copy link
Member Author

ahopkins commented Oct 3, 2021

That is not likely something we will introduce since it would require adding default routes to blueprint handlers as catch-all. Which would and could have some bizarre unintended (and difficult to debug) consequences.

Since NotFound could be raised manually in legit use cases, maybe we need to identify some (like this and NoMethod) that should raise a warning on BP registration.

@AndreasPantle
Copy link

What if the blueprint has a URL prefix? I'd expect the blueprint's error handler to try handling any errors within that URL prefix, if no child handles them before. 🤔

This is exactly how I thought it should work.

There can be some use cases out there: Say, if you have a blueprint for a GUI serving and you like to do special stuff with Exceptions here in a (sub)URL.

@ahopkins
Copy link
Member Author

If that's the case I'd suggest catching the exception with an exception handler and checking if the target path segment is in the current request path.

@stale
Copy link

stale bot commented Mar 2, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If this is incorrect, please respond with an update. Thank you for your contributions.

@stale stale bot added the stale label Mar 2, 2022
@ahopkins
Copy link
Member Author

ahopkins commented Mar 2, 2022

...

@stale stale bot removed the stale label Mar 2, 2022
@stale
Copy link

stale bot commented Jun 12, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If this is incorrect, please respond with an update. Thank you for your contributions.

@stale stale bot added the stale label Jun 12, 2022
@ahopkins ahopkins changed the title PR #2128 should be the solution here. But, I think it is only part of a greater solution needed. Enforce exception handler uniquness Aug 28, 2022
@ahopkins ahopkins removed the stale label Aug 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants