Skip to content

Commit

Permalink
fix: improve route not found error (#2142) (2nd attempt)
Browse files Browse the repository at this point in the history
In practice all route queries are done by domain, hostname and path; so
it's misleading to display an error message that does not contain empty
path and hostname parts.

[#175681744](https://www.pivotaltracker.com/story/show/175681744)
  • Loading branch information
blgm committed Feb 24, 2021
1 parent a354bd1 commit 91c2c94
Show file tree
Hide file tree
Showing 9 changed files with 40 additions and 89 deletions.
2 changes: 1 addition & 1 deletion actor/actionerror/route_bound_to_multiple_apps_error.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ package actionerror

import "fmt"

// RouteNotFoundError is returned when a route cannot be found
// RouteBoundToMultipleAppsError is returned when a route is mapped to more than one app
type RouteBoundToMultipleAppsError struct {
AppName string
RouteURL string
Expand Down
28 changes: 11 additions & 17 deletions actor/actionerror/route_not_found_error.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,29 +5,23 @@ import "fmt"
// RouteNotFoundError is returned when a route cannot be found
type RouteNotFoundError struct {
Host string
DomainGUID string
DomainName string
Path string
Port int
}

func (e RouteNotFoundError) Error() string {
if e.DomainName != "" {
switch {
case e.Host != "" && e.Path != "":
return fmt.Sprintf("Route with host '%s', domain '%s', and path '%s' not found.", e.Host, e.DomainName, e.Path)
case e.Host != "":
return fmt.Sprintf("Route with host '%s' and domain '%s' not found.", e.Host, e.DomainName)
case e.Path != "":
return fmt.Sprintf("Route with domain '%s' and path '%s' not found.", e.DomainName, e.Path)
case e.Port != 0:
return fmt.Sprintf("Route with domain '%s' and port %d not found.", e.DomainName, e.Port)
default:
return fmt.Sprintf("Route with domain '%s' not found.", e.DomainName)
}
switch e.Port {
case 0:
return fmt.Sprintf("Route with host '%s', domain '%s', and path '%s' not found.", e.Host, e.DomainName, e.path())
default:
return fmt.Sprintf("Route with domain '%s' and port %d not found.", e.DomainName, e.Port)
}
if e.Path != "" {
return fmt.Sprintf("Route with host '%s', domain guid '%s', and path '%s' not found.", e.Host, e.DomainGUID, e.Path)
}

func (e RouteNotFoundError) path() string {
if e.Path == "" {
return "/"
}
return fmt.Sprintf("Route with host '%s' and domain guid '%s' not found.", e.Host, e.DomainGUID)
return e.Path
}
81 changes: 21 additions & 60 deletions actor/actionerror/route_not_found_error_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,68 +7,29 @@ import (
)

var _ = Describe("RouteNotFoundError", func() {
It("returns an error message referencing domain, host and path", func() {
err := actionerror.RouteNotFoundError{
DomainName: "some-domain.com",
Path: "mypath",
Host: "hostname",
}
Expect(err).To(MatchError("Route with host 'hostname', domain 'some-domain.com', and path 'mypath' not found."))
})

Describe("Error", func() {
When("DomainName is set", func() {
When("the host and path are specified", func() {
It("returns an error message referencing domain, host and path", func() {
err := actionerror.RouteNotFoundError{
DomainName: "some-domain.com",
Path: "mypath",
Host: "hostname",
}
Expect(err.Error()).To(Equal("Route with host 'hostname', domain 'some-domain.com', and path 'mypath' not found."))
})
})

When("the host is specified", func() {
It("returns an error message referencing domain and host", func() {
err := actionerror.RouteNotFoundError{
DomainName: "some-domain.com",
Host: "hostname",
}
Expect(err.Error()).To(Equal("Route with host 'hostname' and domain 'some-domain.com' not found."))
})
})

When("the path is specified", func() {
It("returns an error message referencing domain and path", func() {
err := actionerror.RouteNotFoundError{
DomainName: "some-domain.com",
Path: "mypath",
}
Expect(err.Error()).To(Equal("Route with domain 'some-domain.com' and path 'mypath' not found."))
})
})

When("the port is specified", func() {
It("returns an error message referencing domain and port", func() {
err := actionerror.RouteNotFoundError{
DomainName: "some-domain.com",
Port: 1052,
}
Expect(err.Error()).To(Equal("Route with domain 'some-domain.com' and port 1052 not found."))
})
})

When("neither host nor path is specified", func() {
It("returns an error message referencing domain", func() {
err := actionerror.RouteNotFoundError{
DomainName: "some-domain.com",
}
Expect(err.Error()).To(Equal("Route with domain 'some-domain.com' not found."))
})
})
When("the hostname and path are empty", func() {
It("return an error with empty hostname and path", func() {
err := actionerror.RouteNotFoundError{DomainName: "some-domain.com"}
Expect(err).To(MatchError("Route with host '', domain 'some-domain.com', and path '/' not found."))
})
When("Domain GUID is set", func() {
It("returns an error message with the GUID of the missing space", func() {
err := actionerror.RouteNotFoundError{
DomainGUID: "some-domain-guid",
Host: "hostname",
Path: "mypath",
}
Expect(err.Error()).To(Equal("Route with host 'hostname', domain guid 'some-domain-guid', and path 'mypath' not found."))
})
})

When("the port is specified", func() {
It("returns an error message referencing domain and port", func() {
err := actionerror.RouteNotFoundError{
DomainName: "some-domain.com",
Port: 1052,
}
Expect(err).To(MatchError("Route with domain 'some-domain.com' and port 1052 not found."))
})
})
})
2 changes: 1 addition & 1 deletion actor/v2action/route.go
Original file line number Diff line number Diff line change
Expand Up @@ -330,7 +330,7 @@ func (actor Actor) GetRouteByComponents(route Route) (Route, Warnings, error) {
if len(ccv2Routes) == 0 {
return Route{}, Warnings(warnings), actionerror.RouteNotFoundError{
Host: route.Host,
DomainGUID: route.Domain.GUID,
DomainName: route.Domain.Name,
Path: route.Path,
Port: route.Port.Value,
}
Expand Down
4 changes: 2 additions & 2 deletions actor/v2action/route_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1645,7 +1645,7 @@ var _ = Describe("Route Actions", func() {
Host: inputRoute.Host,
Path: inputRoute.Path,
Port: inputRoute.Port.Value,
DomainGUID: domain.GUID,
DomainName: domain.Name,
}))
Expect(warnings).To(ConsistOf("get-routes-warning"))
})
Expand Down Expand Up @@ -1794,7 +1794,7 @@ var _ = Describe("Route Actions", func() {
var expectedErr error

BeforeEach(func() {
expectedErr = actionerror.RouteNotFoundError{Host: route.Host, DomainGUID: route.Domain.GUID, Path: route.Path}
expectedErr = actionerror.RouteNotFoundError{Host: route.Host, DomainName: route.Domain.Name, Path: route.Path}
fakeCloudControllerClient.GetRoutesReturns([]ccv2.Route{}, ccv2.Warnings{"get route warning"}, nil)
fakeCloudControllerClient.CheckRouteReturns(false, ccv2.Warnings{"check route warning"}, nil)
})
Expand Down
1 change: 0 additions & 1 deletion actor/v7action/route.go
Original file line number Diff line number Diff line change
Expand Up @@ -360,7 +360,6 @@ func (actor Actor) GetRouteByAttributes(domain resources.Domain, hostname string
if len(routes) < 1 {
return resources.Route{}, Warnings(ccWarnings), actionerror.RouteNotFoundError{
DomainName: domain.Name,
DomainGUID: domain.GUID,
Host: hostname,
Path: path,
Port: port,
Expand Down
3 changes: 0 additions & 3 deletions actor/v7action/route_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1281,7 +1281,6 @@ var _ = Describe("Route Actions", func() {
warnings, err := actor.DeleteRoute("domain.com", "hostname", "/path", 0)
Expect(err).To(Equal(actionerror.RouteNotFoundError{
DomainName: "domain.com",
DomainGUID: "domain-guid",
Host: "hostname",
Path: "/path",
Port: 0,
Expand Down Expand Up @@ -1378,7 +1377,6 @@ var _ = Describe("Route Actions", func() {
Expect(warnings).To(ConsistOf("get-routes-warning"))
Expect(executeErr).To(MatchError(actionerror.RouteNotFoundError{
DomainName: domainName,
DomainGUID: domainGUID,
Host: hostname,
Path: path,
}))
Expand Down Expand Up @@ -1446,7 +1444,6 @@ var _ = Describe("Route Actions", func() {
Expect(warnings).To(ConsistOf("get-routes-warning"))
Expect(executeErr).To(MatchError(actionerror.RouteNotFoundError{
DomainName: domainName,
DomainGUID: domainGUID,
Port: port,
}))
})
Expand Down
4 changes: 2 additions & 2 deletions integration/v7/isolated/delete_route_command_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,7 @@ var _ = Describe("delete-route command", func() {
It("warns the user that it has already been deleted and runs to completion without failing", func() {
session := helpers.CF("delete-route", domainName, "-f")
Eventually(session).Should(Say(`Deleting route %s\.\.\.`, domainName))
Eventually(session).Should(Say(`Unable to delete\. Route with domain '%s' not found\.`, domainName))
Eventually(session).Should(Say(`Unable to delete\. Route with host '', domain '%s', and path '/' not found\.`, domainName))
Eventually(session).Should(Exit(0))
})
})
Expand Down Expand Up @@ -321,7 +321,7 @@ var _ = Describe("delete-route command", func() {
It("fails with a helpful message", func() {
session := helpers.CF("delete-route", domainName, "-f")
Eventually(session).Should(Say(`Deleting route %s\.\.\.`, domainName))
Eventually(session).Should(Say(`Unable to delete\. Route with domain '%s' not found\.`, domainName))
Eventually(session).Should(Say(`Unable to delete\. Route with host '', domain '%s', and path '/' not found\.`, domainName))
Eventually(session).Should(Say(`OK`))
Eventually(session).Should(Exit(0))
})
Expand Down
4 changes: 2 additions & 2 deletions integration/v7/isolated/set_label_command_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -333,8 +333,8 @@ var _ = Describe("set-label command", func() {
It("displays an error", func() {
invalidRoute := "non-existent-host." + domainName
session := helpers.CF("set-label", "route", invalidRoute, "some-key=some-value")
Eventually(session.Err).Should(Say(fmt.Sprintf("Route with host 'non-existent-host' and domain '%s' not found", domainName)))
Eventually(session).Should(Say("FAILED"))
Eventually(session.Err).Should(Say(`Route with host 'non-existent-host', domain '%s', and path '/' not found\.`, domainName))
Eventually(session.Out).Should(Say("FAILED"))
Eventually(session).Should(Exit(1))
})
})
Expand Down

0 comments on commit 91c2c94

Please sign in to comment.