API error improvements

lib/pbench/server/api/resources/__init__.py

@@ -5,6 +5,7 @@
 from json.decoder import JSONDecodeError
 from logging import Logger
 from typing import Any, Callable, Dict, List, NamedTuple, Optional, Union
+import uuid
 
 from dateutil import parser as date_parser
 from flask import request
@@ -32,11 +33,11 @@
 
 
 class APIAbort(Exception):
-    """
-    Used to report an error and abort if there is a failure in processing of API request.
+    """Used to report an error and abort if there is a failure in processing of
+    API request.
     """
 
-    def __init__(self, http_status: int, message: str = None):
+    def __init__(self, http_status: int, message: Optional[str] = None):
         self.http_status = http_status
         self.message = message if message else HTTPStatus(http_status).phrase
 
@@ -47,9 +48,43 @@ def __str__(self) -> str:
         return self.message
 
 
-class UnauthorizedAccess(APIAbort):
+class APIInternalError(APIAbort):
+    """Used to report a server internal error with a UUID value that connects
+    the string reported to the client with a server log entry to aid analysis
+    by an SRE
     """
-    The user is not authorized for the requested operation on the specified
+
+    def __init__(self, details: str):
+        """Construct an "internal server error" exception object.
+
+        This exception is raised and will be used in the API _dispatch method
+        to generate a JSON error message payload to the client. This message
+        contains a UUID string that uniquely identifies this internal server
+        error context. An internal server log message is generated, with
+        traceback, also containing that UUID value so that a server
+        administrator can investigate the cause of the error.
+
+        NOTE: We want to return minimal explanations of internal errors to the
+        client while capturing detailed information for server developers to
+        determine what happened.
+
+        NOTE: We use a fully formatted "details" message here for convenience;
+        we will report this with `logger.exception`, which is never disabled,
+        so deferring the formatting would have no value.
+
+        Args:
+            details: A detailed message to be logged when this exception is caught
+        """
+        u = uuid.uuid4()
+        super().__init__(
+            http_status=HTTPStatus.INTERNAL_SERVER_ERROR,
+            message=f"Internal Pbench Server Error: log reference {u}",
+        )
+        self.details = f"Internal error {u}: {details}"
+
+
+class UnauthorizedAccess(APIAbort):
+    """The user is not authorized for the requested operation on the specified
     resource.
     """
 
@@ -72,8 +107,7 @@ def __str__(self) -> str:
 
 
 class UnauthorizedAdminAccess(UnauthorizedAccess):
-    """
-    A refinement of the UnauthorizedAccess exception where ADMIN access is
+    """A refinement of the UnauthorizedAccess exception where ADMIN access is
     required and we have no associated resource owner and access.
     """
 
@@ -96,9 +130,7 @@ def __str__(self) -> str:
 
 
 class SchemaError(APIAbort):
-    """
-    Generic base class for errors in processing a JSON schema.
-    """
+    """Generic base class for errors in processing a JSON schema."""
 
     def __init__(self, http_status: int = HTTPStatus.BAD_REQUEST):
         super().__init__(http_status=http_status)
@@ -108,9 +140,8 @@ def __str__(self) -> str:
 
 
 class UnverifiedUser(SchemaError):
-    """
-    Attempt by an unauthenticated client to reference a username in a query. An
-    unauthenticated client does not have the right to look up any username.
+    """Attempt by an unauthenticated client to reference a username in a query.
+    An unauthenticated client does not have the right to look up any username.
 
     HTTPStatus.UNAUTHORIZED tells the client that the operation might succeed
     if the request is retried with authentication. (A UI might redirect to a
@@ -126,17 +157,14 @@ def __str__(self):
 
 
 class InvalidRequestPayload(SchemaError):
-    """
-    A required client JSON input document is missing.
-    """
+    """A required client JSON input document is missing."""
 
     def __str__(self) -> str:
         return "Invalid request payload"
 
 
 class UnsupportedAccessMode(SchemaError):
-    """
-    Unsupported values for user or access, or an unsupported combination of
+    """Unsupported values for user or access, or an unsupported combination of
     both.
     """
 
@@ -150,9 +178,8 @@ def __str__(self) -> str:
 
 
 class MissingParameters(SchemaError):
-    """
-    One or more required JSON keys are missing, or the values are unexpectedly
-    empty.
+    """One or more required JSON keys are missing, or the values are
+    unexpectedly empty.
     """
 
     def __init__(self, keys: List[str]):
@@ -164,9 +191,7 @@ def __str__(self):
 
 
 class BadQueryParam(SchemaError):
-    """
-    One or more unrecognized URL query parameters were specified.
-    """
+    """One or more unrecognized URL query parameters were specified."""
 
     def __init__(self, keys: List[str]):
         super().__init__()
@@ -177,8 +202,8 @@ def __str__(self):
 
 
 class RepeatedQueryParam(SchemaError):
-    """
-    A URL query parameter key was repeated, but Pbench supports only one value.
+    """A URL query parameter key was repeated, but Pbench supports only one
+    value.
     """
 
     def __init__(self, key: str):
@@ -190,13 +215,10 @@ def __str__(self):
 
 
 class ConversionError(SchemaError):
-    """
-    Used to report an invalid parameter type
-    """
+    """Used to report an invalid parameter type"""
 
     def __init__(self, value: Any, expected_type: str, **kwargs):
-        """
-        Construct a ConversionError exception
+        """Construct a ConversionError exception
 
         Args:
             value: The value we tried to convert
@@ -212,13 +234,10 @@ def __str__(self):
 
 
 class DatasetConversionError(SchemaError):
-    """
-    Used to report an invalid dataset name.
-    """
+    """Used to report an invalid dataset name."""
 
     def __init__(self, value: str, **kwargs):
-        """
-        Construct a DatasetConversionError exception. This is modeled after
+        """Construct a DatasetConversionError exception. This is modeled after
         DatasetNotFound, but is within the SchemaError exception hierarchy.
 
         Args:
@@ -233,9 +252,7 @@ def __str__(self):
 
 
 class KeywordError(SchemaError):
-    """
-    Used to report an unrecognized keyword value.
-    """
+    """Used to report an unrecognized keyword value."""
 
     def __init__(
         self,
@@ -245,8 +262,7 @@ def __init__(
         *,
         keywords: List[str] = [],
     ):
-        """
-        Construct a KeywordError exception
+        """Construct a KeywordError exception
 
         Args:
             parameter: The Parameter defining the keywords
@@ -266,13 +282,10 @@ def __str__(self):
 
 
 class ListElementError(SchemaError):
-    """
-    Used to report an unrecognized list element value.
-    """
+    """Used to report an unrecognized list element value."""
 
     def __init__(self, parameter: "Parameter", bad: List[str]):
-        """
-        Construct a ListElementError exception
+        """Construct a ListElementError exception
 
         Args:
             parameter: The Parameter defining the list
@@ -292,8 +305,7 @@ def __str__(self):
 
 
 def convert_date(value: str, _) -> datetime:
-    """
-    Convert a date/time string to a datetime.datetime object.
+    """Convert a date/time string to a datetime.datetime object.
 
     Args:
         value: String representation of date/time
@@ -312,9 +324,8 @@ def convert_date(value: str, _) -> datetime:
 
 
 def convert_username(value: Union[str, None], _) -> Union[str, None]:
-    """
-    Validate that the user object referenced by the username string exists, and
-    return the internal representation of that user.
+    """Validate that the user object referenced by the username string exists,
+    and return the internal representation of that user.
 
     We do not want an unauthenticated client to be able to distinguish between
     "invalid user" (ConversionError here) and "valid user I can't access" (some
@@ -1558,15 +1569,18 @@ def _dispatch(
                     method,
                     ApiParams(body=body_params, query=query_params, uri=uri_params),
                 )
+            except APIInternalError as e:
+                self.logger.exception("{} {}", api_name, e.details)
+                abort(e.http_status, message=str(e))
             except APIAbort as e:
                 self.logger.exception("{} {}", api_name, e)
                 abort(e.http_status, message=str(e))
-            except Exception as e:
-                self.logger.exception("{} API error: {}", api_name, e)
-                abort(
-                    HTTPStatus.INTERNAL_SERVER_ERROR,
-                    message=HTTPStatus.INTERNAL_SERVER_ERROR.phrase,
-                )
+            except Exception:
+                # Construct an APIInternalError to get the UUID and standard return
+                # message.
+                x = APIInternalError("Unexpected validation exception")
+                self.logger.exception("{} {}", api_name, x.details)
+                abort(x.http_status, message=str(x))
         else:
             params = ApiParams(None, None, None)
 
@@ -1583,12 +1597,15 @@ def _dispatch(
             except UnauthorizedAccess as e:
                 self.logger.warning("{}: {}", api_name, e)
                 abort(e.http_status, message=str(e))
-            except Exception as e:
-                self.logger.exception("{}: {}", api_name, e)
-                abort(
-                    HTTPStatus.INTERNAL_SERVER_ERROR,
-                    message=HTTPStatus.INTERNAL_SERVER_ERROR.phrase,
-                )
+            except APIInternalError as e:
+                self.logger.exception("{} {}", api_name, e.details)
+                abort(e.http_status, message=str(e))
+            except Exception:
+                # Construct an APIInternalError to get the UUID and standard return
+                # message.
+                x = APIInternalError("Unexpected authorize exception")
+                self.logger.exception("{} {}", api_name, x.details)
+                abort(x.http_status, message=str(x))
 
         audit = None
 
@@ -1629,7 +1646,11 @@ def _dispatch(
                     attributes=auditing["attributes"],
                 )
             return response
+        except APIInternalError as e:
+            self.logger.exception("{} {}", api_name, e.details)
+            abort(e.http_status, message=str(e))
         except APIAbort as e:
+            self.logger.error("{} {}", api_name, e)
             if auditing["finalize"]:
                 attr = auditing.get("attributes", {"message": str(e)})
                 try:
@@ -1640,7 +1661,7 @@ def _dispatch(
                         attributes=attr,
                     )
                 except Exception:
-                    self.logger.exception(
+                    self.logger.error(
                         "Unexpected exception on audit: {}", json.dumps(auditing)
                     )
             abort(e.http_status, message=str(e))
@@ -1657,10 +1678,8 @@ def _dispatch(
                     reason=AuditReason.INTERNAL,
                     attributes=attr,
                 )
-            abort(
-                HTTPStatus.INTERNAL_SERVER_ERROR,
-                message=HTTPStatus.INTERNAL_SERVER_ERROR.phrase,
-            )
+            x = APIInternalError("Unexpected exception")
+            abort(x.http_status, message=x.message)
 
     def _get(self, args: ApiParams, request: Request, context: ApiContext) -> Response:
         """

lib/pbench/server/api/resources/datasets_metadata.py

@@ -1,5 +1,6 @@
 from http import HTTPStatus
 from logging import Logger
+from typing import Any
 
 from flask.json import jsonify
 from flask.wrappers import Request, Response
@@ -186,7 +187,7 @@ def _put(
         # Now update the metadata, which may occur in multiple SQL operations
         # across namespaces. Make a best attempt to update all even if we
         # encounter an unexpected error.
-        fail = False
+        fail: dict[str, Any] = {}
         for k, v in metadata.items():
             native_key = Metadata.get_native_key(k)
             user_id = None
@@ -195,10 +196,10 @@ def _put(
             try:
                 Metadata.setvalue(key=k, value=v, dataset=dataset, user_id=user_id)
             except MetadataError as e:
-                self.logger.warning("Unable to update key {} = {!r}: {}", k, v, str(e))
-                fail = True
+                fail[k] = str(e)
 
-        if fail:
-            raise APIAbort(HTTPStatus.INTERNAL_SERVER_ERROR)
-        results = self._get_dataset_metadata(dataset, list(metadata.keys()))
+        results = {
+            "metadata": self._get_dataset_metadata(dataset, list(metadata.keys())),
+            "errors": fail,
+        }
         return jsonify(results)