Batch Edit: Improve validation and error handling for album and label actions #271

internal/photoprism/batch/actions.go

@@ -20,26 +20,58 @@ const (
 
 // ApplyAlbums adds/removes the given photo to/from albums according to items action.
 func ApplyAlbums(photoUID string, albums Items) error {
+	// Validate photo UID
+	if !rnd.IsUID(photoUID, entity.PhotoUID) {
+		return fmt.Errorf("invalid photo uid: %s", photoUID)
+	}
+
 	var addTargets []string
 
 	for _, it := range albums.Items {
 		switch it.Action {
 		case ActionAdd:
+			// Validate that we have either value or title
+			if it.Value == "" && it.Title == "" {
+				return fmt.Errorf("album value or title required for add action")
+			}
+			
 			// Add by UID if provided, otherwise use title to create/find
 			if it.Value != "" {
+				// If value is provided, validate it's a proper UID format
+				if !rnd.IsUID(it.Value, entity.AlbumUID) {
+					return fmt.Errorf("invalid album uid format: %s", it.Value)
+				}
+				
+				// Check if album exists when adding by UID
+				if _, err := query.AlbumByUID(it.Value); err != nil {
+					return fmt.Errorf("album not found: %s", it.Value)
+				}
+				
 				addTargets = append(addTargets, it.Value)
 			} else if it.Title != "" {
 				addTargets = append(addTargets, it.Title)
 			}
 		case ActionRemove:
-			// Remove only if we have a valid album UID
-			if rnd.IsUID(it.Value, entity.AlbumUID) {
-				if a, err := query.AlbumByUID(it.Value); err != nil {
-					log.Debugf("batch: album %s not found for removal: %s", it.Value, err)
-				} else if a.HasID() {
-					a.RemovePhotos([]string{photoUID})
-				}
+			// Validate that we have a value for removal
+			if it.Value == "" {
+				return fmt.Errorf("album uid required for remove action")
 			}
+			
+			// Remove only if we have a valid album UID
+			if !rnd.IsUID(it.Value, entity.AlbumUID) {
+				return fmt.Errorf("invalid album uid format: %s", it.Value)
+			}
+			
+			if a, err := query.AlbumByUID(it.Value); err != nil {
+				return fmt.Errorf("album not found for removal: %s", it.Value)
+			} else if a.HasID() {
+				a.RemovePhotos([]string{photoUID})
+			}
+		case ActionNone, ActionUpdate:
+			// Valid actions that do nothing for albums
+			continue
+		default:
+			return fmt.Errorf("invalid action: %s", it.Action)
 		}
 	}
 
@@ -64,13 +96,23 @@ func ApplyLabels(photo *entity.Photo, labels Items) error {
 	for _, it := range labels.Items {
 		switch it.Action {
 		case ActionAdd:
+			// Validate that we have either value or title
+			if it.Value == "" && it.Title == "" {
+				return fmt.Errorf("label value or title required for add action")
+			}
+
 			// Try by UID first
 			var labelEntity *entity.Label
 			var err error
 			if it.Value != "" {
+				// If value is provided, validate it's a proper UID format
+				if !rnd.IsUID(it.Value, entity.LabelUID) {
+					return fmt.Errorf("invalid label uid format: %s", it.Value)
+				}
+				
 				labelEntity, err = query.LabelByUID(it.Value)
 				if err != nil {
-					labelEntity = nil
+					return fmt.Errorf("label not found: %s", it.Value)
 				}
 			}
 			if labelEntity == nil && it.Title != "" {
@@ -79,8 +121,7 @@ func ApplyLabels(photo *entity.Photo, labels Items) error {
 			}
 
 			if labelEntity == nil {
-				log.Debugf("batch: could not resolve label to add: value=%s title=%s", it.Value, clean.Log(it.Title))
-				continue
+				return fmt.Errorf("could not resolve label to add: value=%s title=%s", it.Value, clean.Log(it.Title))
 			}
 
 			if err := labelEntity.Restore(); err != nil {
@@ -107,18 +148,21 @@ func ApplyLabels(photo *entity.Photo, labels Items) error {
 
 		case ActionRemove:
 			if it.Value == "" {
-				log.Debugf("batch: label remove skipped (uid required): photo=%s title=%s", photo.PhotoUID, clean.Log(it.Title))
-				continue
+				return fmt.Errorf("label uid required for remove action")
+			}
+
+			// Validate UID format
+			if !rnd.IsUID(it.Value, entity.LabelUID) {
+				return fmt.Errorf("invalid label uid format: %s", it.Value)
 			}
 
 			labelEntity, err := query.LabelByUID(it.Value)
 			if err != nil || labelEntity == nil || !labelEntity.HasID() {
-				log.Debugf("batch: label not found for removal by uid: photo=%s uid=%s", photo.PhotoUID, it.Value)
-				continue
+				return fmt.Errorf("label not found for removal: %s", it.Value)
 			}
 
 			if pl, err := query.PhotoLabel(photo.ID, labelEntity.ID); err != nil {
-				log.Debugf("batch: photo-label not found for removal: photo=%s label_id=%d", photo.PhotoUID, labelEntity.ID)
+				return fmt.Errorf("photo-label not found for removal: photo=%s label_id=%d", photo.PhotoUID, labelEntity.ID)
 			} else if pl != nil {
 				if (pl.LabelSrc == entity.SrcManual || pl.LabelSrc == entity.SrcBatch) && pl.Uncertainty < 100 {
 					if err := entity.Db().Delete(&pl).Error; err != nil {
@@ -143,6 +187,11 @@ func ApplyLabels(photo *entity.Photo, labels Items) error {
 				}
 				_ = photo.RemoveKeyword(labelEntity.LabelName)
 			}
+		case ActionNone, ActionUpdate:
+			// Valid actions that do nothing for labels
+			continue
+		default:
+			return fmt.Errorf("invalid action: %s", it.Action)
 		}
 	}
 

internal/photoprism/batch/actions_test.go

@@ -128,6 +128,116 @@ func TestApplyAlbums(t *testing.T) {
 			t.Error("expected photo to be marked as hidden in album")
 		}
 	})
+
+	// Error cases
+	t.Run("AddPhotoToNonExistingAlbumByUID", func(t *testing.T) {
+		photo := entity.PhotoFixtures.Get("Photo04")
+		nonExistingAlbumUID := "at9lxuqxpoaaaaaa" // Invalid/non-existing UID
+
+		albums := Items{
+			Items: []Item{
+				{Action: ActionAdd, Value: nonExistingAlbumUID},
+			},
+		}
+
+		err := ApplyAlbums(photo.PhotoUID, albums)
+		if err == nil {
+			t.Error("expected error when adding photo to non-existing album, but got none")
+		}
+	})
+
+	t.Run("AddPhotoToAlbumWithInvalidUID", func(t *testing.T) {
+		photo := entity.PhotoFixtures.Get("Photo04")
+		invalidUID := "invalid-uid-format" // Invalid UID format
+
+		albums := Items{
+			Items: []Item{
+				{Action: ActionAdd, Value: invalidUID},
+			},
+		}
+
+		err := ApplyAlbums(photo.PhotoUID, albums)
+		if err == nil {
+			t.Error("expected error when adding photo to album with invalid UID, but got none")
+		}
+	})
+
+	t.Run("RemovePhotoFromNonExistingAlbum", func(t *testing.T) {
+		photo := entity.PhotoFixtures.Get("Photo05")
+		nonExistingAlbumUID := "at9lxuqxpobbbbbb" // Non-existing UID
+
+		albums := Items{
+			Items: []Item{
+				{Action: ActionRemove, Value: nonExistingAlbumUID},
+			},
+		}
+
+		err := ApplyAlbums(photo.PhotoUID, albums)
+		if err == nil {
+			t.Error("expected error when removing photo from non-existing album, but got none")
+		}
+	})
+
+	t.Run("InvalidActionOnAlbum", func(t *testing.T) {
+		photo := entity.PhotoFixtures.Get("Photo06")
+		albumUID := entity.AlbumFixtures.Get("christmas2030").AlbumUID
+
+		albums := Items{
+			Items: []Item{
+				{Action: "invalid-action", Value: albumUID}, // Invalid action
+			},
+		}
+
+		err := ApplyAlbums(photo.PhotoUID, albums)
+		if err == nil {
+			t.Error("expected error for invalid action, but got none")
+		}
+	})
+
+	t.Run("EmptyAlbumItems", func(t *testing.T) {
+		photo := entity.PhotoFixtures.Get("Photo07")
+
+		albums := Items{
+			Items: []Item{}, // Empty items
+		}
+
+		// This should not error, but should be a no-op
+		err := ApplyAlbums(photo.PhotoUID, albums)
+		if err != nil {
+			t.Errorf("expected no error for empty album items, but got: %v", err)
+		}
+	})
+
+	t.Run("AddPhotoToAlbumWithEmptyValueAndTitle", func(t *testing.T) {
+		photo := entity.PhotoFixtures.Get("Photo08")
+
+		albums := Items{
+			Items: []Item{
+				{Action: ActionAdd, Value: "", Title: ""}, // Both empty
+			},
+		}
+
+		err := ApplyAlbums(photo.PhotoUID, albums)
+		if err == nil {
+			t.Error("expected error when both Value and Title are empty, but got none")
+		}
+	})
+
+	t.Run("InvalidPhotoUID", func(t *testing.T) {
+		invalidPhotoUID := "invalid-photo-uid"
+		albumUID := entity.AlbumFixtures.Get("christmas2030").AlbumUID
+
+		albums := Items{
+			Items: []Item{
+				{Action: ActionAdd, Value: albumUID},
+			},
+		}
+
+		err := ApplyAlbums(invalidPhotoUID, albums)
+		if err == nil {
+			t.Error("expected error for invalid photo UID, but got none")
+		}
+	})
 }
 
 func TestApplyLabels(t *testing.T) {
@@ -334,4 +444,117 @@ func TestApplyLabels(t *testing.T) {
 			t.Error("expected error for empty photo")
 		}
 	})
+
+	// Additional error cases
+	t.Run("AddNonExistingLabelByUID", func(t *testing.T) {
+		photo := entity.PhotoFixtures.Pointer("Photo11")
+		nonExistingLabelUID := "lt9lxuqxpoaaaaaa" // Invalid/non-existing UID
+
+		labels := Items{
+			Items: []Item{
+				{Action: ActionAdd, Value: nonExistingLabelUID},
+			},
+		}
+
+		err := ApplyLabels(photo, labels)
+		if err == nil {
+			t.Error("expected error when adding non-existing label by UID, but got none")
+		}
+	})
+
+	t.Run("AddLabelWithInvalidUID", func(t *testing.T) {
+		photo := entity.PhotoFixtures.Pointer("Photo12")
+		invalidUID := "invalid-label-uid-format" // Invalid UID format
+
+		labels := Items{
+			Items: []Item{
+				{Action: ActionAdd, Value: invalidUID},
+			},
+		}
+
+		err := ApplyLabels(photo, labels)
+		if err == nil {
+			t.Error("expected error when adding label with invalid UID, but got none")
+		}
+	})
+
+	t.Run("RemoveNonExistingLabelByUID", func(t *testing.T) {
+		photo := entity.PhotoFixtures.Pointer("Photo13")
+		nonExistingLabelUID := "lt9lxuqxpobbbbbb" // Non-existing UID
+
+		labels := Items{
+			Items: []Item{
+				{Action: ActionRemove, Value: nonExistingLabelUID},
+			},
+		}
+
+		err := ApplyLabels(photo, labels)
+		if err == nil {
+			t.Error("expected error when removing non-existing label, but got none")
+		}
+	})
+
+	t.Run("InvalidActionOnLabel", func(t *testing.T) {
+		photo := entity.PhotoFixtures.Pointer("Photo14")
+		labelUID := entity.LabelFixtures.Get("landscape").LabelUID
+
+		labels := Items{
+			Items: []Item{
+				{Action: "invalid-action", Value: labelUID}, // Invalid action
+			},
+		}
+
+		err := ApplyLabels(photo, labels)
+		if err == nil {
+			t.Error("expected error for invalid action, but got none")
+		}
+	})
+
+	t.Run("EmptyLabelItems", func(t *testing.T) {
+		photo := entity.PhotoFixtures.Pointer("Photo15")
+
+		labels := Items{
+			Items: []Item{}, // Empty items
+		}
+
+		// This should not error, but should be a no-op
+		err := ApplyLabels(photo, labels)
+		if err != nil {
+			t.Errorf("expected no error for empty label items, but got: %v", err)
+		}
+	})
+
+	t.Run("AddLabelWithEmptyValueAndTitle", func(t *testing.T) {
+		photo := entity.PhotoFixtures.Pointer("Photo16")
+
+		labels := Items{
+			Items: []Item{
+				{Action: ActionAdd, Value: "", Title: ""}, // Both empty
+			},
+		}
+
+		err := ApplyLabels(photo, labels)
+		if err == nil {
+			t.Error("expected error when both Value and Title are empty, but got none")
+		}
+	})
+
+	t.Run("RemoveLabelNotAssignedToPhoto", func(t *testing.T) {
+		photo := entity.PhotoFixtures.Pointer("Photo17")
+		labelUID := entity.LabelFixtures.Get("bird").LabelUID
+
+		// Ensure the label is not assigned to this photo
+		entity.Db().Where("photo_id = ? AND label_id = (SELECT id FROM labels WHERE label_uid = ?)", photo.ID, labelUID).Delete(&entity.PhotoLabel{})
+
+		labels := Items{
+			Items: []Item{
+				{Action: ActionRemove, Value: labelUID},
+			},
+		}
+
+		err := ApplyLabels(photo, labels)
+		if err == nil {
+			t.Error("expected error when removing label not assigned to photo, but got none")
+		}
+	})
 }