From 54f11953fbfaa033d5fd510cc7a2110bfc17198a Mon Sep 17 00:00:00 2001 From: Thomas Scholtes Date: Thu, 14 Aug 2014 18:49:44 +0200 Subject: [PATCH] Don't duplicate attachments --- beets/attachments.py | 54 +++++++++++++++++++++++++++--------- test/test_attachments.py | 59 +++++++++++++++++++++++++++++++++++++++- 2 files changed, 99 insertions(+), 14 deletions(-) diff --git a/beets/attachments.py b/beets/attachments.py index 04c3d81a3..831aac9ea 100644 --- a/beets/attachments.py +++ b/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): diff --git a/test/test_attachments.py b/test/test_attachments.py index 43630080a..f1724e5bb 100644 --- a/test/test_attachments.py +++ b/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')