beetsplug: only one plugin can set template_fields per field

[Maxr1998]

Problem

I ran into this when using the new advancedrewrite plugin on my production system together with the old rewrite plugin.
When using these two (or any two other plugins modifying BeetsPlugin.template_fields) together and specifying rules that both target the same field (e.g., artist), only the rules of one plugin concerning this field will be applied. Even worse, which plugin will be used depends on the plugin loading order (the one loaded last will be used), which seems to be random-ish: in the importer, the rewrite plugin is loaded last, when just invoking beet, the advancedrewrite plugin is loaded last. The rules for that specific field from the previously loaded plugin(s) are silently dropped.

Investigation

I've pinned this bug down to the following line: beets.plugins.item_field_getters. The reason this breaks is that the update function on the dict overwrites previous getters, instead of merging them somehow.
Of course, the problem lies in the fact that rewrites can generally affect each other and do thus need a deterministic way of applying them.

Suggested solution (implemented in #5003)

I suggest addressing this issue the following way:

• Rework the getter functions applied to template_fields or album_template_fields so that they either return a replacement value or None (possibly breaking change for 3rd-party plugins).
• Merge getter functions into a dict with lists of functions in plugins.item_field_getters.
• When evaluating the getters in beets.dbcore.Model._get, the list is iterated and the value of the first value that isn't None is used.
• To make sure the value that's used is actually deterministic, the getters can be sorted by the class names of the plugin providing them (alphabetically). In my opinion, that's not ideal, but it's at least consistent.

Setup

• OS: Ubuntu Server 22.04
• Python version: 3.10
• beets version: 1.6.1-master (2023-11-23)
• Turning off plugins made problem go away: not applicable, bug specifically affects plugin handling

[sampsyo]

Thanks for pointing this out; it is pretty confusing that the conflict is resolved silently and that the "winner" is nondeterministic!

While I appreciate the goals in #5003 to find a resolution based on the getter's return value, I am also a teensy bit concerned that it might be a tad more complex than is necessary for the situation. In particular, it seems a little sad to make every dbcore "getter" require a list of functions instead of a single function. It's not terrible, but it does seem a little sad to add this complexity.

Here's one alternative to consider: what about just making this conflict an error? If two plugins try to provide a getter for the same field, we crash with an error message. This is pretty similar to how we handle other plugin conflicts, such as providing two different types for the same field:

beets/beets/plugins.py

Lines 345 to 350 in 2032729

	if field in types and plugin_types[field] != types[field]:
	raise PluginConflictException(
	"Plugin {} defines flexible field {} "
	"which has already been defined with "
	"another type.".format(plugin.name, field)
	)

…and it may actually yield more predictable, less opaque results for the user.

[Maxr1998]

While I appreciate the goals in #5003 to find a resolution based on the getter's return value, I am also a teensy bit concerned that it might be a tad more complex than is necessary for the situation. In particular, it seems a little sad to make every dbcore "getter" require a list of functions instead of a single function. It's not terrible, but it does seem a little sad to add this complexity.

It's definitely not ideal, but in my perception it is already complex with _getters returning a dict of functions. Changing that into a dict of list of functions doesn't increase complexity that much anymore. The name being getters, a plural already implies it's more than one result, so that's fine as well. I think the biggest problem for me personally is really the lack of compile-time checked typing, but it's Python after all.. otherwise that API would be defined once and be easy to enforce after that. Or is your focus more on the performance? Or if it's on the consuming site of the API (not having to define every getter as list), we could also add the same isinstance(list) check in dbcore to support both, if you prefer that.

Here's one alternative to consider: what about just making this conflict an error? If two plugins try to provide a getter for the same field, we crash with an error message. This is pretty similar to how we handle other plugin conflicts, such as providing two different types for the same field:

Honestly, not a fan of that approach. Plugins should preferably not conflict with each other and be able to provide an API mostly independently from other plugins. For the rewrite vs. advancedrewrite case, this is especially important. Only being able to use one of the plugins per field is a severe limitation, since the two plugins have a different syntax and setup complexity. I enjoy using rewrite for the simpler tasks, but still need advancedrewrite for more complex modifications.

For the types case, there's of course no real alternative to throwing an error, since those types are actually defined on the field itself. They don't apply to a subset of items per field like rewrite does.

[sampsyo]

Would you mind discussing a little more about what you use rewrite for when advancedrewrite exists? It might be useful to think a little more about how we could make one plugin fulfill both use cases!

I admit I am still a bit concerned about problem with predictability when two different plugins want to define the same field. There is still an ordering effect here, so I wonder if a more specific solution to this particular case could be preferable to a general solution.

[Maxr1998]

Would you mind discussing a little more about what you use rewrite for when advancedrewrite exists? It might be useful to think a little more about how we could make one plugin fulfill both use cases!

Sure! I've been using the rewrite plugin for quite a while already to change or normalize artist names, album titles, and even labels. There are several reasons for me to do that:

• Tidiness: a few of my releases are by two or more artists, e.g., with featuring relationships. I prefer to keep those in a single folder instead of splitting them out. For example, DAOKO, GuruConnect is rewritten to just DAOKO. Also refer to the Jimi Hendrix example in the rewrite plugin docs — it's just about the same case.
• Rewriting legal names to artist names: some artists are listed with their legal name on MusicBrainz, which is fine, but I personally prefer to use their artist name in my library. For example, 배유빈 and 김미현 are rewritten to 유빈 and 미미, respectively.
• Keeping the previous metadata after something was renamed on MusicBrainz: some artists or albums have been renamed since I imported them, when running mbsync I want to keep them as is to not modify the file paths. This is because my media server re-indexes renamed files and drops all previous statistics like when it was added to the library, liked songs and playlists. Often, I also simply prefer the previous spelling but understand why it has to be changed on MusicBrainz to be correct. This can even be simple things like changed case or added spaces. Examples of this are rewriting the album Love&Evil to Love & Evil or the single [REASON] to REASON (removing the brackets).

Porting this to the advancedrewrite plugin might be possible, but would noticeably inflate the configuration file size due to the elaborateness of the advancedrewrite configuration syntax. The (currently) 60 lines of my rewrite config would at least triple.

Since contributing the advancedrewrite plugin a while ago, I've also been using it extensively. In some cases, statically rewriting a certain artist simply isn't enough, the more advanced syntax allows more complex rules like only rewriting releases after a given year. #4845 contains my reasoning and an example for that.

I admit I am still a bit concerned about problem with predictability when two different plugins want to define the same field. There is still an ordering effect here, so I wonder if a more specific solution to this particular case could be preferable to a general solution.

I agree, however, a part of my changes does at least reduce the number of conflicts.
Currently, the two plugins already conflict when just rewriting the same field — completely independent of which items would even be rewritten by any of the plugins.

That means, it's not possible to rewrite artist A using one plugin and artist B using the other. The changes in my PR allow that to work again, the only thing that's still unsupported is rewriting the same artist A with both of the plugins. In case that's attempted, the advancedrewrite plugin will win because it's applied first due to the name ordering.

Another point — this is precisely what happens already for rules concerning the same entity in either of the plugins. The rule that's evaluated first is applied, the other one is silently dropped. My changes simply port that behavior to happen across multiple plugins.

[sampsyo]

Thanks for explaining a little bit more! To explain farther what I'm worried about here, it's not exactly about the interaction between these two specific plugins—this behavior would clearly be better for this specific case. It's about the effects on the general case of two plugins that want to provide the same field. In that setting, I am skeptical that this kind of "conditional fallback" is likely to be what people want, and a hard error is likely to offer better predictability at less cost in complexity.

To try to satisfy both constraints, I'm thinking a bit about how advancedrewrite could expand to make this cross-plugin interaction less necessary. Maybe we could consider an alternate, shorter syntax for simple rewrites? For example, the current syntax in that plugin looks like this:

- match: artist:foo
  field: artist
  replacement: bar

We could consider adding a second syntax that looks more like classic rewrite:

- artist foo: bar

The idea would be that, in the plugin, we'd detect when the match, field, and replacement keys are missing in the dict, and instead look for a space-separated key/pattern pair. Then the plugin could weave together these simple conditions with the more complex ones, and the config file would look pretty similar to using the simpler plugin separately.

[Maxr1998]

While I think that would work for me personally, I'm still hesitant to limit this functionality to a single plugin, even if they don't cause a conflict.

I can see how two plugins attempting to rewrite the same field for the same item should throw an error, and I am willing to modify my code to do that. But generally throwing an error when attempting to rewrite the same field feels a bit extreme and limiting to me.
Currently, when using the rewrite plugin for the most common fields like artist, album, title, label, etc., you essentially block all other (future) plugins from rewriting these fields.

[sampsyo]

To explain a tiny bit more, the reason I'm pushing back somewhat on this level of generality here is that I'm concerned that this problem might be very specific to the rewrite plugin (and advancedrewrite). This plugin does something very unconventional by defining computed fields that "shadow" existing built-in fields. It is, in fact, not entirely clear this is the way it really should be working; it causes plenty of problems that the underlying metadata is "masked," and it might be better to just modify the metadata or to provide alternate names for the updated fields. This is why I'm not wild about changing the way the plugin system works to encourage more of this behavior.

[Maxr1998]

I understand what you mean. The way those rewrites are transparent to all components reading any field is convenient from a development perspective, but does have a few unexpected side effects.
Maybe the database should always store the actual values, and only when writing out the metadata (or when querying it through the CLI), it should actually be rewritten. But this concept goes way beyond the scope of this bug report/PR.

So, what would you suggest to fix the issue I reported?

Update the advanced rewrite plugin to incorporate the simpler syntax of the rewrite plugin? In that case, I'd also suggest throwing an error if both plugins are enabled at the same time.

Or do you want me to implement what I suggested in my previous post, and make plugins conflict only if they try to rewrite the same item, not just the same field?

[sampsyo]

Thanks for your persistence here! And for helping out by talking over the various options.

My preference here would be to simply produce an error when two different plugins attempt to implement a field with the same name. This will at least prevent the bad/confusing behavior if people were to stumble upon it accidentally.

Separately, I'd be very interested in working on the "simple syntax" for advancedrewrite in a different PR! And maybe we want to do that first before even adding an error, so that this kind of use case can still be accommodated.

[Maxr1998]

Separately, I'd be very interested in working on the "simple syntax" for advancedrewrite in a different PR! And maybe we want to do that first before even adding an error, so that this kind of use case can still be accommodated.

Alright, I can do that, and work on the plugin conflict afterward.
As the advancedrewrite plugin hasn't been part of an official release yet anyway, I suggest the following breaking changes to the configuration syntax:

advancedrewrite:
  # simple syntax
  - artist ODD EYE CIRCLE: 이달의 소녀 오드아이써클
  # new advanced syntax (also allows changing multiple fields for same match)
  - match: "mb_artistid:dec0f331-cb08-4c8e-9c9f-aeb1f0f6d88c year:..2022"
    replacements:
      artist: 이달의 소녀 오드아이써클
  # advanced syntax for multi-valued fields
  - match: "artist:배유빈 feat. 김미현"
    replacements:
      artists:
        - 유빈
        - 미미

The changes not only introduce the new simple syntax, but also allow to apply multiple field replacements to the same query match, and further add support for the newly introduced multi-valued fields.
Would love to hear your thoughts on these changes.

[sampsyo]

Awesome! This all looks really good.