Summary:

Raise an error when a HostingServiceAccount fails to load a hosting service.

Review Request #12667 — Created Oct. 4, 2022 and submitted Oct. 31, 2022, 2:43 p.m.

Information
Owner:	maubin
Repository:	Review Board
Branch:	release-5.0.x
Bugs:
Depends On:
Reviewers
Groups:	reviewboard
People:

Description

Repositories use their associated HostingServiceAccount to fetch hosting
services. The hosting service account will return None if a hosting service
fails to load. This isn't the greatest thing to do since this hides the fact
that the service didn't load, and instead makes the repository behave as if
they do not have a hosting service configured.

This change instead raises an error when a HostingServiceAccount fails to
load a service. This way, a more appropriate and helpful eror message is shown
when, for example, trying to post changes or view diffs linked to repositories
that use a hosting service that can't be loaded.

This also adds some help texts to the HostingServiceAccount's fields. This
was primarily done to clarify that the service_name field should be set to
the hosting service ID rather than the hosting service name. This assumption
is made everywhere in the code except for in one test file which sets the
field to a hosting service name, which we correct in this change.

Testing Done


Ran all unit tests.
Tested viewing review requests and diffs that are from a repository that uses

  a hosting service that can't be loaded, saw the HostingServiceError page

  instead.
Tested rbt post REVISION with a git revision, saw a 

500: Internal Server Error response and saw the HostingServiceError get

  raised in the server logs.

Commits

Summary
Raise an error when a HostingServiceAccount fails to load a service.

Screenshots

Issues

Description	From	Last Updated
With the API, is a 500: Internal Server Error an appropriate response for whenever the HostingServiceError is raised? Should the …	maubin	Oct. 26, 2022, 1:44 p.m.
I'm not sure how to describe these fields, i.e. not sure what I should put in the help text.	maubin	Oct. 26, 2022, 1:47 p.m.
Blank line between class docstrings and members.	chipx86	Oct. 28, 2022, 12:16 p.m.
I expected this would be an instance of the hosting service, but of course it makes sense it's not. Just …	chipx86	Oct. 28, 2022, 12:18 p.m.
The last parameter of these new-fangled function definitions using typing should always have a trailing comma.	chipx86	Oct. 28, 2022, 12:37 p.m.
Rather than have two places where we invoke the constructor, it's often better to build the message up-front and then …	chipx86	Oct. 28, 2022, 12:32 p.m.
For instance variables, the new pattern we're starting to use for typing is to put the following in the class …	chipx86	Oct. 28, 2022, 12:37 p.m.

flake8 passed.

JSHint passed.

Testing Done:

		Ran all unit tests.
		Tested viewing review requests and diffs that are from a repository that uses a hosting service that can't be loaded, saw the `HostingServiceError` page instead.
~		Tested `rbt post REVISION` with a git revision, saw a `500: Internal Server Error` response and saw the `HostingServiceError` get raised in the server logs.
	~	Tested `rbt post REVISION` with a git revision, saw a `500: Internal Server Error` response and saw the `HostingServiceError` get raised in the server logs.

Show all issues

With the API, is a 500: Internal Server Error an appropriate response for whenever the HostingServiceError is raised? Should the response be something that is more specific to the error?

david Oct. 20, 2022, 5:02 p.m.

Which API endpoint is returning the 500?

maubin Oct. 21, 2022, 11:13 a.m.

When I use this rbt post --server http://localhost:8080 --repository reviewboard 0098d161a4b05b68b5e88c24d681c544bf61b918 command for example it leads to this request returning the 500 "GET /api/repositories/?name=reviewboard&only-fields=id%2Cname%2Cmirror_path%2Cpath&only-links=info&tool=Git%2CPerforce%2CSubversion HTTP/1.1" 500 358. But really any page or endpoint that calls Repository.hosting_service or HostingServiceAccount.service with this HostingServiceError being raised leads to the 500 Internal Server Error. I was just wondering if we should have some better user facing error message, and if so how do I do this. Or is it enough for the 500 Internal server error page to be shown, and then users contact their administrator and the administrator looks through the server logs to see what the actual error is.

chipx86 Oct. 25, 2022, 4:10 p.m.

The error condition here is that something is busted server-side (missing extension, for example), preventing the hosting service from loading. Because of that, it is indeed an internal server error, so a 500 is fine.

We probably do want to say something in the message like:

The repository "<name>" cannot load the hosting service "<name>". An administrator should check to make sure any required extensions are loaded.

Something like that. Internally, maybe we should have a MissingHostingServiceError subclass of HostingServiceError to represent this that controls the message, so it's easier to catch that explicitly.

To represent this to the caller properly, API endpoints need to catch HostingServiceError. Many already do, but if any are spitting out a default HTTP 500 with no JSON error payload with the right message, then we're probably just missing a proper try/except.

If we do have one and it's spitting out a 500 with a payload, but we want to tweak the message, we can do something like THE_API_ERROR_TYPE.with_message(str(e))

reviewboard/hostingsvcs/models.py (Diff revision 1)

Show all issues

I'm not sure how to describe these fields, i.e. not sure what I should put in the help text.

chipx86 Oct. 25, 2022, 4:10 p.m.

For data, maybe: "Account data specific to the hosting service. This should generally not be changed."

For visible, how about: "Whether this account shows up as an option when configuring a repository."

Change Summary:

Updated docstrings of methods that might raise a HostingServiceError.

Commits:

	Summary
-	Raise an error when a HostingServiceAccount fails to load a service.
+	Raise an error when a HostingServiceAccount fails to load a service.

Diff:

Revision 2 (+206 -22)

Show changes

	reviewboard/hostingsvcs/models.py
	reviewboard/hostingsvcs/tests/test_hosting_service_account.py
	reviewboard/reviews/tests/test_download_diff_file_view.py
	reviewboard/scmtools/models.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:


Created a MissingHostingServiceError with a better error message (and can include repository information).
Added more unit tests.

Commits:

	Summary
-	Raise an error when a HostingServiceAccount fails to load a service.
+	Raise an error when a HostingServiceAccount fails to load a service.

Diff:

Revision 3 (+332 -24)

Show changes

	reviewboard/hostingsvcs/errors.py
	reviewboard/hostingsvcs/models.py
	reviewboard/hostingsvcs/tests/test_hosting_service_account.py
	reviewboard/reviews/tests/test_download_diff_file_view.py
	reviewboard/scmtools/models.py
	reviewboard/scmtools/tests/test_repository.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:

Added help texts for HostingServiceAccount's data and visible fields.

Commits:

	Summary
-	Raise an error when a HostingServiceAccount fails to load a service.
+	Raise an error when a HostingServiceAccount fails to load a service.

Diff:

Revision 4 (+346 -28)

Show changes

	reviewboard/hostingsvcs/errors.py
	reviewboard/hostingsvcs/models.py
	reviewboard/hostingsvcs/tests/test_hosting_service_account.py
	reviewboard/reviews/tests/test_download_diff_file_view.py
	reviewboard/scmtools/models.py
	reviewboard/scmtools/tests/test_repository.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/hostingsvcs/errors.py (Diff revision 4)
Show all issues
```
Blank line between class docstrings and members.
```

reviewboard/hostingsvcs/errors.py (Diff revision 4)

Show all issues

I expected this would be an instance of the hosting service, but of course it makes sense it's not. Just from the parameter name. How about we call this hosting_service_id and make sure the ID is passed in instead of the name? The ID will better help identify the missing service.

reviewboard/hostingsvcs/errors.py (Diff revision 4)

Show all issues

The last parameter of these new-fangled function definitions using typing should always have a trailing comma.

reviewboard/hostingsvcs/errors.py (Diff revision 4)

Show all issues

Rather than have two places where we invoke the constructor, it's often better to build the message up-front and then pass it to the constructor.

This pattern helps as classes grow in complexity. If, say, we wanted to allow the caller to override the message or add additional arguments that went to HostingServiceError, then it'd help to conditionalize the message but not have to update the parent constructor call in more than one place.

reviewboard/hostingsvcs/errors.py (Diff revision 4)

Show all issues

For instance variables, the new pattern we're starting to use for typing is to put the following in the class body before functions:

######################
# Instance variables #
######################

hosting_service: str

Change Summary:


Some formatting and doc fixes.
Changed hosting_service parameter to hosting_service_id to be more explicit.
Avoids calling the parent constructor for MissingHostingServiceError in more than one place.

Commits:

	Summary
-	Raise an error when a HostingServiceAccount fails to load a service.
+	Raise an error when a HostingServiceAccount fails to load a service.

Diff:

Revision 5 (+384 -28)

Show changes

	reviewboard/hostingsvcs/errors.py
	reviewboard/hostingsvcs/models.py
	reviewboard/hostingsvcs/tests/test_hosting_service_account.py
	reviewboard/reviews/tests/test_download_diff_file_view.py
	reviewboard/scmtools/models.py
	reviewboard/scmtools/tests/test_repository.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-5.0.x (c401845)