Repository: kubesphere/ks-devops
Pull request #280 (2021-09-28 12:52:38)

Fix the wrong using of paging parameter start
fix https://github.com/kubesphere/ks-devops/issues/279

Container image for the test purpose:
```
surenpi/devops-apiserver:dev-v3.2.0-alpha.0-b60b654
```

ks-ci-bot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/kubesphere/ks-devops/pull/280#" title="Author self-approved">LinuxSuRen</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=kubesphere%2Fks-devops).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/kubesphere/ks-devops/blob/master/OWNERS)~~ [LinuxSuRen]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
codecov[bot]: # [Codecov](https://codecov.io/gh/kubesphere/ks-devops/pull/280?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubesphere) Report
> Merging [#280](https://codecov.io/gh/kubesphere/ks-devops/pull/280?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubesphere) (b60b654) into [master](https://codecov.io/gh/kubesphere/ks-devops/commit/625e555ddd5df7f5a2fa5681ebc86af632979b16?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubesphere) (625e555) will **increase** coverage by `0.07%`.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/kubesphere/ks-devops/pull/280/graphs/tree.svg?width=650&height=150&src=pr&token=qSK4RBbyNg&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubesphere)](https://codecov.io/gh/kubesphere/ks-devops/pull/280?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubesphere)

```diff
@@            Coverage Diff            @@
##           master    #280      +/-   ##
=========================================
+ Coverage    7.57%   7.65%   +0.07%     
=========================================
  Files          89      89              
  Lines       21303   21314      +11     
=========================================
+ Hits         1614    1631      +17     
+ Misses      19567   19560       -7     
- Partials      122     123       +1     
```

| Flag | Coverage Δ | |
|---|---|---|
| unittests | `7.65% <100.00%> (+0.07%)` | :arrow_up: |

Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubesphere#carryforward-flags-in-the-pull-request-comment) to find out more.

| [Impacted Files](https://codecov.io/gh/kubesphere/ks-devops/pull/280?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubesphere) | Coverage Δ | |
|---|---|---|
| [pkg/apiserver/query/types.go](https://codecov.io/gh/kubesphere/ks-devops/pull/280/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubesphere#diff-cGtnL2FwaXNlcnZlci9xdWVyeS90eXBlcy5nbw==) | `100.00% <100.00%> (+11.76%)` | :arrow_up: |
| [pkg/kapis/devops/v1alpha2/devops.go](https://codecov.io/gh/kubesphere/ks-devops/pull/280/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubesphere#diff-cGtnL2thcGlzL2Rldm9wcy92MWFscGhhMi9kZXZvcHMuZ28=) | `4.34% <100.00%> (+2.16%)` | :arrow_up: |
| [controllers/s2ibinary/s2ibinary\_controller.go](https://codecov.io/gh/kubesphere/ks-devops/pull/280/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubesphere#diff-Y29udHJvbGxlcnMvczJpYmluYXJ5L3MyaWJpbmFyeV9jb250cm9sbGVyLmdv) | `33.00% <0.00%> (-5.00%)` | :arrow_down: |
| [controllers/s2irun/s2irun\_controller.go](https://codecov.io/gh/kubesphere/ks-devops/pull/280/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubesphere#diff-Y29udHJvbGxlcnMvczJpcnVuL3MyaXJ1bl9jb250cm9sbGVyLmdv) | `33.57% <0.00%> (-3.65%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/kubesphere/ks-devops/pull/280?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubesphere).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubesphere)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/kubesphere/ks-devops/pull/280?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubesphere). Last update [625e555...b60b654](https://codecov.io/gh/kubesphere/ks-devops/pull/280?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubesphere). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubesphere).

JohnNiang: /lgtm

Patch:
diff --git a/pkg/apiserver/query/types.go b/pkg/apiserver/query/types.go
index f2dfcb3..9260118 100644
--- a/pkg/apiserver/query/types.go
+++ b/pkg/apiserver/query/types.go
@@ -29,6 +29,7 @@ const (
 	ParameterName          = "name"
 	ParameterLabelSelector = "labelSelector"
 	ParameterFieldSelector = "fieldSelector"
+	ParameterStart         = "start"
 	ParameterPage          = "page"
 	ParameterLimit         = "limit"
 	ParameterOrderBy       = "sortBy"
@@ -118,19 +119,31 @@ type Filter struct {
 
 func ParseQueryParameter(request *restful.Request) *Query {
 	query := New()
+	var start int
+	var page int
+	var limit int
+	var err error
 
-	limit, err := strconv.Atoi(request.QueryParameter(ParameterLimit))
+	limit, err = strconv.Atoi(request.QueryParameter(ParameterLimit))
 	// equivalent to undefined, use the default value
 	if err != nil || limit < -1 {
 		limit = DefaultLimit
 	}
-	page, err := strconv.Atoi(request.QueryParameter(ParameterPage))
-	// equivalent to undefined, use the default value
-	if err != nil || page < 1 {
-		page = DefaultPage
+	if pageStr := request.QueryParameter(ParameterPage); pageStr != "" {
+		if page, err = strconv.Atoi(pageStr);err != nil {
+			page = DefaultPage
+		}
+	} else {
+		if start, err = strconv.Atoi(request.QueryParameter(ParameterStart)); err != nil {
+			start = 0
+		}
 	}
 
-	query.Pagination = newPagination(limit, (page-1)*limit)
+	if page >= 1 {
+		query.Pagination = newPagination(limit, (page-1)*limit)
+	} else if start >= 0 {
+		query.Pagination = newPagination(limit, start)
+	}
 
 	query.SortBy = Field(defaultString(request.QueryParameter(ParameterOrderBy), FieldCreationTimeStamp))
 
diff --git a/pkg/kapis/devops/v1alpha2/devops.go b/pkg/kapis/devops/v1alpha2/devops.go
index 249c040..7195f32 100644
--- a/pkg/kapis/devops/v1alpha2/devops.go
+++ b/pkg/kapis/devops/v1alpha2/devops.go
@@ -72,9 +72,11 @@ func (h *ProjectPipelineHandler) getPipelinesByRequest(req *restful.Request) (ap
 }
 
 func buildPipelineSearchQueryParam(req *restful.Request, nameReg string) (q *query.Query) {
-	// for pagination compatibility
-	start := req.QueryParameter("start")
-	req.Request.Form.Set(query.ParameterPage, start)
+	startStr := req.QueryParameter(query.ParameterStart)
+	if req.Request.Form.Get(query.ParameterPage) == "" && startStr != "" {
+		// for pagination compatibility
+		req.Request.Form.Set(query.ParameterStart, startStr)
+	}
 
 	q = query.ParseQueryParameter(req)
 


Test patch:
diff --git a/pkg/apiserver/query/types_test.go b/pkg/apiserver/query/types_test.go
index 725621b..1a80270 100644
--- a/pkg/apiserver/query/types_test.go
+++ b/pkg/apiserver/query/types_test.go
@@ -19,7 +19,9 @@ package query
 import (
 	"fmt"
 	"github.com/stretchr/testify/assert"
+	"k8s.io/apimachinery/pkg/labels"
 	"net/http"
+	"reflect"
 	"testing"
 
 	"github.com/emicklei/go-restful"
@@ -32,9 +34,9 @@ func TestParseQueryParameter(t *testing.T) {
 		queryString string
 		expected    *Query
 	}{{
-		"test normal case",
-		"label=app.kubernetes.io/name=book&name=foo&status=Running&page=1&limit=10&ascending=true",
-		&Query{
+		description: "test normal case",
+		queryString: "label=app.kubernetes.io/name=book&name=foo&status=Running&page=1&limit=10&ascending=true",
+		expected: &Query{
 			Pagination: newPagination(10, 0),
 			SortBy:     FieldCreationTimeStamp,
 			Ascending:  true,
@@ -45,9 +47,9 @@ func TestParseQueryParameter(t *testing.T) {
 			},
 		},
 	}, {
-		"test bad case",
-		"xxxx=xxxx&dsfsw=xxxx&page=abc&limit=add&ascending=ssss",
-		&Query{
+		description: "invalid page and limit parameter",
+		queryString: "xxxx=xxxx&dsfsw=xxxx&page=abc&limit=add&ascending=ssss",
+		expected: &Query{
 			Pagination: NoPagination,
 			SortBy:     FieldCreationTimeStamp,
 			Ascending:  false,
@@ -56,8 +58,33 @@ func TestParseQueryParameter(t *testing.T) {
 				Field("dsfsw"): Value("xxxx"),
 			},
 		},
-	},
-	}
+	}, {
+		description: "have parameter 'start' instead of 'page'",
+		queryString: "start=10&limit=10",
+		expected: &Query{
+			Pagination: &Pagination{
+				Limit:  10,
+				Offset: 10,
+			},
+			SortBy: FieldCreationTimeStamp,
+			Filters: map[Field]Value{
+				Field("start"): Value("10"),
+			},
+		},
+	}, {
+		description: "have invalid parameter 'start'",
+		queryString: "start=a&limit=10",
+		expected: &Query{
+			Pagination: &Pagination{
+				Limit:  10,
+				Offset: 0,
+			},
+			SortBy: FieldCreationTimeStamp,
+			Filters: map[Field]Value{
+				Field("start"): Value("a"),
+			},
+		},
+	}}
 
 	for _, test := range tests {
 		req, err := http.NewRequest("GET", fmt.Sprintf("http://localhost?%s", test.queryString), nil)
@@ -153,3 +180,54 @@ func TestPagination_GetValidPagination(t *testing.T) {
 	}
 
 }
+
+func Test_newPagination(t *testing.T) {
+	type args struct {
+		limit  int
+		offset int
+	}
+	tests := []struct {
+		name string
+		args args
+		want *Pagination
+	}{{
+		name: "invalid offset and limit",
+		args: args{
+			offset: -1,
+			limit:  -1,
+		},
+		want: &Pagination{
+			Limit:  DefaultLimit,
+			Offset: 0,
+		},
+	}, {
+		name: "valid offset and limit",
+		args: args{
+			offset: 10,
+			limit:  10,
+		},
+		want: &Pagination{
+			Limit:  10,
+			Offset: 10,
+		},
+	}}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := newPagination(tt.args.limit, tt.args.offset); !reflect.DeepEqual(got, tt.want) {
+				t.Errorf("newPagination() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}
+
+func TestSelector(t *testing.T) {
+	// valid query selector
+	query := &Query{}
+	assert.Equal(t, labels.NewSelector(), query.Selector())
+
+	// invalid query selector
+	query = &Query{
+		LabelSelector: "a+b",
+	}
+	assert.Equal(t, labels.Everything(), query.Selector())
+}
diff --git a/pkg/kapis/devops/v1alpha2/devops_test.go b/pkg/kapis/devops/v1alpha2/devops_test.go
index a7c626f..1a98b56 100644
--- a/pkg/kapis/devops/v1alpha2/devops_test.go
+++ b/pkg/kapis/devops/v1alpha2/devops_test.go
@@ -1,7 +1,9 @@
 package v1alpha2
 
 import (
+	"github.com/emicklei/go-restful"
 	"github.com/stretchr/testify/assert"
+	"net/http"
 	"testing"
 
 	"kubesphere.io/devops/pkg/api/devops/v1alpha3"
@@ -48,3 +50,35 @@ func TestParseNameFilterFromQuery(t *testing.T) {
 		})
 	}
 }
+
+func TestBuildPipelineSearchQueryParam(t *testing.T) {
+	httpReq, _ := http.NewRequest(http.MethodGet, "http://localhost?start=10&limit=10", nil)
+	req := &restful.Request{
+		Request: httpReq,
+	}
+	nameReg := ""
+	query := buildPipelineSearchQueryParam(req, nameReg)
+	assert.NotNil(t, query)
+	assert.Equal(t, 10, query.Pagination.Offset)
+	assert.Equal(t, 10, query.Pagination.Limit)
+
+	// use parameter: 'page'
+	httpReq, _ = http.NewRequest(http.MethodGet, "http://localhost?page=2&limit=20", nil)
+	req = &restful.Request{
+		Request: httpReq,
+	}
+	query = buildPipelineSearchQueryParam(req, nameReg)
+	assert.NotNil(t, query)
+	assert.Equal(t, 20, query.Pagination.Offset)
+	assert.Equal(t, 20, query.Pagination.Limit)
+
+	// mixed with parameter 'start' and 'page', take 'page` as high priority
+	httpReq, _ = http.NewRequest(http.MethodGet, "http://localhost?page=2&limit=20&start=100", nil)
+	req = &restful.Request{
+		Request: httpReq,
+	}
+	query = buildPipelineSearchQueryParam(req, nameReg)
+	assert.NotNil(t, query)
+	assert.Equal(t, 20, query.Pagination.Offset)
+	assert.Equal(t, 20, query.Pagination.Limit)
+}