From 493ec45be6028008bbeb6c2b0051927ef83ff904 Mon Sep 17 00:00:00 2001 From: Avinil Bedarkar <111761529+avinilcode@users.noreply.github.com> Date: Tue, 18 Oct 2022 05:18:04 +0530 Subject: [PATCH] Return return 404 if registry to delete do not exist (#1278) Closes #524 Co-authored-by: 6543 <6543@obermui.de> --- cmd/server/setup.go | 4 +++- server/api/registry.go | 9 ++++++++- server/store/datastore/helper.go | 8 ++++---- server/store/datastore/redirection_test.go | 3 ++- server/store/datastore/registry_test.go | 19 +++++++++++++++++++ server/store/datastore/repo.go | 17 ++++++++++++----- server/store/types/errors.go | 19 +++++++++++++++++++ 7 files changed, 67 insertions(+), 12 deletions(-) create mode 100644 server/store/types/errors.go diff --git a/cmd/server/setup.go b/cmd/server/setup.go index b5a952b63..097a3a170 100644 --- a/cmd/server/setup.go +++ b/cmd/server/setup.go @@ -20,6 +20,7 @@ import ( "crypto/ed25519" "crypto/rand" "encoding/hex" + "errors" "fmt" "net/url" "os" @@ -49,6 +50,7 @@ import ( "github.com/woodpecker-ci/woodpecker/server/remote/gogs" "github.com/woodpecker-ci/woodpecker/server/store" "github.com/woodpecker-ci/woodpecker/server/store/datastore" + "github.com/woodpecker-ci/woodpecker/server/store/types" ) func setupStore(c *cli.Context) (store.Store, error) { @@ -365,7 +367,7 @@ func setupSignatureKeys(_store store.Store) (crypto.PrivateKey, crypto.PublicKey privKeyID := "signature-private-key" privKey, err := _store.ServerConfigGet(privKeyID) - if err != nil && err == datastore.RecordNotExist { + if errors.Is(err, types.RecordNotExist) { _, privKey, err := ed25519.GenerateKey(rand.Reader) if err != nil { log.Fatal().Err(err).Msgf("Failed to generate private key") diff --git a/server/api/registry.go b/server/api/registry.go index 61bd7cdef..caa4f6c9f 100644 --- a/server/api/registry.go +++ b/server/api/registry.go @@ -15,6 +15,7 @@ package api import ( + "errors" "net/http" "github.com/gin-gonic/gin" @@ -22,6 +23,7 @@ import ( "github.com/woodpecker-ci/woodpecker/server" "github.com/woodpecker-ci/woodpecker/server/model" "github.com/woodpecker-ci/woodpecker/server/router/middleware/session" + "github.com/woodpecker-ci/woodpecker/server/store/types" ) // GetRegistry gets the name registry from the database and writes @@ -133,7 +135,12 @@ func DeleteRegistry(c *gin.Context) { repo = session.Repo(c) name = c.Param("registry") ) - if err := server.Config.Services.Registries.RegistryDelete(repo, name); err != nil { + err := server.Config.Services.Registries.RegistryDelete(repo, name) + if err != nil { + if errors.Is(err, types.RecordNotExist) { + c.String(404, "no records found, cannot delete registry") + return + } c.String(500, "Error deleting registry %q. %s", name, err) return } diff --git a/server/store/datastore/helper.go b/server/store/datastore/helper.go index 6fc7744f2..b58d49c21 100644 --- a/server/store/datastore/helper.go +++ b/server/store/datastore/helper.go @@ -14,9 +14,9 @@ package datastore -import "database/sql" - -var RecordNotExist = sql.ErrNoRows +import ( + "github.com/woodpecker-ci/woodpecker/server/store/types" +) // wrapGet return error if err not nil or if requested entry do not exist func wrapGet(exist bool, err error) error { @@ -24,7 +24,7 @@ func wrapGet(exist bool, err error) error { return err } if !exist { - return RecordNotExist + return types.RecordNotExist } return nil } diff --git a/server/store/datastore/redirection_test.go b/server/store/datastore/redirection_test.go index e536bc579..00bf3dc66 100644 --- a/server/store/datastore/redirection_test.go +++ b/server/store/datastore/redirection_test.go @@ -20,6 +20,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/woodpecker-ci/woodpecker/server/model" + "github.com/woodpecker-ci/woodpecker/server/store/types" ) func TestGetRedirection(t *testing.T) { @@ -39,7 +40,7 @@ func TestGetRedirection(t *testing.T) { assert.NotNil(t, redirectionFromStore) assert.Equal(t, redirection.RepoID, redirectionFromStore.RepoID) _, err = store.GetRedirection("foo/baz") - assert.ErrorIs(t, err, RecordNotExist) + assert.ErrorIs(t, err, types.RecordNotExist) } func TestCreateRedirection(t *testing.T) { diff --git a/server/store/datastore/registry_test.go b/server/store/datastore/registry_test.go index 967d273e3..4ed8b05dd 100644 --- a/server/store/datastore/registry_test.go +++ b/server/store/datastore/registry_test.go @@ -20,6 +20,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/woodpecker-ci/woodpecker/server/model" + "github.com/woodpecker-ci/woodpecker/server/store/types" ) func TestRegistryFind(t *testing.T) { @@ -144,3 +145,21 @@ func TestRegistryIndexes(t *testing.T) { t.Errorf("Unexpected error: duplicate address") } } + +func TestRegistryDelete(t *testing.T) { + store, closer := newTestStore(t, new(model.Registry), new(model.Repo)) + defer closer() + + reg1 := &model.Registry{ + RepoID: 1, + Address: "index.docker.io", + Username: "foo", + Password: "bar", + } + if !assert.NoError(t, store.RegistryCreate(reg1)) { + t.FailNow() + } + + assert.NoError(t, store.RegistryDelete(&model.Repo{ID: 1}, "index.docker.io")) + assert.ErrorIs(t, store.RegistryDelete(&model.Repo{ID: 1}, "index.docker.io"), types.RecordNotExist) +} diff --git a/server/store/datastore/repo.go b/server/store/datastore/repo.go index a46a174d9..1f15ed0f0 100644 --- a/server/store/datastore/repo.go +++ b/server/store/datastore/repo.go @@ -15,11 +15,14 @@ package datastore import ( + "errors" + "github.com/rs/zerolog/log" "xorm.io/builder" "xorm.io/xorm" "github.com/woodpecker-ci/woodpecker/server/model" + "github.com/woodpecker-ci/woodpecker/server/store/types" ) func (s storage) GetRepo(id int64) (*model.Repo, error) { @@ -46,7 +49,7 @@ func (s storage) GetRepoNameFallback(remoteID model.RemoteID, fullName string) ( func (s storage) getRepoNameFallback(e *xorm.Session, remoteID model.RemoteID, fullName string) (*model.Repo, error) { repo, err := s.getRepoRemoteID(e, remoteID) - if err == RecordNotExist { + if errors.Is(err, types.RecordNotExist) { return s.getRepoName(e, fullName) } return repo, err @@ -56,7 +59,7 @@ func (s storage) GetRepoName(fullName string) (*model.Repo, error) { sess := s.engine.NewSession() defer sess.Close() repo, err := s.getRepoName(sess, fullName) - if err == RecordNotExist { + if errors.Is(err, types.RecordNotExist) { // the repository does not exist, so look for a redirection redirect, err := s.getRedirection(sess, fullName) if err != nil { @@ -165,11 +168,15 @@ func (s storage) RepoBatch(repos []*model.Repo) error { continue } + exist := true repo, err := s.getRepoNameFallback(sess, repos[i].RemoteID, repos[i].FullName) - if err != nil && err != RecordNotExist { - return err + if err != nil { + if errors.Is(err, types.RecordNotExist) { + exist = false + } else { + return err + } } - exist := err == nil // if there's an error, it must be a RecordNotExist if exist { if repos[i].FullName != repo.FullName { diff --git a/server/store/types/errors.go b/server/store/types/errors.go new file mode 100644 index 000000000..c7901a1c6 --- /dev/null +++ b/server/store/types/errors.go @@ -0,0 +1,19 @@ +// Copyright 2022 Woodpecker Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package types + +import "database/sql" + +var RecordNotExist = sql.ErrNoRows