Automatically update and handle SCMTool IDs on access.

Review Request #12381 — Created June 17, 2022 and submitted — Latest diff uploaded

Information

Review Board
release-5.0.x

Reviewers

During database upgrade from pre-5.0 to 5.0, it's possible for SCMTools
to not successfully migrate over. This can happen if any
extension-provided SCMTools were unavailable at the time of upgrade, and
could also happen if importing or creating data after the upgrade.

This is now handled when instantiating a Repository. If there's no
scmtool_id set, and there is a tool_id, it will try to migrate over
the ID, set it, and save it.

This can be potentially expensive when loading a page of repositories,
but this state migration will only happen once (for any that didn't
migrate during database upgrade), and will ensure those are then
filterable afterward.

Repository.scmtool_class benefits from this, and has also received its
own fixes, restoring some previous behavior that got dropped. It no
longer unconditionally returns None on error. Instead, it raises
ImproperlyConfigured if the tool could not be loaded, as
Tool.scmtool_class does. It also caches the result if not None, as
before.

Any errors are logged and useful exceptions raised, to avoid random
confusing crashes.

While working on this, I found that the new registry, and the callers
accessing it, were assuming that the lack of an item would raise an
ItemLookupError. This imports Review Board's own EntryPointRegistry,
which does not raise exceptions when an item is not found, raising
None instead.

Changing things to use the Djblets version (which does raise an
exception) causes other breakages throughout the product when trying to
register/unregister state, so I haven't changed this. Instead, I've
fixed the documentation and the call sites trying to catch
ItemLookupError.

Unit tests pass.

Tested this with some repositories I had that didn't get updated,
and erased some stored scmtool_id values to test as well. Verified
that this migrated over the IDs.

Also manually checked review requests that lacked a migrated ID
but couldn't look up the tool. Saw the appropriate error message in
debug mode (though we don't catch and show these sorts of errors
in production, unfortunately, but this is not a regression in
behavior).

Commits

Files