Refactor immutable tag rule

Change implementation
Fix some nil pointer issue

Signed-off-by: stonezdj <stonezdj@gmail.com>
This commit is contained in:
stonezdj 2019-11-13 19:09:28 +08:00
parent 75aa29d23e
commit a3c298e9fd
6 changed files with 62 additions and 13 deletions

View File

@ -40,13 +40,22 @@ func (itr *ImmutableTagRuleAPI) Prepare() {
return return
} }
itr.projectID = pid itr.projectID = pid
itr.ctr = immutabletag.ImmuCtr
ruleID, err := itr.GetInt64FromPath(":id") ruleID, err := itr.GetInt64FromPath(":id")
if err == nil || ruleID > 0 { if err == nil || ruleID > 0 {
itr.ID = ruleID itr.ID = ruleID
itRule, err := itr.ctr.GetImmutableRule(itr.ID)
if err != nil {
itr.SendInternalServerError(err)
return
}
if itRule == nil || itRule.ProjectID != itr.projectID {
err := fmt.Errorf("immutable tag rule %v not found", itr.ID)
itr.SendNotFoundError(err)
return
} }
itr.ctr = immutabletag.ImmuCtr }
if strings.EqualFold(itr.Ctx.Request.Method, "get") { if strings.EqualFold(itr.Ctx.Request.Method, "get") {
if !itr.requireAccess(rbac.ActionList) { if !itr.requireAccess(rbac.ActionList) {

View File

@ -228,6 +228,7 @@ func TestImmutableTagRuleAPI_Put(t *testing.T) {
defer mgr.DeleteImmutableRule(id) defer mgr.DeleteImmutableRule(id)
url := fmt.Sprintf("/api/projects/1/immutabletagrules/%d", id) url := fmt.Sprintf("/api/projects/1/immutabletagrules/%d", id)
url2 := fmt.Sprintf("/api/projects/3/immutabletagrules/%d", id)
cases := []*codeCheckingCase{ cases := []*codeCheckingCase{
// 401 // 401
{ {
@ -268,6 +269,16 @@ func TestImmutableTagRuleAPI_Put(t *testing.T) {
}, },
code: http.StatusForbidden, code: http.StatusForbidden,
}, },
// 404
{
request: &testingRequest{
method: http.MethodPut,
url: url2,
credential: projAdmin,
bodyJSON: metadata2,
},
code: http.StatusNotFound,
},
} }
runCodeCheckingCases(t, cases...) runCodeCheckingCases(t, cases...)
} }
@ -302,6 +313,7 @@ func TestImmutableTagRuleAPI_Delete(t *testing.T) {
defer mgr.DeleteImmutableRule(id) defer mgr.DeleteImmutableRule(id)
url := fmt.Sprintf("/api/projects/1/immutabletagrules/%d", id) url := fmt.Sprintf("/api/projects/1/immutabletagrules/%d", id)
wrongURL := fmt.Sprintf("/api/projects/3/immutabletagrules/%d", id)
cases := []*codeCheckingCase{ cases := []*codeCheckingCase{
// 401 // 401
@ -321,6 +333,15 @@ func TestImmutableTagRuleAPI_Delete(t *testing.T) {
}, },
code: http.StatusForbidden, code: http.StatusForbidden,
}, },
// 404
{
request: &testingRequest{
method: http.MethodDelete,
url: wrongURL,
credential: projAdmin,
},
code: http.StatusNotFound,
},
// 200 // 200
{ {
request: &testingRequest{ request: &testingRequest{
@ -330,6 +351,15 @@ func TestImmutableTagRuleAPI_Delete(t *testing.T) {
}, },
code: http.StatusOK, code: http.StatusOK,
}, },
// 404
{
request: &testingRequest{
method: http.MethodDelete,
url: url,
credential: projAdmin,
},
code: http.StatusNotFound,
},
} }
runCodeCheckingCases(t, cases...) runCodeCheckingCases(t, cases...)
} }

View File

@ -1,6 +1,8 @@
package immutabletag package immutabletag
import ( import (
"fmt"
"github.com/goharbor/harbor/src/pkg/immutabletag/model" "github.com/goharbor/harbor/src/pkg/immutabletag/model"
) )
@ -54,6 +56,9 @@ func (r *DefaultAPIController) UpdateImmutableRule(pid int64, m *model.Metadata)
if err != nil { if err != nil {
return err return err
} }
if m0 == nil {
return fmt.Errorf("the immutable tag rule is not found id:%v", m.ID)
}
if m0.Disabled != m.Disabled { if m0.Disabled != m.Disabled {
_, err := r.manager.EnableImmutableRule(m.ID, m.Disabled) _, err := r.manager.EnableImmutableRule(m.ID, m.Disabled)
return err return err

View File

@ -1,6 +1,8 @@
package immutabletag package immutabletag
import ( import (
"testing"
"github.com/goharbor/harbor/src/common/dao" "github.com/goharbor/harbor/src/common/dao"
"github.com/goharbor/harbor/src/common/models" "github.com/goharbor/harbor/src/common/models"
"github.com/goharbor/harbor/src/common/utils/test" "github.com/goharbor/harbor/src/common/utils/test"
@ -8,7 +10,6 @@ import (
"github.com/stretchr/testify/assert" "github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require" "github.com/stretchr/testify/require"
"github.com/stretchr/testify/suite" "github.com/stretchr/testify/suite"
"testing"
) )
type ControllerTestSuite struct { type ControllerTestSuite struct {
@ -88,7 +89,7 @@ func (s *ControllerTestSuite) TestImmutableRule() {
}, },
Disabled: false, Disabled: false,
} }
err = s.ctr.UpdateImmutableRule(1, update) err = s.ctr.UpdateImmutableRule(projectID, update)
s.require.Nil(err) s.require.Nil(err)
getRule, err := s.ctr.GetImmutableRule(s.ruleID) getRule, err := s.ctr.GetImmutableRule(s.ruleID)
@ -119,7 +120,7 @@ func (s *ControllerTestSuite) TestImmutableRule() {
}, },
Disabled: true, Disabled: true,
} }
err = s.ctr.UpdateImmutableRule(1, update2) err = s.ctr.UpdateImmutableRule(projectID, update2)
s.require.Nil(err) s.require.Nil(err)
getRule, err = s.ctr.GetImmutableRule(s.ruleID) getRule, err = s.ctr.GetImmutableRule(s.ruleID)
s.require.Nil(err) s.require.Nil(err)
@ -150,7 +151,7 @@ func (s *ControllerTestSuite) TestImmutableRule() {
s.ruleID, err = s.ctr.CreateImmutableRule(rule2) s.ruleID, err = s.ctr.CreateImmutableRule(rule2)
s.require.Nil(err) s.require.Nil(err)
rules, err := s.ctr.ListImmutableRules(1) rules, err := s.ctr.ListImmutableRules(projectID)
s.require.Nil(err) s.require.Nil(err)
s.require.Equal(2, len(rules)) s.require.Equal(2, len(rules))

View File

@ -2,10 +2,11 @@ package immutabletag
import ( import (
"encoding/json" "encoding/json"
"sort"
"github.com/goharbor/harbor/src/pkg/immutabletag/dao" "github.com/goharbor/harbor/src/pkg/immutabletag/dao"
dao_model "github.com/goharbor/harbor/src/pkg/immutabletag/dao/model" dao_model "github.com/goharbor/harbor/src/pkg/immutabletag/dao/model"
"github.com/goharbor/harbor/src/pkg/immutabletag/model" "github.com/goharbor/harbor/src/pkg/immutabletag/model"
"sort"
) )
var ( var (
@ -62,6 +63,9 @@ func (drm *defaultRuleManager) GetImmutableRule(id int64) (*model.Metadata, erro
return nil, err return nil, err
} }
rule := &model.Metadata{} rule := &model.Metadata{}
if daoRule == nil {
return nil, nil
}
if err = json.Unmarshal([]byte(daoRule.TagFilter), rule); err != nil { if err = json.Unmarshal([]byte(daoRule.TagFilter), rule); err != nil {
return nil, err return nil, err
} }

View File

@ -111,7 +111,7 @@ func (m *managerTestingSuite) TestQueryImmutableRuleByProjectID() {
ID: 1, ID: 1,
ProjectID: 1, ProjectID: 1,
Disabled: false, Disabled: false,
TagFilter: "{\"id\":1, \"projectID\":1,\"priority\":0,\"disabled\":false,\"action\":\"immutable\"," + TagFilter: "{\"id\":1, \"project_id\":1,\"priority\":0,\"disabled\":false,\"action\":\"immutable\"," +
"\"template\":\"immutable_template\"," + "\"template\":\"immutable_template\"," +
"\"tag_selectors\":[{\"kind\":\"doublestar\",\"decoration\":\"matches\",\"pattern\":\"**\"}]," + "\"tag_selectors\":[{\"kind\":\"doublestar\",\"decoration\":\"matches\",\"pattern\":\"**\"}]," +
"\"scope_selectors\":{\"repository\":[{\"kind\":\"doublestar\",\"decoration\":\"repoMatches\",\"pattern\":\"**\"}]}}", "\"scope_selectors\":{\"repository\":[{\"kind\":\"doublestar\",\"decoration\":\"repoMatches\",\"pattern\":\"**\"}]}}",
@ -120,7 +120,7 @@ func (m *managerTestingSuite) TestQueryImmutableRuleByProjectID() {
ID: 2, ID: 2,
ProjectID: 1, ProjectID: 1,
Disabled: false, Disabled: false,
TagFilter: "{\"id\":2, \"projectID\":1,\"priority\":0,\"disabled\":false,\"action\":\"immutable\"," + TagFilter: "{\"id\":2, \"project_id\":1,\"priority\":0,\"disabled\":false,\"action\":\"immutable\"," +
"\"template\":\"immutable_template\"," + "\"template\":\"immutable_template\"," +
"\"tag_selectors\":[{\"kind\":\"doublestar\",\"decoration\":\"matches\",\"pattern\":\"**\"}]," + "\"tag_selectors\":[{\"kind\":\"doublestar\",\"decoration\":\"matches\",\"pattern\":\"**\"}]," +
"\"scope_selectors\":{\"repository\":[{\"kind\":\"doublestar\",\"decoration\":\"repoMatches\",\"pattern\":\"**\"}]}}", "\"scope_selectors\":{\"repository\":[{\"kind\":\"doublestar\",\"decoration\":\"repoMatches\",\"pattern\":\"**\"}]}}",
@ -138,7 +138,7 @@ func (m *managerTestingSuite) TestQueryEnabledImmutableRuleByProjectID() {
ID: 1, ID: 1,
ProjectID: 1, ProjectID: 1,
Disabled: true, Disabled: true,
TagFilter: "{\"id\":1, \"projectID\":1,\"priority\":0,\"disabled\":false,\"action\":\"immutable\"," + TagFilter: "{\"id\":1, \"project_id\":1,\"priority\":0,\"disabled\":false,\"action\":\"immutable\"," +
"\"template\":\"immutable_template\"," + "\"template\":\"immutable_template\"," +
"\"tag_selectors\":[{\"kind\":\"doublestar\",\"decoration\":\"matches\",\"pattern\":\"**\"}]," + "\"tag_selectors\":[{\"kind\":\"doublestar\",\"decoration\":\"matches\",\"pattern\":\"**\"}]," +
"\"scope_selectors\":{\"repository\":[{\"kind\":\"doublestar\",\"decoration\":\"repoMatches\",\"pattern\":\"**\"}]}}", "\"scope_selectors\":{\"repository\":[{\"kind\":\"doublestar\",\"decoration\":\"repoMatches\",\"pattern\":\"**\"}]}}",
@ -147,7 +147,7 @@ func (m *managerTestingSuite) TestQueryEnabledImmutableRuleByProjectID() {
ID: 2, ID: 2,
ProjectID: 1, ProjectID: 1,
Disabled: true, Disabled: true,
TagFilter: "{\"id\":2, \"projectID\":1,\"priority\":0,\"disabled\":false,\"action\":\"immutable\"," + TagFilter: "{\"id\":2, \"project_id\":1,\"priority\":0,\"disabled\":false,\"action\":\"immutable\"," +
"\"template\":\"immutable_template\"," + "\"template\":\"immutable_template\"," +
"\"tag_selectors\":[{\"kind\":\"doublestar\",\"decoration\":\"matches\",\"pattern\":\"**\"}]," + "\"tag_selectors\":[{\"kind\":\"doublestar\",\"decoration\":\"matches\",\"pattern\":\"**\"}]," +
"\"scope_selectors\":{\"repository\":[{\"kind\":\"doublestar\",\"decoration\":\"repoMatches\",\"pattern\":\"**\"}]}}", "\"scope_selectors\":{\"repository\":[{\"kind\":\"doublestar\",\"decoration\":\"repoMatches\",\"pattern\":\"**\"}]}}",
@ -164,7 +164,7 @@ func (m *managerTestingSuite) TestGetImmutableRule() {
ID: 1, ID: 1,
ProjectID: 1, ProjectID: 1,
Disabled: true, Disabled: true,
TagFilter: "{\"id\":1, \"projectID\":1,\"priority\":0,\"disabled\":false,\"action\":\"immutable\"," + TagFilter: "{\"id\":1, \"project_id\":1,\"priority\":0,\"disabled\":false,\"action\":\"immutable\"," +
"\"template\":\"immutable_template\"," + "\"template\":\"immutable_template\"," +
"\"tag_selectors\":[{\"kind\":\"doublestar\",\"decoration\":\"matches\",\"pattern\":\"**\"}]," + "\"tag_selectors\":[{\"kind\":\"doublestar\",\"decoration\":\"matches\",\"pattern\":\"**\"}]," +
"\"scope_selectors\":{\"repository\":[{\"kind\":\"doublestar\",\"decoration\":\"repoMatches\",\"pattern\":\"**\"}]}}", "\"scope_selectors\":{\"repository\":[{\"kind\":\"doublestar\",\"decoration\":\"repoMatches\",\"pattern\":\"**\"}]}}",