Automatically update and handle SCMTool IDs on access.
Review Request #12381 — Created June 17, 2022 and submitted — Latest diff uploaded
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_idset, 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
Repository.scmtool_classbenefits from this, and has also received its
own fixes, restoring some previous behavior that got dropped. It no
longer unconditionally returns
Noneon error. Instead, it raises
ImproperlyConfiguredif the tool could not be loaded, as
Tool.scmtool_classdoes. It also caches the result if not
Any errors are logged and useful exceptions raised, to avoid random
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
which does not raise exceptions when an item is not found, raising
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
Unit tests pass.
Tested this with some repositories I had that didn't get updated,
and erased some stored
scmtool_idvalues 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