distribution client: return structured response for untagged and deleted models (docker#107)

Signed-off-by: Dorin Geman <dorin.geman@docker.com>

Author: doringeman

pkg/distribution/distribution/client.go

@@ -6,7 +6,6 @@ import (
 	"io"
 	"net/http"
 	"os"
-	"strings"
 
 	"github.com/sirupsen/logrus"
 
@@ -243,51 +242,62 @@ func (c *Client) GetModel(reference string) (types.Model, error) {
 	return model, nil
 }
 
+type DeleteModelAction struct {
+	Untagged *string `json:"Untagged,omitempty"`
+	Deleted  *string `json:"Deleted,omitempty"`
+}
+
+type DeleteModelResponse []DeleteModelAction
+
 // DeleteModel deletes a model
-func (c *Client) DeleteModel(reference string, force bool) (string, error) {
+func (c *Client) DeleteModel(reference string, force bool) (*DeleteModelResponse, error) {
 	mdl, err := c.store.Read(reference)
 	if err != nil {
-		return "", err
+		return &DeleteModelResponse{}, err
 	}
 	id, err := mdl.ID()
 	if err != nil {
-		return "", fmt.Errorf("getting model ID: %w", err)
+		return &DeleteModelResponse{}, fmt.Errorf("getting model ID: %w", err)
 	}
 	isTag := id != reference
 
+	resp := DeleteModelResponse{}
+
 	if isTag {
 		c.log.Infoln("Untagging model:", reference)
-		if err := c.store.RemoveTags([]string{reference}); err != nil {
+		tags, err := c.store.RemoveTags([]string{reference})
+		if err != nil {
 			c.log.Errorln("Failed to untag model:", err, "tag:", reference)
-			return "", fmt.Errorf("untagging model: %w", err)
+			return &DeleteModelResponse{}, fmt.Errorf("untagging model: %w", err)
 		}
-	}
-
-	if len(mdl.Tags()) > 1 {
-		if isTag {
-			// TODO: This should return the full matching tag.
-			return fmt.Sprintf("Untagged: %s\n", reference), nil
-		} else if !force {
-			// if the reference is not a tag and there are multiple tags, return an error unless forced
-			return "", fmt.Errorf(
-				"unable to delete %q (must be forced) due to multiple tag references: %w",
-				reference, ErrConflict,
-			)
+		for _, t := range tags {
+			resp = append(resp, DeleteModelAction{Untagged: &t})
+		}
+		if len(mdl.Tags()) > 1 {
+			return &resp, nil
 		}
 	}
 
-	var untaggedInfo strings.Builder
-	for _, tag := range mdl.Tags() {
-		untaggedInfo.WriteString(fmt.Sprintf("Untagged: %s\n", tag))
+	if len(mdl.Tags()) > 1 && !force {
+		// if the reference is not a tag and there are multiple tags, return an error unless forced
+		return &DeleteModelResponse{}, fmt.Errorf(
+			"unable to delete %q (must be forced) due to multiple tag references: %w",
+			reference, ErrConflict,
+		)
 	}
 
 	c.log.Infoln("Deleting model:", id)
-	if err := c.store.Delete(id); err != nil {
+	deletedID, tags, err := c.store.Delete(id)
+	if err != nil {
 		c.log.Errorln("Failed to delete model:", err, "tag:", reference)
-		return "", fmt.Errorf("deleting model: %w", err)
+		return &DeleteModelResponse{}, fmt.Errorf("deleting model: %w", err)
 	}
 	c.log.Infoln("Successfully deleted model:", reference)
-	return untaggedInfo.String(), nil
+	for _, t := range tags {
+		resp = append(resp, DeleteModelAction{Untagged: &t})
+	}
+	resp = append(resp, DeleteModelAction{Deleted: &deletedID})
+	return &resp, nil
 }
 
 // Tag adds a tag to a model

pkg/distribution/distribution/delete_test.go

@@ -1,6 +1,7 @@
 package distribution
 
 import (
+	"encoding/json"
 	"errors"
 	"os"
 	"slices"
@@ -123,26 +124,33 @@ func TestDeleteModel(t *testing.T) {
 			}
 
 			// Attempt to delete the model and check for expected error
-			out, err := client.DeleteModel(tc.ref, tc.force)
+			resp, err := client.DeleteModel(tc.ref, tc.force)
 			if !errors.Is(err, tc.expectedErr) {
 				t.Fatalf("Expected error %v, got: %v", tc.expectedErr, err)
 			}
 			if tc.expectedErr != nil {
 				return
 			}
 
-			expectedOut := ""
+			expectedOut := DeleteModelResponse{}
 			if slices.Contains(tc.tags, tc.ref) {
 				// tc.ref is a tag
-				expectedOut = "Untagged: " + tc.ref + "\n"
+				ref := "index.docker.io/library/" + tc.ref
+				expectedOut = append(expectedOut, DeleteModelAction{Untagged: &ref})
+				if !tc.untagOnly {
+					expectedOut = append(expectedOut, DeleteModelAction{Deleted: &id})
+				}
 			} else {
 				// tc.ref is an ID
 				for _, tag := range tc.tags {
-					expectedOut += "Untagged: " + tag + "\n"
+					expectedOut = append(expectedOut, DeleteModelAction{Untagged: &tag})
 				}
+				expectedOut = append(expectedOut, DeleteModelAction{Deleted: &tc.ref})
 			}
-			if expectedOut != out {
-				t.Fatalf("Expected output %q, got: %q", expectedOut, out)
+			expectedOutJson, _ := json.Marshal(expectedOut)
+			respJson, _ := json.Marshal(resp)
+			if string(expectedOutJson) != string(respJson) {
+				t.Fatalf("Expected output %s, got: %s", expectedOutJson, respJson)
 			}
 
 			// Verify model ref unreachable by ref (untagged)

pkg/distribution/internal/store/index.go

@@ -39,10 +39,10 @@ func (i Index) Tag(reference string, tag string) (Index, error) {
 	return result, nil
 }
 
-func (i Index) UnTag(tag string) Index {
+func (i Index) UnTag(tag string) (name.Tag, Index) {
 	tagRef, err := name.NewTag(tag)
 	if err != nil {
-		return Index{}
+		return name.Tag{}, Index{}
 	}
 
 	result := Index{
@@ -52,7 +52,7 @@ func (i Index) UnTag(tag string) Index {
 		result.Models = append(result.Models, entry.UnTag(tagRef))
 	}
 
-	return result
+	return tagRef, result
 }
 
 func (i Index) Find(reference string) (IndexEntry, int, bool) {

pkg/distribution/internal/store/index_test.go

@@ -146,12 +146,15 @@ func TestUntag(t *testing.T) {
 				},
 			},
 		}
-		idx = idx.UnTag("other-tag")
+		tag, idx := idx.UnTag("other-tag")
 		if len(idx.Models) != 2 {
 			t.Fatalf("Expected 2 models, got %d", len(idx.Models))
 		}
 		if len(idx.Models[0].Tags) != 1 {
 			t.Fatalf("Expected 1 tag, got %d", len(idx.Models[0].Tags))
 		}
+		if tag.String() != "other-tag" {
+			t.Fatalf("Expected tag 'other-tag', got '%s'", tag)
+		}
 	})
 }

pkg/distribution/internal/store/store.go

@@ -7,7 +7,7 @@ import (
 	"path/filepath"
 
 	"github.com/docker/model-distribution/internal/progress"
-
+	"github.com/google/go-containerregistry/pkg/name"
 	v1 "github.com/google/go-containerregistry/pkg/v1"
 )
 
@@ -93,14 +93,14 @@ func (s *LocalStore) List() ([]IndexEntry, error) {
 }
 
 // Delete deletes a model by reference
-func (s *LocalStore) Delete(ref string) error {
+func (s *LocalStore) Delete(ref string) (string, []string, error) {
 	idx, err := s.readIndex()
 	if err != nil {
-		return fmt.Errorf("reading models file: %w", err)
+		return "", nil, fmt.Errorf("reading models file: %w", err)
 	}
 	model, _, ok := idx.Find(ref)
 	if !ok {
-		return ErrModelNotFound
+		return "", nil, ErrModelNotFound
 	}
 
 	// Remove manifest file
@@ -140,7 +140,7 @@ func (s *LocalStore) Delete(ref string) error {
 
 	idx = idx.Remove(model.ID)
 
-	return s.writeIndex(idx)
+	return model.ID, model.Tags, s.writeIndex(idx)
 }
 
 // AddTags adds tags to an existing model
@@ -160,15 +160,18 @@ func (s *LocalStore) AddTags(ref string, newTags []string) error {
 }
 
 // RemoveTags removes tags from models
-func (s *LocalStore) RemoveTags(tags []string) error {
+func (s *LocalStore) RemoveTags(tags []string) ([]string, error) {
 	index, err := s.readIndex()
 	if err != nil {
-		return fmt.Errorf("reading modelss index: %w", err)
+		return nil, fmt.Errorf("reading modelss index: %w", err)
 	}
+	var tagRef name.Tag
+	var tagRefs []string
 	for _, tag := range tags {
-		index = index.UnTag(tag)
+		tagRef, index = index.UnTag(tag)
+		tagRefs = append(tagRefs, tagRef.Name())
 	}
-	return s.writeIndex(index)
+	return tagRefs, s.writeIndex(index)
 }
 
 // Version returns the store version

pkg/distribution/internal/store/store_test.go

@@ -151,10 +151,13 @@ func TestStoreAPI(t *testing.T) {
 
 	// Test RemoveTags
 	t.Run("RemoveTags", func(t *testing.T) {
-		err := s.RemoveTags([]string{"api-model:api-v1.0"})
+		tags, err := s.RemoveTags([]string{"api-model:api-v1.0"})
 		if err != nil {
 			t.Fatalf("RemoveTags failed: %v", err)
 		}
+		if tags[0] != "index.docker.io/library/api-model:api-v1.0" {
+			t.Fatalf("Expected removed tag 'index.docker.io/library/api-model:api-v1.0', got '%s'", tags[0])
+		}
 
 		// Verify tag was removed from list
 		models, err := s.List()
@@ -178,7 +181,7 @@ func TestStoreAPI(t *testing.T) {
 
 	// Test Delete
 	t.Run("Delete", func(t *testing.T) {
-		err := s.Delete("api-model:latest")
+		_, _, err := s.Delete("api-model:latest")
 		if err != nil {
 			t.Fatalf("Delete failed: %v", err)
 		}
@@ -192,7 +195,7 @@ func TestStoreAPI(t *testing.T) {
 
 	// Test Delete Non Existent Model
 	t.Run("Delete", func(t *testing.T) {
-		err := s.Delete("non-existent-model:latest")
+		_, _, err := s.Delete("non-existent-model:latest")
 		if !errors.Is(err, store.ErrModelNotFound) {
 			t.Fatalf("Expected ErrModelNotFound, got %v", err)
 		}
@@ -242,7 +245,7 @@ func TestStoreAPI(t *testing.T) {
 		}
 
 		// Delete the model
-		if err := s.Delete("blob-test:latest"); err != nil {
+		if _, _, err := s.Delete("blob-test:latest"); err != nil {
 			t.Fatalf("Delete failed: %v", err)
 		}
 
@@ -320,7 +323,7 @@ func TestStoreAPI(t *testing.T) {
 		}
 
 		// Delete the first model
-		if err := s.Delete("shared-model-1:latest"); err != nil {
+		if _, _, err := s.Delete("shared-model-1:latest"); err != nil {
 			t.Fatalf("Delete first model failed: %v", err)
 		}
 
@@ -368,7 +371,7 @@ func TestStoreAPI(t *testing.T) {
 		}
 
 		// Delete the second model
-		if err := s.Delete("shared-model-2:latest"); err != nil {
+		if _, _, err := s.Delete("shared-model-2:latest"); err != nil {
 			t.Fatalf("Delete second model failed: %v", err)
 		}