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

Beego Status fix #264

Merged
merged 1 commit into from
Apr 19, 2022
Merged

Beego Status fix #264

merged 1 commit into from
Apr 19, 2022

Conversation

chimanjain
Copy link
Contributor

When the response status is ok, the beego application sets it to 0. To fix that when we send the notice to airbrake, we overwrite the status to StatusOK i.e. 200

When the `response status` is ok, the beego application sets it to `0`. To fix that when we send the notice to airbrake, we overwrite the status to StatusOK i.e. `200`
@chimanjain chimanjain requested a review from kyrylo April 18, 2022 12:35
@kyrylo
Copy link
Contributor

kyrylo commented Apr 18, 2022

Please make the commit title more descriptive.

@kyrylo
Copy link
Contributor

kyrylo commented Apr 18, 2022

When the response status is ok, the beego application sets it to 0

Why is that so? Are you sure beego does it on purpose?

@chimanjain
Copy link
Contributor Author

When the response status is ok, the beego application sets it to 0

Why is that so? Are you sure beego does it on purpose?

Not sure, I came across this issue when I was working on the cli integration.

@kyrylo
Copy link
Contributor

kyrylo commented Apr 18, 2022

We should probably investigate why it's like that. It's not normal that 0 is returned when 200 is meant. Therefore I am vary of merging this PR.

@chimanjain
Copy link
Contributor Author

The default value of ctx.ResponseWriter.Status is 0, so when the programmer set it with Ctx.ResponseWriter.WriteHeader(), it gets set to that status code, otherwise it remains 0.
Generally, when we are returning the response of an api, we don't set the status when the response is ok, i.e. 200, so to overcome that, we overwrite the status to 200 if it was not set.

@kyrylo
Copy link
Contributor

kyrylo commented Apr 18, 2022

If beego leaves statusCode 0, then this if check will fail:

if response.StatusCode != http.StatusOK

Still hard to believe that 0 is intended. How do I replicate that scenario you mentioned in the original commit?

@kyrylo
Copy link
Contributor

kyrylo commented Apr 18, 2022

Example app by @chimanjain which shows the issue that is being fixed:

package main

import (
	"fmt"
	"net/http"
	"time"

	"github.com/beego/beego/v2/server/web/context"

	"github.com/beego/beego/v2/server/web"
)

type Date struct {
	Date int64 `json:"date"`
}

type Controller struct {
	web.Controller
}

func main() {
	ctrl := &Controller{}

	web.Router("/date", ctrl, "get:GetDate")
	web.Router("/datewithstatus", ctrl, "get:GetDateWithStatus")
	web.InsertFilterChain("/*", New())
	web.Run("127.0.0.1:3000")
}

func (ctrl *Controller) GetDate() {
	ctrl.JSONResp(&Date{Date: time.Now().Unix()})
}

func (ctrl *Controller) GetDateWithStatus() {
	ctrl.Ctx.ResponseWriter.WriteHeader(http.StatusOK)
	ctrl.JSONResp(&Date{Date: time.Now().Unix()})
}

func New() web.FilterChain {
	return func(next web.FilterFunc) web.FilterFunc {
		return func(ctx *context.Context) {

			next(ctx)
			fmt.Println(ctx.ResponseWriter.Status)

		}
	}
}

Airbrake API will reject the payload if statusCode is 0, therefore our best assumption is that the endpoint meant to return 200.

Copy link
Contributor

@kyrylo kyrylo left a comment

Choose a reason for hiding this comment

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

LGTM but please change the title of the PR and the commit title.

@chimanjain chimanjain changed the title Bug fix Beego Status fix Apr 18, 2022
@chimanjain chimanjain merged commit 5999c98 into master Apr 19, 2022
@chimanjain chimanjain deleted the beego-fix branch April 19, 2022 05:20
@kyrylo
Copy link
Contributor

kyrylo commented Apr 19, 2022

I can see that the commit title didn't change: d67b534

It says "bug fix". Hopefully, next time we can change that as well so that we have a clean git history.

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.

2 participants