Patch lazy router error response (#11)

* fix inconsistency in lazy router * fix wrong error code in test * update error code
gojek · Mar 14, 2023 · e09582a · e09582a
1 parent 61a0e61
commit e09582a
Show file tree

Hide file tree

Showing 10 changed files with 28 additions and 23 deletions.
diff --git a/eager_router.go b/eager_router.go
@@ -147,7 +147,7 @@ func (fanIn *eagerRouterFanIn) Aggregate(
 					if len(routes) == 0 {
 						masterResponse = NewErrorResponse(errors.ErrRouterStrategyReturnedEmptyRoutes(req.Protocol()))
 					} else {
-						masterResponse = NewErrorResponse(errors.ErrServiceUnavailable(req.Protocol()))
+						masterResponse = NewErrorResponse(errors.ErrNoValidResponseFromRoutes(req.Protocol()))
 					}
 				}
 			}

diff --git a/eager_router_test.go b/eager_router_test.go
@@ -59,7 +59,7 @@ func TestEagerRouter_Dispatch(t *testing.T) {
 			name: "primary route failed, receiving from fallback",
 			responses: map[string][]testUtilsHttp.DelayedResponse{
 				"route-a": {
-					testUtilsHttp.DelayedResponse{Response: testUtilsHttp.MockResp(500, "A-NOK", nil, fiberErrors.ErrServiceUnavailable(protocol.HTTP))},
+					testUtilsHttp.DelayedResponse{Response: testUtilsHttp.MockResp(502, "A-NOK", nil, fiberErrors.ErrNoValidResponseFromRoutes(protocol.HTTP))},
 				},
 				"route-b": {
 					testUtilsHttp.DelayedResponse{Response: testUtilsHttp.MockResp(200, "B-OK", nil, nil)},
@@ -116,7 +116,7 @@ func TestEagerRouter_Dispatch(t *testing.T) {
 			name: "primary route exceeds timeout, fallback route failed, receiving from the next fallback",
 			responses: map[string][]testUtilsHttp.DelayedResponse{
 				"route-a": {
-					testUtilsHttp.DelayedResponse{Response: testUtilsHttp.MockResp(500, "A-NOK", nil, fiberErrors.ErrServiceUnavailable(protocol.HTTP))},
+					testUtilsHttp.DelayedResponse{Response: testUtilsHttp.MockResp(502, "A-NOK", nil, fiberErrors.ErrNoValidResponseFromRoutes(protocol.HTTP))},
 				},
 				"route-b": {
 					testUtilsHttp.DelayedResponse{
@@ -140,7 +140,7 @@ func TestEagerRouter_Dispatch(t *testing.T) {
 			responses: map[string][]testUtilsHttp.DelayedResponse{
 				"route-a": {
 					testUtilsHttp.DelayedResponse{
-						Response: testUtilsHttp.MockResp(500, "A-NOK", nil, fiberErrors.ErrServiceUnavailable(protocol.HTTP)),
+						Response: testUtilsHttp.MockResp(502, "A-NOK", nil, fiberErrors.ErrNoValidResponseFromRoutes(protocol.HTTP)),
 					},
 				},
 				"route-b": {
@@ -159,14 +159,14 @@ func TestEagerRouter_Dispatch(t *testing.T) {
 				"route-a", "route-b", "route-c",
 			},
 			expected: []fiber.Response{
-				testUtilsHttp.MockResp(500, "", nil, fiberErrors.ErrServiceUnavailable(protocol.HTTP)),
+				testUtilsHttp.MockResp(502, "", nil, fiberErrors.ErrNoValidResponseFromRoutes(protocol.HTTP)),
 			},
 		},
 		{
 			name: "routing strategy response comes after all routes replied",
 			responses: map[string][]testUtilsHttp.DelayedResponse{
 				"route-a": {
-					testUtilsHttp.DelayedResponse{Response: testUtilsHttp.MockResp(500, "A-NOK", nil, fiberErrors.ErrServiceUnavailable(protocol.HTTP))},
+					testUtilsHttp.DelayedResponse{Response: testUtilsHttp.MockResp(502, "A-NOK", nil, fiberErrors.ErrNoValidResponseFromRoutes(protocol.HTTP))},
 				},
 				"route-b": {
 					testUtilsHttp.DelayedResponse{Response: testUtilsHttp.MockResp(200, "B-OK", nil, nil)},

diff --git a/errors/error.go b/errors/error.go
@@ -50,7 +50,7 @@ var (
 	}
 
 	// ErrRouterStrategyReturnedEmptyRoutes is a FiberError that's returned when
-	// the routing strategy routing strategy returns an empty routes list
+	// the routing strategy returns an empty routes list
 	ErrRouterStrategyReturnedEmptyRoutes = func(protocol protocol.Protocol) *FiberError {
 		statusCode := http.StatusNotFound
 		if protocol == "GRPC" {
@@ -62,16 +62,16 @@ var (
 		}
 	}
 
-	// ErrServiceUnavailable is a FiberError that's returned when
+	// ErrNoValidResponseFromRoutes is a FiberError that's returned when
 	// none of the routes in the routing strategy return a valid response
-	ErrServiceUnavailable = func(protocol protocol.Protocol) *FiberError {
-		statusCode := http.StatusServiceUnavailable
+	ErrNoValidResponseFromRoutes = func(protocol protocol.Protocol) *FiberError {
+		statusCode := http.StatusBadGateway
 		if protocol == "GRPC" {
 			statusCode = int(codes.Unavailable)
 		}
 		return &FiberError{
 			Code:    statusCode,
-			Message: "fiber: no responses received",
+			Message: "fiber: no valid responses received from routes",
 		}
 	}
 

diff --git a/extras/fastest_response_fan_in.go b/extras/fastest_response_fan_in.go
@@ -28,7 +28,7 @@ func (r *FastestResponseFanIn) Aggregate(
 				return
 			}
 		}
-		out <- fiber.NewErrorResponse(errors.ErrServiceUnavailable(req.Protocol()))
+		out <- fiber.NewErrorResponse(errors.ErrNoValidResponseFromRoutes(req.Protocol()))
 	}()
 	return <-out
 }
diff --git a/fan_out_test.go b/fan_out_test.go
@@ -83,7 +83,7 @@ func TestFanOut_Dispatch(t *testing.T) {
 			name: "one route/one NOK response",
 			responses: map[string][]testUtilsHttp.DelayedResponse{
 				"route-a": {
-					testUtilsHttp.DelayedResponse{Response: testUtilsHttp.MockResp(503, "", nil, errors.ErrServiceUnavailable(protocol.HTTP))},
+					testUtilsHttp.DelayedResponse{Response: testUtilsHttp.MockResp(502, "", nil, errors.ErrNoValidResponseFromRoutes(protocol.HTTP))},
 				},
 			},
 		},

diff --git a/http/handler.go b/http/handler.go
@@ -56,7 +56,7 @@ func (h *Handler) DoRequest(httpReq *http.Request) (fiber.Response, *fiberErrors
 			if ok {
 				return resp, nil
 			}
-			return nil, fiberErrors.ErrServiceUnavailable(protocol.HTTP)
+			return nil, fiberErrors.ErrNoValidResponseFromRoutes(protocol.HTTP)
 		case <-time.After(h.options.Timeout):
 			return nil, fiberErrors.ErrRequestTimeout(protocol.HTTP)
 		}

diff --git a/http/handler_test.go b/http/handler_test.go
@@ -92,12 +92,12 @@ func TestHandler_ServeHTTP(t *testing.T) {
 			request:   newHTTPRequest("POST", "localhost:8080/handler", http.NoBody),
 			responses: []testUtilsHttp.DelayedResponse{},
 			expected: &http.Response{
-				StatusCode: http.StatusServiceUnavailable,
+				StatusCode: http.StatusBadGateway,
 				Header:     http.Header{},
 				Body: makeBody([]byte(
 					`{
-  "code": 503,
-  "error": "fiber: no responses received"
+  "code": 502,
+  "error": "fiber: no valid responses received from routes"
 }`)),
 			},
 			timeout: 100 * time.Millisecond,

diff --git a/integration_test/integration_test.go b/integration_test/integration_test.go
@@ -255,7 +255,7 @@ func TestE2EFromConfig(t *testing.T) {
 				Status: *status.New(codes.Unavailable, ""),
 			},
 			expectedFiberErr: fiber.
-				NewErrorResponse(fiberError.ErrServiceUnavailable(protocol.GRPC)).
+				NewErrorResponse(fiberError.ErrNoValidResponseFromRoutes(protocol.GRPC)).
 				WithLabel("order", []string{route3}...),
 		},
 		{
@@ -264,7 +264,7 @@ func TestE2EFromConfig(t *testing.T) {
 			routesOrder: []string{route3},
 			request:     httpRequest,
 			expectedFiberErr: fiber.
-				NewErrorResponse(fiberError.ErrServiceUnavailable(protocol.HTTP)).
+				NewErrorResponse(fiberError.ErrNoValidResponseFromRoutes(protocol.HTTP)).
 				WithLabel("order", []string{route3}...),
 		},
 	}

diff --git a/lazy_router.go b/lazy_router.go
@@ -2,6 +2,7 @@ package fiber
 
 import (
 	"context"
+
 	"github.com/gojek/fiber/errors"
 	"github.com/gojek/fiber/util"
 )
@@ -108,8 +109,12 @@ func (r *LazyRouter) Dispatch(ctx context.Context, req Request) ResponseQueue {
 					}
 				}
 			}
+			// if there are no valid response from all routes
+			out <- NewErrorResponse(errors.ErrNoValidResponseFromRoutes(req.Protocol()))
+		} else {
+			// if no routes returned from strategy, return error
+			out <- NewErrorResponse(errors.ErrRouterStrategyReturnedEmptyRoutes(req.Protocol())).WithLabels(labels)
 		}
-		out <- NewErrorResponse(errors.ErrRouterStrategyReturnedEmptyRoutes(req.Protocol())).WithLabels(labels)
 	}()
 
 	return queue

diff --git a/lazy_router_test.go b/lazy_router_test.go
@@ -50,7 +50,7 @@ func TestLazyRouter_Dispatch(t *testing.T) {
 			routes: map[string]fiber.Component{
 				"route-a": testutils.NewMockComponent(
 					"route-a",
-					testUtilsHttp.DelayedResponse{Response: testUtilsHttp.MockResp(500, "A-NOK", nil, fiberErrors.ErrServiceUnavailable(protocol.HTTP))}),
+					testUtilsHttp.DelayedResponse{Response: testUtilsHttp.MockResp(502, "A-NOK", nil, fiberErrors.ErrNoValidResponseFromRoutes(protocol.HTTP))}),
 				"route-b": testutils.NewMockComponent(
 					"route-b",
 					testUtilsHttp.DelayedResponse{Response: testUtilsHttp.MockResp(200, "B-OK", nil, nil)}),
@@ -68,7 +68,7 @@ func TestLazyRouter_Dispatch(t *testing.T) {
 			routes: map[string]fiber.Component{
 				"route-a": testutils.NewMockComponent(
 					"route-a",
-					testUtilsHttp.DelayedResponse{Response: testUtilsHttp.MockResp(500, "A-NOK", nil, fiberErrors.ErrServiceUnavailable(protocol.HTTP))}),
+					testUtilsHttp.DelayedResponse{Response: testUtilsHttp.MockResp(502, "A-NOK", nil, fiberErrors.ErrNoValidResponseFromRoutes(protocol.HTTP))}),
 				"route-b": testutils.NewMockComponent(
 					"route-b",
 					testUtilsHttp.DelayedResponse{Response: testUtilsHttp.MockResp(500, "B-NOK", nil, nil)}),
@@ -77,7 +77,7 @@ func TestLazyRouter_Dispatch(t *testing.T) {
 				"route-a", "route-b",
 			},
 			expected: []fiber.Response{
-				testUtilsHttp.MockResp(501, "", nil, fiberErrors.ErrRouterStrategyReturnedEmptyRoutes(protocol.HTTP)),
+				testUtilsHttp.MockResp(502, "", nil, fiberErrors.ErrNoValidResponseFromRoutes(protocol.HTTP)),
 			},
 			timeout: 100 * time.Millisecond,
 		},