Repository: TBD54566975/ssi-service (go)

Issue:
The pageSize and pageToken parameters are perhaps not working when calling the ListSubmissions function.
I found that when calling

http://localhost:3000/v1/presentations/submissions?pageSize=1&filter=status="approved"
the filter parameter is effective, but the pageSize parameter is not.

Furthermore, I found that

`pageSizeStr := framework.GetParam(c, *PageSizeParam*)`

and

`queryPageToken := framework.GetParam(c, *PageTokenParam*)`

in the `ParsePaginationParams` function both return nil.

I modified them to

`pageSizeStr := framework.GetQueryValue(c, *PageSizeParam*)`

and

`queryPageToken := framework.GetQueryValue(c, *PageTokenParam*)`

and it seems to work correctly in terms of the number of results returned. Considering that both functions `ListDIDsByMethod` and `ListSubmissions` that call `ParsePaginationParams` are GET methods, would `GetQueryValue` be a better choice?

Pull request:
Retrieve pagination parameters from the correct location
# Overview
Fixes #626

# Description
Parameters are before the URL path (i.e. before the `?`). Query parameters come after.

# How Has This Been Tested?
Tests were updated to fail before changes were made. And are passing after the changes were made.


Patch:
diff --git a/pkg/server/pagination/pagination.go b/pkg/server/pagination/pagination.go
index 7e819f6..1c9dc0d 100644
--- a/pkg/server/pagination/pagination.go
+++ b/pkg/server/pagination/pagination.go
@@ -25,13 +25,13 @@ const (
 	PageTokenParam = "pageToken"
 )
 
-// ParsePaginationParams reads the PageSizeParam and PageTokenParam from the URL parameters and populates the passed in
+// ParsePaginationQueryValues reads the PageSizeParam and PageTokenParam from the URL parameters and populates the passed in
 // pageRequest. The value encoded in PageTokenParam is assumed to be the base64url encoding of a PageToken. It is an
 // error for the query params to be different from the query params encoded in the PageToken. Any error during the
 // execution is responded to using the passed in gin.Context. The return value corresponds to whether there was an
 // error within the function.
-func ParsePaginationParams(c *gin.Context, pageRequest *PageRequest) bool {
-	pageSizeStr := framework.GetParam(c, PageSizeParam)
+func ParsePaginationQueryValues(c *gin.Context, pageRequest *PageRequest) bool {
+	pageSizeStr := framework.GetQueryValue(c, PageSizeParam)
 
 	if pageSizeStr != nil {
 		pageSize, err := strconv.Atoi(*pageSizeStr)
@@ -48,7 +48,7 @@ func ParsePaginationParams(c *gin.Context, pageRequest *PageRequest) bool {
 		pageRequest.PageSize = &pageSize
 	}
 
-	queryPageToken := framework.GetParam(c, PageTokenParam)
+	queryPageToken := framework.GetQueryValue(c, PageTokenParam)
 	if queryPageToken != nil {
 		errMsg := "token value cannot be decoded"
 		tokenData, err := base64.RawURLEncoding.DecodeString(*queryPageToken)
diff --git a/pkg/server/router/did.go b/pkg/server/router/did.go
index 352bc99..1b48a7b 100644
--- a/pkg/server/router/did.go
+++ b/pkg/server/router/did.go
@@ -276,7 +276,7 @@ func (dr DIDRouter) ListDIDsByMethod(c *gin.Context) {
 		Deleted: getIsDeleted,
 	}
 	var pageRequest pagination.PageRequest
-	if pagination.ParsePaginationParams(c, &pageRequest) {
+	if pagination.ParsePaginationQueryValues(c, &pageRequest) {
 		return
 	}
 	getDIDsRequest.PageRequest = pageRequest.ToServicePage()
diff --git a/pkg/server/router/presentation.go b/pkg/server/router/presentation.go
index 5b8fa2b..80f5c5d 100644
--- a/pkg/server/router/presentation.go
+++ b/pkg/server/router/presentation.go
@@ -405,7 +405,7 @@ func (pr PresentationRouter) ListSubmissions(c *gin.Context) {
 	}
 
 	var pageRequest pagination.PageRequest
-	if pagination.ParsePaginationParams(c, &pageRequest) {
+	if pagination.ParsePaginationQueryValues(c, &pageRequest) {
 		return
 	}
 


Test patch:
diff --git a/pkg/server/server_did_test.go b/pkg/server/server_did_test.go
index 7cfce51..2a2aa46 100644
--- a/pkg/server/server_did_test.go
+++ b/pkg/server/server_did_test.go
@@ -514,12 +514,11 @@ func TestDIDAPI(t *testing.T) {
 
 				w := httptest.NewRecorder()
 				badParams := url.Values{
-					"method":    []string{"key"},
 					"pageSize":  []string{"1"},
 					"pageToken": []string{"made up token"},
 				}
 				req := httptest.NewRequest(http.MethodGet, "https://ssi-service.com/v1/dids/key?"+badParams.Encode(), nil)
-				c := newRequestContextWithURLValues(w, req, badParams)
+				c := newRequestContextWithParams(w, req, map[string]string{"method": "key"})
 				didService.ListDIDsByMethod(c)
 				assert.Contains(tt, w.Body.String(), "token value cannot be decoded")
 			})
@@ -536,11 +535,10 @@ func TestDIDAPI(t *testing.T) {
 
 					w := httptest.NewRecorder()
 					params := url.Values{
-						"method":   []string{"key"},
 						"pageSize": []string{"1"},
 					}
 					req := httptest.NewRequest(http.MethodGet, "https://ssi-service.com/v1/dids/key?"+params.Encode(), nil)
-					c := newRequestContextWithURLValues(w, req, params)
+					c := newRequestContextWithParams(w, req, map[string]string{"method": "key"})
 
 					didRouter.ListDIDsByMethod(c)
 
@@ -553,7 +551,7 @@ func TestDIDAPI(t *testing.T) {
 					w = httptest.NewRecorder()
 					params["pageToken"] = []string{listDIDsByMethodResponse.NextPageToken}
 					req = httptest.NewRequest(http.MethodGet, "https://ssi-service.com/v1/dids/key?"+params.Encode(), nil)
-					c = newRequestContextWithURLValues(w, req, params)
+					c = newRequestContextWithParams(w, req, map[string]string{"method": "key"})
 
 					didRouter.ListDIDsByMethod(c)
 
@@ -576,12 +574,11 @@ func TestDIDAPI(t *testing.T) {
 
 					w := httptest.NewRecorder()
 					params := url.Values{
-						"method":   []string{"key"},
 						"pageSize": []string{"1"},
 					}
 					req := httptest.NewRequest(http.MethodGet, "https://ssi-service.com/v1/dids/key?"+params.Encode(), nil)
 
-					c := newRequestContextWithURLValues(w, req, params)
+					c := newRequestContextWithParams(w, req, map[string]string{"method": "key"})
 					didRouter.ListDIDsByMethod(c)
 					assert.True(tt, util.Is2xxResponse(w.Result().StatusCode))
 
@@ -595,7 +592,7 @@ func TestDIDAPI(t *testing.T) {
 					params["pageToken"] = []string{listDIDsByMethodResponse.NextPageToken}
 					params["deleted"] = []string{"true"}
 					req = httptest.NewRequest(http.MethodGet, "https://ssi-service.com/v1/dids/key?"+params.Encode(), nil)
-					c = newRequestContextWithURLValues(w, req, params)
+					c = newRequestContextWithParams(w, req, map[string]string{"method": "key"})
 					didRouter.ListDIDsByMethod(c)
 					assert.Equal(tt, http.StatusBadRequest, w.Result().StatusCode)
 					assert.Contains(tt, w.Body.String(), "page token must be for the same query")
diff --git a/pkg/server/server_presentation_test.go b/pkg/server/server_presentation_test.go
index d379365..a20a4d4 100644
--- a/pkg/server/server_presentation_test.go
+++ b/pkg/server/server_presentation_test.go
@@ -410,7 +410,7 @@ func TestPresentationAPI(t *testing.T) {
 						"pageSize": []string{"-1"},
 					}
 					req := httptest.NewRequest(http.MethodGet, "https://ssi-service.com/v1/presentations/submissions?"+badParams.Encode(), nil)
-					c := newRequestContextWithURLValues(w, req, badParams)
+					c := newRequestContext(w, req)
 					pRouter.ListSubmissions(c)
 
 					assert.Contains(tt, w.Body.String(), "'pageSize' must be greater than 0")
@@ -426,7 +426,7 @@ func TestPresentationAPI(t *testing.T) {
 						"pageToken": []string{"made up token"},
 					}
 					req := httptest.NewRequest(http.MethodGet, "https://ssi-service.com/v1/presentations/submissions?"+badParams.Encode(), nil)
-					c := newRequestContextWithURLValues(w, req, badParams)
+					c := newRequestContext(w, req)
 					pRouter.ListSubmissions(c)
 
 					assert.Contains(tt, w.Body.String(), "token value cannot be decoded")
@@ -467,7 +467,7 @@ func TestPresentationAPI(t *testing.T) {
 						"pageSize": []string{"1"},
 					}
 					req := httptest.NewRequest(http.MethodGet, "https://ssi-service.com/v1/presentations/submissions?"+params.Encode(), nil)
-					c := newRequestContextWithURLValues(w, req, params)
+					c := newRequestContext(w, req)
 
 					pRouter.ListSubmissions(c)
 
@@ -480,7 +480,7 @@ func TestPresentationAPI(t *testing.T) {
 					w = httptest.NewRecorder()
 					params["pageToken"] = []string{listSubmissionResponse.NextPageToken}
 					req = httptest.NewRequest(http.MethodGet, "https://ssi-service.com/v1/presentations/submissions?"+params.Encode(), nil)
-					c = newRequestContextWithURLValues(w, req, params)
+					c = newRequestContext(w, req)
 
 					pRouter.ListSubmissions(c)
 
@@ -526,7 +526,7 @@ func TestPresentationAPI(t *testing.T) {
 						"pageSize": []string{"1"},
 					}
 					req := httptest.NewRequest(http.MethodGet, "https://ssi-service.com/v1/presentations/submissions?"+params.Encode(), nil)
-					c := newRequestContextWithURLValues(w, req, params)
+					c := newRequestContext(w, req)
 
 					pRouter.ListSubmissions(c)
 
@@ -540,7 +540,7 @@ func TestPresentationAPI(t *testing.T) {
 					params["pageToken"] = []string{listSubmissionResponse.NextPageToken}
 					params["filter"] = []string{"status=\"pending\""}
 					req = httptest.NewRequest(http.MethodGet, "https://ssi-service.com/v1/presentations/submissions?"+params.Encode(), nil)
-					c = newRequestContextWithURLValues(w, req, params)
+					c = newRequestContext(w, req)
 
 					pRouter.ListSubmissions(c)
 					assert.Equal(tt, http.StatusBadRequest, w.Result().StatusCode)
diff --git a/pkg/server/server_test.go b/pkg/server/server_test.go
index 429b09f..00b5fc4 100644
--- a/pkg/server/server_test.go
+++ b/pkg/server/server_test.go
@@ -5,37 +5,32 @@ import (
 	"io"
 	"net/http"
 	"net/http/httptest"
-	"net/url"
 	"os"
 	"testing"
 
 	"github.com/TBD54566975/ssi-sdk/credential/exchange"
-	"github.com/gin-gonic/gin"
-
-	"github.com/tbd54566975/ssi-service/internal/util"
-	"github.com/tbd54566975/ssi-service/pkg/service/issuance"
-	"github.com/tbd54566975/ssi-service/pkg/service/manifest/model"
-	"github.com/tbd54566975/ssi-service/pkg/service/webhook"
-	"github.com/tbd54566975/ssi-service/pkg/testutil"
-
 	manifestsdk "github.com/TBD54566975/ssi-sdk/credential/manifest"
 	"github.com/TBD54566975/ssi-sdk/crypto"
+	"github.com/gin-gonic/gin"
 	"github.com/goccy/go-json"
 	"github.com/google/uuid"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
-
-	credmodel "github.com/tbd54566975/ssi-service/internal/credential"
-
 	"github.com/tbd54566975/ssi-service/config"
+	credmodel "github.com/tbd54566975/ssi-service/internal/credential"
+	"github.com/tbd54566975/ssi-service/internal/util"
 	"github.com/tbd54566975/ssi-service/pkg/server/router"
 	"github.com/tbd54566975/ssi-service/pkg/service/credential"
 	"github.com/tbd54566975/ssi-service/pkg/service/did"
 	svcframework "github.com/tbd54566975/ssi-service/pkg/service/framework"
+	"github.com/tbd54566975/ssi-service/pkg/service/issuance"
 	"github.com/tbd54566975/ssi-service/pkg/service/keystore"
 	"github.com/tbd54566975/ssi-service/pkg/service/manifest"
+	"github.com/tbd54566975/ssi-service/pkg/service/manifest/model"
 	"github.com/tbd54566975/ssi-service/pkg/service/schema"
+	"github.com/tbd54566975/ssi-service/pkg/service/webhook"
 	"github.com/tbd54566975/ssi-service/pkg/storage"
+	"github.com/tbd54566975/ssi-service/pkg/testutil"
 )
 
 const (
@@ -131,16 +126,6 @@ func newRequestContextWithParams(w http.ResponseWriter, req *http.Request, param
 	return c
 }
 
-func newRequestContextWithURLValues(w http.ResponseWriter, req *http.Request, params url.Values) *gin.Context {
-	c := newRequestContext(w, req)
-	for k, vs := range params {
-		for _, v := range vs {
-			c.AddParam(k, v)
-		}
-	}
-	return c
-}
-
 func getValidCreateManifestRequest(issuerDID, verificationMethodID, schemaID string) router.CreateManifestRequest {
 	return router.CreateManifestRequest{
 		IssuerDID:            issuerDID,