Simplify ParamsFromRequest (#20384)

web/params.go

@@ -101,29 +101,12 @@ func ParamsFromRequest(r *http.Request) *Params {
 	props := mux.Vars(r)
 	query := r.URL.Query()
 
-	if val, ok := props["user_id"]; ok {
-		params.UserId = val
-	}
-
-	if val, ok := props["team_id"]; ok {
-		params.TeamId = val
-	}
-
-	if val, ok := props["category_id"]; ok {
-		params.CategoryId = val
-	}
-
-	if val, ok := props["invite_id"]; ok {
-		params.InviteId = val
-	}
-
-	if val, ok := props["token_id"]; ok {
-		params.TokenId = val
-	}
-
-	if val, ok := props["thread_id"]; ok {
-		params.ThreadId = val
-	}
+	params.UserId = props["user_id"]
+	params.TeamId = props["team_id"]
+	params.CategoryId = props["category_id"]
+	params.InviteId = props["invite_id"]
+	params.TokenId = props["token_id"]
+	params.ThreadId = props["thread_id"]
 
 	if val, ok := props["channel_id"]; ok {
 		params.ChannelId = val
@@ -131,116 +114,34 @@ func ParamsFromRequest(r *http.Request) *Params {
 		params.ChannelId = query.Get("channel_id")
 	}
 
-	if val, ok := props["post_id"]; ok {
-		params.PostId = val
-	}
-
-	if val, ok := props["policy_id"]; ok {
-		params.PolicyId = val
-	}
-
-	if val, ok := props["file_id"]; ok {
-		params.FileId = val
-	}
-
+	params.PostId = props["post_id"]
+	params.PolicyId = props["policy_id"]
+	params.FileId = props["file_id"]
 	params.Filename = query.Get("filename")
-
-	if val, ok := props["upload_id"]; ok {
-		params.UploadId = val
-	}
-
-	if val, ok := props["plugin_id"]; ok {
-		params.PluginId = val
-	}
-
-	if val, ok := props["command_id"]; ok {
-		params.CommandId = val
-	}
-
-	if val, ok := props["hook_id"]; ok {
-		params.HookId = val
-	}
-
-	if val, ok := props["report_id"]; ok {
-		params.ReportId = val
-	}
-
-	if val, ok := props["emoji_id"]; ok {
-		params.EmojiId = val
-	}
-
-	if val, ok := props["app_id"]; ok {
-		params.AppId = val
-	}
-
-	if val, ok := props["email"]; ok {
-		params.Email = val
-	}
-
-	if val, ok := props["username"]; ok {
-		params.Username = val
-	}
-
-	if val, ok := props["team_name"]; ok {
-		params.TeamName = strings.ToLower(val)
-	}
-
-	if val, ok := props["channel_name"]; ok {
-		params.ChannelName = strings.ToLower(val)
-	}
-
-	if val, ok := props["category"]; ok {
-		params.Category = val
-	}
-
-	if val, ok := props["service"]; ok {
-		params.Service = val
-	}
-
-	if val, ok := props["preference_name"]; ok {
-		params.PreferenceName = val
-	}
-
-	if val, ok := props["emoji_name"]; ok {
-		params.EmojiName = val
-	}
-
-	if val, ok := props["job_id"]; ok {
-		params.JobId = val
-	}
-
-	if val, ok := props["job_type"]; ok {
-		params.JobType = val
-	}
-
-	if val, ok := props["action_id"]; ok {
-		params.ActionId = val
-	}
-
-	if val, ok := props["role_id"]; ok {
-		params.RoleId = val
-	}
-
-	if val, ok := props["role_name"]; ok {
-		params.RoleName = val
-	}
-
-	if val, ok := props["scheme_id"]; ok {
-		params.SchemeId = val
-	}
-
-	if val, ok := props["group_id"]; ok {
-		params.GroupId = val
-	}
-
-	if val, ok := props["remote_id"]; ok {
-		params.RemoteId = val
-	}
-
-	if val, ok := props["invoice_id"]; ok {
-		params.InvoiceId = val
-	}
-
+	params.UploadId = props["upload_id"]
+	params.PluginId = props["plugin_id"]
+	params.CommandId = props["command_id"]
+	params.HookId = props["hook_id"]
+	params.ReportId = props["report_id"]
+	params.EmojiId = props["emoji_id"]
+	params.AppId = props["app_id"]
+	params.Email = props["email"]
+	params.Username = props["username"]
+	params.TeamName = strings.ToLower(props["team_name"])
+	params.ChannelName = strings.ToLower(props["channel_name"])
+	params.Category = props["category"]
+	params.Service = props["service"]
+	params.PreferenceName = props["preference_name"]
+	params.EmojiName = props["emoji_name"]
+	params.JobId = props["job_id"]
+	params.JobType = props["job_type"]
+	params.ActionId = props["action_id"]
+	params.RoleId = props["role_id"]
+	params.RoleName = props["role_name"]
+	params.SchemeId = props["scheme_id"]
+	params.GroupId = props["group_id"]
+	params.RemoteId = props["remote_id"]
+	params.InvoiceId = props["invoice_id"]
 	params.Scope = query.Get("scope")
 
 	if val, err := strconv.Atoi(query.Get("page")); err != nil || val < 0 {
@@ -256,12 +157,9 @@ func ParamsFromRequest(r *http.Request) *Params {
 	}
 
 	params.TimeRange = query.Get("time_range")
-
-	if val, err := strconv.ParseBool(query.Get("permanent")); err == nil {
-		params.Permanent = val
-	}
-
+	params.Permanent, _ = strconv.ParseBool(query.Get("permanent"))
 	params.PerPage = getPerPageFromQuery(query)
+
 	if val, err := strconv.Atoi(query.Get("logs_per_page")); err != nil || val < 0 {
 		params.LogsPerPage = LogsPerPageDefault
 	} else if val > LogsPerPageMaximum {
@@ -286,23 +184,16 @@ func ParamsFromRequest(r *http.Request) *Params {
 		params.LimitBefore = val
 	}
 
-	if val, ok := props["syncable_id"]; ok {
-		params.SyncableId = val
-	}
-
-	if val, ok := props["syncable_type"]; ok {
-		switch val {
-		case "teams":
-			params.SyncableType = model.GroupSyncableTypeTeam
-		case "channels":
-			params.SyncableType = model.GroupSyncableTypeChannel
-		}
-	}
-
-	if val, ok := props["bot_user_id"]; ok {
-		params.BotUserId = val
+	params.SyncableId = props["syncable_id"]
+
+	switch props["syncable_type"] {
+	case "teams":
+		params.SyncableType = model.GroupSyncableTypeTeam
+	case "channels":
+		params.SyncableType = model.GroupSyncableTypeChannel
 	}
 
+	params.BotUserId = props["bot_user_id"]
 	params.Q = query.Get("q")
 
 	if val, err := strconv.ParseBool(query.Get("is_linked")); err == nil {
@@ -315,57 +206,27 @@ func ParamsFromRequest(r *http.Request) *Params {
 
 	params.NotAssociatedToTeam = query.Get("not_associated_to_team")
 	params.NotAssociatedToChannel = query.Get("not_associated_to_channel")
-
-	if val, err := strconv.ParseBool(query.Get("filter_allow_reference")); err == nil {
-		params.FilterAllowReference = val
-	}
-
-	if val, err := strconv.ParseBool(query.Get("filter_parent_team_permitted")); err == nil {
-		params.FilterParentTeamPermitted = val
-	}
+	params.FilterAllowReference, _ = strconv.ParseBool(query.Get("filter_allow_reference"))
+	params.FilterParentTeamPermitted, _ = strconv.ParseBool(query.Get("filter_parent_team_permitted"))
 
 	if val, err := strconv.ParseBool(query.Get("paginate")); err == nil {
 		params.Paginate = &val
 	}
 
-	if val, err := strconv.ParseBool(query.Get("include_member_count")); err == nil {
-		params.IncludeMemberCount = val
-	}
-
+	params.IncludeMemberCount, _ = strconv.ParseBool(query.Get("include_member_count"))
 	params.NotAssociatedToGroup = query.Get("not_associated_to_group")
-
-	if val, err := strconv.ParseBool(query.Get("exclude_default_channels")); err == nil {
-		params.ExcludeDefaultChannels = val
-	}
-
+	params.ExcludeDefaultChannels, _ = strconv.ParseBool(query.Get("exclude_default_channels"))
 	params.GroupIDs = query.Get("group_ids")
-
-	if val, err := strconv.ParseBool(query.Get("include_total_count")); err == nil {
-		params.IncludeTotalCount = val
-	}
-
-	if val, err := strconv.ParseBool(query.Get("include_deleted")); err == nil {
-		params.IncludeDeleted = val
-	}
-
-	if val, ok := props["warn_metric_id"]; ok {
-		params.WarnMetricId = val
-	}
-
-	if val, ok := props["export_name"]; ok {
-		params.ExportName = val
-	}
-
-	if val, err := strconv.ParseBool(query.Get("exclude_policy_constrained")); err == nil {
-		params.ExcludePolicyConstrained = val
-	}
+	params.IncludeTotalCount, _ = strconv.ParseBool(query.Get("include_total_count"))
+	params.IncludeDeleted, _ = strconv.ParseBool(query.Get("include_deleted"))
+	params.WarnMetricId = props["warn_metric_id"]
+	params.ExportName = props["export_name"]
+	params.ExcludePolicyConstrained, _ = strconv.ParseBool(query.Get("exclude_policy_constrained"))
 
 	if val := query.Get("group_source"); val != "" {
 		switch val {
 		case "custom":
 			params.GroupSource = model.GroupSourceCustom
-		case "ldap":
-			params.GroupSource = model.GroupSourceLdap
 		default:
 			params.GroupSource = model.GroupSourceLdap
 		}

web/params_test.go

@@ -3,9 +3,12 @@
 package web
 
 import (
+	"net/http"
 	"net/url"
 	"testing"
 
+	"github.com/gorilla/mux"
+	"github.com/mattermost/mattermost-server/v6/model"
 	"github.com/stretchr/testify/require"
 )
 
@@ -32,3 +35,452 @@ func TestGetPerPageFromQuery(t *testing.T) {
 		require.Equal(t, 100, perPage)
 	})
 }
+
+func TestParamsFromRequest(t *testing.T) {
+	testCases := []struct {
+		Description string
+		URL         *url.URL
+		Vars        map[string]string
+		Params      *Params
+	}{
+		{
+			"empty params",
+			mustURL("/"),
+			nil,
+			&Params{
+				PerPage:     PerPageDefault,
+				LogsPerPage: LogsPerPageDefault,
+				LimitAfter:  LimitDefault,
+				LimitBefore: LimitDefault,
+			},
+		},
+		{
+			"query params",
+			mustURL("/page?" +
+				"channel_id=abc123&" +
+				"filename=file.ext&" +
+				"page=42&" +
+				"time_range=then-till-now&" +
+				"permanent=1&" +
+				"logs_per_page=5&" +
+				"limit_after=6&" +
+				"limit_before=7&" +
+				"q=picard&" +
+				"is_linked=t&" +
+				"is_configured=TRUE&" +
+				"not_associated_to_team=this_team&" +
+				"not_associated_to_channel=this_channel&" +
+				"filter_allow_reference=true&" +
+				"filter_parent_team_permitted=True&" +
+				"paginate=T&" +
+				"include_member_count=1&" +
+				"not_associated_to_group=test&" +
+				"exclude_default_channels=1&" +
+				"group_ids=hello,world&" +
+				"include_total_count=T&" +
+				"include_deleted=True&" +
+				"exclude_policy_constrained=TRUE&" +
+				"filter_has_member=xyz"),
+			nil,
+			&Params{
+				ChannelId:                 "abc123",
+				Filename:                  "file.ext",
+				Page:                      42,
+				TimeRange:                 "then-till-now",
+				PerPage:                   PerPageDefault,
+				Permanent:                 true,
+				LogsPerPage:               5,
+				LimitAfter:                6,
+				LimitBefore:               7,
+				Q:                         "picard",
+				IsLinked:                  boolPtr(true),
+				IsConfigured:              boolPtr(true),
+				NotAssociatedToTeam:       "this_team",
+				NotAssociatedToChannel:    "this_channel",
+				FilterAllowReference:      true,
+				FilterParentTeamPermitted: true,
+				Paginate:                  boolPtr(true),
+				IncludeMemberCount:        true,
+				NotAssociatedToGroup:      "test",
+				ExcludeDefaultChannels:    true,
+				GroupIDs:                  "hello,world",
+				IncludeTotalCount:         true,
+				IncludeDeleted:            true,
+				ExcludePolicyConstrained:  true,
+				FilterHasMember:           "xyz",
+			},
+		},
+		{
+			"page invalid",
+			mustURL("?page=hello"),
+			nil,
+			&Params{
+				Page: PageDefault,
+
+				PerPage:     PerPageDefault,
+				LogsPerPage: LogsPerPageDefault,
+				LimitAfter:  LimitDefault,
+				LimitBefore: LimitDefault,
+			},
+		},
+		{
+			"page negative",
+			mustURL("?page=-1"),
+			nil,
+			&Params{
+				Page: PageDefault,
+
+				PerPage:     PerPageDefault,
+				LogsPerPage: LogsPerPageDefault,
+				LimitAfter:  LimitDefault,
+				LimitBefore: LimitDefault,
+			},
+		},
+		{
+			"per page valid",
+			mustURL("?per_page=123"),
+			nil,
+			&Params{
+				PerPage: 123,
+
+				LogsPerPage: LogsPerPageDefault,
+				LimitAfter:  LimitDefault,
+				LimitBefore: LimitDefault,
+			},
+		},
+		{
+			"per page too small",
+			mustURL("?per_page=-100"),
+			nil,
+			&Params{
+				PerPage: PerPageDefault,
+
+				LogsPerPage: LogsPerPageDefault,
+				LimitAfter:  LimitDefault,
+				LimitBefore: LimitDefault,
+			},
+		},
+		{
+			"per page too big",
+			mustURL("?per_page=100000"),
+			nil,
+			&Params{
+				PerPage: PerPageMaximum,
+
+				LogsPerPage: LogsPerPageDefault,
+				LimitAfter:  LimitDefault,
+				LimitBefore: LimitDefault,
+			},
+		},
+		{
+			"logs per page valid",
+			mustURL("?logs_per_page=512"),
+			nil,
+			&Params{
+				LogsPerPage: 512,
+
+				PerPage:     PerPageDefault,
+				LimitAfter:  LimitDefault,
+				LimitBefore: LimitDefault,
+			},
+		},
+		{
+			"logs per page invalid",
+			mustURL("?logs_per_page=logs"),
+			nil,
+			&Params{
+				LogsPerPage: LogsPerPageDefault,
+
+				PerPage:     PerPageDefault,
+				LimitAfter:  LimitDefault,
+				LimitBefore: LimitDefault,
+			},
+		},
+		{
+			"logs per page too small",
+			mustURL("?logs_per_page=-512"),
+			nil,
+			&Params{
+				LogsPerPage: LogsPerPageDefault,
+
+				PerPage:     PerPageDefault,
+				LimitAfter:  LimitDefault,
+				LimitBefore: LimitDefault,
+			},
+		},
+		{
+			"logs per page too big",
+			mustURL("?logs_per_page=99999999"),
+			nil,
+			&Params{
+				LogsPerPage: LogsPerPageMaximum,
+
+				PerPage:     PerPageDefault,
+				LimitAfter:  LimitDefault,
+				LimitBefore: LimitDefault,
+			},
+		},
+		{
+			"limit before valid",
+			mustURL("?limit_before=100"),
+			nil,
+			&Params{
+				LimitBefore: 100,
+
+				PerPage:     PerPageDefault,
+				LogsPerPage: LogsPerPageDefault,
+				LimitAfter:  LimitDefault,
+			},
+		},
+		{
+			"limit before invalid",
+			mustURL("?limit_before=limit"),
+			nil,
+			&Params{
+				LimitBefore: LimitDefault,
+
+				PerPage:     PerPageDefault,
+				LogsPerPage: LogsPerPageDefault,
+				LimitAfter:  LimitDefault,
+			},
+		},
+		{
+			"limit before too small",
+			mustURL("?limit_before=-100"),
+			nil,
+			&Params{
+				LimitBefore: LimitDefault,
+
+				PerPage:     PerPageDefault,
+				LogsPerPage: LogsPerPageDefault,
+				LimitAfter:  LimitDefault,
+			},
+		},
+		{
+			"limit before too big",
+			mustURL("?limit_before=99999"),
+			nil,
+			&Params{
+				LimitBefore: LimitMaximum,
+
+				PerPage:     PerPageDefault,
+				LogsPerPage: LogsPerPageDefault,
+				LimitAfter:  LimitDefault,
+			},
+		},
+		{
+			"limit after valid",
+			mustURL("?limit_after=100"),
+			nil,
+			&Params{
+				LimitAfter: 100,
+
+				PerPage:     PerPageDefault,
+				LogsPerPage: LogsPerPageDefault,
+				LimitBefore: LimitDefault,
+			},
+		},
+		{
+			"limit after invalid",
+			mustURL("?limit_after=limit"),
+			nil,
+			&Params{
+				LimitAfter: LimitDefault,
+
+				PerPage:     PerPageDefault,
+				LogsPerPage: LogsPerPageDefault,
+				LimitBefore: LimitDefault,
+			},
+		},
+		{
+			"limit after too small",
+			mustURL("?limit_aftere=-100"),
+			nil,
+			&Params{
+				LimitAfter: LimitDefault,
+
+				PerPage:     PerPageDefault,
+				LogsPerPage: LogsPerPageDefault,
+				LimitBefore: LimitDefault,
+			},
+		},
+		{
+			"limit after too big",
+			mustURL("?limit_after=99999"),
+			nil,
+			&Params{
+				LimitAfter: LimitMaximum,
+
+				PerPage:     PerPageDefault,
+				LogsPerPage: LogsPerPageDefault,
+				LimitBefore: LimitDefault,
+			},
+		},
+		{
+			"group source custom",
+			mustURL("?group_source=custom"),
+			nil,
+			&Params{
+				GroupSource: model.GroupSourceCustom,
+
+				PerPage:     PerPageDefault,
+				LogsPerPage: LogsPerPageDefault,
+				LimitBefore: LimitDefault,
+				LimitAfter:  LimitDefault,
+			},
+		},
+		{
+			"group source LDAP",
+			mustURL("?group_source=ldap"),
+			nil,
+			&Params{
+				GroupSource: model.GroupSourceLdap,
+
+				PerPage:     PerPageDefault,
+				LogsPerPage: LogsPerPageDefault,
+				LimitBefore: LimitDefault,
+				LimitAfter:  LimitDefault,
+			},
+		},
+		{
+			"group source other",
+			mustURL("?group_source=aabbcc"),
+			nil,
+			&Params{
+				GroupSource: model.GroupSourceLdap,
+
+				PerPage:     PerPageDefault,
+				LogsPerPage: LogsPerPageDefault,
+				LimitBefore: LimitDefault,
+				LimitAfter:  LimitDefault,
+			},
+		},
+		{
+			"group source empty",
+			mustURL("?group_souce="),
+			nil,
+			&Params{
+				GroupSource: "",
+
+				PerPage:     PerPageDefault,
+				LogsPerPage: LogsPerPageDefault,
+				LimitBefore: LimitDefault,
+				LimitAfter:  LimitDefault,
+			},
+		},
+		{
+			"timestamp valid",
+			mustURL("/"),
+			map[string]string{
+				"timestamp": "1234567",
+			},
+			&Params{
+				Timestamp: 1234567,
+
+				PerPage:     PerPageDefault,
+				LogsPerPage: LogsPerPageDefault,
+				LimitBefore: LimitDefault,
+				LimitAfter:  LimitDefault,
+			},
+		},
+		{
+			"timestamp valid",
+			mustURL("/"),
+			map[string]string{
+				"timestamp": "yes",
+			},
+			&Params{
+				Timestamp: 0,
+
+				PerPage:     PerPageDefault,
+				LogsPerPage: LogsPerPageDefault,
+				LimitBefore: LimitDefault,
+				LimitAfter:  LimitDefault,
+			},
+		},
+		{
+			"timestamp too small",
+			mustURL("/"),
+			map[string]string{
+				"timestamp": "-1234567",
+			},
+			&Params{
+				Timestamp: 0,
+
+				PerPage:     PerPageDefault,
+				LogsPerPage: LogsPerPageDefault,
+				LimitBefore: LimitDefault,
+				LimitAfter:  LimitDefault,
+			},
+		},
+		{
+			"syncable type teams",
+			mustURL("/"),
+			map[string]string{
+				"syncable_type": "teams",
+			},
+			&Params{
+				SyncableType: model.GroupSyncableTypeTeam,
+
+				PerPage:     PerPageDefault,
+				LogsPerPage: LogsPerPageDefault,
+				LimitBefore: LimitDefault,
+				LimitAfter:  LimitDefault,
+			},
+		},
+		{
+			"syncable type channels",
+			mustURL("/"),
+			map[string]string{
+				"syncable_type": "channels",
+			},
+			&Params{
+				SyncableType: model.GroupSyncableTypeChannel,
+
+				PerPage:     PerPageDefault,
+				LogsPerPage: LogsPerPageDefault,
+				LimitBefore: LimitDefault,
+				LimitAfter:  LimitDefault,
+			},
+		},
+		{
+			"syncable type other",
+			mustURL("/"),
+			map[string]string{
+				"syncable_type": "unknownvalue",
+			},
+			&Params{
+				SyncableType: "",
+
+				PerPage:     PerPageDefault,
+				LogsPerPage: LogsPerPageDefault,
+				LimitBefore: LimitDefault,
+				LimitAfter:  LimitDefault,
+			},
+		},
+	}
+
+	for _, testCase := range testCases {
+		testCase := testCase
+		t.Run(testCase.Description, func(t *testing.T) {
+			t.Parallel()
+
+			r := &http.Request{URL: testCase.URL}
+			r = mux.SetURLVars(r, testCase.Vars)
+			params := ParamsFromRequest(r)
+			require.Equal(t, testCase.Params, params)
+		})
+	}
+}
+
+func mustURL(u string) *url.URL {
+	parsed, err := url.Parse(u)
+	if err != nil {
+		panic(err)
+	}
+	return parsed
+}
+
+func boolPtr(b bool) *bool {
+	return &b
+}