echo.Context should embed net/context

[asdine]

I believe netContext.Context should be embedded in echo.Context instead of being a field, and context.Value() should call context.Set().

I think it would be much more interesting if echo.Context were compatible with context.Context :

• Mocking context would be much much simpler (instead of having to implement our own 47 method Context Interface)
• If needed we could create functions that receive a context.Context and pass them either echo.Context for real code, either a context.Context when testing. Again, super easy to test.
• Decoupling Echo from the rest of the application that doesn't necessarily need to be bound to a specific framework.

[vishr]

@asdine I am looking into this suggestion and I need some more inputs. Here is what we have today:

• Echo.Context interface already embeds net/context.Context so you can easily pass it on to any function accepting net/context.Context
• With Context#SetNetContext you can load your context.
• Mocking Echo.Context is already super easy https://echo.labstack.com/guide/testing

Please provide some code snippet to get a better understanding of the proposal.

As I am closing v2 issues and headed to cut v2 release very soon 👍, your timely response/feedback will be appreciated.

/cc @upamune @Taik @ipfans @dimiro1 @jmunson @lugorian @DenisNeustroev @bryanl @CaptainCodeman @mtojek

[CaptainCodeman]

Check out Go 1.7 - looks like there are changes coming to the core language and Context.

https://tip.golang.org/doc/go1.7#context

(being added to Request)

[asdine]

The thing is it is not embedded here, it is a field in the echoContext type that is the actual type passed to each request.
The problem that is see is that we have two stores: echoContext.store which is a map[string]interface{} and the echoContext.context which is a context.Context. Both have the same purpose, one is custom (which is fine) and the other is about to be standard and is made to be interoperable with other libraries.

I think what would be nice is the following behaviour:

c.Set("a", "b")

func (ctx context.Context) {
  fmt.Println(ctx.Value("a").(string))
  // "b"
}(c)

fmt.Println(c.Get("a") == c.Value("a"))
// true

Setwould update the internal embedded context.Context. and Getwould call context.Value, no need to use the map[string]interface{} internally.

echo.Contextwould be a context.Context with superpowers

[vishr]

@asdine Can you provide signatures for Context.Set and Context.Get? I am a bit confused.

I think with this proposal we are limiting the context to be of type value? The current implementation allows you to use different implementation of context.Context.

[vishr]

How about this or may be this is what you meant?

Context#Set(ctx context.Context) - to set context
Context#Get() context.Context - to get context

This way you can set context.Context of any type.
To use it as store, get the context and call Value on it.

Edit: actually you will directly call Value on Echo context.

[asdine]

The idea is not to change the signature of echo.Context#Get and echo.Context#Set, the current signatures of these functions are great.

What i propose is to change what is inside of echo.echoContext:

• echo.echoContext#context: embed context.Context instead of having a field
• echo.echoContext#Set: change the body with something like this:

c.context = context.WithValue(c.context, key, val)

• echo.echoContext#Get: change the body with something like this:

return c.context.Value(key)

[asdine]

With this, if we pass echo.Context to a function that needs a context.Context it will work

package service

const Key = "myService"

...

func FromContext(ctx context.Context) *Service {
  return ctx.Value(key).(*Service)
}

func ToContext(ctx context.Context, s *Service) context.Context {
  return context.WithValue(ctx, key, s)
}

package main

...

func myEndpoint(c echo.Context) error {
  // we can set values as the usual
  c.Set("a", "b")
  c.Set("c", 100.0)

  // we can also set via the context
  c.SetContext(c.WithValue(ctx, "d", 150))
  c.SetContext(service.ToContext(c, service.New()))

  // we can get values as the usual
  a := c.Get("a").(string)
  a = c.Value("a").(string)
  b := c.Get("c").(float64)
  d := c.Get("d").(int)
  d = c.Value("d")
  srvc := c.Get(service.Key).(*service.Service)
  srvc = service.FromContext(c)
  ...
}

EDIT: better example

[asdine]

For the users, echo.Context#Get and echo.Context#Set don't change. They work the same as before.

The difference is that now it uses context.Context internally and the values stored in the context are available when using echo.Context as a context.Context.

[vishr]

Looks cool.

[asdine]

I can make a PR this week if you want

[vishr]

I just did. Please verify if it needs any adjustments.

[asdine]

It's perfect

[arodland]

Pretty sure this breaks code that calls Get without previously calling Set or otherwise setting a context, as c.Context may be nil at that point, resulting in a panic. Before, Get would just return a nil interface. This is a problem in code that doesn't explicitly use contexts, _maybe_ calls Set, and then calls Get later to retrieve a value that might or might not be there.

[vishr]

We just need to modify Context#Reset to

func (c *echoContext) Reset(req engine.Request, res engine.Response) {
    c.Context = context.Background()
    c.request = req
    c.response = res
    c.handler = notFoundHandler
}