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

Error handlers are responsible for HTTP response #252

Merged
merged 4 commits into from
May 10, 2023

Conversation

bittrance
Copy link
Contributor

@bittrance bittrance commented Apr 26, 2023

Allow error handlers to return an abstract HTTP response which the framework is then responsible for rendering. This allows users to return e.g. structured and localized responses from the middleware.

This is a breaking change in that existing error handlers need a signature change and return nil to retain existing behavior.

This is an alternative to #251 as discussed in that PR. Fixes #249 .

@bittrance bittrance changed the base branch from main to oidctoken-tests April 26, 2023 13:43
@bittrance bittrance changed the base branch from oidctoken-tests to main April 27, 2023 09:57
@bittrance bittrance force-pushed the errorhandler-does-http branch 2 times, most recently from c2aa5e6 to f020306 Compare April 28, 2023 14:20
bittrance added a commit that referenced this pull request May 4, 2023
…ddleware.

There are two reasons for this: the first is that the packaged middleware
tends to return 400 rather than more correct 4XX codes. The second reason
is that this switch helps #252.
@bittrance bittrance changed the base branch from main to echo-no-jwt May 4, 2023 15:44
@bittrance bittrance marked this pull request as ready for review May 4, 2023 15:45
@bittrance bittrance requested a review from landerss1 May 4, 2023 15:45
@coveralls
Copy link

Pull Request Test Coverage Report for Build 4884765398

  • 61 of 61 (100.0%) changed or added relevant lines in 5 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.4%) to 85.349%

Totals Coverage Status
Change from base Build 4884262367: 0.4%
Covered Lines: 1270
Relevant Lines: 1488

💛 - Coveralls

@bittrance bittrance changed the base branch from echo-no-jwt to main May 8, 2023 14:15
Copy link
Member

@simongottschlag simongottschlag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few nitpicks

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
oidcgin/gin.go Outdated Show resolved Hide resolved
options/options.go Outdated Show resolved Hide resolved
options/options.go Outdated Show resolved Hide resolved
options/options.go Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
bittrance added a commit that referenced this pull request May 9, 2023
Copy link
Member

@simongottschlag simongottschlag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fantastic work @bittrance! 🚀

@bittrance bittrance merged commit 88c83b7 into main May 10, 2023
8 checks passed
@bittrance bittrance deleted the errorhandler-does-http branch May 10, 2023 07:16
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

Successfully merging this pull request may close these issues.

gin: Possibility to not AbortWithError in errorhandler if status code is not 5xx
3 participants