diff --git a/internal/common/requests.go b/internal/common/requests.go index 6c40f94..ca8e20c 100644 --- a/internal/common/requests.go +++ b/internal/common/requests.go @@ -11,11 +11,11 @@ type PublisherPost struct { } type PublisherPatch struct { - CodeHosting []CodeHosting `json:"codeHosting" validate:"gt=0"` - Description string `json:"description"` - Email string `json:"email" validate:"email"` - Active *bool `json:"active"` - AlternativeID string `json:"alternativeId" validate:"max=255"` + CodeHosting *[]CodeHosting `json:"codeHosting" validate:"omitempty,gt=0,dive"` + Description *string `json:"description"` + Email *string `json:"email" validate:"omitempty,email"` + Active *bool `json:"active"` + AlternativeID *string `json:"alternativeId" validate:"omitempty,max=255"` } type CodeHosting struct { diff --git a/internal/handlers/publishers.go b/internal/handlers/publishers.go index 4faf6f5..c96aa3a 100644 --- a/internal/handlers/publishers.go +++ b/internal/handlers/publishers.go @@ -2,10 +2,11 @@ package handlers import ( "errors" - "fmt" "net/url" + "sort" "github.com/italia/developers-italia-api/internal/database" + "golang.org/x/exp/slices" "github.com/italia/developers-italia-api/internal/handlers/general" @@ -135,69 +136,83 @@ func (p *Publisher) PostPublisher(ctx *fiber.Ctx) error { } // PatchPublisher updates the publisher with the given ID. CodeHosting URLs will be overwritten from the request. -func (p *Publisher) PatchPublisher(ctx *fiber.Ctx) error { - requests := new(common.PublisherPatch) - - if err := common.ValidateRequestEntity(ctx, requests, "can't update Publisher"); err != nil { - return err //nolint:wrapcheck - } - +func (p *Publisher) PatchPublisher(ctx *fiber.Ctx) error { //nolint:cyclop // mostly error handling ifs + publisherReq := new(common.PublisherPatch) publisher := models.Publisher{} - if err := p.db.Transaction(func(gormTrx *gorm.DB) error { - return p.updatePublisherTrx(gormTrx, publisher, ctx, requests) - }); err != nil { - return err //nolint:wrapcheck - } - - return ctx.JSON(&publisher) -} - -func (p *Publisher) updatePublisherTrx( - gormTrx *gorm.DB, - publisher models.Publisher, - ctx *fiber.Ctx, - request *common.PublisherPatch, -) error { - if err := gormTrx.Model(&models.Publisher{}).Preload("CodeHosting"). - First(&publisher, "id = ?", ctx.Params("id")).Error; err != nil { + // Preload will load all the associated CodeHosting. We'll manually handle that later. + if err := p.db.Preload("CodeHosting").First(&publisher, "id = ?", ctx.Params("id")). + Error; err != nil { if errors.Is(err, gorm.ErrRecordNotFound) { - return common.Error(fiber.StatusNotFound, "Not found", "can't update Publisher. Publisher was not found") + return common.Error(fiber.StatusNotFound, "can't update Publisher", "Publisher was not found") } - return common.Error(fiber.StatusInternalServerError, + return common.Error( + fiber.StatusInternalServerError, "can't update Publisher", - fmt.Errorf("db error: %w", err).Error()) + fiber.ErrInternalServerError.Message, + ) } - if request.Description != "" { - publisher.Description = request.Description + if err := common.ValidateRequestEntity(ctx, publisherReq, "can't update Publisher"); err != nil { + return err //nolint:wrapcheck } - if request.Email != "" { - normalizedEmail := common.NormalizeEmail(&request.Email) - publisher.Email = normalizedEmail - } + // Slice of CodeHosting URLs that we expect in the database after the PATCH + var expectedURLs []string - if request.AlternativeID != "" { - publisher.AlternativeID = &request.AlternativeID + // application/merge-patch+json semantics: change CodeHosting only if + // the request specifies a "CodeHosting" key. + if publisherReq.CodeHosting != nil { + for _, ch := range *publisherReq.CodeHosting { + expectedURLs = append(expectedURLs, purell.MustNormalizeURLString(ch.URL, normalizeFlags)) + } + } else { + for _, ch := range publisher.CodeHosting { + expectedURLs = append(expectedURLs, ch.URL) + } } - if request.CodeHosting != nil && len(request.CodeHosting) > 0 { - gormTrx.Delete(&publisher.CodeHosting) + if err := p.db.Transaction(func(tran *gorm.DB) error { + codeHosting, err := syncCodeHosting(tran, publisher, expectedURLs) + if err != nil { + return err + } - for _, URLAddress := range request.CodeHosting { - publisher.CodeHosting = append(publisher.CodeHosting, models.CodeHosting{ID: utils.UUIDv4(), URL: URLAddress.URL}) + if publisherReq.Description != nil { + publisher.Description = *publisherReq.Description + } + if publisherReq.Email != nil { + publisher.Email = common.NormalizeEmail(publisherReq.Email) + } + if publisherReq.Active != nil { + publisher.Active = publisherReq.Active + } + if publisher.AlternativeID != nil { + publisher.AlternativeID = publisherReq.AlternativeID } - } - if err := gormTrx.Updates(&publisher).Error; err != nil { - return common.Error(fiber.StatusInternalServerError, - "can't update Publisher", - fmt.Errorf("db error: %w", err).Error()) + // Set CodeHosting to a zero value, so it's not touched by gorm's Update(), + // because we handle the alias manually + publisher.CodeHosting = []models.CodeHosting{} + + if err := tran.Updates(&publisher).Error; err != nil { + return err + } + + publisher.CodeHosting = codeHosting + + return nil + }); err != nil { + return common.Error(fiber.StatusInternalServerError, "can't update Publisher", err.Error()) } - return nil + // Sort the aliases to always have a consistent output + sort.Slice(publisher.CodeHosting, func(a int, b int) bool { + return publisher.CodeHosting[a].URL < publisher.CodeHosting[b].URL + }) + + return ctx.JSON(&publisher) } // DeletePublisher deletes the publisher with the given ID. @@ -214,3 +229,59 @@ func (p *Publisher) DeletePublisher(ctx *fiber.Ctx) error { return ctx.SendStatus(fiber.StatusNoContent) } + +// syncCodeHosting synchs the CodeHosting for a `publisher` in the database to reflect the +// passed slice of `codeHosting` URLs. +// +// It returns the slice of CodeHosting in the database. +func syncCodeHosting( //nolint:cyclop // mostly error handling ifs + gormdb *gorm.DB, publisher models.Publisher, codeHosting []string, +) ([]models.CodeHosting, error) { + toRemove := []string{} // Slice of CodeHosting ids to remove from the database + toAdd := []models.CodeHosting{} // Slice of CodeHosting to add to the database + + // Map mirroring the state of CodeHosting for this software in the database, + // keyed by url + urlMap := map[string]models.CodeHosting{} + + for _, ch := range publisher.CodeHosting { + urlMap[ch.URL] = ch + } + + for url, ch := range urlMap { + if !slices.Contains(codeHosting, url) { + toRemove = append(toRemove, ch.ID) + + delete(urlMap, url) + } + } + + for _, url := range codeHosting { + _, exists := urlMap[url] + if !exists { + ch := models.CodeHosting{ID: utils.UUIDv4(), URL: url} + + toAdd = append(toAdd, ch) + urlMap[url] = ch + } + } + + if len(toRemove) > 0 { + if err := gormdb.Delete(&models.CodeHosting{}, toRemove).Error; err != nil { + return nil, err + } + } + + if len(toAdd) > 0 { + if err := gormdb.Create(toAdd).Error; err != nil { + return nil, err + } + } + + retCodeHosting := make([]models.CodeHosting, 0, len(urlMap)) + for _, ch := range urlMap { + retCodeHosting = append(retCodeHosting, ch) + } + + return retCodeHosting, nil +} diff --git a/main_test.go b/main_test.go index d05f9b7..bfffd9e 100644 --- a/main_test.go +++ b/main_test.go @@ -748,50 +748,103 @@ func TestPublishersEndpoints(t *testing.T) { assert.Equal(t, "invalid json", response["detail"]) }, }, + + // PATCH /publishers/:id { - description: "PATCH non-existing publishers", - query: "PATCH /v1/publishers/NO_SUCH_publishers", - body: `{"codeHosting": [{"url" : "https://www.example.com"}], "email":"example@example.com"}`, + description: "PATCH non existing publisher", + query: "PATCH /v1/publishers/NO_SUCH_PUBLISHER", + body: ``, headers: map[string][]string{ "Authorization": {goodToken}, "Content-Type": {"application/json"}, }, expectedCode: 404, - expectedBody: `{"title":"Not found","detail":"can't update Publisher. Publisher was not found","status":404}`, - expectedContentType: "application/problem+json", - }, - //TODO fix database locked test - /* - { - query: "PATCH /v1/publishers/15fda7c4-6bbf-4387-8f89-258c1e6fafb1", - body: `{"codeHosting": [{"url" : "https://www.example.com"}], "email":"example@example.com"}`, - 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.IsType(t, []interface{}{}, response["codeHosting"]) - assert.Equal(t, 3, len(response["codeHosting"].([]interface{}))) - - match, err := regexp.MatchString(UUID_REGEXP, response["id"].(string)) - assert.Nil(t, err) - assert.True(t, match) + expectedBody: `{"title":"can't update Publisher","detail":"Publisher was not found","status":404}`, + expectedContentType: "application/problem+json", + }, + { + description: "PATCH a publisher", + query: "PATCH /v1/publishers/2ded32eb-c45e-4167-9166-a44e18b8adde", + body: `{"description": "new PATCHed description", "codeHosting": [{"url": "https://gitlab.example.org/patched-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, "new PATCHed description", response["description"]) + assert.IsType(t, []interface{}{}, response["codeHosting"]) - created, err := time.Parse(time.RFC3339, response["createdAt"].(string)) - assert.Nil(t, err) + codeHosting := response["codeHosting"].([]interface{}) + assert.Equal(t, 1, len(codeHosting)) - updated, err := time.Parse(time.RFC3339, response["updatedAt"].(string)) - assert.Nil(t, err) + firstCodeHosting := codeHosting[0].(map[string]interface{}) + + assert.Equal(t, "https://gitlab.example.org/patched-repo", firstCodeHosting["url"]) + assert.Equal(t, "2ded32eb-c45e-4167-9166-a44e18b8adde", response["id"]) + + 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 publishers with no codeHosting (should leave current codeHosting untouched)", + query: "PATCH /v1/publishers/2ded32eb-c45e-4167-9166-a44e18b8adde", + body: `{"description": "new description"}`, + 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, "2ded32eb-c45e-4167-9166-a44e18b8adde", response["id"]) + assert.Equal(t, "new description", response["description"]) + assert.Equal(t, "foobar@1.example.org", response["email"]) + + assert.IsType(t, []interface{}{}, response["codeHosting"]) + + codeHosting := response["codeHosting"].([]interface{}) + assert.Equal(t, 2, len(codeHosting)) + + firstCodeHosting := codeHosting[0].(map[string]interface{}) + assert.Equal(t, "https://1-a.example.org/code/repo", firstCodeHosting["url"]) + secondCodeHosting := codeHosting[1].(map[string]interface{}) + assert.Equal(t, "https://1-b.example.org/code/repo", secondCodeHosting["url"]) + + assert.Equal(t, "2018-05-01T00:00:00Z", response["createdAt"]) + 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) - }, - },*/ + assert.Greater(t, updated, created) + }, + }, + { + description: "PATCH publishers with empty codeHosting", + query: "PATCH /v1/publishers/2ded32eb-c45e-4167-9166-a44e18b8adde", + body: `{"description": "new description", "codeHosting": []}`, + headers: map[string][]string{ + "Authorization": {goodToken}, + "Content-Type": {"application/json"}, + }, + + expectedCode: 422, + expectedContentType: "application/problem+json", + expectedBody: `{"title":"can't update Publisher","detail":"invalid format","status":422,"validationErrors":[{"field":"codeHosting","rule":"gt"}]}`, + }, { description: "PATCH publishers - wrong token", - query: "PATCH /v1/publishers/15fda7c4-6bbf-4387-8f89-258c1e6fafb1", + query: "PATCH /v1/publishers/2ded32eb-c45e-4167-9166-a44e18b8adde", body: ``, headers: map[string][]string{ "Authorization": {badToken}, @@ -802,8 +855,8 @@ func TestPublishersEndpoints(t *testing.T) { expectedContentType: "application/problem+json", }, { - description: "PATCH publishers with invalid JSON", - query: "PATCH /v1/publishers/15fda7c4-6bbf-4387-8f89-258c1e6fafb1", + description: "PATCH publisher with invalid JSON", + query: "PATCH /v1/publishers/2ded32eb-c45e-4167-9166-a44e18b8adde", body: `INVALID_JSON`, headers: map[string][]string{ "Authorization": {goodToken}, @@ -816,37 +869,37 @@ func TestPublishersEndpoints(t *testing.T) { assert.Equal(t, "invalid json", response["detail"]) }, }, - //TODO fix database locked test - /* - { - description: "PATCH publishers with validation errors", - query: "PATCH /v1/publishers/15fda7c4-6bbf-4387-8f89-258c1e6fafb1", - body: `{"codeHosting": [{"url" : "INVALID_URL"}], "email":"example@example.com"}`, - headers: map[string][]string{ - "Authorization": {goodToken}, - "Content-Type": {"application/json"}, - }, - expectedCode: 422, - expectedContentType: "application/problem+json", - validateFunc: func(t *testing.T, response map[string]interface{}) { - assert.Equal(t, `can't update Publisher`, response["title"]) - assert.Equal(t, "invalid format", response["detail"]) - - assert.IsType(t, []interface{}{}, response["validationErrors"]) - - validationErrors := response["validationErrors"].([]interface{}) - assert.Equal(t, 1, len(validationErrors)) - - firstValidationError := validationErrors[0].(map[string]interface{}) - - for key := range firstValidationError { - assert.Contains(t, []string{"field", "rule", "value"}, key) - } - }, - },*/ + // TODO: make this pass + // { + // description: "PATCH publishers with JSON with extra fields", + // query: "PATCH /v1/publishers", + // body: `{"description": "new description", EXTRA_FIELD: "extra field not in schema"}`, + // headers: map[string][]string{ + // "Authorization": {goodToken}, + // "Content-Type": {"application/json"}, + // }, + // expectedCode: 422, + // expectedContentType: "application/problem+json", + // validateFunc: func(t *testing.T, response map[string]interface{}) { + // assert.Equal(t, `can't create Publisher`, response["title"]) + // assert.Equal(t, "invalid json", response["detail"]) + // }, + // }, + { + description: "PATCH publisher with validation errors", + query: "PATCH /v1/publishers/2ded32eb-c45e-4167-9166-a44e18b8adde", + body: `{"description": "new description", "codeHosting": [{"url": "INVALID_URL"}]}`, + headers: map[string][]string{ + "Authorization": {goodToken}, + "Content-Type": {"application/json"}, + }, + expectedCode: 422, + expectedContentType: "application/problem+json", + expectedBody: `{"title":"can't update Publisher","detail":"invalid format","status":422,"validationErrors":[{"field":"url","rule":"url","value":"INVALID_URL"}]}`, + }, { description: "PATCH publishers with empty body", - query: "PATCH /v1/publishers/15fda7c4-6bbf-4387-8f89-258c1e6fafb1", + query: "PATCH /v1/publishers/2ded32eb-c45e-4167-9166-a44e18b8adde", body: "", headers: map[string][]string{ "Authorization": {goodToken}, @@ -854,11 +907,17 @@ func TestPublishersEndpoints(t *testing.T) { }, expectedCode: 400, expectedContentType: "application/problem+json", - validateFunc: func(t *testing.T, response map[string]interface{}) { - assert.Equal(t, `can't update Publisher`, response["title"]) - assert.Equal(t, "invalid json", response["detail"]) - }, + expectedBody: `{"title":"can't update Publisher","detail":"invalid json","status":400}`, }, + // TODO: enforce this? + // { + // query: "PATCH /v1/publishers with no Content-Type", + // body: "", + // headers: map[string][]string{ + // "Authorization": {goodToken}, + // }, + // expectedCode: 404, + // } // DELETE /publishers/:id {