Path config query does not work with album-level flexible attributes

[lycofron]

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 -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

[sampsyo]

Perfect description. Thank you for filing this detailed bug!

There are two fixes we can consider:

• Make the --set flag set item-level fields even in album-level imports (perhaps in addition to the album-level equivalent).
• Make path queries match album-level fields.

The second is probably the better solution, but it's also trickier. We probably don't want to make all item-level queries also match album-level fields, because that would be fairly slow, so we may need special logic just for path queries.

[lycofron]

May I suggest a third option? What if there was a qualifier to point specifically to album-level fields, e.g. albfld so, this way, let's say, year will refer to item-level year field, whild albfld.year or albfld_year will refer to album-level year only. (There can be a respective itmfld qualifier in the future.)

I don't know if it's possible, just something I thought of.

[sampsyo]

Hmm; good point. An explicit way for queries on items to access album-level data could be useful. For most built-in fields, the data is mirrored between albums and their constituent items, but it's also possible for these to get "out of sync," as they have in this case, so offering this kind of lookup could be useful in general.

[ayancey]

I think the current system isn't too bad, but I would really like an option to add item flexible fields when importing an album. The documentation didn't do a very good job explaining the difference between item and album flex fields.

[sampsyo]

If you have any suggestions, please consider contributing some expansions to the docs! Pull requests for the documentation are always greatly appreciated.

[ToxicFrog]

Another manifestation of this bit me; beet import --set grouping=... ends up creating and setting a flexible album attribute that is not respected in paths, rather than setting the contentgroup tag on the files being imported.

I think @lycofron 's suggestion is a good one, but also that doing that, and setting album-level fields, are solving slightly different problems.

Specifically, I can think of valid use cases for all of these:

• album and track fields are allowed to diverge; path/query expressions have a way of explicitly asking for "the track version" and "the album version"
• album and track fields are allowed to diverge; the album field is taken as the default in cases where the track field is not set
• album and track fields are not allowed to diverge; setting the album field sets it for all the tracks in that album.

I think any of those would solve my problem, but I'm not sure which is most generally useful.

[FichteFoll]

So, I looked into this a little and tried adding a fallback to Item.__getitem__ which would also check the album's flexattrs in case the key couldn't be found, but that hasn't been successful so far. Tracking down the code for query generation, I determined this to be the most likely place to add it (via FieldQuery.match).

However, what I'm confused about now is how test_library.ItemFormattedMappingTest.test_album_flex_field doesn't fail. It sets a flexattr on the album but that is then reflected in the item's formatted mapping, which doesn't do any lookup on the album itself. Could anyone explain this to me?

beets/test/test_library.py

Lines 520 to 524 in 9577a51

	def test_album_flex_field(self):
	album = self.lib.add_album([self.i])
	album['flex'] = u'foo'
	album.store()
	self.assertEqual(u'foo', self.i.formatted().get('flex'))

Edit: Okay, nvm. I missed that the item's _formatter is FormattedItemMapping.

[FichteFoll]

All right, I added __getitem__ and __contains__ (needed for Model.get, though I should probably override keys instead) to fall back to an item's album's flexattrs if the key can't be found for the item itself.

However, this breaks the ipfs tests because they set flexattrs on both the item and the album and then proceed under the assumption that the ipfs attr will only be available for the item it was set for rather than all items on the album (via the new fallback).

Thus, we end up with the problem that in some instances you'd want to have the fallback, while in others you specifically do not want that. That in return leaves the questions-

1. Which should be the default and which should get an additional method added?
2. Where would we need to use the explicit non-default behavior (depending on the answer to 1.)?

Since I only digged into the code a bit for today, I certainly do not feel in the position to decide this. However, I'll note that the change I did only resulten in a single test failure (the one for ipfs). I'd be happy to implement the decision of this in a PR. Since I don't rely on ipfs, I can already use the patched version like this, though.

Here's a quick patch if you want to check it locally.

diff --git a/beets/library.py b/beets/library.py
index 919d4cf7..805c2e6f 100644
--- a/beets/library.py
+++ b/beets/library.py
@@ -554,6 +554,27 @@ class Item(LibModel):
         if changed and key in MediaFile.fields():
             self.mtime = 0  # Reset mtime on dirty.
 
+    def __getitem__(self, key):
+        """Add a fallback to the item's album's fields.
+        Needed for album flexattrs.
+        """
+        try:
+            return super(Item, self).__getitem__(key)
+        except KeyError:
+            # TODO this could be optimized
+            album = self.get_album()
+            if not album:
+                raise
+            # return self.get_album()[key]
+            return album._values_flex[key]
+
+    def __contains__(self, key):
+        if super(Item, self).__contains__(key):
+            return True
+        # TODO this could be optimized
+        album = self.get_album()
+        return album and key in album._values_flex
+
     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_library.py b/test/test_library.py
index ea566965..ebf52f9c 100644
--- a/test/test_library.py
+++ b/test/test_library.py
@@ -133,6 +133,15 @@ class GetSetTest(_common.TestCase):
     def test_invalid_field_raises_attributeerror(self):
         self.assertRaises(AttributeError, getattr, self.i, u'xyzzy')
 
+    def test_album_field_fallback(self):
+        lib = beets.library.Library(':memory:')
+        i = item(lib)
+        album = lib.add_album([i])
+        album['flex'] = u'foo'
+        album.store()
+        self.assertEqual(i['flex'], u'foo')
+        self.assertEqual(i.get('flex'), u'foo')
+
 
 class DestinationTest(_common.TestCase):
     def setUp(self):
@@ -492,6 +501,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):

(I should probably move test_album_field_query to test_query.py, though.)

[sampsyo]

Wow; that's awesome! Thanks for digging in and working that out so quickly.

I agree, first of all, that those are the operative questions. It does seem important to be able to explicitly opt into the opposite behavior. My first hunch, however, says that the "cascade" from album to item should be the default—since that's the way that album-level attributes normally "feel" during formatting and other operations. It might be a good idea for the ipfs plugin to move to two separate field names, for example—an album-level one and an item-level one.

This would mean that querying the item-level attributes alone would be an "advanced" feature, only to be needed in special circumstances—ordinarily, when something has gone wrong and items are somehow out of sync with albums. We could invent some funky query syntax to specify when to use that advanced behavior.

How does that sound to everybody?

[lycofron]

To me, from an end user point of view, it sounds excellent.

[FichteFoll]

Found some time to work on this again. I'm satisfied with the API in general, but I have a pretty big architecture problem that I don't feel comfortable deciding on my own.

Basically, I started by adding a fallback mapping attribute to the dbcore.db.Model class that I would assign the item's album whenever its album_id item is set. The idea is to do some basic caching through this as I don't need to re-query the database for the associated album for each key lookup (as would be the case otherwise). However, through this precise practice I end up with different versions of the same album, each in a separate object. One is the object I create with Library.add_album and the other is the one that gets queried during the assignment of album_id.

Now, when I set a flexattr (or any other attr) on the original album and then call store on it, the database gets updated but the previously queried and still cached version does not! This is critical.

Options to resolve this are to:

1. Create a new and up to date album object whenever the item's keys are requested and an attribute from the album needs to be fetched. Depending on how frequently that is used, this could be quite inefficient.
2. Cache albums for the same id and update all current instances (one!) at the same time whenever something changes. This would be efficient but risky depending on how albums are used.

Because the second option is quite a significant change in architecture, I'll go ahead with option one for now. Feedback appreciated.