Fix the edit plugin displaying bogus data or even crashing on re-imports

[wisp3rwind]

There seems to be a bug in the edit plugin when re-importing (beet import -L) items that are in the library already:

As can be seen in the diff, the plugin assigns temporary ids to the items to be edited (presumably for applying the changes to the correct obejct in EditPlugin.apply_data or because some function deeper in the guts of beets requires a non-None id). This doesn't really cause any problems if none of the items is in the database: EditPlugin.edit_objects will create a deepcopy for any object with a false-y Item._db and hand that to beets.ui.show_model_changes.

If however Item._db is set, EditPlugin.edit_objects will provide None as the old object to beets.ui.show_model_changes which will in turn query the database with the (invalid temporary!) id of the new object to obtain the previous object and show the diff. Now, since the id is invalid, two things can happen:

• an object with that id actually exists: No crash will happen (and the edit finishes cleanly), but the old state for the diff will be a totally unrelated Item.
• No such object exists: The database query will return None, and the following traceback happens:

Traceback (most recent call last):
  File "/home/[...]/bin/beet", line 11, in <module>
    sys.exit(main())
  File "/home/[...]/lib/python3.6/site-packages/beets/ui/__init__.py", line 1258, in main
    _raw_main(args)
  File "/home/[...]/lib/python3.6/site-packages/beets/ui/__init__.py", line 1245, in _raw_main
    subcommand.func(lib, suboptions, subargs)
  File "/home/[...]/lib/python3.6/site-packages/beets/ui/commands.py", line 934, in import_func
    import_files(lib, paths, query)
  File "/home/[...]/lib/python3.6/site-packages/beets/ui/commands.py", line 911, in import_files
    session.run()
  File "/home/[...]/lib/python3.6/site-packages/beets/importer.py", line 325, in run
    pl.run_parallel(QUEUE_SIZE)
  File "/home/[...]/lib/python3.6/site-packages/beets/util/pipeline.py", line 445, in run_parallel
    six.reraise(exc_info[0], exc_info[1], exc_info[2])
  File "/home/[...]/lib/python3.6/site-packages/six.py", line 686, in reraise
    raise value
  File "/home/[...]/lib/python3.6/site-packages/beets/util/pipeline.py", line 312, in run
    out = self.coro.send(msg)
  File "/home/[...]/lib/python3.6/site-packages/beets/util/pipeline.py", line 171, in coro
    task = func(*(args + (task,)))
  File "/home/[...]/lib/python3.6/site-packages/beets/importer.py", line 1317, in user_query
    task.choose_match(session)
  File "/home/[...]/lib/python3.6/site-packages/beets/importer.py", line 889, in choose_match
    choice = session.choose_item(self)
  File "/home/[...]/lib/python3.6/site-packages/beets/ui/commands.py", line 755, in choose_item
    post_choice = choice.callback(self, task)
  File "/home/[...]/lib/python3.6/site-packages/beetsplug/edit.py", line 376, in importer_edit
    success = self.edit_objects(task.items, fields)
  File "/home/[...]/lib/python3.6/site-packages/beetsplug/edit.py", line 293, in edit_objects
    changed |= ui.show_model_changes(obj, obj_old)
  File "/home/[...]/lib/python3.6/site-packages/beets/ui/__init__.py", line 707, in show_model_changes
    for field in old:
TypeError: 'NoneType' object is not iterable

I'd appreciate if someone could check the correctness of that analysis. I assumed negative temporary ids to be safe (see the code) since these items are never comitted to the database (which would create a new id anyway). I'm not an exactly expert on the database code, though. Is that a reasonable assumption?

(The actual context how I found this bug is somewhat different: I'm writing a plugin, that puts new Items in the database in order to attach a few flexattrs. It then runs a TerminalImportSession to fill in the remaining metadata. Using the eDit option in that situation triggers the same bug.)

[mention-bot]

@wordofglass, thanks for your PR! By analyzing the history of the files in this pull request, we identified @sampsyo, @diego-plan9 and @jmwatte to be potential reviewers.

[sampsyo]

Oh dear! That is a pretty serious problem, and I agree with your analysis. In the end, it's possible that the "temporary id" hack was not the greatest idea; perhaps we should have invented a new unique key just for editing that could not conflict with in-database ids.

In any case, the fix looks right to me, and having the additional checks for a missing _db field make the code clearer. Thanks for the careful fix. I'd say this is ready to merge—would you mind summarizing the problem in a changelog entry too?

[wisp3rwind]

Done 🎉

[sampsyo]

Excellent! Thanks again. 👏