From 72515448ad505ef0b73c0d0aa4aa424419b55755 Mon Sep 17 00:00:00 2001 From: FichteFoll Date: Wed, 5 Jun 2019 02:38:46 +0200 Subject: [PATCH 1/8] Add fallback for item access to album's attributes Allows queries (especially for pathspecs) based on an album's flexattrs while operating on items. Fixes #2797. --- beets/dbcore/db.py | 29 +++++++++++++++-------------- beets/library.py | 37 ++++++++++++++++++++++++++++++++++++- test/test_ipfs.py | 5 +++-- test/test_library.py | 33 +++++++++++++++++++++++++++++++++ test/test_query.py | 13 ++++++++++++- 5 files changed, 99 insertions(+), 18 deletions(-) diff --git a/beets/dbcore/db.py b/beets/dbcore/db.py index 3195b52c9..5aa418632 100755 --- a/beets/dbcore/db.py +++ b/beets/dbcore/db.py @@ -345,9 +345,9 @@ class Model(object): """ return cls._fields.get(key) or cls._types.get(key) or types.DEFAULT - def __getitem__(self, key): - """Get the value for a field. Raise a KeyError if the field is - not available. + def _get(self, key, default=None, raise_=False): + """Get the value for a field, or `default`. Alternatively, + raise a KeyError if the field is not available. """ getters = self._getters() if key in getters: # Computed. @@ -359,8 +359,18 @@ class Model(object): return self._type(key).null elif key in self._values_flex: # Flexible. return self._values_flex[key] - else: + elif raise_: raise KeyError(key) + else: + return default + + get = _get + + def __getitem__(self, key): + """Get the value for a field. Raise a KeyError if the field is + not available. + """ + return self._get(key, raise_=True) def _setitem(self, key, value): """Assign the value for a field, return whether new and old value @@ -435,19 +445,10 @@ class Model(object): for key in self: yield key, self[key] - def get(self, key, default=None): - """Get the value for a given key or `default` if it does not - exist. - """ - if key in self: - return self[key] - else: - return default - def __contains__(self, key): """Determine whether `key` is an attribute on this object. """ - return key in self.keys(True) + return key in self.keys(computed=True) def __iter__(self): """Iterate over the available field names (excluding computed diff --git a/beets/library.py b/beets/library.py index bb49d0e99..5740f20ee 100644 --- a/beets/library.py +++ b/beets/library.py @@ -386,7 +386,7 @@ class FormattedItemMapping(dbcore.db.FormattedMapping): def album_keys(self): album_keys = [] if self.album: - for key in self.album.keys(True): + for key in self.album.keys(computed=True): if key in Album.item_keys \ or key not in self.item._fields.keys(): album_keys.append(key) @@ -571,6 +571,41 @@ class Item(LibModel): if changed and key in MediaFile.fields(): self.mtime = 0 # Reset mtime on dirty. + def __getitem__(self, key): + """Get the value for a field, falling back to the album if + necessary. Raise a KeyError if the field is not available. + """ + try: + return super(Item, self).__getitem__(key) + except KeyError: + album = self.get_album() + if album: + return album[key] + raise + + def keys(self, computed=False, with_album=True): + """Get a list of available field names. `with_album` + controls whether the album's fields are included. + """ + keys = super(Item, self).keys(computed=computed) + if with_album: + album = self.get_album() + if album: + keys += album.keys(computed=computed) + return keys + + def get(self, key, default=None, with_album=True): + """Get the value for a given key or `default` if it does not + exist. Set `with_album` to false to skip album fallback. + """ + try: + return self._get(key, default, raise_=with_album) + except KeyError: + album = self.get_album() + if album: + return album.get(key, default) + return default + def update(self, values): """Set all key/value pairs in the mapping. If mtime is specified, it is not reset (as it might otherwise be). diff --git a/test/test_ipfs.py b/test/test_ipfs.py index d670bfc25..2fe89e7e5 100644 --- a/test/test_ipfs.py +++ b/test/test_ipfs.py @@ -49,7 +49,7 @@ class IPFSPluginTest(unittest.TestCase, TestHelper): want_item = test_album.items()[2] for check_item in added_album.items(): try: - if check_item.ipfs: + if check_item.get('ipfs', with_album=False): ipfs_item = os.path.basename(want_item.path).decode( _fsencoding(), ) @@ -57,7 +57,8 @@ class IPFSPluginTest(unittest.TestCase, TestHelper): ipfs_item) want_path = bytestring_path(want_path) self.assertEqual(check_item.path, want_path) - self.assertEqual(check_item.ipfs, want_item.ipfs) + self.assertEqual(check_item.get('ipfs', with_album=False), + want_item.ipfs) self.assertEqual(check_item.title, want_item.title) found = True except AttributeError: diff --git a/test/test_library.py b/test/test_library.py index 4e3be878c..51171b1f8 100644 --- a/test/test_library.py +++ b/test/test_library.py @@ -132,6 +132,21 @@ class GetSetTest(_common.TestCase): def test_invalid_field_raises_attributeerror(self): self.assertRaises(AttributeError, getattr, self.i, u'xyzzy') + def test_album_fallback(self): + # integration test of item-album fallback + lib = beets.library.Library(':memory:') + i = item(lib) + album = lib.add_album([i]) + album['flex'] = u'foo' + album.store() + + self.assertTrue('flex' in i) + self.assertFalse('flex' in i.keys(with_album=False)) + self.assertEqual(i['flex'], u'foo') + self.assertEqual(i.get('flex'), u'foo') + self.assertEqual(i.get('flex', with_album=False), None) + self.assertEqual(i.get('flexx'), None) + class DestinationTest(_common.TestCase): def setUp(self): @@ -491,6 +506,24 @@ class DestinationTest(_common.TestCase): dest = self.i.destination() self.assertEqual(dest[-2:], b'XX') + def test_album_field_query(self): + self.lib.directory = b'one' + self.lib.path_formats = [(u'default', u'two'), + (u'flex:foo', u'three')] + album = self.lib.add_album([self.i]) + self.assertEqual(self.i.destination(), np('one/two')) + album['flex'] = u'foo' + album.store() + self.assertEqual(self.i.destination(), np('one/three')) + + def test_album_field_in_template(self): + self.lib.directory = b'one' + self.lib.path_formats = [(u'default', u'$flex/two')] + album = self.lib.add_album([self.i]) + album['flex'] = u'foo' + album.store() + self.assertEqual(self.i.destination(), np('one/foo/two')) + class ItemFormattedMappingTest(_common.LibTestCase): def test_formatted_item_value(self): diff --git a/test/test_query.py b/test/test_query.py index c0ab2a171..56134da7b 100644 --- a/test/test_query.py +++ b/test/test_query.py @@ -109,7 +109,7 @@ class DummyDataTestCase(_common.TestCase, AssertsMixin): items[2].comp = False for item in items: self.lib.add(item) - self.lib.add_album(items[:2]) + self.album = self.lib.add_album(items[:2]) def assert_items_matched_all(self, results): self.assert_items_matched(results, [ @@ -300,6 +300,17 @@ class GetTest(DummyDataTestCase): results = self.lib.items(q) self.assertFalse(results) + def test_album_field_fallback(self): + self.album['albumflex'] = u'foo' + self.album.store() + + q = u'albumflex:foo' + results = self.lib.items(q) + self.assert_items_matched(results, [ + u'foo bar', + u'baz qux', + ]) + def test_invalid_query(self): with self.assertRaises(InvalidQueryArgumentValueError) as raised: dbcore.query.NumericQuery('year', u'199a') From a112358f766260298e4b9d8be9661e85df3a7576 Mon Sep 17 00:00:00 2001 From: FichteFoll Date: Sun, 22 Jul 2018 20:26:23 +0200 Subject: [PATCH 2/8] Override FormattedItemMapping.model_keys model_keys was inferred from `self.keys(True)`, which would include the fallback album keys. Since FormattedItemMapping has its own algorithm for album attributes, we only care about the item's *actual* keys. --- beets/library.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/beets/library.py b/beets/library.py index 5740f20ee..ba822c513 100644 --- a/beets/library.py +++ b/beets/library.py @@ -376,6 +376,9 @@ class FormattedItemMapping(dbcore.db.FormattedMapping): def __init__(self, item, for_path=False): super(FormattedItemMapping, self).__init__(item, for_path) + # We treat album and item keys specially here, + # so exclude transitive album keys from the model's keys. + self.model_keys = item.keys(computed=True, with_album=False) self.item = item @lazy_property From e17c478f74f3331a465af851d5f995c74db62ca6 Mon Sep 17 00:00:00 2001 From: FichteFoll Date: Wed, 5 Jun 2019 02:39:22 +0200 Subject: [PATCH 3/8] Cache an item's album with a property Use Album.load() whenever the album is requested, which causes it to be reloaded from the database. Drawback: This adds a slowdown of 100% (6.2s to 12.6s) to `beet list` on my setup. --- beets/importer.py | 2 +- beets/library.py | 45 ++++++++++++++++++++++++++++++++------------ beetsplug/convert.py | 2 +- 3 files changed, 35 insertions(+), 14 deletions(-) diff --git a/beets/importer.py b/beets/importer.py index d2943b511..4c4316a8e 100644 --- a/beets/importer.py +++ b/beets/importer.py @@ -772,7 +772,7 @@ class ImportTask(BaseImportTask): if (not dup_item.album_id or dup_item.album_id in replaced_album_ids): continue - replaced_album = dup_item.get_album() + replaced_album = dup_item._cached_album if replaced_album: replaced_album_ids.add(dup_item.album_id) self.replaced_albums[replaced_album.path] = replaced_album diff --git a/beets/library.py b/beets/library.py index ba822c513..87bbcd134 100644 --- a/beets/library.py +++ b/beets/library.py @@ -395,9 +395,9 @@ class FormattedItemMapping(dbcore.db.FormattedMapping): album_keys.append(key) return album_keys - @lazy_property + @property def album(self): - return self.item.get_album() + return self.item._cached_album def _get(self, key): """Get the value for a key, either from the album or the item. @@ -542,6 +542,29 @@ class Item(LibModel): _format_config_key = 'format_item' + __album = None + """Cached album object. Read-only.""" + + @property + def _cached_album(self): + """The Album object that this item belongs to, if any, or + None if the item is a singleton or is not associated with a + library. + The instance is cached and refreshed on access. + + DO NOT MODIFY! + If you want a copy to modify, use :meth:`get_album`. + """ + if not self.__album and self._db: + self.__album = self._db.get_album(self) + elif self.__album: + self.__album.load() + return self.__album + + @_cached_album.setter + def _cached_album(self, album): + self.__album = album + @classmethod def _getters(cls): getters = plugins.item_field_getters() @@ -568,6 +591,8 @@ class Item(LibModel): value = bytestring_path(value) elif isinstance(value, BLOB_TYPE): value = bytes(value) + elif key == 'album_id': + self._cached_album = None changed = super(Item, self)._setitem(key, value) @@ -581,9 +606,8 @@ class Item(LibModel): try: return super(Item, self).__getitem__(key) except KeyError: - album = self.get_album() - if album: - return album[key] + if self._cached_album: + return self._cached_album[key] raise def keys(self, computed=False, with_album=True): @@ -591,10 +615,8 @@ class Item(LibModel): controls whether the album's fields are included. """ keys = super(Item, self).keys(computed=computed) - if with_album: - album = self.get_album() - if album: - keys += album.keys(computed=computed) + if with_album and self._cached_album: + keys += self._cached_album.keys(computed=computed) return keys def get(self, key, default=None, with_album=True): @@ -604,9 +626,8 @@ class Item(LibModel): try: return self._get(key, default, raise_=with_album) except KeyError: - album = self.get_album() - if album: - return album.get(key, default) + if self._cached_album: + return self._cached_album.get(key, default) return default def update(self, values): diff --git a/beetsplug/convert.py b/beetsplug/convert.py index 6ed139da0..c873ec7c1 100644 --- a/beetsplug/convert.py +++ b/beetsplug/convert.py @@ -332,7 +332,7 @@ class ConvertPlugin(BeetsPlugin): item.store() # Store new path and audio data. if self.config['embed']: - album = item.get_album() + album = item._cached_album if album and album.artpath: self._log.debug(u'embedding album art from {}', util.displayable_path(album.artpath)) From dfc23e8efe23d4e9b4092c0cdad0c6eba639bf0f Mon Sep 17 00:00:00 2001 From: FichteFoll Date: Fri, 14 Sep 2018 02:43:38 +0200 Subject: [PATCH 4/8] Implement database and model revision checking Prevents reloading a model from the database when it hasn't changed. Now we're back to almost the same speed as before the addition of album field fallbacks. --- beets/dbcore/db.py | 35 ++++++++++++++++++++++++++++++-- test/test_dbcore.py | 49 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 82 insertions(+), 2 deletions(-) diff --git a/beets/dbcore/db.py b/beets/dbcore/db.py index 5aa418632..3786bbbd2 100755 --- a/beets/dbcore/db.py +++ b/beets/dbcore/db.py @@ -251,6 +251,11 @@ class Model(object): value is the same as the old value (e.g., `o.f = o.f`). """ + _revision = -1 + """A revision number from when the model was loaded from or written + to the database. + """ + @classmethod def _getters(cls): """Return a mapping from field names to getter functions. @@ -303,9 +308,11 @@ class Model(object): def clear_dirty(self): """Mark all fields as *clean* (i.e., not needing to be stored to - the database). + the database). Also update the revision. """ self._dirty = set() + if self._db: + self._revision = self._db.revision def _check_db(self, need_id=True): """Ensure that this object is associated with a database row: it @@ -533,8 +540,14 @@ class Model(object): def load(self): """Refresh the object's metadata from the library database. + + If check_revision is true, the database is only queried loaded when a + transaction has been committed since the item was last loaded. """ self._check_db() + if not self._dirty and self._db.revision == self._revision: + # Exit early + return stored_obj = self._db._get(type(self), self.id) assert stored_obj is not None, u"object {0} not in DB".format(self.id) self._values_fixed = LazyConvertDict(self) @@ -789,6 +802,12 @@ class Transaction(object): """A context manager for safe, concurrent access to the database. All SQL commands should be executed through a transaction. """ + + _mutated = False + """A flag storing whether a mutation has been executed in the + current transaction. + """ + def __init__(self, db): self.db = db @@ -810,12 +829,15 @@ class Transaction(object): entered but not yet exited transaction. If it is the last active transaction, the database updates are committed. """ + # Beware of races; currently secured by db._db_lock + self.db.revision += self._mutated with self.db._tx_stack() as stack: assert stack.pop() is self empty = not stack if empty: # Ending a "root" transaction. End the SQLite transaction. self.db._connection().commit() + self._mutated = False self.db._db_lock.release() def query(self, statement, subvals=()): @@ -831,7 +853,6 @@ class Transaction(object): """ try: cursor = self.db._connection().execute(statement, subvals) - return cursor.lastrowid except sqlite3.OperationalError as e: # In two specific cases, SQLite reports an error while accessing # the underlying database file. We surface these exceptions as @@ -841,9 +862,14 @@ class Transaction(object): raise DBAccessError(e.args[0]) else: raise + else: + self._mutated = True + return cursor.lastrowid def script(self, statements): """Execute a string containing multiple SQL statements.""" + # We don't know whether this mutates, but quite likely it does. + self._mutated = True self.db._connection().executescript(statements) @@ -859,6 +885,11 @@ class Database(object): supports_extensions = hasattr(sqlite3.Connection, 'enable_load_extension') """Whether or not the current version of SQLite supports extensions""" + revision = 0 + """The current revision of the database. To be increased whenever + data is written in a transaction. + """ + def __init__(self, path, timeout=5.0): self.path = path self.timeout = timeout diff --git a/test/test_dbcore.py b/test/test_dbcore.py index 9bf78de67..c3a8a60ee 100644 --- a/test/test_dbcore.py +++ b/test/test_dbcore.py @@ -224,6 +224,31 @@ class MigrationTest(unittest.TestCase): self.fail("select failed") +class TransactionTest(unittest.TestCase): + def setUp(self): + self.db = DatabaseFixture1(':memory:') + + def tearDown(self): + self.db._connection().close() + + def test_mutate_increase_revision(self): + old_rev = self.db.revision + with self.db.transaction() as tx: + tx.mutate( + 'INSERT INTO {0} ' + '(field_one) ' + 'VALUES (?);'.format(ModelFixture1._table), + (111,), + ) + self.assertGreater(self.db.revision, old_rev) + + def test_query_no_increase_revision(self): + old_rev = self.db.revision + with self.db.transaction() as tx: + tx.query('PRAGMA table_info(%s)' % ModelFixture1._table) + self.assertEqual(self.db.revision, old_rev) + + class ModelTest(unittest.TestCase): def setUp(self): self.db = DatabaseFixture1(':memory:') @@ -245,6 +270,30 @@ class ModelTest(unittest.TestCase): row = self.db._connection().execute('select * from test').fetchone() self.assertEqual(row['field_one'], 123) + def test_revision(self): + old_rev = self.db.revision + model = ModelFixture1() + model.add(self.db) + model.store() + self.assertEqual(model._revision, self.db.revision) + self.assertGreater(self.db.revision, old_rev) + + mid_rev = self.db.revision + model2 = ModelFixture1() + model2.add(self.db) + model2.store() + self.assertGreater(model2._revision, mid_rev) + self.assertGreater(self.db.revision, model._revision) + + # revision changed, so the model should be re-loaded + model.load() + self.assertEqual(model._revision, self.db.revision) + + # revision did not change, so no reload + mod2_old_rev = model2._revision + model2.load() + self.assertEqual(model2._revision, mod2_old_rev) + def test_retrieve_by_id(self): model = ModelFixture1() model.add(self.db) From 26b1aa990e9b9dc5810097437eece16b81fccb0e Mon Sep 17 00:00:00 2001 From: FichteFoll Date: Fri, 14 Sep 2018 02:52:53 +0200 Subject: [PATCH 5/8] Add changelog for #2797 --- docs/changelog.rst | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/docs/changelog.rst b/docs/changelog.rst index e58325482..862074124 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -17,6 +17,11 @@ New features: to MPD commands we don't support yet. Let us know if you find an MPD client that doesn't get along with BPD! :bug:`3214` :bug:`800` +* Fields in queries now fall back to an item's album and check its fields too. + Notably, this allows querying items by an album flex attribute (also in path + configuration). Plugins: Also applies to normal item access. + Thanks to :user:`FichteFoll`. + :bug:`2797` :bug:`2988` Fixes: From ed76da57e53dcc8be96eac8cd5e11f1103f48a9a Mon Sep 17 00:00:00 2001 From: FichteFoll Date: Mon, 17 Sep 2018 23:51:38 +0200 Subject: [PATCH 6/8] Add changelog section for developers --- docs/changelog.rst | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index 862074124..dd1e265c3 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -18,8 +18,8 @@ New features: that doesn't get along with BPD! :bug:`3214` :bug:`800` * Fields in queries now fall back to an item's album and check its fields too. - Notably, this allows querying items by an album flex attribute (also in path - configuration). Plugins: Also applies to normal item access. + Notably, this allows querying items by an album flex attribute, also in path + configuration. Thanks to :user:`FichteFoll`. :bug:`2797` :bug:`2988` @@ -49,6 +49,12 @@ For plugin developers: is almost identical apart from the name change. Again, we'll re-export at the old location (with a deprecation warning) for backwards compatibility, but might stop doing this in a future release. +* Item (and attribute) access on an item now falls back to the album's + attributes as well. If you specifically want to access an item's attributes, + use ``Item.get(key, with_album=False)``. :bug:`2988` +* ``Item.keys`` also has a ``with_album`` argument now, defaulting to ``True``. +* A ``revision`` attribute has been added to ``Database``. It is increased on + every transaction that mutates it. :bug:`2988` For packagers: From 9c35da69bab1c18f3828aa562e97e2a299272629 Mon Sep 17 00:00:00 2001 From: FichteFoll Date: Wed, 5 Jun 2019 02:13:25 +0200 Subject: [PATCH 7/8] Update the items' type information from plugins Plugins can provide item and album attributes. We need to carry over the type information of album attributes so that our item-to-album fallback has these and allows for e.g. range queries. Courtesy of @arcresu (via https://github.com/beetbox/beets/pull/2988#issuecomment-492444925). --- beets/ui/__init__.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/beets/ui/__init__.py b/beets/ui/__init__.py index a88ed9aef..d798f5134 100644 --- a/beets/ui/__init__.py +++ b/beets/ui/__init__.py @@ -1146,8 +1146,13 @@ def _setup(options, lib=None): plugins.send("library_opened", lib=lib) # Add types and queries defined by plugins. - library.Item._types.update(plugins.types(library.Item)) - library.Album._types.update(plugins.types(library.Album)) + plugin_types_album = plugins.types(library.Album) + library.Album._types.update(plugin_types_album) + item_types = plugin_types_album.copy() + item_types.update(library.Item._types) + item_types.update(plugins.types(library.Item)) + library.Item._types = item_types + library.Item._queries.update(plugins.named_queries(library.Item)) library.Album._queries.update(plugins.named_queries(library.Album)) From 2d024d2f38fcc8ac7a134ec290cb6a6b9f3b5ca4 Mon Sep 17 00:00:00 2001 From: FichteFoll Date: Mon, 2 Nov 2020 01:06:05 +0100 Subject: [PATCH 8/8] Avoid overeager inclusion of album attributes Co-Authored-By: Curtis Rueden --- beets/dbcore/db.py | 5 +++-- beets/library.py | 3 ++- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/beets/dbcore/db.py b/beets/dbcore/db.py index 524ee0b00..409ecc9af 100755 --- a/beets/dbcore/db.py +++ b/beets/dbcore/db.py @@ -56,10 +56,11 @@ class FormattedMapping(Mapping): are replaced. """ - def __init__(self, model, for_path=False): + def __init__(self, model, for_path=False, compute_keys=True): self.for_path = for_path self.model = model - self.model_keys = model.keys(True) + if compute_keys: + self.model_keys = model.keys(True) def __getitem__(self, key): if key in self.model_keys: diff --git a/beets/library.py b/beets/library.py index 5e6a0ec8a..f9b5a2ab4 100644 --- a/beets/library.py +++ b/beets/library.py @@ -375,9 +375,10 @@ class FormattedItemMapping(dbcore.db.FormattedMapping): """ def __init__(self, item, for_path=False): - super(FormattedItemMapping, self).__init__(item, for_path) # We treat album and item keys specially here, # so exclude transitive album keys from the model's keys. + super(FormattedItemMapping, self).__init__(item, for_path, + compute_keys=False) self.model_keys = item.keys(computed=True, with_album=False) self.item = item