From 67e2c81aa7d0f18fd54459aef605c8fd0caa9f45 Mon Sep 17 00:00:00 2001 From: Sam Wedgwood Date: Wed, 16 Aug 2023 16:38:34 +0100 Subject: [PATCH] clean up comments --- clientapi/routing/sendevent.go | 7 +++---- clientapi/routing/state.go | 28 +++++++--------------------- syncapi/synctypes/clientevent.go | 29 ++++++++--------------------- 3 files changed, 18 insertions(+), 46 deletions(-) diff --git a/clientapi/routing/sendevent.go b/clientapi/routing/sendevent.go index 6aff1c725..f81e9c1e4 100644 --- a/clientapi/routing/sendevent.go +++ b/clientapi/routing/sendevent.go @@ -93,9 +93,7 @@ func SendEvent( } } - // Only enable translation logic on 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) + // Translate user ID state keys to room keys in pseudo ID rooms 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, diff --git a/clientapi/routing/state.go b/clientapi/routing/state.go index 6ca6680f4..7648dc474 100644 --- a/clientapi/routing/state.go +++ b/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 - // 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) + // Translate user ID state keys to room keys in pseudo ID rooms if roomVer == gomatrixserverlib.RoomVersionPseudoIDs { parsedRoomID, err := spec.NewRoomID(roomID) if err != nil { @@ -239,24 +234,15 @@ func OnIncomingStateTypeRequest( JSON: spec.InvalidParam("invalid room ID"), } } - - // Handle user ID state keys appropriately - newStateKey, invalidUserIDOrNoSender, err := synctypes.FromClientStateKey(*parsedRoomID, stateKey, func(roomID spec.RoomID, userID spec.UserID) (*spec.SenderID, error) { + newStateKey, 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 { - // 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"), - } + // TODO: work out better logic for failure cases (e.g. sender ID not found) + util.GetLogger(ctx).WithError(err).Error("synctypes.FromClientStateKey failed") + return util.JSONResponse{ + Code: http.StatusInternalServerError, + JSON: spec.Unknown("internal server error"), } } stateKey = *newStateKey diff --git a/syncapi/synctypes/clientevent.go b/syncapi/synctypes/clientevent.go index e991c4ed4..a78aea1c6 100644 --- a/syncapi/synctypes/clientevent.go +++ b/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. -// 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. +// # This function either returns the state key that should be used, or an error // -// TODO: it's currently unclear how translation logic should behave in the above two cases - two options for each case: -// - Starts with @ but invalid user ID: -// -- 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) { +// TODO: handle failure cases better (e.g. no sender ID) +func FromClientStateKey(roomID spec.RoomID, stateKey string, senderIDQuery spec.SenderIDForUser) (*string, 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 } }