fix: properly implement JSON merge patch for Software (#210)

Fix #173

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

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=

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}
 }

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"`
 }
 

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
 }

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"`