MM-65970: New way to build routes in Client4 (#34499)

* Add Client4 route building functions

* Make DoAPIRequestWithHeaders add the API URL

This makes it consistent with the other DoAPIXYZ functions, which all
prepend the provided URL with the client's API URL.

* Use the new route building logic in Client4

* Address review comments

- clean renamed to cleanSegment
- JoinRoutes and JoinSegments joined in Join
- newClientRoute uses Join

* Fix new routes from merge

* Remove unused import

* Simplify error handling around clientRoute (#34870)

---------

Co-authored-by: Jesse Hallam <jesse@mattermost.com>
Co-authored-by: Jesse Hallam <jesse.hallam@gmail.com>
Co-authored-by: Mattermost Build <build@mattermost.com>

server/channels/api4/user_test.go

@@ -4476,7 +4476,7 @@ func TestLoginCookies(t *testing.T) {
 
 		r, _ := th.Client.DoAPIRequestWithHeaders(context.Background(),
 			http.MethodPost,
-			th.Client.APIURL+"/users/login/cws",
+			"/users/login/cws",
 			form.Encode(),
 			map[string]string{
 				"Content-Type": "application/x-www-form-urlencoded",

server/public/model/client4_route.go

@@ -0,0 +1,46 @@
+package model
+
+import (
+	"net/url"
+	"strings"
+)
+
+type clientRoute struct {
+	segments []string
+}
+
+func cleanSegment(segment string) string {
+	s := strings.ReplaceAll(segment, "..", "")
+	return url.PathEscape(s)
+}
+
+func newClientRoute(segment string) clientRoute {
+	return clientRoute{}.Join(segment)
+}
+
+func (r clientRoute) Join(segments ...any) clientRoute {
+	for _, segment := range segments {
+		if s, ok := segment.(string); ok {
+			r.segments = append(r.segments, cleanSegment(s))
+		}
+
+		if s, ok := segment.(clientRoute); ok {
+			r.segments = append(r.segments, s.segments...)
+		}
+	}
+
+	return r
+}
+
+func (r clientRoute) URL() (*url.URL, error) {
+	path, err := r.String()
+	if err != nil {
+		return nil, err
+	}
+	return &url.URL{Path: path}, nil
+}
+
+func (r clientRoute) String() (string, error) {
+	// Make sure that there is a leading slash
+	return url.JoinPath("/", strings.Join(r.segments, "/"))
+}

server/public/model/client4_route_test.go

@@ -0,0 +1,399 @@
+package model
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func TestNewClientRoute(t *testing.T) {
+	tests := []struct {
+		name     string
+		input    string
+		expected string
+	}{
+		{
+			name:     "simple path",
+			input:    "api",
+			expected: "/api",
+		},
+		{
+			name:     "path with special characters",
+			input:    "hello world",
+			expected: "/hello%20world",
+		},
+		{
+			name:     "path with slashes",
+			input:    "api/v4",
+			expected: "/api%2Fv4",
+		},
+		{
+			name:     "empty string",
+			input:    "",
+			expected: "/",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			route := newClientRoute(tt.input)
+			result, err := route.String()
+			require.NoError(t, err)
+			require.Equal(t, tt.expected, result)
+		})
+	}
+}
+
+func TestClientRouteJoinRoute(t *testing.T) {
+	tests := []struct {
+		name     string
+		base     clientRoute
+		join     clientRoute
+		expected string
+		wantErr  bool
+	}{
+		{
+			name:     "join two valid routes",
+			base:     newClientRoute("api"),
+			join:     newClientRoute("v4"),
+			expected: "/api/v4",
+			wantErr:  false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			result := tt.base.Join(tt.join)
+			str, err := result.String()
+			if tt.wantErr {
+				require.Error(t, err)
+			} else {
+				require.NoError(t, err)
+				require.Equal(t, tt.expected, str)
+			}
+		})
+	}
+}
+
+func TestClientRouteJoinSegment(t *testing.T) {
+	tests := []struct {
+		name     string
+		base     clientRoute
+		segment  string
+		expected string
+		wantErr  bool
+	}{
+		{
+			name:     "valid segment",
+			base:     newClientRoute("api"),
+			segment:  "v4",
+			expected: "/api/v4",
+			wantErr:  false,
+		},
+		{
+			name:     "segment with spaces",
+			base:     newClientRoute("api"),
+			segment:  "hello world",
+			expected: "/api/hello%20world",
+			wantErr:  false,
+		},
+		{
+			name:     "segment with slash - escaped",
+			base:     newClientRoute("api"),
+			segment:  "v4/users",
+			expected: "/api/v4%2Fusers",
+			wantErr:  false,
+		},
+		{
+			name:     "empty segment",
+			base:     newClientRoute("api"),
+			segment:  "",
+			expected: "/api/",
+			wantErr:  false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			result := tt.base.Join(tt.segment)
+			str, err := result.String()
+			if tt.wantErr {
+				require.Error(t, err)
+			} else {
+				require.NoError(t, err)
+				require.Equal(t, tt.expected, str)
+			}
+		})
+	}
+}
+
+func TestClientRouteJoin(t *testing.T) {
+	tests := []struct {
+		name     string
+		base     clientRoute
+		segments []any
+		expected string
+		wantErr  bool
+	}{
+		{
+			name:     "multiple valid segments",
+			base:     newClientRoute("api"),
+			segments: []any{"v4", "users", "me"},
+			expected: "/api/v4/users/me",
+			wantErr:  false,
+		},
+		{
+			name:     "no segments",
+			base:     newClientRoute("api"),
+			segments: []any{},
+			expected: "/api",
+			wantErr:  false,
+		},
+		{
+			name:     "segment with slash - escaped",
+			base:     newClientRoute("api"),
+			segments: []any{"v4", "users/me"},
+			expected: "/api/v4/users%2Fme",
+			wantErr:  false,
+		},
+		{
+			name:     "empty segment",
+			base:     newClientRoute("api"),
+			segments: []any{"v4", "users", "", "me"},
+			expected: "/api/v4/users/me",
+			wantErr:  false,
+		},
+		{
+			name:     "empty segment at the end",
+			base:     newClientRoute("api"),
+			segments: []any{"v4", "users", ""},
+			expected: "/api/v4/users/",
+			wantErr:  false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			result := tt.base.Join(tt.segments...)
+			str, err := result.String()
+			if tt.wantErr {
+				require.Error(t, err)
+			} else {
+				require.NoError(t, err)
+				require.Equal(t, tt.expected, str)
+			}
+		})
+	}
+}
+
+func TestClientRouteURL(t *testing.T) {
+	tests := []struct {
+		name        string
+		route       clientRoute
+		expectedURL string
+		wantErr     bool
+	}{
+		{
+			name:        "simple route",
+			route:       newClientRoute("api").Join(newClientRoute("v4")),
+			expectedURL: "/api/v4",
+			wantErr:     false,
+		},
+		{
+			name:        "empty route",
+			route:       clientRoute{},
+			expectedURL: "/",
+			wantErr:     false,
+		},
+		{
+			name:        "complex route",
+			route:       newClientRoute("api").Join(newClientRoute("v4"), "users"),
+			expectedURL: "/api/v4/users",
+			wantErr:     false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			result, err := tt.route.URL()
+			if tt.wantErr {
+				require.Error(t, err)
+				require.Nil(t, result)
+			} else {
+				require.NoError(t, err)
+				require.NotNil(t, result)
+				require.Equal(t, tt.expectedURL, result.Path)
+			}
+		})
+	}
+}
+
+func TestClientRouteString(t *testing.T) {
+	tests := []struct {
+		name     string
+		route    clientRoute
+		expected string
+		wantErr  bool
+	}{
+		{
+			name:     "simple route",
+			route:    newClientRoute("api"),
+			expected: "/api",
+			wantErr:  false,
+		},
+		{
+			name:     "empty route",
+			route:    clientRoute{},
+			expected: "/",
+			wantErr:  false,
+		},
+		{
+			name:     "complex route",
+			route:    newClientRoute("api").Join(newClientRoute("v4"), "users", "me"),
+			expected: "/api/v4/users/me",
+			wantErr:  false,
+		},
+		{
+			name:     "route with special characters",
+			route:    newClientRoute("api").Join("hello world"),
+			expected: "/api/hello%20world",
+			wantErr:  false,
+		},
+		{
+			name:     "route with special characters",
+			route:    newClientRoute("").Join(".."),
+			expected: "/",
+			wantErr:  false,
+		},
+		{
+			name:     "route with control characters, which make url.JoinPath to error out",
+			route:    newClientRoute("[fe80::1%en0]"),
+			expected: "/%5Bfe80::1%25en0%5D",
+			wantErr:  false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			result, err := tt.route.String()
+			if tt.wantErr {
+				require.Error(t, err)
+				require.Equal(t, tt.expected, result)
+			} else {
+				require.NoError(t, err)
+				require.Equal(t, tt.expected, result)
+			}
+		})
+	}
+}
+
+func TestClientRouteLeadingSlash(t *testing.T) {
+	tests := []struct {
+		name  string
+		route clientRoute
+	}{
+		{
+			name:  "single segment",
+			route: newClientRoute("api"),
+		},
+		{
+			name:  "multiple segments",
+			route: newClientRoute("api").Join("v4"),
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			// Test String() has leading slash
+			str, err := tt.route.String()
+			require.NoError(t, err)
+			require.True(t, len(str) > 0 && str[0] == '/', "String() result should have leading slash")
+
+			// Test URL() has leading slash
+			u, err := tt.route.URL()
+			require.NoError(t, err)
+			require.True(t, len(u.Path) > 0 && u.Path[0] == '/', "URL() result path should have leading slash")
+		})
+	}
+}
+
+func TestClean(t *testing.T) {
+	tests := []struct {
+		name     string
+		input    string
+		expected string
+	}{
+		{
+			name:     "simple string",
+			input:    "api",
+			expected: "api",
+		},
+		{
+			name:     "string with spaces",
+			input:    "hello world",
+			expected: "hello%20world",
+		},
+		{
+			name:     "string with slashes",
+			input:    "api/v4",
+			expected: "api%2Fv4",
+		},
+		{
+			name:     "just two dots",
+			input:    "..",
+			expected: "",
+		},
+		{
+			name:     "two dots at start",
+			input:    "../api",
+			expected: "%2Fapi",
+		},
+		{
+			name:     "two dots at end",
+			input:    "api/..",
+			expected: "api%2F",
+		},
+		{
+			name:     "two dots in middle",
+			input:    "api/../v4",
+			expected: "api%2F%2Fv4",
+		},
+		{
+			name:     "multiple two dots",
+			input:    "../../api",
+			expected: "%2F%2Fapi",
+		},
+		{
+			name:     "empty string",
+			input:    "",
+			expected: "",
+		},
+		{
+			name:     "special characters",
+			input:    "user@example.com",
+			expected: "user@example.com",
+		},
+		{
+			name:     "single dot",
+			input:    ".",
+			expected: ".",
+		},
+		{
+			name:     "three dots",
+			input:    "...",
+			expected: ".",
+		},
+		{
+			name:     "four dots",
+			input:    "....",
+			expected: "",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			result := cleanSegment(tt.input)
+			require.Equal(t, tt.expected, result)
+		})
+	}
+}

server/public/model/marketplace_plugin.go

@@ -94,7 +94,12 @@ type MarketplacePluginFilter struct {
 
 // ApplyToURL modifies the given url to include query string parameters for the request.
 func (filter *MarketplacePluginFilter) ApplyToURL(u *url.URL) {
-	q := u.Query()
+	u.RawQuery = filter.ToValues().Encode()
+}
+
+// ToValues converts the filter to url.Values for use in query strings.
+func (filter *MarketplacePluginFilter) ToValues() url.Values {
+	q := url.Values{}
 	q.Add("page", strconv.Itoa(filter.Page))
 	if filter.PerPage > 0 {
 		q.Add("per_page", strconv.Itoa(filter.PerPage))
@@ -109,7 +114,7 @@ func (filter *MarketplacePluginFilter) ApplyToURL(u *url.URL) {
 	q.Add("platform", filter.Platform)
 	q.Add("plugin_id", filter.PluginId)
 	q.Add("return_all_versions", strconv.FormatBool(filter.ReturnAllVersions))
-	u.RawQuery = q.Encode()
+	return q
 }
 
 // InstallMarketplacePluginRequest struct describes parameters of the requested plugin.