feat(publishers): make alternativeId work as an id in endpoints (#176)

italia · Oct 24, 2022 · de88cdc · de88cdc
1 parent c1dd6a6
commit de88cdc
Show file tree

Hide file tree

Showing 3 changed files with 137 additions and 9 deletions.
diff --git a/developers-italia.oas.yaml b/developers-italia.oas.yaml
@@ -910,7 +910,7 @@ paths:
         required: true
     get:
       summary: Get a Publisher
-      description: Get a Publisher by its id
+      description: Get a Publisher by its id or alternativeId
       tags:
         - publishers
       responses:
@@ -927,7 +927,7 @@ paths:
       operationId: show-publisher-publisherId
     patch:
       summary: Update a Publisher
-      description: Update a Publisher by its id. Only the fields that are provided will be updated, codeHosting field will be overwritten entirely.
+      description: Update a Publisher by its id or alternativeId
       tags:
         - publishers
       security:
@@ -958,7 +958,7 @@ paths:
               $ref: '#/components/schemas/Publisher'
     delete:
       summary: Delete a Publisher
-      description: Delete a Publisher by its id
+      description: Delete a Publisher by its id or alternativeId
       tags:
         - publishers
       security:
@@ -1531,8 +1531,13 @@ components:
         alternativeId:
           type: string
           description: |
-            Optional alternative custom id for this Publisher. This is useful for example if this
-            Publisher has another id or code in a different database.
+            Optional alternative user-provided identifier for this Publisher.
+
+            If present, you can use it in place of the Publisher `id` as path query
+            argument.
+
+            This is useful for example if this Publisher has another id or code in a
+            different database.
           maxLength: 255
           example: 'ID-1234'
           pattern: '.*'

diff --git a/internal/handlers/publishers.go b/internal/handlers/publishers.go
@@ -2,6 +2,7 @@ package handlers
 
 import (
 	"errors"
+	"fmt"
 	"net/url"
 	"sort"
 
@@ -72,8 +73,9 @@ func (p *Publisher) GetPublishers(ctx *fiber.Ctx) error {
 // GetPublisher gets the publisher with the given ID and returns any error encountered.
 func (p *Publisher) GetPublisher(ctx *fiber.Ctx) error {
 	publisher := models.Publisher{}
+	id := ctx.Params("id")
 
-	if err := p.db.Preload("CodeHosting").First(&publisher, "id = ?", ctx.Params("id")).Error; err != nil {
+	if err := p.db.Preload("CodeHosting").First(&publisher, "id = ? or alternative_id = ?", id, id).Error; err != nil {
 		if errors.Is(err, gorm.ErrRecordNotFound) {
 			return common.Error(fiber.StatusNotFound, "can't get Publisher", "Publisher was not found")
 		}
@@ -93,6 +95,23 @@ func (p *Publisher) PostPublisher(ctx *fiber.Ctx) error {
 		return err //nolint:wrapcheck
 	}
 
+	if request.AlternativeID != nil {
+		//nolint:godox // postpone the fix
+		// FIXME: Possible TOCTTOU race here
+		result := p.db.Limit(1).Find(&models.Publisher{ID: *request.AlternativeID})
+
+		if result.Error != nil {
+			return common.Error(fiber.StatusInternalServerError, "can't create Publisher", "db error")
+		}
+
+		if result.RowsAffected != 0 {
+			return common.Error(fiber.StatusConflict,
+				"can't create Publisher",
+				fmt.Sprintf("Publisher with id '%s' already exists", *request.AlternativeID),
+			)
+		}
+	}
+
 	normalizedEmail := common.NormalizeEmail(request.Email)
 
 	publisher := &models.Publisher{
@@ -136,12 +155,13 @@ 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 { //nolint:cyclop // mostly error handling ifs
+func (p *Publisher) PatchPublisher(ctx *fiber.Ctx) error { //nolint:cyclop,funlen // mostly error handling ifs
 	publisherReq := new(common.PublisherPatch)
 	publisher := models.Publisher{}
+	id := ctx.Params("id")
 
 	// 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")).
+	if err := p.db.Preload("CodeHosting").First(&publisher, "id = ? or alternative_id = ?", id, id).
 		Error; err != nil {
 		if errors.Is(err, gorm.ErrRecordNotFound) {
 			return common.Error(fiber.StatusNotFound, "can't update Publisher", "Publisher was not found")
@@ -158,6 +178,23 @@ func (p *Publisher) PatchPublisher(ctx *fiber.Ctx) error { //nolint:cyclop // mo
 		return err //nolint:wrapcheck
 	}
 
+	if publisherReq.AlternativeID != nil {
+		//nolint:godox // postpone the fix
+		// FIXME: Possible TOCTTOU race here
+		result := p.db.Limit(1).Find(&models.Publisher{ID: *publisherReq.AlternativeID})
+
+		if result.Error != nil {
+			return common.Error(fiber.StatusInternalServerError, "can't update Publisher", "db error")
+		}
+
+		if result.RowsAffected != 0 {
+			return common.Error(fiber.StatusConflict,
+				"can't update Publisher",
+				fmt.Sprintf("Publisher with id '%s' already exists", *publisherReq.AlternativeID),
+			)
+		}
+	}
+
 	// Slice of CodeHosting URLs that we expect in the database after the PATCH
 	var expectedURLs []string
 
@@ -217,7 +254,8 @@ func (p *Publisher) PatchPublisher(ctx *fiber.Ctx) error { //nolint:cyclop // mo
 
 // DeletePublisher deletes the publisher with the given ID.
 func (p *Publisher) DeletePublisher(ctx *fiber.Ctx) error {
-	result := p.db.Select("CodeHosting").Delete(&models.Publisher{ID: ctx.Params("id")})
+	id := ctx.Params("id")
+	result := p.db.Select("CodeHosting").Where("id = ? or alternative_id = ?", id, id).Delete(&models.Publisher{})
 
 	if result.Error != nil {
 		return common.Error(fiber.StatusInternalServerError, "can't delete Publisher", "db error")

diff --git a/main_test.go b/main_test.go
@@ -399,6 +399,25 @@ func TestPublishersEndpoints(t *testing.T) {
 				}
 			},
 		},
+		{
+			description:         "GET publisher with alternativeId",
+			query:               "GET /v1/publishers/alternative-id-12345",
+			expectedCode:        200,
+			expectedContentType: "application/json",
+			validateFunc: func(t *testing.T, response map[string]interface{}) {
+				assert.Equal(t, "15fda7c4-6bbf-4387-8f89-258c1e6facb0", response["id"])
+				assert.Equal(t, "alternative-id-12345", response["alternativeId"])
+
+				_, err := time.Parse(time.RFC3339, response["createdAt"].(string))
+				assert.Nil(t, err)
+				_, err = time.Parse(time.RFC3339, response["updatedAt"].(string))
+				assert.Nil(t, err)
+
+				for key := range response {
+					assert.Contains(t, []string{"id", "createdAt", "updatedAt", "codeHosting", "email", "description", "active", "alternativeId"}, key)
+				}
+			},
+		},
 
 		// POST /publishers
 		{
@@ -479,6 +498,18 @@ func TestPublishersEndpoints(t *testing.T) {
 			expectedContentType: "application/problem+json",
 			expectedBody:        `{"title":"can't create Publisher","detail":"description, alternativeId or codeHosting URL already exists","status":409}`,
 		},
+		{
+			description: "POST publisher with alternativeId matching an existing id",
+			query: "POST /v1/publishers",
+			body:  `{"alternativeId": "2ded32eb-c45e-4167-9166-a44e18b8adde", "description":"new description", "codeHosting": [{"url" : "https://example-testcase-xx3.com"}]}`,
+			headers: map[string][]string{
+				"Authorization": {goodToken},
+				"Content-Type":  {"application/json"},
+			},
+			expectedCode:        409,
+			expectedContentType: "application/problem+json",
+			expectedBody:        `{"title":"can't create Publisher","detail":"Publisher with id '2ded32eb-c45e-4167-9166-a44e18b8adde' already exists","status":409}`,
+		},
 		{
 			description: "POST publisher with empty alternativeId",
 			query: "POST /v1/publishers",
@@ -842,6 +873,49 @@ func TestPublishersEndpoints(t *testing.T) {
 			expectedContentType: "application/problem+json",
 			expectedBody: `{"title":"can't update Publisher","detail":"invalid format","status":422,"validationErrors":[{"field":"codeHosting","rule":"gt"}]}`,
 		},
+		{
+			description: "PATCH a publisher via alternativeId",
+			query: "PATCH /v1/publishers/alternative-id-12345",
+			body:  `{"description": "new PATCHed description via alternativeId", "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 via alternativeId", response["description"])
+				assert.IsType(t, []interface{}{}, response["codeHosting"])
+
+				codeHosting := response["codeHosting"].([]interface{})
+				assert.Equal(t, 1, len(codeHosting))
+
+				firstCodeHosting := codeHosting[0].(map[string]interface{})
+
+				assert.Equal(t, "https://gitlab.example.org/patched-repo", firstCodeHosting["url"])
+				assert.Equal(t, "15fda7c4-6bbf-4387-8f89-258c1e6facb0", 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 a publisher with alternativeId matching an existing id",
+			query: "PATCH /v1/publishers/2ded32eb-c45e-4167-9166-a44e18b8adde",
+			body:  `{"alternativeId": "47807e0c-0613-4aea-9917-5455cc6eddad"}`,
+			headers: map[string][]string{
+				"Authorization": {goodToken},
+				"Content-Type":  {"application/json"},
+			},
+			expectedCode:        409,
+			expectedContentType: "application/problem+json",
+			expectedBody:        `{"title":"can't update Publisher","detail":"Publisher with id '47807e0c-0613-4aea-9917-5455cc6eddad' already exists","status":409}`,
+		},
 		{
 			description: "PATCH publishers - wrong token",
 			query:       "PATCH /v1/publishers/2ded32eb-c45e-4167-9166-a44e18b8adde",
@@ -952,6 +1026,17 @@ func TestPublishersEndpoints(t *testing.T) {
 			expectedBody:        "",
 			expectedContentType: "",
 		},
+		{
+			description: "DELETE publisher via alternativeId",
+			query: "DELETE /v1/publishers/alternative-id-12345",
+			headers: map[string][]string{
+				"Authorization": {goodToken},
+				"Content-Type":  {"application/json"},
+			},
+			expectedCode:        204,
+			expectedBody:        "",
+			expectedContentType: "",
+		},
 
 		// WebHooks