Skip to content
This repository has been archived by the owner on Feb 24, 2024. It is now read-only.

added assert middleware to assert handler's behavior. (fix #2339) #2345

Merged
merged 2 commits into from
Oct 27, 2022

Conversation

sio4
Copy link
Member

@sio4 sio4 commented Oct 25, 2022

For the net/http#ResponseWriter, Write() will set the response status as "200 OK" if a handler exited without calling WriteHeader() but returns nil. However, the Buffalo's behavior is slightly different. The Buffalo's actual responding behavior is basically the same since it uses the same, but the buffalo's middlewares (especially poptx from buffalo-pop) could treat them as a failed action when no response status is found. This inconsistency made some issues and this PR will fix the issue.

See more details in #2339 and other linked issues.

fixes #2339
fixes gobuffalo/buffalo-pop#25
fixes gobuffalo/buffalo-pop#19

related issues/PRs: #2300, #2334, #2335,

@sio4 sio4 added the enhancement New feature or request label Oct 25, 2022
@sio4 sio4 added this to the v1.1.0 milestone Oct 25, 2022
@sio4 sio4 self-assigned this Oct 25, 2022
@sio4 sio4 requested a review from a team as a code owner October 25, 2022 12:18
Copy link
Member

@paganotoni paganotoni left a comment

Choose a reason for hiding this comment

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

I like it. @sio4 Is there a chance we could add some tests to leave this behavior specified and guaranteed after v1.1.0 ?

@sio4
Copy link
Member Author

sio4 commented Oct 26, 2022

I like it. @sio4 Is there a chance we could add some tests to leave this behavior specified and guaranteed after v1.1.0 ?

Sure, I agree. Will do that soon. (Maybe by this week)

@sio4 sio4 force-pushed the adding-assert-middleware branch from 821474b to 0439594 Compare October 27, 2022 12:47
@sio4 sio4 requested a review from paganotoni October 27, 2022 13:01
Copy link
Member

@paganotoni paganotoni left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -242,3 +243,67 @@ func Test_Middleware_Remove(t *testing.T) {
_ = w.HTML("/no_log_autos/1").Get()
r.Len(log, 0)
}

func Test_AssertMiddleware_NilStatus200(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

This is great! Thanks @sio4 !

@paganotoni paganotoni merged commit b29eff1 into v1 Oct 27, 2022
@paganotoni paganotoni deleted the adding-assert-middleware branch October 27, 2022 13:03
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
2 participants