From b552c293ca894afbf9c655b4931afa85d10ac07f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20Guillot?= Date: Sat, 24 Jun 2023 11:03:26 -0700 Subject: [PATCH] Add unique index enclosures_user_entry_url_idx --- database/migrations.go | 28 +++++++++ storage/enclosure.go | 92 +++++++++++------------------ ui/entry_enclosure_save_position.go | 14 +++-- 3 files changed, 71 insertions(+), 63 deletions(-) diff --git a/database/migrations.go b/database/migrations.go index a9d2dae3..55f80529 100644 --- a/database/migrations.go +++ b/database/migrations.go @@ -674,4 +674,32 @@ var migrations = []func(tx *sql.Tx) error{ _, err = tx.Exec(sql) return err }, + func(tx *sql.Tx) (err error) { + // Delete duplicated rows + sql := ` + DELETE FROM enclosures a USING enclosures b + WHERE a.id < b.id + AND a.user_id = b.user_id + AND a.entry_id = b.entry_id + AND a.url = b.url; + ` + _, err = tx.Exec(sql) + if err != nil { + return err + } + + // Remove previous index + _, err = tx.Exec(`DROP INDEX enclosures_user_entry_url_idx`) + if err != nil { + return err + } + + // Create unique index + _, err = tx.Exec(`CREATE UNIQUE INDEX enclosures_user_entry_url_unique_idx ON enclosures(user_id, entry_id, md5(url))`) + if err != nil { + return err + } + + return nil + }, } diff --git a/storage/enclosure.go b/storage/enclosure.go index 048c3fde..3573609b 100644 --- a/storage/enclosure.go +++ b/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 } diff --git a/ui/entry_enclosure_save_position.go b/ui/entry_enclosure_save_position.go index d74e3745..91fd0078 100644 --- a/ui/entry_enclosure_save_position.go +++ b/ui/entry_enclosure_save_position.go @@ -12,10 +12,6 @@ import ( "miniflux.app/http/response/json" ) -type enclosurePositionSaveRequest struct { - Progression int64 `json:"progression"` -} - func (h *handler) saveEnclosureProgression(w http.ResponseWriter, r *http.Request) { enclosureID := request.RouteInt64Param(r, "enclosureID") enclosure, err := h.store.GetEnclosure(enclosureID) @@ -23,6 +19,16 @@ func (h *handler) saveEnclosureProgression(w http.ResponseWriter, r *http.Reques json.ServerError(w, r, err) return } + + if enclosure == nil { + json.NotFound(w, r) + return + } + + type enclosurePositionSaveRequest struct { + Progression int64 `json:"progression"` + } + var postData enclosurePositionSaveRequest body, err := io.ReadAll(r.Body) if err != nil {