internal/server/handlers.go

@@ -675,10 +675,7 @@ func (s *Server) handleIPInfo() http.HandlerFunc {
 		}
 
 		ctx := r.Context()
-		response := IPLookupResponse{
-			Query:   target,
-			Results: make([]*database.IPInfo, 0),
-		}
+		response := IPLookupResponse{Query: target, Results: make([]*database.IPInfo, 0)}
 
 		// Collect all IPs to look up
 		ipsToLookup, resolveErrors := s.resolveLookupTargets(ctx, target)
@@ -708,11 +705,16 @@ func (s *Server) handleIPInfo() http.HandlerFunc {
 				return
 			}
 
-			if err != nil {
-				response.Errors = append(response.Errors,
-					ip+": "+s.lookupErrorMessage(ip, err))
+			switch {
+			case errors.Is(err, database.ErrNoRoute), errors.Is(err, database.ErrInvalidIP):
+				response.Errors = append(response.Errors, ip+": "+err.Error())
 
 				continue
+			case err != nil:
+				s.logger.Error("Failed to look up IP", "ip", ip, "error", err)
+				s.writeJSONError(w, http.StatusInternalServerError, internalErrorMessage)
+
+				return
 			}
 
 			// Do PTR lookup for this IP
@@ -751,19 +753,6 @@ func (s *Server) handleIPInfo() http.HandlerFunc {
 	}
 }
 
-// lookupErrorMessage returns what an IP lookup answer says about err, the
-// failed lookup of ip: err's own text when ip is not a valid address or has no
-// route, otherwise a fixed message, with err in the log.
-func (s *Server) lookupErrorMessage(ip string, err error) string {
-	if errors.Is(err, database.ErrNoRoute) || errors.Is(err, database.ErrInvalidIP) {
-		return err.Error()
-	}
-
-	s.logger.Error("Failed to look up IP", "ip", ip, "error", err)
-
-	return internalErrorMessage
-}
-
 // lookupTarget returns what an IP lookup request asks for: the ip or addr URL
 // parameter, or else the client's own address.
 func lookupTarget(r *http.Request) string {

internal/server/handlers_internal_test.go

@@ -806,8 +806,7 @@ func (failingDB) GetPrefixDetailsContext(
 // the health check with a store whose reads fail with the driver's error text,
 // and a prefix that does not parse, and checks that no answer carries the
 // driver's or the prefix parser's text. Before the fix the answers sent it to
-// the client (https://git.eeqj.de/sneak/routewatch/issues/108). The IP lookup
-// answers 404, as it does whenever no address could be looked up.
+// the client (https://git.eeqj.de/sneak/routewatch/issues/108).
 func TestJSONRoutesDoNotSendErrorDetails(t *testing.T) {
 	t.Parallel()
 
@@ -823,7 +822,7 @@ func TestJSONRoutesDoNotSendErrorDetails(t *testing.T) {
 		{statsPath, http.StatusInternalServerError},
 		{"/api/v1/as/64500", http.StatusInternalServerError},
 		{"/api/v1/prefix/192.0.2.0/24", http.StatusInternalServerError},
-		{"/api/v1/ip/192.0.2.1", http.StatusNotFound},
+		{"/api/v1/ip/192.0.2.1", http.StatusInternalServerError},
 		{"/api/v1/prefix/foo/24", http.StatusBadRequest},
 	}
 
@@ -846,6 +845,78 @@ func TestJSONRoutesDoNotSendErrorDetails(t *testing.T) {
 	}
 }
 
+// The addresses failingIPLookupDB fails the read of and finds a route for.
+const (
+	failingLookupIP = "192.0.2.1"
+	routedLookupIP  = "192.0.2.2"
+)
+
+// failingIPLookupDB embeds database.Store (left nil) and answers the IP lookup
+// with errDiskIO for failingLookupIP, a route for routedLookupIP, and no route
+// for any other address. Every other Store method is unused and would panic if
+// called.
+type failingIPLookupDB struct {
+	database.Store
+}
+
+func (failingIPLookupDB) GetIPInfoContext(
+	_ context.Context, ip string,
+) (*database.IPInfo, error) {
+	switch ip {
+	case failingLookupIP:
+		return nil, errDiskIO
+	case routedLookupIP:
+		return &database.IPInfo{IP: ip}, nil
+	default:
+		return nil, database.ErrNoRoute
+	}
+}
+
+// TestIPLookupAnswers500WhenDatabaseReadFails sends IP lookups through the
+// server's router with a store whose read of one address fails with the
+// driver's error. A lookup that asks for that address answers 500 with the
+// JSON error body, as the AS and prefix JSON routes do; an address with no
+// route still answers 404. Before the fix the failed read was reported as if
+// the address had no route: 404 for that address alone, and 200 beside a
+// routed address (https://git.eeqj.de/sneak/routewatch/issues/109).
+func TestIPLookupAnswers500WhenDatabaseReadFails(t *testing.T) {
+	t.Parallel()
+
+	s := New(failingIPLookupDB{}, streamer.New(logger.New(), metrics.New()),
+		logger.New(), &config.Config{})
+
+	tests := []struct {
+		path     string
+		wantCode int
+	}{
+		{
+			path:     "/api/v1/ip/" + failingLookupIP,
+			wantCode: http.StatusInternalServerError,
+		},
+		{
+			path:     "/api/v1/ip/" + failingLookupIP + "," + routedLookupIP,
+			wantCode: http.StatusInternalServerError,
+		},
+		{
+			path:     "/api/v1/ip/203.0.113.1",
+			wantCode: http.StatusNotFound,
+		},
+	}
+
+	for _, tt := range tests {
+		rec := httptest.NewRecorder()
+		s.router.ServeHTTP(rec, httptest.NewRequestWithContext(
+			t.Context(), http.MethodGet, tt.path, nil))
+
+		if rec.Code != tt.wantCode {
+			t.Errorf("%s: status %d, want %d; body %s",
+				tt.path, rec.Code, tt.wantCode, rec.Body)
+		}
+
+		checkJSONErrorCode(t, tt.path, rec, tt.wantCode)
+	}
+}
+
 // settledGoroutineCount lets transient goroutines finish, then reports the
 // current count.
 func settledGoroutineCount() int {