[suggestion] Make ProxyBalancer interface Next() method return error as well

[Hristov821]

Use case:

Currently I need to forward request from my server to another ones. Unfortunatly I can not know in advance their urls (as suggested in documentation) as I need to make some calculations based on provided path and other rules.

What I did:

I created custom struct that satisfies ProxyBalancer interface

It works great but if I encounter error during the selection of next ProxyTarget it can't report the error and I need to panic, thus I would like to change the ProxyBalancer interface from:

ProxyBalancer interface {
	AddTarget(*ProxyTarget) bool
	RemoveTarget(string) bool
	Next(echo.Context) *ProxyTarget
}

to

ProxyBalancer interface {
	AddTarget(*ProxyTarget) bool
	RemoveTarget(string) bool
	Next(echo.Context) (*ProxyTarget, error)
}

The interface is used only in proxy.go, so the change IMO wont hurt.

If the change makes sense I could provide the necessary changes in PR.

Thanks in advance for your time.

[aldas]

We can not change interfaces as it would be breaking change.

I suggest ugly workaround. We introduce new ProxyConfig field TargetHandlerFunc(c echo.Context, target *ProxyTarget) error which by default sets target into context as this line does

echo/middleware/proxy.go

Line 235 in 8f2bf82

c.Set(config.ContextKey, tgt)

but you can configure your own and have specific logic to return error which would stop middleware execution

And for your balancer - return specific ProxyTarget value that contains error in ProxyTarget.Meta map so you can check it from TargetHandler

It is ugly but this does not break backwards compatibility

[aldas]

or create additional interface with single method NextTarget(echo.Context) (*ProxyTarget, error) and when setting up the middleware we can check if provided balancer instance supports that method and if it does - we use it instead when middleware func is called for request.

something like that

var tgt *ProxyTarget
ntBalancer, ok := config.Balancer.(interface {
    NextTarget(echo.Context) (*ProxyTarget, error)
})
if ok {
    tgt, err = ntBalancer.NextTarget(c)
    if err != nil {
        return err
    }
} else {
    tgt = config.Balancer.Next(c)
}

[Hristov821]

We can not change interfaces as it would be breaking change.

I suggest ugly workaround. We introduce new ProxyConfig field TargetHandlerFunc(c echo.Context, target *ProxyTarget) error which by default sets target into context as this line does

echo/middleware/proxy.go

Line 235 in 8f2bf82

c.Set(config.ContextKey, tgt)

but you can configure your own and have specific logic to return error which would stop middleware execution
And for your balancer - return specific ProxyTarget value that contains error in ProxyTarget.Meta map so you can check it from TargetHandler

It is ugly but this does not break backwards compatibility

Thanks for your input, If I understand correctly you suggest to create closure function to the default handler which will set the
target and then the balancer will only return what is already set. I think I need to use the default one because it uses private functions such as proxyHTTP and proxyRaw

[Hristov821]

or create additional interface with single method NextTarget(echo.Context) (*ProxyTarget, error) and when setting up the middleware we can check if provided balancer instance supports that method and if it does - we use it instead when middleware func is called for request.

something like that

var tgt *ProxyTarget
ntBalancer, ok := config.Balancer.(interface {
    NextTarget(echo.Context) (*ProxyTarget, error)
})
if ok {
    tgt, err = ntBalancer.NextTarget(c)
    if err != nil {
        return err
    }
} else {
    tgt = config.Balancer.Next(c)
}

Would that break existing codebases if added to the echo?

[aldas]

ntBalancer, ok := config.Balancer.(interface {
    NextTarget(echo.Context) (*ProxyTarget, error)
})

would not be a breaking change as it does not touch existing interfaces and it is just additional check if we implement (some new anonymous interface) and then act on it.

---

Add something like that to near line 225

	ntBalancer, isImplementingNextTarget := config.Balancer.(interface {
		NextTarget(echo.Context) (*ProxyTarget, error)
	})

and replace line

			tgt := config.Balancer.Next(c)

with

			var tgt *ProxyTarget
			if isImplementingNextTarget {
				tgt, err = ntBalancer.NextTarget(c)
				if err != nil {
					return err
				}
			} else {
				tgt = config.Balancer.Next(c)
			}

NB: I have not tested this code if it really work. but seems to compile

[Hristov821]

Resolved in this