Add unique index enclosures_user_entry_url_idx

storage/enclosure.go

@@ -6,26 +6,13 @@ package storage // import "miniflux.app/storage"
 import (
 	"database/sql"
 	"fmt"
+	"strings"
 
 	"miniflux.app/model"
 )
 
 // GetEnclosures returns all attachments for the given entry.
 func (s *Storage) GetEnclosures(entryID int64) (model.EnclosureList, error) {
-	tx, err := s.db.Begin()
-	if err != nil {
-		return nil, fmt.Errorf(`store: unable to start transaction: %v`, err)
-	}
-	// As the transaction is only created to use the txGetEnclosures function, we can commit it and close it.
-	// to avoid leaving an open transaction as I don't have any idea if it will be closed automatically,
-	// I manually close it. I chose `commit` over `rollback` because I assumed it cost less on SGBD, but I'm no Database
-	// administrator so any better solution is welcome.
-	defer tx.Commit()
-	return s.txGetEnclosures(tx, entryID)
-}
-
-// GetEnclosures returns all attachments for the given entry within a Database transaction
-func (s *Storage) txGetEnclosures(tx *sql.Tx, entryID int64) (model.EnclosureList, error) {
 	query := `
 		SELECT
 			id,
@@ -42,7 +29,7 @@ func (s *Storage) txGetEnclosures(tx *sql.Tx, entryID int64) (model.EnclosureLis
 		ORDER BY id ASC
 	`
 
-	rows, err := tx.Query(query, entryID)
+	rows, err := s.db.Query(query, entryID)
 	if err != nil {
 		return nil, fmt.Errorf(`store: unable to fetch enclosures: %v`, err)
 	}
@@ -109,7 +96,8 @@ func (s *Storage) GetEnclosure(enclosureID int64) (*model.Enclosure, error) {
 }
 
 func (s *Storage) createEnclosure(tx *sql.Tx, enclosure *model.Enclosure) error {
-	if enclosure.URL == "" {
+	enclosureURL := strings.TrimSpace(enclosure.URL)
+	if enclosureURL == "" {
 		return nil
 	}
 
@@ -118,76 +106,60 @@ func (s *Storage) createEnclosure(tx *sql.Tx, enclosure *model.Enclosure) error
 			(url, size, mime_type, entry_id, user_id, media_progression)
 		VALUES
 			($1, $2, $3, $4, $5, $6)
-		RETURNING
-			id
+		ON CONFLICT (user_id, entry_id, md5(url)) DO NOTHING
 	`
-	err := tx.QueryRow(
+	_, err := tx.Exec(
 		query,
-		enclosure.URL,
+		enclosureURL,
 		enclosure.Size,
 		enclosure.MimeType,
 		enclosure.EntryID,
 		enclosure.UserID,
 		enclosure.MediaProgression,
-	).Scan(&enclosure.ID)
+	)
 
 	if err != nil {
-		return fmt.Errorf(`store: unable to create enclosure %q: %v`, enclosure.URL, err)
+		return fmt.Errorf(`store: unable to create enclosure: %v`, err)
 	}
 
 	return nil
 }
 
 func (s *Storage) updateEnclosures(tx *sql.Tx, userID, entryID int64, enclosures model.EnclosureList) error {
-	originalEnclosures, err := s.txGetEnclosures(tx, entryID)
-	if err != nil {
-		return fmt.Errorf(`store: unable fetch enclosures for entry #%d : %v`, entryID, err)
+	if len(enclosures) == 0 {
+		return nil
 	}
 
-	// this map will allow to identify enclosure already in the database based on their URL.
-	originalEnclosuresByURL := map[string]*model.Enclosure{}
-	for _, enclosure := range originalEnclosures {
-		originalEnclosuresByURL[enclosure.URL] = enclosure
-	}
-
-	// in order to keep enclosure ID consistent I need to identify already existing one to keep them as is, and only
-	// add/delete enclosure that need to be
-	enclosuresToAdd := map[string]*model.Enclosure{}
-	enclosuresToDelete := map[string]*model.Enclosure{}
-	enclosuresToKeep := map[string]*model.Enclosure{}
+	sqlValues := []any{userID, entryID}
+	sqlPlaceholders := []string{}
 
 	for _, enclosure := range enclosures {
-		originalEnclosure, alreadyExist := originalEnclosuresByURL[enclosure.URL]
-		if alreadyExist {
-			enclosuresToKeep[originalEnclosure.URL] = originalEnclosure // we keep the original already in the database
-		} else {
-			enclosuresToAdd[enclosure.URL] = enclosure // we insert the new one
-		}
-	}
+		sqlPlaceholders = append(sqlPlaceholders, fmt.Sprintf(`$%d`, len(sqlValues)+1))
+		sqlValues = append(sqlValues, strings.TrimSpace(enclosure.URL))
 
-	// we know what to keep, and add. We need to find what's in the database that need to be deleted
-	for _, enclosure := range originalEnclosures {
-		_, existToAdd := enclosuresToAdd[enclosure.URL]
-		_, existToKeep := enclosuresToKeep[enclosure.URL]
-		if !existToKeep && !existToAdd { // if it does not exist to keep or add this mean it has been deleted.
-			enclosuresToDelete[enclosure.URL] = enclosure
-		}
-	}
-
-	for _, enclosure := range enclosuresToDelete {
-		if _, err := tx.Exec(`DELETE FROM enclosures WHERE user_id=$1 AND entry_id=$2 and id=$3`, userID, entryID, enclosure.ID); err != nil {
-			return err
-		}
-	}
-
-	for _, enclosure := range enclosuresToAdd {
 		if err := s.createEnclosure(tx, enclosure); err != nil {
 			return err
 		}
 	}
 
+	query := `
+		DELETE FROM enclosures
+		WHERE
+			user_id=$1 AND
+			entry_id=$2 AND
+			url NOT IN (%s)
+	`
+
+	query = fmt.Sprintf(query, strings.Join(sqlPlaceholders, `,`))
+
+	_, err := tx.Exec(query, sqlValues...)
+	if err != nil {
+		return fmt.Errorf(`store: unable to delete old enclosures: %v`, err)
+	}
+
 	return nil
 }
+
 func (s *Storage) UpdateEnclosure(enclosure *model.Enclosure) error {
 	query := `
 		UPDATE
@@ -211,8 +183,10 @@ func (s *Storage) UpdateEnclosure(enclosure *model.Enclosure) error {
 		enclosure.MediaProgression,
 		enclosure.ID,
 	)
+
 	if err != nil {
 		return fmt.Errorf(`store: unable to update enclosure #%d : %v`, enclosure.ID, err)
 	}
+
 	return nil
 }