clean up comments

clientapi/routing/sendevent.go

@@ -93,9 +93,7 @@ func SendEvent(
 		}
 	}
 
-	// Only enable translation logic on pseudo ID rooms
+	// Translate user ID state keys to room keys in pseudo ID rooms
-	// TODO: remove the if check, synctypes.FromClientStateKey handles non-pseudo ID rooms,
-	//       but has some logic to be worked out (see comment in synctypes.FromClientStateKey for details)
 	if roomVersion == gomatrixserverlib.RoomVersionPseudoIDs && stateKey != nil {
 		parsedRoomID, innerErr := spec.NewRoomID(roomID)
 		if innerErr != nil {
@@ -105,10 +103,11 @@ func SendEvent(
 			}
 		}
 
-		newStateKey, _, innerErr := synctypes.FromClientStateKey(*parsedRoomID, *stateKey, func(roomID spec.RoomID, userID spec.UserID) (*spec.SenderID, error) {
+		newStateKey, innerErr := synctypes.FromClientStateKey(*parsedRoomID, *stateKey, func(roomID spec.RoomID, userID spec.UserID) (*spec.SenderID, error) {
 			return rsAPI.QuerySenderIDForUser(req.Context(), roomID, userID)
 		})
 		if innerErr != nil {
+			// TODO: work out better logic for failure cases (e.g. sender ID not found)
 			util.GetLogger(req.Context()).WithError(innerErr).Error("synctypes.FromClientStateKey failed")
 			return util.JSONResponse{
 				Code: http.StatusInternalServerError,

clientapi/routing/state.go

@@ -210,9 +210,6 @@ func OnIncomingStateRequest(ctx context.Context, device *userapi.Device, rsAPI a
 // state to see if there is an event with that type and state key, if there
 // is then (by default) we return the content, otherwise a 404.
 // If eventFormat=true, sends the whole event else just the content.
-//
-// TODO: remove this nolint when inner TODO resolved
-// nolint:gocyclo
 func OnIncomingStateTypeRequest(
 	ctx context.Context, device *userapi.Device, rsAPI api.ClientRoomserverAPI,
 	roomID, evType, stateKey string, eventFormat bool,
@@ -228,9 +225,7 @@ func OnIncomingStateTypeRequest(
 		}
 	}
 
-	// Only enable translation logic on pseudo ID rooms
+	// Translate user ID state keys to room keys in pseudo ID rooms
-	// TODO: remove the if check, synctypes.FromClientStateKey handles non-pseudo ID rooms,
-	//       but has some logic to be worked out (see comment in synctypes.FromClientStateKey for details)
 	if roomVer == gomatrixserverlib.RoomVersionPseudoIDs {
 		parsedRoomID, err := spec.NewRoomID(roomID)
 		if err != nil {
@@ -239,26 +234,17 @@ func OnIncomingStateTypeRequest(
 				JSON: spec.InvalidParam("invalid room ID"),
 			}
 		}
-
+		newStateKey, err := synctypes.FromClientStateKey(*parsedRoomID, stateKey, func(roomID spec.RoomID, userID spec.UserID) (*spec.SenderID, error) {
-		// Handle user ID state keys appropriately
-		newStateKey, invalidUserIDOrNoSender, err := synctypes.FromClientStateKey(*parsedRoomID, stateKey, func(roomID spec.RoomID, userID spec.UserID) (*spec.SenderID, error) {
 			return rsAPI.QuerySenderIDForUser(ctx, roomID, userID)
 		})
 		if err != nil {
-			if invalidUserIDOrNoSender {
+			// TODO: work out better logic for failure cases (e.g. sender ID not found)
-				// Currently treat this as no state found - see comment for FromClientStateKey
-				return util.JSONResponse{
-					Code: http.StatusNotFound,
-					JSON: spec.NotFound(fmt.Sprintf("Cannot find state event for %q", evType)),
-				}
-			} else {
 			util.GetLogger(ctx).WithError(err).Error("synctypes.FromClientStateKey failed")
 			return util.JSONResponse{
 				Code: http.StatusInternalServerError,
 				JSON: spec.Unknown("internal server error"),
 			}
 		}
-		}
 		stateKey = *newStateKey
 	}
 

syncapi/synctypes/clientevent.go

@@ -124,40 +124,27 @@ func ToClientEventDefault(userIDQuery spec.UserIDForSender, event gomatrixserver
 // If provided state key is a user ID (state keys beginning with @ are reserved for this purpose)
 // fetch it's associated sender ID and use that instead. Otherwise returns the same state key back.
 //
-// This function either returns the state key that should be used, or a JSON error response that should be returned to the user.
+// # This function either returns the state key that should be used, or an error
-// A boolean is also returned, which, if true, means the err is a result of either:
-//   - State key begins with @, but does not contain a valid user ID.
-//   - State key contains a valid user ID, but this user ID does not have a sender ID.
 //
-// TODO: it's currently unclear how translation logic should behave in the above two cases - two options for each case:
+// TODO: handle failure cases better (e.g. no sender ID)
-//   - Starts with @ but invalid user ID:
+func FromClientStateKey(roomID spec.RoomID, stateKey string, senderIDQuery spec.SenderIDForUser) (*string, error) {
-//     -- Reject request (e.g. as 404), but people may have state keys, that, against the spec,
-//     begin with @ and dont contain a valid user ID, which they would be unable to interact with.
-//     -- Silently ignore, and let them use the invalid user ID without any translation attempt. This will probably work
-//     but could cause issues down the line with user ID grammar.
-//   - Valid user ID, but does not have a sender ID
-//     -- Reject reuquest (e.g. as 404), but people may wish to set a state event with a key containing
-//     a user ID for someone who has not yet joined the room - this prevents that.
-//     -- Silently ignore and don't translate - could cause issues where a state event is set with a user ID, then that user joins
-//     and now querying that same state key returns different state event (as the user now has a pseudo ID)
-func FromClientStateKey(roomID spec.RoomID, stateKey string, senderIDQuery spec.SenderIDForUser) (*string, bool, error) {
 	if len(stateKey) >= 1 && stateKey[0] == '@' {
 		parsedStateKey, err := spec.NewUserID(stateKey, true)
 		if err != nil {
 			// If invalid user ID, then there is no associated state event.
-			return nil, true, fmt.Errorf("Provided state key begins with @ but is not a valid user ID: %s", err.Error())
+			return nil, fmt.Errorf("Provided state key begins with @ but is not a valid user ID: %s", err.Error())
 		}
 		senderID, err := senderIDQuery(roomID, *parsedStateKey)
 		if err != nil {
-			return nil, false, fmt.Errorf("Failed to query sender ID: %s", err.Error())
+			return nil, fmt.Errorf("Failed to query sender ID: %s", err.Error())
 		}
 		if senderID == nil {
 			// If no sender ID, then there is no associated state event.
-			return nil, true, fmt.Errorf("No associated sender ID found.")
+			return nil, fmt.Errorf("No associated sender ID found.")
 		}
 		newStateKey := string(*senderID)
-		return &newStateKey, false, nil
+		return &newStateKey, nil
 	} else {
-		return &stateKey, false, nil
+		return &stateKey, nil
 	}
 }