diff --git a/go.mod b/go.mod index 0864923..d458dc9 100644 --- a/go.mod +++ b/go.mod @@ -57,6 +57,8 @@ require ( require ( github.com/andybalholm/brotli v1.0.5 // indirect github.com/davecgh/go-spew v1.1.1 // indirect + github.com/evanphx/json-patch v0.5.2 // indirect + github.com/evanphx/json-patch/v5 v5.6.0 // indirect github.com/go-playground/locales v0.14.0 // indirect github.com/go-playground/universal-translator v0.18.0 // indirect github.com/jackc/pgpassfile v1.0.0 // indirect diff --git a/go.sum b/go.sum index 64c17c3..8c11bb8 100644 --- a/go.sum +++ b/go.sum @@ -80,6 +80,10 @@ github.com/envoyproxy/go-control-plane v0.9.0/go.mod h1:YTl/9mNaCwkRvm6d1a2C3ymF github.com/envoyproxy/go-control-plane v0.9.1-0.20191026205805-5f8ba28d4473/go.mod h1:YTl/9mNaCwkRvm6d1a2C3ymFceY/DCBVvsKhRF0iEA4= github.com/envoyproxy/go-control-plane v0.9.4/go.mod h1:6rpuAdCZL397s3pYoYcLgu1mIlRU8Am5FuJP05cCM98= github.com/envoyproxy/protoc-gen-validate v0.1.0/go.mod h1:iSmxcyjqTsJpI2R4NaDN7+kN2VEUnK/pcBlmesArF7c= +github.com/evanphx/json-patch v0.5.2 h1:xVCHIVMUu1wtM/VkR9jVZ45N3FhZfYMMYGorLCR8P3k= +github.com/evanphx/json-patch v0.5.2/go.mod h1:ZWS5hhDbVDyob71nXKNL0+PWn6ToqBHMikGIFbs31qQ= +github.com/evanphx/json-patch/v5 v5.6.0 h1:b91NhWfaz02IuVxO9faSllyAtNXHMPkC5J8sJCLunww= +github.com/evanphx/json-patch/v5 v5.6.0/go.mod h1:G79N1coSVB93tBe7j6PhzjmR3/2VvlbKOFpnXhI9Bw4= github.com/go-gl/glfw v0.0.0-20190409004039-e6da0acd62b1/go.mod h1:vR7hzQXu2zJy9AVAgeJqvqgH9Q5CA+iKCZ2gyEVpxRU= github.com/go-gl/glfw/v3.3/glfw v0.0.0-20191125211704-12ad95a8df72/go.mod h1:tQ2UAYgL5IevRw8kRxooKSPJfGvJ9fJQFa0TUsXzTg8= github.com/go-gl/glfw/v3.3/glfw v0.0.0-20200222043503-6f7a984d4dc4/go.mod h1:tQ2UAYgL5IevRw8kRxooKSPJfGvJ9fJQFa0TUsXzTg8= @@ -226,6 +230,7 @@ github.com/jackc/puddle v0.0.0-20190608224051-11cab39313c9/go.mod h1:m4B5Dj62Y0f github.com/jackc/puddle v1.1.0/go.mod h1:m4B5Dj62Y0fbyuIc15OsIqK0+JU8nkqQjsgx7dvjSWk= github.com/jackc/puddle v1.1.1/go.mod h1:m4B5Dj62Y0fbyuIc15OsIqK0+JU8nkqQjsgx7dvjSWk= github.com/jackc/puddle v1.1.3/go.mod h1:m4B5Dj62Y0fbyuIc15OsIqK0+JU8nkqQjsgx7dvjSWk= +github.com/jessevdk/go-flags v1.4.0/go.mod h1:4FA24M0QyGHXBuZZK/XkWh8h0e1EYbRYJSGM75WSRxI= github.com/jinzhu/inflection v1.0.0 h1:K317FqzuhWc8YvSVlFMCCUb36O/S9MCKRDI7QkRKD/E= github.com/jinzhu/inflection v1.0.0/go.mod h1:h+uFLlag+Qp1Va5pdKtLDYj+kHp5pxUVkryuEj+Srlc= github.com/jinzhu/now v1.1.1/go.mod h1:d3SSVoowX0Lcu0IBviAWJpolVfI5UJVZZ7cO71lE/z8= diff --git a/internal/common/errors.go b/internal/common/errors.go index 1daa375..95859c5 100644 --- a/internal/common/errors.go +++ b/internal/common/errors.go @@ -12,6 +12,10 @@ var ( ErrKeyLen = errors.New("PASETO_KEY must be 32 bytes long once base64-decoded") ) +func InternalServerError(title string) ProblemJSONError { + return Error(fiber.StatusInternalServerError, title, fiber.ErrInternalServerError.Message) +} + func Error(status int, title string, detail string) ProblemJSONError { return ProblemJSONError{Title: title, Detail: detail, Status: status} } diff --git a/internal/common/requests.go b/internal/common/requests.go index ca8e20c..95fc904 100644 --- a/internal/common/requests.go +++ b/internal/common/requests.go @@ -31,9 +31,9 @@ type SoftwarePost struct { } type SoftwarePatch struct { - URL string `json:"url" validate:"url"` + URL *string `json:"url" validate:"omitempty,url"` Aliases *[]string `json:"aliases" validate:"omitempty,dive,url"` - PubliccodeYml string `json:"publiccodeYml"` + PubliccodeYml *string `json:"publiccodeYml"` Active *bool `json:"active"` } diff --git a/internal/handlers/software.go b/internal/handlers/software.go index 38c25d3..9ba4343 100644 --- a/internal/handlers/software.go +++ b/internal/handlers/software.go @@ -1,6 +1,7 @@ package handlers import ( + "encoding/json" "errors" "sort" @@ -12,6 +13,8 @@ import ( "github.com/italia/developers-italia-api/internal/handlers/general" "github.com/italia/developers-italia-api/internal/models" "gorm.io/gorm" + + jsonpatch "github.com/evanphx/json-patch" ) type SoftwareInterface interface { @@ -26,6 +29,11 @@ type Software struct { db *gorm.DB } +var ( + errLoadNotFound = errors.New("Software was not found") + errLoad = errors.New("error while loading Software") +) + func NewSoftware(db *gorm.DB) *Software { return &Software{db: db} } @@ -112,36 +120,16 @@ func (p *Software) GetAllSoftware(ctx *fiber.Ctx) error { //nolint:cyclop // mos // GetSoftware gets the software with the given ID and returns any error encountered. func (p *Software) GetSoftware(ctx *fiber.Ctx) error { + const errMsg = "can't get Software" + software := models.Software{} - if err := p.db.First(&software, "id = ?", ctx.Params("id")).Error; err != nil { - if errors.Is(err, gorm.ErrRecordNotFound) { - return common.Error(fiber.StatusNotFound, "can't get Software", "Software was not found") + if err := loadSoftware(p.db, &software, ctx.Params("id")); err != nil { + if errors.Is(err, errLoadNotFound) { + return common.Error(fiber.StatusNotFound, errMsg, "Software was not found") } - return common.Error( - fiber.StatusInternalServerError, - "can't get Software", - fiber.ErrInternalServerError.Message, - ) - } - - if err := p.db. - Where("software_id = ? AND id <> ?", software.ID, software.SoftwareURLID).Find(&software.Aliases). - Error; err != nil { - return common.Error( - fiber.StatusInternalServerError, - "can't get Software", - fiber.ErrInternalServerError.Message, - ) - } - - if err := p.db.Where("id = ?", software.SoftwareURLID).First(&software.URL).Error; err != nil { - return common.Error( - fiber.StatusInternalServerError, - "can't get Software", - fiber.ErrInternalServerError.Message, - ) + return common.InternalServerError(errMsg) } return ctx.JSON(&software) @@ -183,83 +171,93 @@ func (p *Software) PostSoftware(ctx *fiber.Ctx) error { } // PatchSoftware updates the software with the given ID. -func (p *Software) PatchSoftware(ctx *fiber.Ctx) error { +func (p *Software) PatchSoftware(ctx *fiber.Ctx) error { //nolint:funlen,cyclop const errMsg = "can't update Software" - softwareReq := new(common.SoftwarePatch) + softwareReq := common.SoftwarePatch{} software := models.Software{} - // Preload will load all the associated aliases, which include - // also the canonical url. We'll manually handle that later. - if err := p.db.Preload("Aliases").First(&software, "id = ?", ctx.Params("id")). - Error; err != nil { - if errors.Is(err, gorm.ErrRecordNotFound) { - return common.Error(fiber.StatusNotFound, "can't update Software", "Software was not found") + if err := loadSoftware(p.db, &software, ctx.Params("id")); err != nil { + if errors.Is(err, errLoadNotFound) { + return common.Error(fiber.StatusNotFound, errMsg, "Software was not found") } - return common.Error(fiber.StatusInternalServerError, "can't update Software", "internal server error") + return common.Error(fiber.StatusInternalServerError, errMsg, err.Error()) } - if err := common.ValidateRequestEntity(ctx, softwareReq, errMsg); err != nil { + if err := common.ValidateRequestEntity(ctx, &softwareReq, errMsg); err != nil { return err //nolint:wrapcheck } - // Slice of urls that we expect in the database after the PATCH (url + aliases) - var expectedURLs []string + softwareJSON, err := json.Marshal(&software) + if err != nil { + return common.Error(fiber.StatusInternalServerError, errMsg, err.Error()) + } - // application/merge-patch+json semantics: change url only if - // the request specifies an "url" key. - url := software.URL.URL - if softwareReq.URL != "" { - url = softwareReq.URL + updatedJSON, err := jsonpatch.MergePatch(softwareJSON, ctx.Body()) + if err != nil { + return common.Error(fiber.StatusInternalServerError, errMsg, err.Error()) } - // application/merge-patch+json semantics: change aliases only if - // the request specifies an "aliases" key. - if softwareReq.Aliases != nil { - expectedURLs = *softwareReq.Aliases - } else { - for _, alias := range software.Aliases { - expectedURLs = append(expectedURLs, alias.URL) - } + var updatedSoftware models.Software + + err = json.Unmarshal(updatedJSON, &updatedSoftware) + if err != nil { + return common.Error(fiber.StatusInternalServerError, errMsg, err.Error()) } - expectedURLs = append(expectedURLs, url) + // Slice of aliases that we expect to be in the database after the PATCH + expectedAliases := make([]string, 0, len(updatedSoftware.Aliases)) + for _, alias := range updatedSoftware.Aliases { + expectedAliases = append(expectedAliases, alias.URL) + } if err := p.db.Transaction(func(tran *gorm.DB) error { - updatedURL, aliases, err := syncAliases(tran, software, url, expectedURLs) + //nolint:gocritic // it's fine, we want to another slice + currentURLs := append(software.Aliases, software.URL) + + updatedURL, aliases, err := syncAliases( + tran, + software.ID, + currentURLs, + updatedSoftware.URL.URL, + expectedAliases, + ) if err != nil { return err } - software.PubliccodeYml = softwareReq.PubliccodeYml - software.Active = softwareReq.Active - // Manually set the canonical URL via the foreign key because of a limitation in gorm - software.SoftwareURLID = updatedURL.ID - software.URL = *updatedURL + updatedSoftware.SoftwareURLID = updatedURL.ID + updatedSoftware.URL = *updatedURL // Set Aliases to a zero value, so it's not touched by gorm's Update(), // because we handle the alias manually - software.Aliases = []models.SoftwareURL{} + updatedSoftware.Aliases = []models.SoftwareURL{} - if err := tran.Updates(&software).Error; err != nil { + if err := tran.Updates(&updatedSoftware).Error; err != nil { return err } - software.Aliases = aliases + updatedSoftware.Aliases = aliases return nil }); err != nil { - return common.Error(fiber.StatusInternalServerError, "can't update Software", err.Error()) + switch { + case errors.Is(err, gorm.ErrDuplicatedKey): + return common.Error(fiber.StatusConflict, errMsg, "URL already exists") + default: + //nolint:wrapcheck // default to not wrap other errors, the handler will take care of this + return err + } } // Sort the aliases to always have a consistent output - sort.Slice(software.Aliases, func(a int, b int) bool { - return software.Aliases[a].URL < software.Aliases[b].URL + sort.Slice(updatedSoftware.Aliases, func(a int, b int) bool { + return updatedSoftware.Aliases[a].URL < updatedSoftware.Aliases[b].URL }) - return ctx.JSON(&software) + return ctx.JSON(&updatedSoftware) } // DeleteSoftware deletes the software with the given ID. @@ -277,12 +275,38 @@ func (p *Software) DeleteSoftware(ctx *fiber.Ctx) error { return ctx.SendStatus(fiber.StatusNoContent) } -// syncAliases synchs the SoftwareURLs for a `software` in the database to reflect the -// passed list of `expectedURLs` and the canonical `url`. +func loadSoftware(gormdb *gorm.DB, software *models.Software, id string) error { + if err := gormdb.First(&software, "id = ?", id).Error; err != nil { + if errors.Is(err, gorm.ErrRecordNotFound) { + return errLoadNotFound + } + + return errLoad + } + + if err := gormdb. + Where("software_id = ? AND id <> ?", software.ID, software.SoftwareURLID).Find(&software.Aliases). + Error; err != nil { + return errLoad + } + + if err := gormdb.Debug().Where("id = ?", software.SoftwareURLID).First(&software.URL).Error; err != nil { + return errLoad + } + + return nil +} + +// syncAliases synchs the SoftwareURLs for a `Software` in the database to reflect the +// passed list of `expectedAliases` and the canonical `url`. // // It returns the new canonical SoftwareURL and the new slice of aliases or an error if any. func syncAliases( //nolint:cyclop // mostly error handling ifs - gormdb *gorm.DB, software models.Software, canonicalURL string, expectedURLs []string, + gormdb *gorm.DB, + softwareID string, + currentURLs []models.SoftwareURL, + expectedURL string, + expectedAliases []string, ) (*models.SoftwareURL, []models.SoftwareURL, error) { toRemove := []string{} // Slice of SoftwareURL ids to remove from the database toAdd := []models.SoftwareURL{} // Slice of SoftwareURLs to add to the database @@ -291,25 +315,28 @@ func syncAliases( //nolint:cyclop // mostly error handling ifs // keyed by url urlMap := map[string]models.SoftwareURL{} - for _, alias := range software.Aliases { - urlMap[alias.URL] = alias + for _, url := range currentURLs { + urlMap[url.URL] = url } - for url, alias := range urlMap { - if !slices.Contains(expectedURLs, url) { - toRemove = append(toRemove, alias.ID) + //nolint:gocritic // it's fine, we want to another slice + allSoftwareURLs := append(expectedAliases, expectedURL) - delete(urlMap, url) + for urlStr, softwareURL := range urlMap { + if !slices.Contains(allSoftwareURLs, urlStr) { + toRemove = append(toRemove, softwareURL.ID) + + delete(urlMap, urlStr) } } - for _, url := range expectedURLs { - _, exists := urlMap[url] + for _, urlStr := range allSoftwareURLs { + _, exists := urlMap[urlStr] if !exists { - alias := models.SoftwareURL{ID: utils.UUIDv4(), URL: url, SoftwareID: software.ID} + su := models.SoftwareURL{ID: utils.UUIDv4(), URL: urlStr, SoftwareID: softwareID} - toAdd = append(toAdd, alias) - urlMap[url] = alias + toAdd = append(toAdd, su) + urlMap[urlStr] = su } } @@ -325,17 +352,16 @@ func syncAliases( //nolint:cyclop // mostly error handling ifs } } - updatedCanonicalURL := urlMap[canonicalURL] + updatedURL := urlMap[expectedURL] - // Remove the canonical URL from the aliases, because it need to be its own - // field. It was loaded previously together with the other aliases in Preload(), - // because of limitation in gorm. - delete(urlMap, canonicalURL) + // Remove the canonical URL from the rest of the URLs, so we can return + // URL and aliases in different fields. + delete(urlMap, expectedURL) aliases := make([]models.SoftwareURL, 0, len(urlMap)) for _, alias := range urlMap { aliases = append(aliases, alias) } - return &updatedCanonicalURL, aliases, nil + return &updatedURL, aliases, nil } diff --git a/internal/models/models.go b/internal/models/models.go index 4d5bed6..a2185db 100644 --- a/internal/models/models.go +++ b/internal/models/models.go @@ -1,6 +1,7 @@ package models import ( + "encoding/json" "fmt" "time" @@ -88,18 +89,24 @@ func (s Software) UUID() string { return s.ID } +//nolint:musttag // we are using a custom MarshalJSON method type SoftwareURL struct { ID string `gorm:"primarykey"` URL string `gorm:"uniqueIndex"` SoftwareID string `gorm:"not null"` - CreatedAt time.Time `json:"createdAt" gorm:"index"` - UpdatedAt time.Time `json:"updatedAt"` + CreatedAt time.Time `gorm:"index"` + UpdatedAt time.Time } func (su SoftwareURL) MarshalJSON() ([]byte, error) { return ([]byte)(fmt.Sprintf(`"%s"`, su.URL)), nil } +func (su *SoftwareURL) UnmarshalJSON(data []byte) error { + //nolint:wrapcheck // we want to pass along the error here + return json.Unmarshal(data, &su.URL) +} + type Webhook struct { ID string `json:"id" gorm:"primaryKey"` URL string `json:"url" gorm:"index:idx_webhook_url,unique"` diff --git a/main_test.go b/main_test.go index 5858ef9..2969f42 100644 --- a/main_test.go +++ b/main_test.go @@ -1835,6 +1835,7 @@ func TestSoftwareEndpoints(t *testing.T) { expectedContentType: "application/problem+json", }, { + description: "PATCH a software resource", query: "PATCH /v1/software/59803fb7-8eec-4fe5-a354-8926009c364a", body: `{"publiccodeYml": "publiccodedata", "url": "https://software-new.example.org", "aliases": ["https://software.example.com", "https://software-old.example.org"]}`, headers: map[string][]string{ @@ -1845,6 +1846,7 @@ func TestSoftwareEndpoints(t *testing.T) { expectedCode: 200, expectedContentType: "application/json", validateFunc: func(t *testing.T, response map[string]interface{}) { + assert.Equal(t, true, response["active"]) assert.Equal(t, "https://software-new.example.org", response["url"]) assert.IsType(t, []interface{}{}, response["aliases"]) @@ -1881,16 +1883,17 @@ func TestSoftwareEndpoints(t *testing.T) { expectedCode: 200, expectedContentType: "application/json", + expectedBody: "", validateFunc: func(t *testing.T, response map[string]interface{}) { + assert.Equal(t, true, response["active"]) assert.Equal(t, "https://software-new.example.org", response["url"]) assert.IsType(t, []interface{}{}, response["aliases"]) aliases := response["aliases"].([]interface{}) - assert.Equal(t, 2, len(aliases)) + assert.Equal(t, 1, len(aliases)) - assert.Equal(t, "https://18-a.example.org/code/repo", aliases[0]) - assert.Equal(t, "https://18-b.example.org/code/repo", aliases[1]) + assert.Equal(t, "https://18-b.example.org/code/repo", aliases[0]) assert.Equal(t, "publiccodedata", response["publiccodeYml"]) @@ -1919,6 +1922,7 @@ func TestSoftwareEndpoints(t *testing.T) { expectedCode: 200, expectedContentType: "application/json", validateFunc: func(t *testing.T, response map[string]interface{}) { + assert.Equal(t, true, response["active"]) assert.Equal(t, "https://software-new.example.org", response["url"]) assert.IsType(t, []interface{}{}, response["aliases"]) @@ -1941,6 +1945,118 @@ func TestSoftwareEndpoints(t *testing.T) { assert.Greater(t, updated, created) }, }, + { + description: "PATCH software with an already existing URL (of another software)", + query: "PATCH /v1/software/59803fb7-8eec-4fe5-a354-8926009c364a", + body: `{"publiccodeYml": "publiccodedata", "url": "https://21-b.example.org/code/repo"}`, + headers: map[string][]string{ + "Authorization": {goodToken}, + "Content-Type": {"application/json"}, + }, + + expectedCode: 409, + expectedContentType: "application/problem+json", + expectedBody: `{"title":"can't update Software","detail":"URL already exists","status":409}`, + }, + { + description: "PATCH software, change active", + query: "PATCH /v1/software/59803fb7-8eec-4fe5-a354-8926009c364a", + body: `{"active": false}`, + headers: map[string][]string{ + "Authorization": {goodToken}, + "Content-Type": {"application/json"}, + }, + + expectedCode: 200, + expectedContentType: "application/json", + validateFunc: func(t *testing.T, response map[string]interface{}) { + assert.Equal(t, false, response["active"]) + assert.Equal(t, "https://18-a.example.org/code/repo", response["url"]) + + assert.IsType(t, []interface{}{}, response["aliases"]) + + aliases := response["aliases"].([]interface{}) + assert.Equal(t, 1, len(aliases)) + + assert.Equal(t, "https://18-b.example.org/code/repo", aliases[0]) + + assert.Equal(t, "-", response["publiccodeYml"]) + + match, err := regexp.MatchString(UUID_REGEXP, response["id"].(string)) + assert.Nil(t, err) + assert.True(t, match) + + created, err := time.Parse(time.RFC3339, response["createdAt"].(string)) + assert.Nil(t, err) + + updated, err := time.Parse(time.RFC3339, response["updatedAt"].(string)) + assert.Nil(t, err) + + assert.Greater(t, updated, created) + }, + }, + { + description: "PATCH software, switch url and alias", + query: "PATCH /v1/software/59803fb7-8eec-4fe5-a354-8926009c364a", + body: `{"url": "https://18-b.example.org/code/repo", "aliases": ["https://18-a.example.org/code/repo"]}`, + headers: map[string][]string{ + "Authorization": {goodToken}, + "Content-Type": {"application/json"}, + }, + + expectedCode: 200, + expectedContentType: "application/json", + validateFunc: func(t *testing.T, response map[string]interface{}) { + assert.Equal(t, "https://18-b.example.org/code/repo", response["url"]) + + assert.IsType(t, []interface{}{}, response["aliases"]) + + aliases := response["aliases"].([]interface{}) + assert.Equal(t, 1, len(aliases)) + + assert.Equal(t, "https://18-a.example.org/code/repo", aliases[0]) + + assert.Equal(t, "-", response["publiccodeYml"]) + + match, err := regexp.MatchString(UUID_REGEXP, response["id"].(string)) + assert.Nil(t, err) + assert.True(t, match) + + created, err := time.Parse(time.RFC3339, response["createdAt"].(string)) + assert.Nil(t, err) + + updated, err := time.Parse(time.RFC3339, response["updatedAt"].(string)) + assert.Nil(t, err) + + assert.Greater(t, updated, created) + }, + }, + { + description: "PATCH software using an already taken URL as url", + query: "PATCH /v1/software/59803fb7-8eec-4fe5-a354-8926009c364a", + body: `{"url": "https://15-b.example.org/code/repo"}`, + headers: map[string][]string{ + "Authorization": {goodToken}, + "Content-Type": {"application/json"}, + }, + + expectedCode: 409, + expectedContentType: "application/problem+json", + expectedBody: `{"title":"can't update Software","detail":"URL already exists","status":409}`, + }, + { + description: "PATCH software using an already taken URL as an alias", + query: "PATCH /v1/software/59803fb7-8eec-4fe5-a354-8926009c364a", + body: `{"aliases": ["https://16-b.example.org/code/repo"]}`, + headers: map[string][]string{ + "Authorization": {goodToken}, + "Content-Type": {"application/json"}, + }, + + expectedCode: 409, + expectedContentType: "application/problem+json", + expectedBody: `{"title":"can't update Software","detail":"URL already exists","status":409}`, + }, { description: "PATCH software - wrong token", query: "PATCH /v1/software/59803fb7-8eec-4fe5-a354-8926009c364a", @@ -2006,6 +2122,19 @@ func TestSoftwareEndpoints(t *testing.T) { } }, }, + { + description: "PATCH software with an empty url", + query: "PATCH /v1/software/59803fb7-8eec-4fe5-a354-8926009c364a", + body: `{"url": ""}`, + headers: map[string][]string{ + "Authorization": {goodToken}, + "Content-Type": {"application/json"}, + }, + + expectedCode: 422, + expectedContentType: "application/problem+json", + expectedBody: `{"title":"can't update Software","detail":"invalid format: url is invalid","status":422,"validationErrors":[{"field":"url","rule":"url","value":""}]}`, + }, { description: "PATCH software with empty body", query: "PATCH /v1/software/59803fb7-8eec-4fe5-a354-8926009c364a",