Merge pull request #347 from blackcandy-org/clean-duplicate

Prevent syncing duplicate songs with same md5_hash

app/models/song.rb

@@ -6,6 +6,7 @@ class Song < ApplicationRecord
   include SortableConcern
 
   validates :name, :file_path, :file_path_hash, :md5_hash, presence: true
+  validates :md5_hash, uniqueness: true
 
   belongs_to :album, touch: true
   belongs_to :artist, touch: true

db/migrate/20240206051609_add_unique_index_on_song_md5_hash.rb

@@ -0,0 +1,14 @@
+class AddUniqueIndexOnSongMd5Hash < ActiveRecord::Migration[7.1]
+  def up
+    # Remove duplicate songs
+    Song.where.not(id: Song.group(:md5_hash).select("min(id)")).destroy_all
+
+    remove_index :songs, :md5_hash
+    add_index :songs, :md5_hash, unique: true
+  end
+
+  def down
+    remove_index :songs, :md5_hash
+    add_index :songs, :md5_hash
+  end
+end

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema[7.1].define(version: 2024_02_02_024633) do
+ActiveRecord::Schema[7.1].define(version: 2024_02_06_051609) do
   create_table "active_storage_attachments", force: :cascade do |t|
     t.string "name", null: false
     t.string "record_type", null: false
@@ -211,7 +211,7 @@
     t.index ["album_id"], name: "index_songs_on_album_id"
     t.index ["artist_id"], name: "index_songs_on_artist_id"
     t.index ["file_path_hash"], name: "index_songs_on_file_path_hash"
-    t.index ["md5_hash"], name: "index_songs_on_md5_hash"
+    t.index ["md5_hash"], name: "index_songs_on_md5_hash", unique: true
     t.index ["name"], name: "index_songs_on_name"
   end
 

test/models/album_test.rb

@@ -21,9 +21,9 @@ class AlbumTest < ActiveSupport::TestCase
 
     album.songs.create!(
       [
-        {name: "test_song_1", file_path: "fake_path", file_path_hash: "fake_path_hash", md5_hash: "fake_md5", tracknum: 2, discnum: 2, artist: artist},
-        {name: "test_song_2", file_path: "fake_path", file_path_hash: "fake_path_hash", md5_hash: "fake_md5", tracknum: 3, discnum: 1, artist: artist},
-        {name: "test_song_3", file_path: "fake_path", file_path_hash: "fake_path_hash", md5_hash: "fake_md5", tracknum: 1, discnum: 1, artist: artist}
+        {name: "test_song_1", file_path: "fake_path", file_path_hash: "fake_path_hash", md5_hash: "fake_md5_0", tracknum: 2, discnum: 2, artist: artist},
+        {name: "test_song_2", file_path: "fake_path", file_path_hash: "fake_path_hash", md5_hash: "fake_md5_1", tracknum: 3, discnum: 1, artist: artist},
+        {name: "test_song_3", file_path: "fake_path", file_path_hash: "fake_path_hash", md5_hash: "fake_md5_2", tracknum: 1, discnum: 1, artist: artist}
       ]
     )
 

test/models/song_test.rb

@@ -82,4 +82,18 @@ class SongTest < ActiveSupport::TestCase
   test "should use default sort when use invalid sort value" do
     assert_equal songs(:flac_sample), Song.sort_records(:invalid).first
   end
+
+  test "should get unique error when create song with same md5_hash" do
+    song = Song.new(
+      name: "song_test",
+      file_path: Rails.root.join("test/fixtures/files/artist1_album2.mp3"),
+      file_path_hash: "fake_path_hash",
+      md5_hash: songs(:flac_sample).md5_hash,
+      artist_id: artists(:artist1).id,
+      album_id: albums(:album1).id
+    )
+
+    assert_not song.valid?
+    assert_equal ["has already been taken"], song.errors[:md5_hash]
+  end
 end

test/system/songs_test.rb

@@ -10,7 +10,7 @@ class SongsSystemTest < ApplicationSystemTestCase
         name: "song_test_#{n}",
         file_path: Rails.root.join("test/fixtures/files/artist1_album2.mp3"),
         file_path_hash: "fake_path_hash",
-        md5_hash: "fake_md5",
+        md5_hash: "fake_md5_#{n}",
         artist_id: artists(:artist1).id,
         album_id: albums(:album1).id
       )