fix: Ignore concurrent deletes in GetCategories

GetCategories() would fail if a category was deleted between the initial
ListCategories() and GetCategory() details loop which is not atomic.

Closes: #2710
Signed-off-by: Michael Gasch <[email protected]>

Author: Michael Gasch

vapi/tags/categories.go

@@ -20,11 +20,13 @@ import (
 	"context"
 	"fmt"
 	"net/http"
+	"strings"
 
 	"github.com/vmware/govmomi/vapi/internal"
 )
 
-// Category provides methods to create, read, update, delete, and enumerate categories.
+// Category provides methods to create, read, update, delete, and enumerate
+// categories.
 type Category struct {
 	ID              string   `json:"id,omitempty"`
 	Name            string   `json:"name,omitempty"`
@@ -92,7 +94,8 @@ func (c *Manager) CreateCategory(ctx context.Context, category *Category) (strin
 	return res, c.Do(ctx, url.Request(http.MethodPost, spec), &res)
 }
 
-// UpdateCategory can update one or more of the AssociableTypes, Cardinality, Description and Name fields.
+// UpdateCategory updates one or more of the AssociableTypes, Cardinality,
+// Description and Name fields.
 func (c *Manager) UpdateCategory(ctx context.Context, category *Category) error {
 	spec := struct {
 		Category Category `json:"update_spec"`
@@ -108,7 +111,7 @@ func (c *Manager) UpdateCategory(ctx context.Context, category *Category) error
 	return c.Do(ctx, url.Request(http.MethodPatch, spec), nil)
 }
 
-// DeleteCategory deletes an existing category.
+// DeleteCategory deletes a category.
 func (c *Manager) DeleteCategory(ctx context.Context, category *Category) error {
 	url := c.Resource(internal.CategoryPath).WithID(category.ID)
 	return c.Do(ctx, url.Request(http.MethodDelete), nil)
@@ -141,7 +144,7 @@ func (c *Manager) ListCategories(ctx context.Context) ([]string, error) {
 	return res, c.Do(ctx, url.Request(http.MethodGet), &res)
 }
 
-// GetCategories fetches an array of category information in the system.
+// GetCategories fetches a list of category information in the system.
 func (c *Manager) GetCategories(ctx context.Context) ([]Category, error) {
 	ids, err := c.ListCategories(ctx)
 	if err != nil {
@@ -152,11 +155,13 @@ func (c *Manager) GetCategories(ctx context.Context) ([]Category, error) {
 	for _, id := range ids {
 		category, err := c.GetCategory(ctx, id)
 		if err != nil {
-			return nil, fmt.Errorf("get category %s: %s", id, err)
+			if strings.Contains(err.Error(), http.StatusText(http.StatusNotFound)) {
+				continue // deleted since last fetch
+			}
+			return nil, fmt.Errorf("get category %s: %v", id, err)
 		}
-
 		categories = append(categories, *category)
-
 	}
+
 	return categories, nil
 }

vapi/tags/categories_test.go

@@ -0,0 +1,87 @@
+package tags
+
+import (
+	"context"
+	"fmt"
+	"net/http"
+	"strings"
+	"testing"
+
+	"github.com/vmware/govmomi/simulator"
+	"github.com/vmware/govmomi/vapi/rest"
+	"github.com/vmware/govmomi/vim25"
+)
+
+func TestManager_GetCategories(t *testing.T) {
+	simulator.Run(func(ctx context.Context, client *vim25.Client) error {
+		rc := rest.NewClient(client)
+
+		tr := testRoundTripper{
+			t:         t,
+			transport: rc.Client.Client.Transport,
+		}
+		rc.Client.Client.Transport = &tr
+
+		err := rc.Login(ctx, simulator.DefaultLogin)
+		if err != nil {
+			t.Fatalf("VAPI login: %v", err)
+		}
+
+		tm := NewManager(rc)
+
+		// categories to createCount and (concurrently) delete while retrieving categories
+		const (
+			createCount = 5
+			deleteCount = 2
+		)
+
+		var created []string
+		for i := 1; i <= createCount; i++ {
+			cat, err := tm.CreateCategory(ctx, &Category{
+				Name: fmt.Sprintf("testcat-%d", i),
+			})
+			if err != nil {
+				t.Fatalf("createCount category: %v", err)
+			}
+			created = append(created, cat)
+		}
+
+		for i := 0; i < deleteCount; i++ {
+			tr.deleted = append(tr.deleted, created[i])
+		}
+
+		got, err := tm.GetCategories(ctx)
+		if err != nil {
+			t.Fatalf("get categories: %v", err)
+		}
+
+		if len(got) != createCount-deleteCount {
+			t.Errorf("category count mismatch: got=%d, want=%d", len(got), createCount-deleteCount)
+		}
+
+		return nil
+	})
+}
+
+// testRoundTripper returns 404 for all categories in deleted
+type testRoundTripper struct {
+	t *testing.T
+
+	deleted   []string
+	transport http.RoundTripper
+}
+
+func (tr *testRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) {
+	for _, id := range tr.deleted {
+		if strings.Contains(req.URL.Path, id) {
+			return &http.Response{
+				StatusCode: http.StatusNotFound,
+				Status:     http.StatusText(http.StatusNotFound),
+				Body:       nil,
+				Request:    req.Clone(context.Background()),
+			}, nil
+		}
+	}
+
+	return tr.transport.RoundTrip(req)
+}