refactor context to add a BaseContext and reduce private context struct size

[lunny]

BaseContext will only contain resp, req and data which could be a general context for private request, http and some others.

[codecov-commenter]

Codecov Report

❗ No coverage uploaded for pull request base (main@b59875a). Click here to learn what that means.
The diff coverage is 64.02%.

@@           Coverage Diff           @@
##             main   #16455   +/-   ##
=======================================
  Coverage        ?   45.36%           
=======================================
  Files           ?      816           
  Lines           ?    90718           
  Branches        ?        0           
=======================================
  Hits            ?    41151           
  Misses          ?    42998           
  Partials        ?     6569           

Impacted Files	Coverage Δ
routers/install/install.go	0.57% <0.00%> (ø)
routers/private/hook_proc_receive.go	40.00% <20.00%> (ø)
services/agit/agit.go	56.49% <28.57%> (ø)
modules/context/base.go	63.01% <63.01%> (ø)
routers/private/hook_pre_receive.go	40.28% <63.63%> (ø)
modules/context/api.go	74.29% <100.00%> (ø)
modules/context/context.go	60.67% <100.00%> (ø)
modules/context/form.go	88.46% <100.00%> (ø)
modules/context/private.go	100.00% <100.00%> (ø)
modules/test/context_tests.go	67.69% <100.00%> (ø)
... and 1 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b59875a...f497ec6. Read the comment docs.

[zeripath]

I think I understand the aim but I'm not certain this is getting us going in the right direction.

---

My understanding is that you're trying to reduce the number of things that our context does and just simplify what's there because it's kinda fragile, opaque and smells.

I'm not sure that this is actually getting us much out of the hole of having to pass around this special grab bag of things.

I think in the end with the way go is built we're going to end up having to pass in a go context into most of our functions - and we're going to have to pass in the context as a grab bag. However our current context is bad for two reasons:

1. Everything that gets a modules/context/Context now thinks that everything on it is ready when it's not.
2. Anything we add to the context means that everything depends on it.

Go gets around this dependency problem by using the Value(key interface{}) interface{} and by casting to interfaces.

We could do that by for example instead of using ctx.Resp have instead ctx.Resp() then our context is RespProvider and so on.

---

So my feeling therefore is that we should be trying to get ourselves into a state whereby we pass around gocontext.Contexts and use functions on those to grab things like a Renderer:

package 

type Renderer struct { // Maybe IRenderer (not sure - depends on which package)
  HTML(status int, name base.TplName)
  RenderWithErr(msg string, tpl base.TplName, form interface{})
  NotFound(title string, err error)
  ServerError(title string, err error)
  NotFoundOrServerError(title string, errck func(error) bool, err error)
  Header() http.Header
  HandleText(status int, title string)
  ServeContent(name string, r io.ReadSeeker, params ...interface{})
  PlainText(status int, bs []byte)
  ServeFile(file string, names ...string)
  ServeStream(rd io.Reader, name string)
  Error(status int, contents ...string)
  JSON(status int, content interface{})
  Redirect(location string, status ...int)
}

(Perhaps not all of these - clearly some of these are bit more specialised and could be another thing like a instead.)

I have a number of ideas for how this could go but none of them is completely fleshed out.

For example, to get a Renderer from a go context the function: context.Renderer(gocontext.Context) would check if the context is itself a Renderer, has a Renderer() provider function or if Value(...) can provide a renderer. If not then it could even try to create one itself by getting the ResponseWriter. (Which in turn would do its own searches.)

This could then eventually lead to context becoming:

type BaseContext struct {
	gocontext.Context
	values map[interface{}]interface{}
}

func (b *BaseContext) Value(key interface{}) interface{} {
	if value, ok := b.values[key]; ok {
		return value
	}
	return b.Context.Value(key)
}

func (b *BaseContext) SetValue(key, value interface{}) interface{} {
	if old, ok := b.values[key]; ok {
		b.values[key] = value
		return old
	}
	b.values[key] = value
	return nil
}

var _ (gocontext.Context) = &BaseContext{gocontext.Background(), map[interface{}]interface{}{}}

type SettableContext interface {
  gocontext.Context
  SetValue(key, value interface{}) interface{}
}

func WithValue(ctx gocontext.Context, key, value interface{}) (ret gocontext.Context, cancel gocontext.CancelFunc) {
  if base, ok := ctx.(SettableContext); ok {
     base.SetValue(key, value)
     ret = base
     return
  }
  return gocontext.WithValue(ctx, key, value)
}

So things would then get stored on the BaseContext.

An alternative is potentially something like:

type RendererContext struct {
  gocontext.Context
  Renderer
}

type SessionedContext struct {
  gocontext.Context
  Sessioned
}

so functionality could be chained on to the context but then you're in the realm of having to more clearly handle the chain of context creations although this technique allows you to assert types at compile time. (The above interfaces could eventually be asserted at package level and generators could do the rest.)

---

I guess the question is what is driving and depending on this particular PR? Is there something else depending on this particular way of doing things?

If you've got some other PRs or ideas that are waiting on this particular implementation then that's more of a reason to merge as is because I do agree it's better than what we've currently got - I'm just not sure it's the best thing to do.

[lunny]

Call it Renderer or Context is not different, but Context is a common word from many other Golang web framework. As Renderer seems only a middleware to render page or template from the imagination. To let contributors easily guess what's Gitea's code mean, I would like use some words they are familiar.

And I don't think everything should be interface, sometimes it will make things complicated. I think it should be changed as interface only when it's necessary.

Yes, I plan to refactor Smart HTTP at first if this is merged. Those routes are different from web page routes. They should not keep session/cookie/csrf/renderer and something others, so a base context is OK. This will have a slight performance benefit for Smart HTTP requests of memory. But it will make the code more simple and clear.

[wxiaoguang]

I do not think we have a chance to call ctx.Status again here. Because there is a ctx.Resp.WriteHeader(status) above.

[lunny]

Yes, it's really a problem except we encode the content before write header.

[wxiaoguang]

As above

[stale]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs during the next 2 months. Thank you for your contributions.

[lunny]

Closed as so many files conflicted.