Summary

Automatically update and handle SCMTool IDs on access.

Review Request #12381 — Created June 17, 2022 and submitted June 20, 2022, 9:47 p.m.

Information

Owner

chipx86

Repository

Review Board

Branch

release-5.0.x

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

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.

Testing Done

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

Summary	ID
Automatically update and handle SCMTool IDs on access. 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`.	733096f0898b7daa6354b1c9f35e4c1da57a86d0

Summary

Automatically update and handle SCMTool IDs on access.

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`.

733096f0898b7daa6354b1c9f35e4c1da57a86d0

flake8 passed.

JSHint passed.

Change Summary:

Changed the approach for scmtool_id migration to be done at Repository initialization time, so other code accessing it (such as conditions) will work.

Description:

		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.

~		`Repository.scmtool_class` now handles this case. If there's no
	~	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 also restores some previous behavior that got dropped. It no
~		unconditionally returns `None` on error. Instead, it raises
	~	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`.

Commits:

	Summary	ID
	Automatically update and handle SCMTool IDs on access. 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. `Repository.scmtool_class` now handles this case. 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 also restores some previous behavior that got dropped. It no 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`.	d417fe24c9fb768ce0e91f7597bf7197c479e76b
	Automatically update and handle SCMTool IDs on access. 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`.	733096f0898b7daa6354b1c9f35e4c1da57a86d0

Diff:

Revision 2 (+456 -34)

Show changes

	reviewboard/scmtools/apps.py
	reviewboard/scmtools/models.py
	reviewboard/scmtools/registry.py
	reviewboard/scmtools/signal_handlers.py
	reviewboard/scmtools/tests/test_repository.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to release-5.0.x (1bd0760)