feat!(software): change model and properly implement PATCH (#138)

* Introduce the `url` and `alias` fields instead of a single `urls`
  field, to better express the concepts.
* Implement PATCH with application/merge-patch+json semantics.
* Don't soft delete Software and SoftwareURLs: because of the unique
  columns it wouldn't be possible to add a software with a previously
  used (and deleted) object, because is still there, albeit invisible
  to the API users.
* Manually handle SoftwareURLs associations, because Update(&software)
  is not smart enough to synch the associations and remove them when
  necessary.
  Replace() with Session(&gorm.Session{FullSaveAssociations: true})
  doesn't work either because the foreign key is set to null instead of
  being deleted and, more importantly, wouldn't even work with our
  NOT NULL constraint on that foreign key.
  (go-gorm/gorm#4010 (comment))
* Add tests

developers-italia.oas.yaml

@@ -1387,12 +1387,20 @@ components:
           minLength: 1
           maxLength: 99999
           pattern: '.*'
-        urls:
+        url:
+          type: string
+          format: uri
+          minLength: 1
+          maxLength: 255
+          description: >
+            Repository URL this software is available at.
+            This is the current and canonical one.
+        aliases:
           type: array
           description: >
-            List of repository URLs this software is available at.
-            The first element of the array is the current and canonical one, while the others, if any,
-            are aliases to the current repository.
+            List of aliases for the current repository URL.
+            This is useful to keep track, for example, of previous addresses that redirect
+            to the current one.
           minItems: 1
           maxItems: 255
           items:
@@ -1419,6 +1427,12 @@ components:
           example: '2022-06-07T14:56:23Z'
           description: The time the log was updated (RFC 3339 datetime)
           readOnly: true
+      required:
+        - id
+        - url
+        - publiccodeYml
+        - createdAt
+        - updatedAt
     Publisher:
       title: Publisher
       type: object

go.mod

@@ -14,6 +14,8 @@ require (
 	gorm.io/gorm v1.23.6
 )
 
+require golang.org/x/exp v0.0.0-20220827204233-334a2380cb91
+
 require (
 	github.com/aead/chacha20 v0.0.0-20180709150244-8b13a72661da // indirect
 	github.com/aead/chacha20poly1305 v0.0.0-20201124145622-1a5aba2a8b29 // indirect
@@ -48,7 +50,7 @@ require (
 	github.com/valyala/fasthttp v1.38.0 // indirect
 	github.com/valyala/tcplisten v1.0.0 // indirect
 	golang.org/x/crypto v0.0.0-20220622213112-05595931fe9d // indirect
-	golang.org/x/sys v0.0.0-20220704084225-05e143d24a9e // indirect
+	golang.org/x/sys v0.0.0-20220722155257-8c9f86f7a55f // indirect
 	golang.org/x/text v0.3.7 // indirect
 	gopkg.in/yaml.v3 v3.0.1 // indirect
 )

go.sum

@@ -230,6 +230,8 @@ golang.org/x/crypto v0.0.0-20211215153901-e495a2d5b3d3/go.mod h1:IxCIyHEi3zRg3s0
 golang.org/x/crypto v0.0.0-20220214200702-86341886e292/go.mod h1:IxCIyHEi3zRg3s0A5j5BB6A9Jmi73HwBIUl50j+osU4=
 golang.org/x/crypto v0.0.0-20220622213112-05595931fe9d h1:sK3txAijHtOK88l68nt020reeT1ZdKLIYetKl95FzVY=
 golang.org/x/crypto v0.0.0-20220622213112-05595931fe9d/go.mod h1:IxCIyHEi3zRg3s0A5j5BB6A9Jmi73HwBIUl50j+osU4=
+golang.org/x/exp v0.0.0-20220827204233-334a2380cb91 h1:tnebWN09GYg9OLPss1KXj8txwZc6X6uMr6VFdcGNbHw=
+golang.org/x/exp v0.0.0-20220827204233-334a2380cb91/go.mod h1:cyybsKvd6eL0RnXn6p/Grxp8F5bW7iYuBgsNCOHpMYE=
 golang.org/x/lint v0.0.0-20190930215403-16217165b5de/go.mod h1:6SW0HCj/g11FgYtHlgUYUwCkIfeOF89ocIRzGO/8vkc=
 golang.org/x/mod v0.0.0-20190513183733-4bf6d317e70e/go.mod h1:mXi4GBBbnImb6dmsKGUJ2LatrhH/nqhxcFungHvyanc=
 golang.org/x/mod v0.1.1-0.20191105210325-c90efee705ee/go.mod h1:QqPTAvyqsEbceGzBzNggFXnrqF1CaUcvgkdR5Ot7KZg=
@@ -259,8 +261,8 @@ golang.org/x/sys v0.0.0-20210615035016-665e8c7367d1/go.mod h1:oPkhp1MJrh7nUepCBc
 golang.org/x/sys v0.0.0-20210806184541-e5e7981a1069/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
 golang.org/x/sys v0.0.0-20211216021012-1d35b9e2eb4e/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
 golang.org/x/sys v0.0.0-20220227234510-4e6760a101f9/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
-golang.org/x/sys v0.0.0-20220704084225-05e143d24a9e h1:CsOuNlbOuf0mzxJIefr6Q4uAUetRUwZE4qt7VfzP+xo=
-golang.org/x/sys v0.0.0-20220704084225-05e143d24a9e/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
+golang.org/x/sys v0.0.0-20220722155257-8c9f86f7a55f h1:v4INt8xihDGvnrfjMDVXGxw9wrfxYyCjk0KbXjhR55s=
+golang.org/x/sys v0.0.0-20220722155257-8c9f86f7a55f/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
 golang.org/x/term v0.0.0-20201117132131-f5c789dd3221/go.mod h1:Nr5EML6q2oocZ2LXRh80K7BxOlk5/8JxuGnuhpl+muw=
 golang.org/x/term v0.0.0-20201126162022-7de9c90e9dd1/go.mod h1:bj7SfCRtBDWHUb9snDiAeCFNEtKQo2Wmx5Cou7ajbmo=
 golang.org/x/term v0.0.0-20210927222741-03fcf44c2211/go.mod h1:jbD1KX2456YbFQfuXm/mYQcufACuNUgVhRMnK/tPxf8=

internal/common/requests.go

@@ -12,12 +12,20 @@ type CodeHosting struct {
 	Group *bool  `json:"group"`
 }
 
-type Software struct {
-	URLs          []string `json:"urls" validate:"required,gt=0,dive,url"`
+type SoftwarePost struct {
+	URL           string   `json:"url" validate:"required,url"`
+	Aliases       []string `json:"aliases" validate:"dive,url"`
 	PubliccodeYml string   `json:"publiccodeYml" validate:"required"`
 	Active        *bool    `json:"active"`
 }
 
+type SoftwarePatch struct {
+	URL           string    `json:"url" validate:"url"`
+	Aliases       *[]string `json:"aliases" validate:"omitempty,dive,url"`
+	PubliccodeYml string    `json:"publiccodeYml"`
+	Active        *bool     `json:"active"`
+}
+
 type Log struct {
 	Message string `json:"message" validate:"required,gt=1"`
 }

internal/handlers/software.go

@@ -2,6 +2,9 @@ package handlers
 
 import (
 	"errors"
+	"sort"
+
+	"golang.org/x/exp/slices"
 
 	"github.com/gofiber/fiber/v2"
 	"github.com/gofiber/fiber/v2/utils"
@@ -28,10 +31,12 @@ func NewSoftware(db *gorm.DB) *Software {
 }
 
 // GetAllSoftware gets the list of all software and returns any error encountered.
-func (p *Software) GetAllSoftware(ctx *fiber.Ctx) error {
+func (p *Software) GetAllSoftware(ctx *fiber.Ctx) error { //nolint:cyclop // mostly error handling ifs
 	var software []models.Software
 
-	stmt := p.db.Preload("URLs")
+	// Preload will load all the associated aliases, which include
+	// also the canonical url. We'll manually handle that later.
+	stmt := p.db.Preload("Aliases")
 
 	stmt, err := general.Clauses(ctx, stmt, "")
 	if err != nil {
@@ -42,10 +47,6 @@ func (p *Software) GetAllSoftware(ctx *fiber.Ctx) error {
 		)
 	}
 
-	if all := ctx.Query("all", ""); all == "" {
-		stmt = stmt.Scopes(models.Active)
-	}
-
 	// Return just software with a certain URL if the 'url' query filter
 	// is used.
 	if url := ctx.Query("url", ""); url != "" {
@@ -61,6 +62,10 @@ func (p *Software) GetAllSoftware(ctx *fiber.Ctx) error {
 		}
 
 		stmt.Where("id = ?", softwareURL.SoftwareID)
+	} else {
+		if all := ctx.Query("all", ""); all == "" {
+			stmt = stmt.Scopes(models.Active)
+		}
 	}
 
 	paginator := general.NewPaginator(ctx)
@@ -82,14 +87,34 @@ func (p *Software) GetAllSoftware(ctx *fiber.Ctx) error {
 		)
 	}
 
+	// 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.
+	for swIdx := range software {
+		swr := &software[swIdx]
+
+		for aliasIdx := range swr.Aliases {
+			alias := &swr.Aliases[aliasIdx]
+
+			if alias.ID == swr.SoftwareURLID {
+				swr.URL = *alias
+
+				swr.Aliases[aliasIdx] = swr.Aliases[len(swr.Aliases)-1]
+				swr.Aliases = swr.Aliases[:len(swr.Aliases)-1]
+
+				break
+			}
+		}
+	}
+
 	return ctx.JSON(fiber.Map{"data": &software, "links": general.PaginationLinks(cursor)})
 }
 
 // GetSoftware gets the software with the given ID and returns any error encountered.
 func (p *Software) GetSoftware(ctx *fiber.Ctx) error {
 	software := models.Software{}
 
-	if err := p.db.Preload("URLs").First(&software, "id = ?", ctx.Params("id")).Error; err != nil {
+	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")
 		}
@@ -101,12 +126,30 @@ func (p *Software) GetSoftware(ctx *fiber.Ctx) error {
 		)
 	}
 
+	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 ctx.JSON(&software)
 }
 
 // PostSoftware creates a new software.
 func (p *Software) PostSoftware(ctx *fiber.Ctx) error {
-	softwareReq := new(common.Software)
+	softwareReq := new(common.SoftwarePost)
 
 	if err := ctx.BodyParser(&softwareReq); err != nil {
 		return common.Error(fiber.StatusBadRequest, "can't create Software", "invalid json")
@@ -118,14 +161,20 @@ func (p *Software) PostSoftware(ctx *fiber.Ctx) error {
 		)
 	}
 
-	softwareURLs := []models.SoftwareURL{}
-	for _, u := range softwareReq.URLs {
-		softwareURLs = append(softwareURLs, models.SoftwareURL{ID: utils.UUIDv4(), URL: u})
+	aliases := []models.SoftwareURL{}
+	for _, u := range softwareReq.Aliases {
+		aliases = append(aliases, models.SoftwareURL{ID: utils.UUIDv4(), URL: u})
 	}
 
+	url := models.SoftwareURL{ID: utils.UUIDv4(), URL: softwareReq.URL}
 	software := models.Software{
-		ID:            utils.UUIDv4(),
-		URLs:          softwareURLs,
+		ID: utils.UUIDv4(),
+
+		// Manually set the URL and its foreign key because of a limitation in gorm
+		URL:           url,
+		SoftwareURLID: url.ID,
+
+		Aliases:       aliases,
 		PubliccodeYml: softwareReq.PubliccodeYml,
 		Active:        softwareReq.Active,
 	}
@@ -138,12 +187,14 @@ func (p *Software) PostSoftware(ctx *fiber.Ctx) error {
 }
 
 // PatchSoftware updates the software with the given ID.
-func (p *Software) PatchSoftware(ctx *fiber.Ctx) error {
-	softwareReq := new(common.Software)
-
+func (p *Software) PatchSoftware(ctx *fiber.Ctx) error { //nolint:cyclop // mostly error handling ifs
+	softwareReq := new(common.SoftwarePatch)
 	software := models.Software{}
 
-	if err := p.db.First(&software, "id = ?", ctx.Params("id")).Error; err != nil {
+	// 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")
 		}
@@ -161,19 +212,61 @@ func (p *Software) PatchSoftware(ctx *fiber.Ctx) error {
 		)
 	}
 
-	softwareURLs := []models.SoftwareURL{}
-	for _, u := range softwareReq.URLs {
-		softwareURLs = append(softwareURLs, models.SoftwareURL{ID: utils.UUIDv4(), URL: u})
+	// Slice of urls that we expect in the database after the PATCH (url + aliases)
+	var expectedURLs []string
+
+	// 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
 	}
 
-	software.URLs = softwareURLs
-	software.PubliccodeYml = softwareReq.PubliccodeYml
-	software.Active = softwareReq.Active
+	// 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)
+		}
+	}
 
-	if err := p.db.Updates(&software).Error; err != nil {
-		return common.Error(fiber.StatusInternalServerError, "can't update Software", "db error")
+	expectedURLs = append(expectedURLs, url)
+
+	if err := p.db.Transaction(func(tran *gorm.DB) error {
+		updatedURL, aliases, err := syncAliases(tran, software, url, expectedURLs)
+		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
+
+		// 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{}
+
+		if err := tran.Updates(&software).Error; err != nil {
+			return err
+		}
+
+		software.Aliases = aliases
+
+		return nil
+	}); err != nil {
+		return common.Error(fiber.StatusInternalServerError, "can't update Software", err.Error())
 	}
 
+	// 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
+	})
+
 	return ctx.JSON(&software)
 }
 
@@ -189,3 +282,66 @@ func (p *Software) DeleteSoftware(ctx *fiber.Ctx) error {
 
 	return ctx.Status(fiber.StatusNoContent).JSON("")
 }
+
+// syncAliases synchs the SoftwareURLs for a `software` in the database to reflect the
+// passed list of `expectedURLs` 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,
+) (*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
+
+	// Map mirroring the state of SoftwareURLs for this software in the database,
+	// keyed by url
+	urlMap := map[string]models.SoftwareURL{}
+
+	for _, alias := range software.Aliases {
+		urlMap[alias.URL] = alias
+	}
+
+	for url, alias := range urlMap {
+		if !slices.Contains(expectedURLs, url) {
+			toRemove = append(toRemove, alias.ID)
+
+			delete(urlMap, url)
+		}
+	}
+
+	for _, url := range expectedURLs {
+		_, exists := urlMap[url]
+		if !exists {
+			alias := models.SoftwareURL{ID: utils.UUIDv4(), URL: url, SoftwareID: software.ID}
+
+			toAdd = append(toAdd, alias)
+			urlMap[url] = alias
+		}
+	}
+
+	if len(toRemove) > 0 {
+		if err := gormdb.Delete(&models.SoftwareURL{}, toRemove).Error; err != nil {
+			return nil, nil, err
+		}
+	}
+
+	if len(toAdd) > 0 {
+		if err := gormdb.Create(toAdd).Error; err != nil {
+			return nil, nil, err
+		}
+	}
+
+	updatedCanonicalURL := urlMap[canonicalURL]
+
+	// 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)
+
+	aliases := make([]models.SoftwareURL, 0, len(urlMap))
+	for _, alias := range urlMap {
+		aliases = append(aliases, alias)
+	}
+
+	return &updatedCanonicalURL, aliases, nil
+}