Add user profile tests, refactor user API methods (#3030)

This adds tests for `/profile`.
Also, as a first change in this regard, refactors the methods defined on
the `UserInternalAPI` to not use structs as the request/response
parameters.

clientapi/routing/joinroom.go

@@ -18,6 +18,7 @@ import (
 	"net/http"
 	"time"
 
+	appserviceAPI "github.com/matrix-org/dendrite/appservice/api"
 	"github.com/matrix-org/dendrite/clientapi/httputil"
 	"github.com/matrix-org/dendrite/clientapi/jsonerror"
 	roomserverAPI "github.com/matrix-org/dendrite/roomserver/api"
@@ -61,21 +62,19 @@ func JoinRoomByIDOrAlias(
 	// Work out our localpart for the client profile request.
 
 	// Request our profile content to populate the request content with.
-	res := &api.QueryProfileResponse{}
-	err := profileAPI.QueryProfile(req.Context(), &api.QueryProfileRequest{UserID: device.UserID}, res)
-	if err != nil || !res.UserExists {
-		if !res.UserExists {
-			util.GetLogger(req.Context()).Error("Unable to query user profile, no profile found.")
-			return util.JSONResponse{
-				Code: http.StatusInternalServerError,
-				JSON: jsonerror.Unknown("Unable to query user profile, no profile found."),
-			}
-		}
+	profile, err := profileAPI.QueryProfile(req.Context(), device.UserID)
 
-		util.GetLogger(req.Context()).WithError(err).Error("UserProfileAPI.QueryProfile failed")
-	} else {
-		joinReq.Content["displayname"] = res.DisplayName
-		joinReq.Content["avatar_url"] = res.AvatarURL
+	switch err {
+	case nil:
+		joinReq.Content["displayname"] = profile.DisplayName
+		joinReq.Content["avatar_url"] = profile.AvatarURL
+	case appserviceAPI.ErrProfileNotExists:
+		util.GetLogger(req.Context()).Error("Unable to query user profile, no profile found.")
+		return util.JSONResponse{
+			Code: http.StatusInternalServerError,
+			JSON: jsonerror.Unknown("Unable to query user profile, no profile found."),
+		}
+	default:
 	}
 
 	// Ask the roomserver to perform the join.

clientapi/routing/profile.go

@@ -36,14 +36,14 @@ import (
 
 // GetProfile implements GET /profile/{userID}
 func GetProfile(
-	req *http.Request, profileAPI userapi.ClientUserAPI, cfg *config.ClientAPI,
+	req *http.Request, profileAPI userapi.ProfileAPI, cfg *config.ClientAPI,
 	userID string,
 	asAPI appserviceAPI.AppServiceInternalAPI,
 	federation *gomatrixserverlib.FederationClient,
 ) util.JSONResponse {
 	profile, err := getProfile(req.Context(), profileAPI, cfg, userID, asAPI, federation)
 	if err != nil {
-		if err == eventutil.ErrProfileNoExists {
+		if err == appserviceAPI.ErrProfileNotExists {
 			return util.JSONResponse{
 				Code: http.StatusNotFound,
 				JSON: jsonerror.NotFound("The user does not exist or does not have a profile"),
@@ -56,7 +56,7 @@ func GetProfile(
 
 	return util.JSONResponse{
 		Code: http.StatusOK,
-		JSON: eventutil.ProfileResponse{
+		JSON: eventutil.UserProfile{
 			AvatarURL:   profile.AvatarURL,
 			DisplayName: profile.DisplayName,
 		},
@@ -65,34 +65,28 @@ func GetProfile(
 
 // GetAvatarURL implements GET /profile/{userID}/avatar_url
 func GetAvatarURL(
-	req *http.Request, profileAPI userapi.ClientUserAPI, cfg *config.ClientAPI,
+	req *http.Request, profileAPI userapi.ProfileAPI, cfg *config.ClientAPI,
 	userID string, asAPI appserviceAPI.AppServiceInternalAPI,
 	federation *gomatrixserverlib.FederationClient,
 ) util.JSONResponse {
-	profile, err := getProfile(req.Context(), profileAPI, cfg, userID, asAPI, federation)
-	if err != nil {
-		if err == eventutil.ErrProfileNoExists {
-			return util.JSONResponse{
-				Code: http.StatusNotFound,
-				JSON: jsonerror.NotFound("The user does not exist or does not have a profile"),
-			}
-		}
-
-		util.GetLogger(req.Context()).WithError(err).Error("getProfile failed")
-		return jsonerror.InternalServerError()
+	profile := GetProfile(req, profileAPI, cfg, userID, asAPI, federation)
+	p, ok := profile.JSON.(eventutil.UserProfile)
+	// not a profile response, so most likely an error, return that
+	if !ok {
+		return profile
 	}
 
 	return util.JSONResponse{
 		Code: http.StatusOK,
-		JSON: eventutil.AvatarURL{
-			AvatarURL: profile.AvatarURL,
+		JSON: eventutil.UserProfile{
+			AvatarURL: p.AvatarURL,
 		},
 	}
 }
 
 // SetAvatarURL implements PUT /profile/{userID}/avatar_url
 func SetAvatarURL(
-	req *http.Request, profileAPI userapi.ClientUserAPI,
+	req *http.Request, profileAPI userapi.ProfileAPI,
 	device *userapi.Device, userID string, cfg *config.ClientAPI, rsAPI api.ClientRoomserverAPI,
 ) util.JSONResponse {
 	if userID != device.UserID {
@@ -102,7 +96,7 @@ func SetAvatarURL(
 		}
 	}
 
-	var r eventutil.AvatarURL
+	var r eventutil.UserProfile
 	if resErr := httputil.UnmarshalJSONRequest(req, &r); resErr != nil {
 		return *resErr
 	}
@@ -134,24 +128,20 @@ func SetAvatarURL(
 		}
 	}
 
-	setRes := &userapi.PerformSetAvatarURLResponse{}
-	if err = profileAPI.SetAvatarURL(req.Context(), &userapi.PerformSetAvatarURLRequest{
-		Localpart:  localpart,
-		ServerName: domain,
-		AvatarURL:  r.AvatarURL,
-	}, setRes); err != nil {
+	profile, changed, err := profileAPI.SetAvatarURL(req.Context(), localpart, domain, r.AvatarURL)
+	if err != nil {
 		util.GetLogger(req.Context()).WithError(err).Error("profileAPI.SetAvatarURL failed")
 		return jsonerror.InternalServerError()
 	}
 	// No need to build new membership events, since nothing changed
-	if !setRes.Changed {
+	if !changed {
 		return util.JSONResponse{
 			Code: http.StatusOK,
 			JSON: struct{}{},
 		}
 	}
 
-	response, err := updateProfile(req.Context(), rsAPI, device, setRes.Profile, userID, cfg, evTime)
+	response, err := updateProfile(req.Context(), rsAPI, device, profile, userID, cfg, evTime)
 	if err != nil {
 		return response
 	}
@@ -164,34 +154,28 @@ func SetAvatarURL(
 
 // GetDisplayName implements GET /profile/{userID}/displayname
 func GetDisplayName(
-	req *http.Request, profileAPI userapi.ClientUserAPI, cfg *config.ClientAPI,
+	req *http.Request, profileAPI userapi.ProfileAPI, cfg *config.ClientAPI,
 	userID string, asAPI appserviceAPI.AppServiceInternalAPI,
 	federation *gomatrixserverlib.FederationClient,
 ) util.JSONResponse {
-	profile, err := getProfile(req.Context(), profileAPI, cfg, userID, asAPI, federation)
-	if err != nil {
-		if err == eventutil.ErrProfileNoExists {
-			return util.JSONResponse{
-				Code: http.StatusNotFound,
-				JSON: jsonerror.NotFound("The user does not exist or does not have a profile"),
-			}
-		}
-
-		util.GetLogger(req.Context()).WithError(err).Error("getProfile failed")
-		return jsonerror.InternalServerError()
+	profile := GetProfile(req, profileAPI, cfg, userID, asAPI, federation)
+	p, ok := profile.JSON.(eventutil.UserProfile)
+	// not a profile response, so most likely an error, return that
+	if !ok {
+		return profile
 	}
 
 	return util.JSONResponse{
 		Code: http.StatusOK,
-		JSON: eventutil.DisplayName{
-			DisplayName: profile.DisplayName,
+		JSON: eventutil.UserProfile{
+			DisplayName: p.DisplayName,
 		},
 	}
 }
 
 // SetDisplayName implements PUT /profile/{userID}/displayname
 func SetDisplayName(
-	req *http.Request, profileAPI userapi.ClientUserAPI,
+	req *http.Request, profileAPI userapi.ProfileAPI,
 	device *userapi.Device, userID string, cfg *config.ClientAPI, rsAPI api.ClientRoomserverAPI,
 ) util.JSONResponse {
 	if userID != device.UserID {
@@ -201,7 +185,7 @@ func SetDisplayName(
 		}
 	}
 
-	var r eventutil.DisplayName
+	var r eventutil.UserProfile
 	if resErr := httputil.UnmarshalJSONRequest(req, &r); resErr != nil {
 		return *resErr
 	}
@@ -233,25 +217,20 @@ func SetDisplayName(
 		}
 	}
 
-	profileRes := &userapi.PerformUpdateDisplayNameResponse{}
-	err = profileAPI.SetDisplayName(req.Context(), &userapi.PerformUpdateDisplayNameRequest{
-		Localpart:   localpart,
-		ServerName:  domain,
-		DisplayName: r.DisplayName,
-	}, profileRes)
+	profile, changed, err := profileAPI.SetDisplayName(req.Context(), localpart, domain, r.DisplayName)
 	if err != nil {
 		util.GetLogger(req.Context()).WithError(err).Error("profileAPI.SetDisplayName failed")
 		return jsonerror.InternalServerError()
 	}
 	// No need to build new membership events, since nothing changed
-	if !profileRes.Changed {
+	if !changed {
 		return util.JSONResponse{
 			Code: http.StatusOK,
 			JSON: struct{}{},
 		}
 	}
 
-	response, err := updateProfile(req.Context(), rsAPI, device, profileRes.Profile, userID, cfg, evTime)
+	response, err := updateProfile(req.Context(), rsAPI, device, profile, userID, cfg, evTime)
 	if err != nil {
 		return response
 	}
@@ -308,9 +287,9 @@ func updateProfile(
 // getProfile gets the full profile of a user by querying the database or a
 // remote homeserver.
 // Returns an error when something goes wrong or specifically
-// eventutil.ErrProfileNoExists when the profile doesn't exist.
+// eventutil.ErrProfileNotExists when the profile doesn't exist.
 func getProfile(
-	ctx context.Context, profileAPI userapi.ClientUserAPI, cfg *config.ClientAPI,
+	ctx context.Context, profileAPI userapi.ProfileAPI, cfg *config.ClientAPI,
 	userID string,
 	asAPI appserviceAPI.AppServiceInternalAPI,
 	federation *gomatrixserverlib.FederationClient,
@@ -325,7 +304,7 @@ func getProfile(
 		if fedErr != nil {
 			if x, ok := fedErr.(gomatrix.HTTPError); ok {
 				if x.Code == http.StatusNotFound {
-					return nil, eventutil.ErrProfileNoExists
+					return nil, appserviceAPI.ErrProfileNotExists
 				}
 			}
 

clientapi/routing/register.go

@@ -888,13 +888,7 @@ func completeRegistration(
 	}
 
 	if displayName != "" {
-		nameReq := userapi.PerformUpdateDisplayNameRequest{
-			Localpart:   username,
-			ServerName:  serverName,
-			DisplayName: displayName,
-		}
-		var nameRes userapi.PerformUpdateDisplayNameResponse
-		err = userAPI.SetDisplayName(ctx, &nameReq, &nameRes)
+		_, _, err = userAPI.SetDisplayName(ctx, username, serverName, displayName)
 		if err != nil {
 			return util.JSONResponse{
 				Code: http.StatusInternalServerError,

clientapi/routing/register_test.go

@@ -611,11 +611,9 @@ func TestRegisterUserWithDisplayName(t *testing.T) {
 
 		assert.Equal(t, http.StatusOK, response.Code)
 
-		req := api.QueryProfileRequest{UserID: "@user:server"}
-		var res api.QueryProfileResponse
-		err := userAPI.QueryProfile(processCtx.Context(), &req, &res)
+		profile, err := userAPI.QueryProfile(processCtx.Context(), "@user:server")
 		assert.NoError(t, err)
-		assert.Equal(t, expectedDisplayName, res.DisplayName)
+		assert.Equal(t, expectedDisplayName, profile.DisplayName)
 	})
 }
 
@@ -662,10 +660,8 @@ func TestRegisterAdminUsingSharedSecret(t *testing.T) {
 		)
 		assert.Equal(t, http.StatusOK, response.Code)
 
-		profilReq := api.QueryProfileRequest{UserID: "@alice:server"}
-		var profileRes api.QueryProfileResponse
-		err = userAPI.QueryProfile(processCtx.Context(), &profilReq, &profileRes)
+		profile, err := userAPI.QueryProfile(processCtx.Context(), "@alice:server")
 		assert.NoError(t, err)
-		assert.Equal(t, expectedDisplayName, profileRes.DisplayName)
+		assert.Equal(t, expectedDisplayName, profile.DisplayName)
 	})
 }

clientapi/routing/server_notices.go

@@ -295,30 +295,28 @@ func getSenderDevice(
 	}
 
 	// Set the avatarurl for the user
-	avatarRes := &userapi.PerformSetAvatarURLResponse{}
-	if err = userAPI.SetAvatarURL(ctx, &userapi.PerformSetAvatarURLRequest{
-		Localpart:  cfg.Matrix.ServerNotices.LocalPart,
-		ServerName: cfg.Matrix.ServerName,
-		AvatarURL:  cfg.Matrix.ServerNotices.AvatarURL,
-	}, avatarRes); err != nil {
+	profile, avatarChanged, err := userAPI.SetAvatarURL(ctx,
+		cfg.Matrix.ServerNotices.LocalPart,
+		cfg.Matrix.ServerName,
+		cfg.Matrix.ServerNotices.AvatarURL,
+	)
+	if err != nil {
 		util.GetLogger(ctx).WithError(err).Error("userAPI.SetAvatarURL failed")
 		return nil, err
 	}
 
-	profile := avatarRes.Profile
-
 	// Set the displayname for the user
-	displayNameRes := &userapi.PerformUpdateDisplayNameResponse{}
-	if err = userAPI.SetDisplayName(ctx, &userapi.PerformUpdateDisplayNameRequest{
-		Localpart:   cfg.Matrix.ServerNotices.LocalPart,
-		ServerName:  cfg.Matrix.ServerName,
-		DisplayName: cfg.Matrix.ServerNotices.DisplayName,
-	}, displayNameRes); err != nil {
+	_, displayNameChanged, err := userAPI.SetDisplayName(ctx,
+		cfg.Matrix.ServerNotices.LocalPart,
+		cfg.Matrix.ServerName,
+		cfg.Matrix.ServerNotices.DisplayName,
+	)
+	if err != nil {
 		util.GetLogger(ctx).WithError(err).Error("userAPI.SetDisplayName failed")
 		return nil, err
 	}
 
-	if displayNameRes.Changed {
+	if displayNameChanged {
 		profile.DisplayName = cfg.Matrix.ServerNotices.DisplayName
 	}
 
@@ -334,7 +332,7 @@ func getSenderDevice(
 	// We've got an existing account, return the first device of it
 	if len(deviceRes.Devices) > 0 {
 		// If there were changes to the profile, create a new membership event
-		if displayNameRes.Changed || avatarRes.Changed {
+		if displayNameChanged || avatarChanged {
 			_, err = updateProfile(ctx, rsAPI, &deviceRes.Devices[0], profile, accRes.Account.UserID, cfg, time.Now())
 			if err != nil {
 				return nil, err