Don't duplicate attachments

beets/attachments.py

@@ -95,14 +95,10 @@ class Attachment(dbcore.db.Model):
         `self.entity == entity`.
         """
         self._entity = entity
-
-    def store(self):
-        entity = self._entity
-        if entity is None or entity.id is None:
-            raise ValueError('{} must have an id'.format(entity))
-        self.ref_type = ref_type(entity)
-        self.ref = entity.id
-        super(Attachment, self).store()
+        # FIXME we cannot use self.ref = None because none is converted
+        # to 0.
+        self._values_fixed['ref'] = None
+        self._values_fixed['ref_type'] = None
 
     def move(self, dest=None, copy=False, overwrite=False):
         """Moves the attachment from its original `path` to `dest` and
@@ -204,10 +200,29 @@ class Attachment(dbcore.db.Model):
                 return Template(template_str)
         return Template(DEFAULT_TEMPLATE)
 
+    def store(self):
+        self._validate()
+        super(Attachment, self).store()
+
+    def add(self, db=None):
+        if db:
+            self._db = db
+        self._check_db(False)
+        self._validate()
+
+        if self.id:
+            self.store()
+        else:
+            super(Attachment, self).add()
+        return self
+
     def _validate(self):
-        # TODO integrate this into the `store()` method.
-        assert self.entity
-        assert re.match(r'^[a-zA-Z][-\w]*', self.type)
+        if self.ref is None or self.ref_type is None:
+            entity = self.entity
+            if entity is None or entity.id is None:
+                raise ValueError('{} must have an id'.format(entity))
+            self.ref_type = ref_type(entity)
+            self.ref = entity.id
 
     def __getattr__(self, key):
         # Called only if attribute was not found on self or in the class
@@ -363,6 +378,10 @@ class AttachmentFactory(object):
         set retrieves meta data from registered collectors and and adds
         it as flexible attributes.
 
+        If an attachment with the same path, ref, ref_type and type
+        attributes already exists in the database, it returns that
+        record
+
         Also sets the attachments's basename to the value returned by
         `Attachment.basename()`. Therefore, if the entity is moved
         later, we retain the basename instead of recalculating it
@@ -371,6 +390,16 @@ class AttachmentFactory(object):
         # TODO entity should not be optional
         attachment = Attachment(db=self._db, path=path,
                                 entity=entity, type=type)
+        if entity and entity.id:
+            existing = self.find(AndQuery([
+                MatchQuery('path', attachment.path),
+                MatchQuery('ref', entity.id),
+                MatchQuery('ref_type', ref_type(entity)),
+                MatchQuery('type', attachment.type),
+            ])).get()
+            if existing is not None:
+                attachment = existing
+
         attachment.basename = self.basename(path, entity)
         for key, value in self._collect_meta(type, attachment.path).items():
             attachment[key] = value
@@ -382,8 +411,7 @@ class AttachmentFactory(object):
         This is the same as calling `create()` and then adding the
         attachment to the database.
         """
-        attachment = self.create(path, type, entity)
-        self._db.add(attachment)
+        attachment = self.create(path, type, entity).add()
         return attachment
 
     def find(self, attachment_query=None, entity_query=None):

test/test_attachments.py

@@ -224,7 +224,7 @@ class AttachmentDestinationTest(unittest.TestCase, AttachmentTestHelper):
 
 
 class AttachmentTest(unittest.TestCase, AttachmentTestHelper):
-    """Test `attachment.move()`.
+    """Test `attachment.move()` and `attachment.entity`.
     """
 
     def setUp(self):
@@ -233,6 +233,8 @@ class AttachmentTest(unittest.TestCase, AttachmentTestHelper):
     def tearDown(self):
         self.teardown_beets()
 
+    # attachment.move()
+
     def test_move(self):
         attachment = self.create_album_attachment(self.touch('a'))
         original_path = attachment.path
@@ -295,6 +297,44 @@ class AttachmentTest(unittest.TestCase, AttachmentTestHelper):
             'overwrite attachment destination {0}'.format(dest), logs
         )
 
+    # attachment.entity()
+
+    def test_set_entity(self):
+        album = self.add_album()
+        attachment = self.factory.create('/path/to/attachment', 'coverart')
+        attachment.entity = album
+        self.assertEqual(attachment.entity, album)
+
+    def test_set_entity_and_add(self):
+        album = self.add_album()
+        attachment = self.factory.create('/path/to/attachment', 'coverart')
+        attachment.entity = album
+        attachment.add()
+
+        attachment = self.factory.find().get()
+        self.assertEqual(attachment.entity.id, album.id)
+
+    def test_set_entity_and_store(self):
+        album1 = self.add_album()
+        self.factory.add('/path/to/attachment', 'coverart', album1)
+
+        attachment = self.factory.find().get()
+        album2 = self.add_album()
+        attachment.entity = album2
+        attachment.store()
+        self.assertNotEqual(attachment.entity.id, album1.id)
+        self.assertEqual(attachment.entity.id, album2.id)
+
+    def test_set_entity_and_add_entity(self):
+        album = Album(db=self.lib)
+        attachment = self.factory.create('/path/to/attachment', 'coverart')
+        attachment.entity = album
+        album.add()
+        attachment.add()
+
+        self.assertEqual(attachment.ref, album.id)
+        self.assertEqual(attachment.entity.id, album.id)
+
 
 class AttachmentFactoryTest(unittest.TestCase, AttachmentTestHelper):
     """Tests the following methods of `AttachmentFactory`
@@ -337,6 +377,14 @@ class AttachmentFactoryTest(unittest.TestCase, AttachmentTestHelper):
         attachment = self.factory.create('/path/to/attachment', 'noart')
         self.assertNotIn('mime', attachment)
 
+    def test_create_retrieves_existing(self):
+        item = self.add_item('track')
+        attachment1 = self.factory.create('/path/to/a', 'coverart', item)
+        attachment1.add()
+
+        attachment2 = self.factory.create('/path/to/a', 'coverart', item)
+        self.assertEqual(attachment1.id, attachment2.id)
+
     # factory.detect()
 
     def test_detect_plugin_types(self):
@@ -348,6 +396,7 @@ class AttachmentFactoryTest(unittest.TestCase, AttachmentTestHelper):
         self.assertEqual(len(attachments), 1)
         self.assertEqual(attachments[0].type, 'image')
 
+    # TODO Glob and RegExp
     def test_detect_config_types(self):
         self.config['attachments']['types'] = {
             '.*\.jpg': 'image'
@@ -600,6 +649,14 @@ class AttachCommandTest(unittest.TestCase, AttachmentTestHelper):
         attachment = album.attachments().get()
         self.assertEqual(attachment.path, attachment_path)
 
+    def test_do_not_attach_existing(self):
+        album = self.add_album('albumtitle')
+        attachment_path = self.touch('attachment.log')
+
+        self.runcli('attach', '--no-move', attachment_path, 'albumtitle')
+        self.runcli('attach', '--no-move', attachment_path, 'albumtitle')
+        self.assertEqual(len(list(album.attachments())), 1)
+
     def test_unknown_type_warning(self):
         album = self.add_album('albumtitle')
         attachment_path = self.touch('unkown')