Repository: beetbox/beets

Issue:
Path config query does not work with album-level flexible attributes
### Problem

Path queries do not work with attributes that are set at album level. More specifically, I import an album setting at the same time a flexible attribute. I have various paths that are supposed to be modified by this attribute. Though the attribute is set at import time, the query at the path will not match, neither at import time, nor at a later time.

My configuration:
```

item_fields:
    initial: (albumartist_sort or artist_sort or albumartist or artist or '_')[0].upper()
    disc_count:  u'%02i.' % (disc)
    disc_and_track: u'%02i.%02i' % (disc, track) if
                    disctotal > 1 else u'%02i' % (track)
    definite_year: u'%04i' % original_year or original_date[:3] or year or date[:3] or 0

album_fields:
    cond_release_category: |
            if albumstatus.lower()=='official':
                    if albumtype.lower() in ['album', 'compilation', 'ep']:
                            return None
                    elif supergenre=='Soundtrack' and albumtype.lower() in ['soundtrack']:
                            return None
                    else:
                            return albumtype.capitalize()
            else:
                    return albumstatus.capitalize()
    boxset_folder: |
            if boxset==1:
                    boxdisc_folder=u'%i. %s' % disc, disctitle
                    if boxdisc_year: boxdisc_folder=u'%s %s' % boxdisc_folder, boxdisc_year
                    return boxdisc_folder
            else:
                    return None

paths:
    default: $supergenre/%asciify{$initial}/$albumartist/%if{$cond_release_category,$cond_release_category/}[$definite_year] $album%aunique{albumartist album,albumdisambig label catalognum year}/$disc_and_track - $title
    boxset:1: $supergenre/%asciify{$initial}/$albumartist/%if{$cond_release_category,$cond_release_category/}[$definite_year] $album%aunique{albumartist album,albumdisambig label catalognum year}/$disc_count $disctitle%ifdef{boxdisc_year, ($boxdisc_year)}/$track - $title
    singleton: Non-Album/$artist - $title
    comp: $supergenre/Compilations/$album%aunique{}/$disc_and_track - $title
    supergenre:Soundtrack: Soundtrack/$album/%if{$disctotal>1,$disc.}$track $title
```

And this is the result:

```user@beethost:~$ beet ls supergenre:Soundtrack
user@beethost:~$ beet ls -a supergenre:Soundtrack
...
Michael Nyman - The Piano
...
user@beethost:~$ ls -l /music/Soundtrack/N/Michael\ Nyman/
total 0
drwxrwxr-x 2 user user 0 Jan 26 08:50 [1993] The Piano```

Note that the path follows the default template and that the album attribute expands correctly in the template.

### Setup
beets version 1.4.6
Python version 2.7.13

Relevant thread at discussion board:
https://discourse.beets.io/t/path-config-and-album-level-flexible-ttributes-at-import-time/278/3



Discussion:
FichteFoll: 1. Not sure what you mean by this. Formatting works exactly like it did before. However, I missed that `self.model_keys` is fetched from `model.keys(True)`, which includes the album fallback. I'll override `self.model_keys` in `FormattedItemMapping`'s `__init__`.

2. I see. So, I could keep an Item-internal reference to an album and just run `load` on that before trying to access album attributes (or `keys()`). Is `load` smart enough not to update itself when there haven't been any changes to the database since the last fetch? If it doesn't, then this change would be more involved and requires some care to actually make the caching useful. (I was thinking about storing the same album reference in multiple items for example, but I'd need to call `load` for each item individually anyway because we can't be sure the database hasn't been updated in the meantime.) Otherwise, just keeping a lazy-loaded album attribute per item individually would probably already be an improvement.

3. I'm not sure I understand the problem you're describing because, as far as I'm aware, all of this is still possible. Attribute access on the item itself is prioritized over the album fallback, for their standard fields and even for flexattrs. `_setitem` was not modified, so you can also still set an item's field or flexattr to override an album's.
sampsyo: > Not sure what you mean by this. Formatting works exactly like it did before. However, I missed that self.model_keys is fetched from model.keys(True), which includes the album fallback (and results in the item's formatter for a field being requested rather than the album's). I'll override self.model_keys in FormattedItemMapping's __init__.

Thanks for catching that!

What I'm worried about is not a direct conflict or anything—just that we're implementing the same logic ("fallback" between item and album attributes) twice. If evaluating the expression `item.field` already looks up `field` in `item`'s album, then ideally we would not need FormattedItemMapping—the plain old FormattedMapping from dbcore would do the trick.

But as you discovered, there's subtlety about which formatter gets used. Maybe there's an elegant way to provide a merged view without the duplication, but maybe this division of responsibilities is OK.

> I see. So, I could keep an Item-internal reference to an album and just run load on that before trying to access album attributes (or keys()). Is load smart enough not to update itself when there haven't been any changes to the database since the last fetch? If it doesn't, then this change would be more involved and requires some care to actually make the caching useful. (I was thinking about storing the same album reference in multiple items for example, but I'd need to call load for each item individually anyway because we can't be sure the database hasn't been updated in the meantime.) Otherwise, just keeping a lazy-loaded album attribute per item individually would probably already be an improvement.

No, `load` always loads the latest data. (Otherwise, we'd need some mechanism on the side for tracking when the database has changed—which likely would be no faster to check than just loading from the database.)

> I'm not sure I understand the problem you're describing because, as far as I'm aware, all of this is still possible. Attribute access on the item itself is prioritized over the album fallback, for their standard fields and even for flexattrs. _setitem was not modified, so you can also still set an item's field or flexattr to override an album's.

OK, good point! I had missed that existing values on items take precedence. That means, unless I'm mistaken, that item-level fixed attributes *always* take precedence—because it's impossible to remove them. Sounds good!
FichteFoll: ```
~ λ hyperfine "beet list" -m 2
Benchmark #1: beet list
  Time (mean ± σ):      7.207 s ±  0.018 s    [User: 5.438 s, System: 0.759 s]
  Range (min … max):    7.194 s …  7.220 s

~/code/beets ∃ hyperfine "python -m beets list" -m 2
Benchmark #1: python -m beets list
  Time (mean ± σ):     17.757 s ±  0.093 s    [User: 12.581 s, System: 2.135 s]
  Range (min … max):   17.691 s … 17.823 s
```

Well, not looking so bright. It's a >100% slowdown. This'd need some smart caching, probably. I do wonder why the difference is so hight, though. I mean, the ItemFormatter needed to access the item's album before as well. Maybe `keys` is run more often than I expected?

Also, I should probably add some documentation about this change.
sampsyo: Hmm, that is a little worrisome. Let's dig a little deeper and see if we can't mitigate some of the effects. (Thanks for the tip about hyperfine, btw!)
FichteFoll: (I just found out about hyperfine today as I browsed the fd Readme by the same author.)

This is probably the point where I would start to look into profiling as I'm still not too familiar with the code base and believe this would provide a good starting point. Have you ever done this in python and have some recommendations for tools or other tips? (I haven't.) 
sampsyo: I think that `cProfile`, in the standard library, is still probably the best profiler out there. One tip I do have, however, is that [SnakeViz](https://jiffyclub.github.io/snakeviz/) is a really nice browser-based GUI for viewing/navigating profile data.
FichteFoll: Took a look back at this. I used [py-spy](https://github.com/benfred/py-spy) for some quick effort-less profiling and it was quite obvious that the majority of the time is being spent with database access in `get_album` (or rather the `album` property, as I changed it). Just uncommenting the `album.load()` code removes the entire performance impact, but it also means the albums we're trying to print could be outdated.

I considered the simplest solution forward to be what I suggested earlier:

1. Make the item cache its album field and provide access through a property. The album returned by this property is *read only* since it is, well, cached. I decided against preventive measures here and instead made the property "hidden" with an underscore and provided documentation.
2. Only load database model objects when they have changed by tracking a revision number that I added to the database and increase on each mutating transaction. Had to tweak this for a little while until it passed all tests, but I suppose this is fairly safe going forward now. I added a comment clarifying on the possibility of race conditions, but as long as the `_db_lock` is aquired, we are fine.

Let me know what you think.

Also, I wasn't sure if I should add a section regarding the API to the changelog. It would mention the fallback of item access on Item and that re-loading is now lazy, although the latter should be transparent.
FichteFoll: I see. Yes, that would probably be useful for the aunique feature and might even warrant a proper implementation (i.e. a "public API"), but I'd rather not do this in here.

Regarding the changelog, I can do that. I deliberately made `_cached_album` an internal property (with the underscore) because I wasn't confident in exposing it. It is, after all, kind of a workaround, although it's the  best I could think of. But as long as it's private, it can be changed.

By the way, as you can see from the commit that references this PR, I started using this in production and haven't encountered any issues so far. I probably could have done that before as well, as I wasn't too concerned about the performance, but for this PR it was a must.
sampsyo: OK, great. Since it’s a sensitive change, it might be wise to put out a call for testing so folks can try it out with funky configurations. I’ll post something to Discourse.
sampsyo: Post added: https://discourse.beets.io/t/call-for-testers-better-queries-for-album-level-fields-a-performance-improvement/477
sampsyo: (Thanks. I restarted that Travis job and everything’s fine.)
FichteFoll: Any updates on this? Doesn't seem like the discourse thread attracted much attention.
FichteFoll: Rebased to fix the merge conflict on the changelog. AppVeyor has some errors in the setup phase with chocolatey.
FichteFoll: Still nobody using this, it seems. :disappointed:

Let me know when you intend to merge this, so I only need to fix the changelog conflict once (or you do it :man_shrugging: ).
FichteFoll: Someone was asking for this a few days ago on IRC, but I missed them and couldn't point towards this PR.

Anyway, I've been using this branch for half a year now with exactly 0 issues so far. I don't use the entire feature set of beets, but importing and path styles based on album flexattrs, which is my primary use case, are just fine.

I'll try to remember making a new speed comparison since my library grew a bit over time, but I don't expect it to be much different compared to the last time.
kergoth: FYI, I ran into a number of issues with this, mostly relating to types in the fallback, both in path format queries and in `beet ls`. See https://discourse.beets.io/t/ranges-not-working-in-beet-ls-with-album-fields-in-item-track-context/
arcresu: I wasn't aware of this when I threw together the diff on the discourse thread @kergoth mentioned. I'll just reproduce it here:
```diff
diff --git a/beets/library.py b/beets/library.py
index 16db1e97..71b6db22 100644
--- a/beets/library.py
+++ b/beets/library.py
@@ -526,7 +526,17 @@ class Item(LibModel):
 
     @classmethod
     def _getters(cls):
-        getters = plugins.item_field_getters()
+        def atoi(f, ag):
+            def ig(i):
+                a = i.get_album()
+                if a:
+                    return ag(a)
+                else:
+                    return cls._type(f).null
+            return ig
+        getters = {f: atoi(f, g)
+                   for f, g in plugins.album_field_getters().items()}
+        getters.update(plugins.item_field_getters())
         getters['singleton'] = lambda i: i.album_id is None
         getters['filesize'] = Item.try_filesize  # In bytes.
         return getters
diff --git a/beets/ui/__init__.py b/beets/ui/__init__.py
index 327db6b0..c3adc72d 100644
--- a/beets/ui/__init__.py
+++ b/beets/ui/__init__.py
@@ -1145,7 +1145,10 @@ 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))
+    at = plugins.types(library.Album)
+    at.update(library.Item._types)
+    at.update(plugins.types(library.Item))
+    library.Item._types = at
     library.Album._types.update(plugins.types(library.Album))
     library.Item._queries.update(plugins.named_queries(library.Item))
     library.Album._queries.update(plugins.named_queries(library.Album))
```
This wasn't intended to be a final implementation, but my approach was a little bit different in that I thought the album-item relationship was something beets-specific and therefore should be reflected in `library.py` rather than dbcore. I used the existing getter mechanism. The `atoi` function takes an album-level getter and converts it into an item-level one that fetches the item's album and delegates to the original getter. Item-level properties still have precedence, as in this PR.

I did find that it was necessary to also change `Item._types` in order to get queries to work as intended since otherwise the album-level fields don't have type information when accessed on `Item`s.

Note that we recently picked up a helper for memoisation in another PR:
https://github.com/beetbox/beets/blob/909fd1eb272691d5cccaa0426db74871c770a8bb/beets/util/__init__.py#L1037-L1057
FichteFoll: Thanks for the headsup. I suspect that the problem with ranges is related to me not updating the items' type information, as you did in your diff. I was entirely new to the code base before working on this, so I just never considered that to be relevant.

The `lazy_property` is similar so something I drafted earlier in the process but ended up scraping because of what I outlined in an earlier comment (https://github.com/beetbox/beets/pull/2988#issuecomment-406897912). The problem here is that the cached album is a snapshot of the database at whatever time it was first accessed, but the db may change during runtime and the lazy property will have no way to consider that fact.

I'll take a closer look at your getter approach when I find some time to work on this again.

(I'd like to mention that I cannot use beets without this feature anymore, so even if there is a huge update going on, I'll continue using my fork until I updated the PR for the changes.)

Patch:
diff --git a/beets/dbcore/db.py b/beets/dbcore/db.py
index 46b47a2e1..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:
@@ -257,6 +258,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.
@@ -309,9 +315,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
@@ -351,9 +359,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.
@@ -365,8 +373,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
@@ -441,19 +459,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
@@ -538,8 +547,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)
@@ -794,6 +809,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
 
@@ -815,12 +836,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=()):
@@ -836,7 +860,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
@@ -846,9 +869,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)
 
 
@@ -864,6 +892,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/beets/importer.py b/beets/importer.py
index 3220b260f..c5701ff30 100644
--- a/beets/importer.py
+++ b/beets/importer.py
@@ -786,7 +786,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 78552bb61..3ba79d069 100644
--- a/beets/library.py
+++ b/beets/library.py
@@ -375,7 +375,11 @@ 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
 
     @lazy_property
@@ -386,15 +390,15 @@ 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)
         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.
@@ -545,6 +549,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()
@@ -571,12 +598,45 @@ 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)
 
         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:
+            if self._cached_album:
+                return self._cached_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 and self._cached_album:
+            keys += self._cached_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:
+            if self._cached_album:
+                return self._cached_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/beets/ui/__init__.py b/beets/ui/__init__.py
index 28879a731..362e8752a 100644
--- a/beets/ui/__init__.py
+++ b/beets/ui/__init__.py
@@ -1155,8 +1155,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))
 
diff --git a/beetsplug/convert.py b/beetsplug/convert.py
index 275703e97..45a571d38 100644
--- a/beetsplug/convert.py
+++ b/beetsplug/convert.py
@@ -358,7 +358,7 @@ class ConvertPlugin(BeetsPlugin):
                 item.store()  # Store new path and audio data.
 
             if self.config['embed'] and not linked:
-                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))
diff --git a/docs/changelog.rst b/docs/changelog.rst
index b9020621b..df286a10a 100644
--- a/docs/changelog.rst
+++ b/docs/changelog.rst
@@ -178,6 +178,11 @@ New features:
 * :doc:`/plugins/replaygain` now does its analysis in parallel when using
   the ``command`` or ``ffmpeg`` backends.
   :bug:`3478`
+* 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.
+  Thanks to :user:`FichteFoll`.
+  :bug:`2797` :bug:`2988`
 * Removes usage of the bs1770gain replaygain backend.
   Thanks to :user:`SamuelCook`.
 * Added ``trackdisambig`` which stores the recording disambiguation from
@@ -344,6 +349,12 @@ For plugin developers:
   :bug:`3355`
 * The autotag hooks have been modified such that they now take 'bpm',
   'musical_key' and a per-track based 'genre' as attributes.
+* 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:
 


Test patch:
diff --git a/test/test_dbcore.py b/test/test_dbcore.py
index 0d40896da..1dd2284c6 100644
--- a/test/test_dbcore.py
+++ b/test/test_dbcore.py
@@ -225,6 +225,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:')
@@ -246,6 +271,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)
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 f88a12c92..4017ff44b 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')