Draft: Allow plugins to inhibit file operations via hooks

[anarcat]

This reflects the try_write approach will handles write failures elegantly, by logging the error and continuing. We do the same with move, art_set and move_art.

We also handle exceptions on before_item_moved and art_set plugins hooks, the latter of which is moved before the file operations to remain consistent with other hook configurations.

That might be a mistake and API-breaking change, another approach would be to have a new before_art_set hook instead.

We also introduce a new hook (move_art) for those operations as well.

The point of this patch is to make it possible for plugins to send a signal (through the already FileOperationError exception) to callers that it should skip a specific item or artwork.

This is essential to allow beets to better integrate with other utilities like bittorrent clients which may rewrite those files. The rationale here is that some music collections will have parts of them managed by such clients in which case we should be careful not to overwrite or move those files.

Operations like copy or hardlink are not handled by this, for that reason. We may also want to do proper error handling for those as well, that said, but that seems out of scope for this specific issue (#2617).

To Do

• [ ] Documentation. (If you've add a new command-line flag, for example, find the appropriate page under docs/ to describe it.)
• [ ] Changelog. (Add an entry to docs/changelog.rst near the top of the document.)
• [ ] Tests. (Encouraged but not strictly required.)

[wisp3rwind]

I haven't thought about this in depth, but here are some initial comments. Generally, I think one point we need to pay attention to is never to update the database with new paths when the operation is actually skipped (seems to be the case for now).

This reflects the try_write approach will handles write failures elegantly, by logging the error and continuing. We do the same with move, art_set and move_art.

As explained in more detail below, I think we should really distinguish actual errors, and a plugin saying "Let's skip this".

We also handle exceptions on before_item_moved and art_set plugins hooks, the latter of which is moved before the file operations to remain consistent with other hook configurations.

That might be a mistake and API-breaking change, another approach would be to have a new before_art_set hook instead.

Yes, we should not move that hook. It'd be much more consistent to have the before_art_set hook, and also think about it's precise design, cf. below.

We also introduce a new hook (move_art) for those operations as well.

The point of this patch is to make it possible for plugins to send a signal (through the already FileOperationError exception) to callers that it should skip a specific item or artwork.

I think this API design is extremely confusing, since no real FileOperationError is happening, I'd much prefer to use something here that clearly means "skip this move operation". Plugin event handlers can return values, so one alternative would be to have a special MoveOperation.SKIP, and use it along the lines of

results = plugins.send("before_item_moved", item=self, source=self.path,
                 destination=dest)
if any(r == MoveOperation.SKIP for r in results):
    # Really not an error, a plugin _chose_ not to move the file.
    log.warning("Skipping move of item {self}")
    return

Incidentally, the before_item_moved event is placed in an unfortunate location, it would be much more useful to have an event before the MoveOperation switch and pass the particular MoveOperation as an argument to this event. Maybe, we could add such an event (before_item_moved_v2?), and deprecate before_item_moved?

Relatedly, from #2617

we also need to somehow pass information from the plugin to the caller. right now the plugins.send callers mostly ignore the return value, at least in the above hooks (there are rare exceptions elsewhere in the code). i wonder if we should return a truth value or raise an exception to signal a problem.

maybe the safest would be to raise an exception from the plugin. older versions of beets would just fail to do the operation and crash (a possible acceptable failure mode, as in "fail closed"), newer versions could handle the exception and behave correctly (e.g. skip or abort, depending on the exception)?

I don't think that justifies designing a confusing new API. Plugins that require this functionality should simply declare a dependency on a sufficiently recent version of beets.

Operations like copy or hardlink are not handled by this, for that reason. We may also want to do proper error handling for those as well, that said, but that seems out of scope for this specific issue (#2617).

By having before_item_moved_v2 as detailed above, plugins could do that without further effort.

[wisp3rwind]

I went ahead and rephrased the PR's title to my understanding of what we really want to achieve here (which is not really error handling).

[anarcat]

On 2022-01-15 01:35:54, Benedikt wrote:

I haven't thought about this in depth, but here are some initial comments. Generally, I think one point we need to pay attention to is never to update the database with new paths when the operation is actually skipped (seems to be the case for now).

That's a good point, it was a rough draft and it certainly misses some key problems like that.

This reflects the try_write approach will handles write failures elegantly, by logging the error and continuing. We do the same with move, art_set and move_art.

As explained in more detail below, I think we should really distinguish actual errors, and a plugin saying "Let's skip this".

Agreed.

We also handle exceptions on before_item_moved and art_set plugins hooks, the latter of which is moved before the file operations to remain consistent with other hook configurations.

That might be a mistake and API-breaking change, another approach would be to have a new before_art_set hook instead.

Yes, we should not move that hook. It'd be much more consistent to have the before_art_set hook, and also think about it's precise design, cf. below.

Agreed.

We also introduce a new hook (move_art) for those operations as well.

The point of this patch is to make it possible for plugins to send a signal (through the already FileOperationError exception) to callers that it should skip a specific item or artwork.

I think this API design is extremely confusing, since no real FileOperationError is happening, I'd much prefer to use something here that clearly means "skip this move operation". Plugin event handlers can return values, so one alternative would be to have a special MoveOperation.SKIP, and use it along the lines of

results = plugins.send("before_item_moved", item=self, source=self.path,
                 destination=dest)
if any(r == MoveOperation.SKIP for r in results):
    # Really not an error, a plugin _chose_ not to move the file.
    log.warning("Skipping move of item {self}")
    return

I had the feeling that returning an error was more confusing, because right now data is returned by hooks that do return data, not errors. Raising an exception makes it easier to handle it, and to handle it in a backwards-compatible way.

If FileOperationError is not the right exception (and I tend to agree, i just picked that because it was already there), we could make a new one. If it's a subclass of HumanReadableException, then it would even be backwards-compatible because currently such errors are handled gracefully (with an exit) in beets.

I have this in my checkout, which we could reuse:

modified   beets/plugins.py
@@ -25,6 +25,7 @@
 
 import beets
 from beets import logging
+from beets.util import HumanReadableException
 import mediafile
 
 
@@ -63,6 +64,15 @@ def filter(self, record):
         return True
 
 
+class PluginSkipException(HumanReadableException):
+    """An exception raised from a plugin to signal the caller to skip
+    whatever operation they are currently doing.
+
+    For example, this could signal a `move` operation to skip this
+    particular item because it is under control of another system that
+    would overwrite its changes, say a bittorrent client.
+    """
+
 # Managing the plugins themselves.
 
 class BeetsPlugin:

Incidentally, the before_item_moved event is placed in an unfortunate location, it would be much more useful to have an event before the MoveOperation switch and pass the particular MoveOperation as an argument to this event. Maybe, we could add such an event (before_item_moved_v2?), and deprecate before_item_moved?

Yeah, so the way plugins are fired now is really confusing. write is fired before, but item_moved is after. Maybe it would make sense to rewire all of them in a v2 event set?

Relatedly, from #2617

we also need to somehow pass information from the plugin to the caller. right now the plugins.send callers mostly ignore the return value, at least in the above hooks (there are rare exceptions elsewhere in the code). i wonder if we should return a truth value or raise an exception to signal a problem.

maybe the safest would be to raise an exception from the plugin. older versions of beets would just fail to do the operation and crash (a possible acceptable failure mode, as in "fail closed"), newer versions could handle the exception and behave correctly (e.g. skip or abort, depending on the exception)?

I don't think that justifies designing a confusing new API. Plugins that require this functionality should simply declare a dependency on a sufficiently recent version of beets.

I didn't know you could do that, maybe that would work. But how about using a new exception instead?

Operations like copy or hardlink are not handled by this, for that reason. We may also want to do proper error handling for those as well, that said, but that seems out of scope for this specific issue (#2617).

By having before_item_moved_v2 as detailed above, plugins could do that without further effort.

but that would still require error handling in beets itself.

[wisp3rwind]

I had the feeling that returning an error was more confusing, because right now data is returned by hooks that do return data, not errors. Raising an exception makes it easier to handle it, and to handle it in a backwards-compatible way.

IMO, handling the return value is just as easy (unless you'd want to handle the exception at a higher level, which I'd argue is a bad idea).

Yeah, so the way plugins are fired now is really confusing. write is fired before, but item_moved is after. Maybe it would make sense to rewire all of them in a v2 event set?

Well, I guess they've all been added individually whenever a plugin author needed another hook, so it's not very surprising that they're not completely consistent. I'd only change (as in, add "v2" versions) events for which you have a use right now, otherwise there's probably a risk of making changes that nobody will use, or where the new call site is even slightly wrong for whoever ends up actually needing it.

Plugins that require this functionality should simply declare a dependency on a sufficiently recent version of beets. I didn't know you could do that, maybe that would work.

Not via beets' plugin mechanism, but beets plugins are just python packages, which can have version constraints on their dependencies.

[anarcat]

On 2022-01-17 14:11:46, Benedikt wrote:

I had the feeling that returning an error was more confusing, because right now data is returned by hooks that do return data, not errors. Raising an exception makes it easier to handle it, and to handle it in a backwards-compatible way.

IMO, handling the return value is just as easy (unless you'd want to handle the exception at a higher level, which I'd argue is a bad idea).

Well that's the thing: if we do use a derivative of HumanReadableException, it will be handled at a higher level by older versions of beets.

Yeah, so the way plugins are fired now is really confusing. write is fired before, but item_moved is after. Maybe it would make sense to rewire all of them in a v2 event set?

Well, I guess they've all been added individually whenever a plugin author needed another hook, so it's not very surprising that they're not completely consistent. I'd only change (as in, add "v2" versions) events for which you have a use right now, otherwise there's probably a risk of making changes that nobody will use, or where the new call site is even slightly wrong for whoever ends up actually needing it.

Makes sense.

[stale[bot]]

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[wisp3rwind]

It's unfortunate that this has gone stale; but as explained above at length, I'm not sold on using an Exception to communicate plugin decisions here, it seems like the wrong API for the problem. Do others have an opinion or new insight in this matter?

IMO, handling the return value is just as easy (unless you'd want to handle the exception at a higher level, which I'd argue is a bad idea).

Well that's the thing: if we do use a derivative of HumanReadableException, it will be handled at a higher level by older versions of beets.

(emphasis added to the quote)

I still don't see why we would want to handle this at a higher level. Any plugin that uses the new functionality will need to declare that it requires beets>=1.6.X. With that in place, if we are still concerned that incompatible versions of beets and plugins might be installed (which I'm not sure of that we should be), the solution is probably to verify that version requirement on plugin load (and abort then, instead of a late crash midway during an import).

[stale[bot]]

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.