gin-gonic / gin

Gin is a HTTP web framework written in Go (Golang). It features a Martini-like API with much better performance -- up to 40 times faster. If you need smashing performance, get yourself some Gin.
https://gin-gonic.com/
MIT License
77.95k stars 7.97k forks source link

Is Gin Context violating the golang context styleguide? #4040

Closed xr closed 1 week ago

xr commented 2 weeks ago

Hey guys,

We're having some discussions regarding whether or not Gin Context violates the Go context style guide. I'd like to hear your thoughts on this.

According to the Go style guide (reference: Go Style Guide on Custom Contexts), it clearly states:

Do not create custom context types or use interfaces other than context.Context in function signatures. There are no exceptions to this rule.

However, in the case of Gin's HandlerFunc, it passes a custom *Context type.

Even though the Gin Context implements all the methods of the context.Context interface,

Deadline() (deadline time.Time, ok bool)
Done() <-chan struct{}
Err() error
Value(key any) any

as shown here

We still need to interpret the Go style guide. There seem to be two possible views on this:

  1. Gin's Context is acceptable: Since it implements the context.Context interface, it can be considered valid, and thus doesn't violate the style guide. Similarly, we could create other types like this:

    type interface CustomContext {
        context.Context
        // ...additional APIs that we need
    }
  2. Strict compliance: To adhere to the Go style guide strictly, we should always pass context.Context directly in function signatures. In this case, the design would need to change to separate the Context from custom APIs. For example, the function signature could be:

    type HandlerFunc func(ctx context.Context, api CustomAPIs)

What do you guys think?

FarmerChillax commented 2 weeks ago

As much as I want to upvote Strict compliance, but this is Break change. Unrealistic :p

some case:

mgerasimchuk commented 23 hours ago

@xr I think it would be better to keep this issue in open state as an improvement reminder for the next major version of the gin

it's extremely disorienting that I must not use the gin.Context and need to use (*gin.Context).Request.Context() instead

PS:\ I faced the similar situation with otel like @pepea23 in https://github.com/gin-gonic/gin/issues/3993, and when I found that my otel.Span just disappeared from the context I was very surprised

PPS:\ Another funny thing that there is no one sample in the gin's documentation with the context usage in a case when we should pass it deeper to other components. I think it should the very first line in the README, for real