Improve room hiearchy error handling + comments

clientapi/routing/room_hierarchy.go

@@ -26,6 +26,7 @@ import (
 	"github.com/matrix-org/gomatrixserverlib/fclient"
 	"github.com/matrix-org/gomatrixserverlib/spec"
 	"github.com/matrix-org/util"
+	log "github.com/sirupsen/logrus"
 )
 
 type RoomHierarchyPaginationCache struct {
@@ -54,6 +55,9 @@ func (c *RoomHierarchyPaginationCache) AddLine(line roomserverAPI.CachedRoomHier
 	return token
 }
 
+// Query the hierarchy of a room/space
+//
+// Implements /_matrix/client/v1/rooms/{roomID}/hierarchy
 func QueryRoomHierarchy(req *http.Request, device *userapi.Device, roomIDStr string, rsAPI roomserverAPI.ClientRoomserverAPI, paginationCache *RoomHierarchyPaginationCache) util.JSONResponse {
 	parsedRoomID, err := spec.NewRoomID(roomIDStr)
 	if err != nil {
@@ -127,7 +131,19 @@ func QueryRoomHierarchy(req *http.Request, device *userapi.Device, roomIDStr str
 	discoveredRooms, err := walker.NextPage(limit)
 
 	if err != nil {
-		// TODO
+		switch err.(type) {
+		case roomserverAPI.ErrRoomUnknownOrNotAllowed:
+			return util.JSONResponse{
+				Code: http.StatusForbidden,
+				JSON: spec.Forbidden("room is unknown/forbidden"),
+			}
+		default:
+			log.WithError(err).Errorf("failed to fetch next page of room hierarchy (CS API)")
+			return util.JSONResponse{
+				Code: http.StatusInternalServerError,
+				JSON: spec.Unknown("internal server error"),
+			}
+		}
 	}
 
 	nextBatch := ""

clientapi/routing/routing.go

@@ -508,6 +508,7 @@ func Setup(
 	).Methods(http.MethodPut, http.MethodOptions)
 
 	// Defined outside of handler to persist between calls
+	// TODO: clear based on some criteria
 	roomHierarchyPaginationCache := new(RoomHierarchyPaginationCache)
 	v1mux.Handle("/rooms/{roomID}/hierarchy",
 		httputil.MakeAuthAPI("spaces", userAPI, func(req *http.Request, device *userapi.Device) util.JSONResponse {

federationapi/routing/query.go

@@ -27,6 +27,7 @@ import (
 	"github.com/matrix-org/gomatrixserverlib/fclient"
 	"github.com/matrix-org/gomatrixserverlib/spec"
 	"github.com/matrix-org/util"
+	log "github.com/sirupsen/logrus"
 )
 
 // RoomAliasToID converts the queried alias into a room ID and returns it
@@ -118,6 +119,9 @@ func RoomAliasToID(
 	}
 }
 
+// Query the immediate children of a room/space
+//
+// Implements /_matrix/federation/v1/hierarchy/{roomID}
 func QueryRoomHierarchy(httpReq *http.Request, request *fclient.FederationRequest, roomIDStr string, rsAPI roomserverAPI.FederationRoomserverAPI) util.JSONResponse {
 	parsedRoomID, err := spec.NewRoomID(roomIDStr)
 	if err != nil {
@@ -146,7 +150,19 @@ func QueryRoomHierarchy(httpReq *http.Request, request *fclient.FederationReques
 	discoveredRooms, err := walker.NextPage(-1)
 
 	if err != nil {
-		// TODO
+		switch err.(type) {
+		case roomserverAPI.ErrRoomUnknownOrNotAllowed:
+			return util.JSONResponse{
+				Code: http.StatusNotFound,
+				JSON: spec.NotFound("room is unknown/forbidden"),
+			}
+		default:
+			log.WithError(err).Errorf("failed to fetch next page of room hierarchy (SS API)")
+			return util.JSONResponse{
+				Code: http.StatusInternalServerError,
+				JSON: spec.Unknown("internal server error"),
+			}
+		}
 	}
 
 	if len(discoveredRooms) == 0 {

roomserver/api/api.go

@@ -34,6 +34,17 @@ func (e ErrNotAllowed) Error() string {
 	return e.Err.Error()
 }
 
+// ErrRoomUnknownOrNotAllowed is an error return if either the provided
+// room ID does not exist, or points to a room that the requester does
+// not have access to.
+type ErrRoomUnknownOrNotAllowed struct {
+	Err error
+}
+
+func (e ErrRoomUnknownOrNotAllowed) Error() string {
+	return e.Err.Error()
+}
+
 type RestrictedJoinAPI interface {
 	CurrentStateEvent(ctx context.Context, roomID spec.RoomID, eventType string, stateKey string) (gomatrixserverlib.PDU, error)
 	InvitePending(ctx context.Context, roomID spec.RoomID, senderID spec.SenderID) (bool, error)

roomserver/api/query.go

@@ -512,9 +512,17 @@ type QueryRoomHierarchyRequest struct {
 	From          int  `json:"json"`
 }
 
+// An iterator-like interface for walking a room/space hierarchy, returning each rooms information.
+//
+// Used for implementing space summaries / room hierarchies
 type RoomHierarchyWalker interface {
+	// Walk the room hierarchy to retrieve room information until either
+	// no room left, or provided limit reached. If limit provided is -1, then this is
+	// treated as no limit.
 	NextPage(limit int) ([]fclient.MSC2946Room, error)
+	// Returns true if there are no more rooms left to walk
 	Done() bool
+	// Returns a stripped down version of the hiearchy walker suitable for pagination caching
 	GetCached() CachedRoomHierarchyWalker
 }
 

roomserver/internal/query/query_room_hierarchy.go

@@ -17,6 +17,7 @@ package query
 import (
 	"context"
 	"encoding/json"
+	"fmt"
 	"sort"
 	"strings"
 
@@ -92,9 +93,12 @@ const (
 	ConstSpaceParentEventType         = "m.space.parent"
 )
 
+// Walk the room hierarchy to retrieve room information until either
+// no room left, or provided limit reached. If limit provided is -1, then this is
+// treated as no limit.
 func (w *RoomHierarchyWalker) NextPage(limit int) ([]fclient.MSC2946Room, error) {
 	if authorised, _ := w.authorised(w.rootRoomID, ""); !authorised {
-		return nil, spec.Forbidden("room is unknown/forbidden")
+		return nil, roomserver.ErrRoomUnknownOrNotAllowed{Err: fmt.Errorf("room is unknown/forbidden")}
 	}
 
 	var discoveredRooms []fclient.MSC2946Room
@@ -277,7 +281,7 @@ func (w *RoomHierarchyWalker) federatedRoomInfo(roomID string, vias []string) *f
 		}
 		res, err := w.fsAPI.RoomHierarchies(ctx, w.thisServer, spec.ServerName(serverName), roomID, w.suggestedOnly)
 		if err != nil {
-			util.GetLogger(w.ctx).WithError(err).Warnf("failed to call MSC2946Spaces on server %s", serverName)
+			util.GetLogger(w.ctx).WithError(err).Warnf("failed to call RoomHierarchies on server %s", serverName)
 			continue
 		}
 		// ensure nil slices are empty as we send this to the client sometimes