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

Review Request #12667 — Created Oct. 4, 2022 and submitted

Information

Review Board
release-5.0.x

Reviewers

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.

  • 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.
Summary ID
Raise an error when a HostingServiceAccount fails to load a service.
188156bbd370d9b8f45e2a9aa4c88bc7a7cbc645
Description From Last Updated

With the API, is a 500: Internal Server Error an appropriate response for whenever the HostingServiceError is raised? Should the …

maubinmaubin

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

maubinmaubin

Blank line between class docstrings and members.

chipx86chipx86

I expected this would be an instance of the hosting service, but of course it makes sense it's not. Just …

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86
maubin
maubin
  1. 
      
  2. 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?

    1. Which API endpoint is returning the 500?

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

    3. 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))

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

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

    1. 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."

  4. 
      
maubin
maubin
maubin
chipx86
  1. 
      
  2. reviewboard/hostingsvcs/errors.py (Diff revision 4)
     
     
     

    Blank line between class docstrings and members.

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

    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.

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

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

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

    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.

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

    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
    
  7. 
      
maubin
chipx86
  1. Ship It!
  2. 
      
maubin
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-5.0.x (c401845)
Loading...