diff --git a/changelog.d/20230501_212027_sirosen_fix_error_shapes_with_messages.rst b/changelog.d/20230501_212027_sirosen_fix_error_shapes_with_messages.rst new file mode 100644 index 000000000..4237cf2fc --- /dev/null +++ b/changelog.d/20230501_212027_sirosen_fix_error_shapes_with_messages.rst @@ -0,0 +1,52 @@ +* Error parsing in the SDK has been enhanced to better support JSON:API and + related error formats with multiple sub-errors. Several attributes are + added or changed. For most SDK users, the changes will be completely + transparent or a minor improvement. (:pr:`NUMBER`) + + * Error parsing now attempts to detect the format of the error data and will + parse JSON:API data differently from non-JSON:API data. Furthermore, + parsing is stricter about the expectations about fields and their types. + JSON:API parsing now has its own distinct parsing path, followed only when + the JSON:API mimetype is present. + + * A new attribute is added to API error objects, ``errors``. This is a list + of subdocuments parsed from the error data, especially relevant for + JSON:API errors and similar formats. See the + :ref:`ErrorSubdocument documentation ` for details. + + * A new attribute is now present on API error objects, ``messages``. This is + a list of messages parsed from the error data, for errors with multiple + messages. When there is only one message, ``messages`` will only contain + one item. + + * The ``message`` field is now an alias for a joined string of + ``messages``. Assigning a string to ``message`` is supported for error + subclasses, but is deprecated. + + * ``message`` will now be ``None`` when no messages can be parsed from the error data. + Previously, the default for ``message`` would be an alias for ``text``. + + * All error types now support ``request_id`` as an attribute, but it will + default to ``None`` for errors which do not include a ``request_id``. + + * An additional field is checked by default for error message data, + ``title``. This is useful when errors contain ``title`` but no + ``detail`` field. The extraction of messages from errors has been made + stricter, especially in the JSON:API case. + + * The ``code`` field of errors will no longer attempt to parse only the first + ``code`` from multiple sub-errors. Instead, ``code`` will first parse a + top-level ``code`` field, and then fallback to checking if *all* sub-errors + have the same ``code`` value. The result is that certain errors which would + populate a non-default ``code`` value no longer will, but the ``code`` will + also no longer be misleading when multiple errors with different codes are + present in an error object. + + * The ``code`` field of an error may now be ``None``. This is specifically + possible when the error format is detected to be known as JSON:API and + there is no ``code`` present in any responses. + +* Behavior has changed slightly specifically for ``TimerAPIError``. When parsing + fails, the ``code`` will be ``Error`` and the ``messages`` will be empty. The + ``detail`` field will be treated as the ``errors`` array for these errors + when it is present and contains an array of objects. diff --git a/docs/core/exceptions.rst b/docs/core/exceptions.rst index 0f95062bd..70837a349 100644 --- a/docs/core/exceptions.rst +++ b/docs/core/exceptions.rst @@ -65,7 +65,6 @@ Malformed calls to Globus SDK methods typically raise ``GlobusSDKUsageError``, but, in rare cases, may raise standard python exceptions (``ValueError``, ``OSError``, etc.) - Error Classes ------------- @@ -97,6 +96,34 @@ Error Classes :members: :show-inheritance: +.. _error_subdocuments: + +ErrorSubdocuments +----------------- + +Errors returned from APIs may define a series of subdocuments, each containing +an error object. This is used if there were multiple errors encountered and the +API wants to send them back all at once. + +All instances of ``GlobusAPIError`` define an attribute, ``errors``, which is +an array of ``ErrorSubdocument``\s. + +Error handling code can inspect these sub-errors like so: + +.. code-block:: python + + try: + some_complex_globus_operation() + except GlobusAPIError as e: + if e.errors: + print("sub-errors encountered") + print("(code, message)") + for suberror in e.errors: + print(f"({suberror.code}, {suberror.message}") + +.. autoclass:: globus_sdk.exc.ErrorSubdocument + :members: + .. _error_info: ErrorInfo diff --git a/src/globus_sdk/__init__.py b/src/globus_sdk/__init__.py index a3961bb2e..4911552e3 100644 --- a/src/globus_sdk/__init__.py +++ b/src/globus_sdk/__init__.py @@ -32,6 +32,7 @@ def _force_eager_imports() -> None: }, "exc": { "GlobusAPIError", + "ErrorSubdocument", "GlobusConnectionError", "GlobusConnectionTimeoutError", "GlobusError", @@ -134,6 +135,7 @@ def _force_eager_imports() -> None: from .authorizers import RefreshTokenAuthorizer from .client import BaseClient from .exc import GlobusAPIError + from .exc import ErrorSubdocument from .exc import GlobusConnectionError from .exc import GlobusConnectionTimeoutError from .exc import GlobusError @@ -257,6 +259,7 @@ def __getattr__(name: str) -> t.Any: "CollectionPolicies", "ConfidentialAppAuthClient", "DeleteData", + "ErrorSubdocument", "FlowsAPIError", "FlowsClient", "GCSAPIError", diff --git a/src/globus_sdk/_generate_init.py b/src/globus_sdk/_generate_init.py index c1bd5874e..4d2cbf5a1 100755 --- a/src/globus_sdk/_generate_init.py +++ b/src/globus_sdk/_generate_init.py @@ -81,6 +81,7 @@ def __getattr__(name: str) -> t.Any: "exc", ( "GlobusAPIError", + "ErrorSubdocument", "GlobusConnectionError", "GlobusConnectionTimeoutError", "GlobusError", diff --git a/src/globus_sdk/_testing/data/flows/create_flow.py b/src/globus_sdk/_testing/data/flows/create_flow.py index c0dfa303b..06a1139e1 100644 --- a/src/globus_sdk/_testing/data/flows/create_flow.py +++ b/src/globus_sdk/_testing/data/flows/create_flow.py @@ -29,4 +29,38 @@ json=TWO_HOP_TRANSFER_FLOW_DOC, match=[matchers.json_params_matcher(params=_two_hop_transfer_create_request)], ), + bad_admin_principal_error=RegisteredResponse( + service="flows", + path="/flows", + method="POST", + status=422, + json={ + "error": { + "code": "UNPROCESSABLE_ENTITY", + "detail": [ + { + "loc": ["flow_administrators", 0], + "msg": ( + "Unrecognized principal string: " + '"ae341a98-d274-11e5-b888-dbae3a8ba545". ' + 'Allowed principal types in role "FlowAdministrator": ' + "[, ]" + ), + "type": "value_error", + }, + { + "loc": ["flow_administrators", 1], + "msg": ( + "Unrecognized principal string: " + '"4fab4345-6d20-43a0-9a25-16b2e3bbe765". ' + 'Allowed principal types in role "FlowAdministrator": ' + "[, ]" + ), + "type": "value_error", + }, + ], + }, + "debug_id": "cf71b1d1-ab7e-48b1-8c54-764201d28ded", + }, + ), ) diff --git a/src/globus_sdk/_testing/data/flows/run_flow.py b/src/globus_sdk/_testing/data/flows/run_flow.py index 38fa17cb9..c6a172375 100644 --- a/src/globus_sdk/_testing/data/flows/run_flow.py +++ b/src/globus_sdk/_testing/data/flows/run_flow.py @@ -20,4 +20,18 @@ json=TWO_HOP_TRANSFER_RUN, match=[matchers.json_params_matcher(params=_request_params)], ), + missing_scope_error=RegisteredResponse( + service="flows", + method="POST", + path=f"/flows/{TWO_HOP_TRANSFER_FLOW_ID}/run", + status=403, + json={ + "error": { + "code": "MISSING_SCOPE", + "detail": ( + "This action requires the following scope: frobulate[demuddle]" + ), + } + }, + ), ) diff --git a/src/globus_sdk/_testing/data/timer/create_job.py b/src/globus_sdk/_testing/data/timer/create_job.py index ffbe48ebd..18dc62fbe 100644 --- a/src/globus_sdk/_testing/data/timer/create_job.py +++ b/src/globus_sdk/_testing/data/timer/create_job.py @@ -9,7 +9,33 @@ path="/jobs/", method="POST", json=JOB_JSON, - metadata={"job_id": JOB_ID}, status=201, ), + validation_error=RegisteredResponse( + service="timer", + path="/jobs/", + method="POST", + json={ + "detail": [ + { + "loc": ["body", "start"], + "msg": "field required", + "type": "value_error.missing", + }, + { + "loc": ["body", "callback_url"], + "msg": "field required", + "type": "value_error.missing", + }, + ] + }, + metadata={ + "job_id": JOB_ID, + "expect_messages": [ + "field required: body.start", + "field required: body.callback_url", + ], + }, + status=422, + ), ) diff --git a/src/globus_sdk/_testing/data/timer/get_job.py b/src/globus_sdk/_testing/data/timer/get_job.py index 2e870a0d2..f2a7bdf5b 100644 --- a/src/globus_sdk/_testing/data/timer/get_job.py +++ b/src/globus_sdk/_testing/data/timer/get_job.py @@ -118,4 +118,17 @@ method="GET", json=JOB_JSON, ), + simple_500_error=RegisteredResponse( + service="timer", + path=f"/jobs/{JOB_ID}", + method="GET", + status=500, + json={ + "error": { + "code": "ERROR", + "detail": "Request failed terribly", + "status": 500, + } + }, + ), ) diff --git a/src/globus_sdk/_testing/models.py b/src/globus_sdk/_testing/models.py index 8b1d7ea9c..427cc8751 100644 --- a/src/globus_sdk/_testing/models.py +++ b/src/globus_sdk/_testing/models.py @@ -57,9 +57,6 @@ def __init__( self.method = method.upper() self.json = json self.body = body - - if headers is None: - headers = {"Content-Type": "application/json"} self.headers = headers self._metadata = metadata diff --git a/src/globus_sdk/exc/__init__.py b/src/globus_sdk/exc/__init__.py index e85e03f97..47190780c 100644 --- a/src/globus_sdk/exc/__init__.py +++ b/src/globus_sdk/exc/__init__.py @@ -1,4 +1,4 @@ -from .api import GlobusAPIError +from .api import ErrorSubdocument, GlobusAPIError from .base import GlobusError, GlobusSDKUsageError from .convert import ( GlobusConnectionError, @@ -19,6 +19,7 @@ "GlobusError", "GlobusSDKUsageError", "GlobusAPIError", + "ErrorSubdocument", "NetworkError", "GlobusTimeoutError", "GlobusConnectionTimeoutError", diff --git a/src/globus_sdk/exc/api.py b/src/globus_sdk/exc/api.py index 8cf6798d8..340bfed3c 100644 --- a/src/globus_sdk/exc/api.py +++ b/src/globus_sdk/exc/api.py @@ -1,5 +1,6 @@ from __future__ import annotations +import enum import logging import typing as t @@ -11,33 +12,67 @@ log = logging.getLogger(__name__) +_CACHE_SENTINEL = object() + + +class _ErrorFormat(enum.Enum): + undefined = enum.auto() + jsonapi = enum.auto() + type_zero = enum.auto() + class GlobusAPIError(GlobusError): """ Wraps errors returned by a REST API. - :ivar http_status: HTTP status code (int) - :ivar code: Error code from the API (str), - or "Error" for unclassified errors - :ivar message: Error message from the API. In general, this will be more - useful to developers, but there may be cases where it's - suitable for display to end users. + :ivar int http_status: HTTP status code + :ivar str code: Error code from the API or "Error" for unclassified errors + :ivar str request_id: The 'request_id' included in the error data, if any. + :ivar list[str] messages: A list of error messages, extracted from the response + data. If the data cannot be parsed or does not contain any clear message fields, + this list may be empty. + :ivar list[GlobusSubError] errors: A list of sub-error documents, as would be + presented by JSON:API APIs and similar interfaces. """ - MESSAGE_FIELDS = ["message", "detail"] + MESSAGE_FIELDS = ["message", "detail", "title"] RECOGNIZED_AUTHZ_SCHEMES = ["bearer", "basic", "globus-goauthtoken"] def __init__(self, r: requests.Response, *args: t.Any, **kwargs: t.Any): + self._cached_raw_json: t.Any = _CACHE_SENTINEL + self.http_status = r.status_code # defaults, may be rewritten during parsing - self.code = "Error" - self.message = r.text + self.code: str | None = "Error" + self.request_id: str | None = None + self.messages: list[str] = [] + self.errors: list[ErrorSubdocument] = [] self._info: ErrorInfoContainer | None = None self._underlying_response = r self._parse_response() super().__init__(*self._get_args()) + @property + def message(self) -> str | None: + """ + An error message from the API. + + If there are multiple messages available, this will contain all messages joined + with semicolons. If there is no message available, this will be ``None``. + """ + if self.messages: + return "; ".join(self.messages) + return None + + @message.setter + def message(self, value: str) -> None: + warn_deprecated( + "Setting a message on GlobusAPIError objects is deprecated. " + "This overwrites any parsed messages. Append to 'messages' instead." + ) + self.messages = [value] + @property def http_reason(self) -> str: """ @@ -63,11 +98,23 @@ def headers(self) -> t.Mapping[str, str]: def content_type(self) -> str | None: return self.headers.get("Content-Type") - def _json_content_type(self) -> bool: - r = self._underlying_response - return "Content-Type" in r.headers and ( - "application/json" in r.headers["Content-Type"] - ) + def _get_mimetype(self, content_type: str) -> str: + return content_type.split(";")[0].strip() + + def _jsonapi_mimetype(self) -> bool: + if self.content_type is None: + return False + return self._get_mimetype(self.content_type) == "application/vnd.api+json" + + def _json_mimetype(self) -> bool: + if self.content_type is None: + return False + mimetype = self._get_mimetype(self.content_type) + if mimetype == "application/json": + return True + if mimetype.startswith("application/") and mimetype.endswith("+json"): + return True + return False @property def raw_json(self) -> dict[str, t.Any] | None: @@ -77,21 +124,30 @@ def raw_json(self) -> dict[str, t.Any] | None: If the body cannot be loaded as JSON, this is None """ - r = self._underlying_response - if not self._json_content_type(): - return None + if self._cached_raw_json == _CACHE_SENTINEL: + self._cached_raw_json = None + if self._json_mimetype(): + try: + # technically, this could be a non-dict JSON type, like a list or + # string but in those cases the user can just cast -- the "normal" + # case is a dict + self._cached_raw_json = self._underlying_response.json() + except ValueError: + log.error( + "Error body could not be JSON decoded! " + "This means the Content-Type is wrong, or the " + "body is malformed!" + ) + return t.cast("dict[str, t.Any]", self._cached_raw_json) - try: - # technically, this could be a non-dict JSON type, like a list or string - # but in those cases the user can just cast -- the "normal" case is a dict - return t.cast(t.Dict[str, t.Any], r.json()) - except ValueError: - log.error( - "Error body could not be JSON decoded! " - "This means the Content-Type is wrong, or the " - "body is malformed!" - ) - return None + @property + def _dict_data(self) -> dict[str, t.Any]: + """ + A "type asserting" wrapper over raw_json which errors if the type is not dict. + """ + if not isinstance(self.raw_json, dict): + raise ValueError("cannot use _dict_data when data is non-dict type") + return self.raw_json @property def text(self) -> str: @@ -126,8 +182,7 @@ def info(self) -> ErrorInfoContainer: could not be parsed. """ if self._info is None: - rawjson = self.raw_json - json_data = rawjson if isinstance(rawjson, dict) else None + json_data = self.raw_json if isinstance(self.raw_json, dict) else None self._info = ErrorInfoContainer(json_data) return self._info @@ -146,7 +201,7 @@ def _get_args(self) -> list[t.Any]: Get arguments to pass to the Exception base class. These args are displayed in stack traces. """ - return [ + args = [ self._underlying_response.request.method, self._underlying_response.url, self._get_request_authorization_scheme(), @@ -157,67 +212,262 @@ def _get_args(self) -> list[t.Any]: # https://datatracker.ietf.org/doc/html/rfc7231#section-6.1 self.message or self._underlying_response.reason, ] + if self.request_id: + args.append(self.request_id) + return args - def _parse_response(self) -> None: + def _parse_response(self) -> bool: """ This is an intermediate step between 'raw_json' (loading bare JSON data) - and the "real" parsing method, '_load_from_json' + and the "real" parsing methods. + + In order to better support subclassing with short-circuiting behaviors if + parsing goes awry, all of the parsing methods return True if parsing should + continue or False if it was aborted. _parse_response() pulls the JSON body and does the following: - - if the data is not a dict, ensure _load_from_json is not called (so it only - gets called on dict data) - - if the data contains an 'errors' array, pull out the first error document and - pass that to _load_from_json() - - log a warning on non-dict JSON data - """ - json_data = self.raw_json - if json_data is None: + + - on non-dict JSON data, log and abort early. Don't error since this + is already part of exception construction, just stop parsing. + + - Attempt to detect the error format in use, then dispatch to the relevant + subparser: JSON:API, Type Zero, or Undefined + + - if subparsing succeeded, call the `_post_parse_hook` hook to allow + subclasses to trivially add more computed attributes. This could also be + done by altering `__init__` but it's nicer to have a dedicated hook because + it is guaranteed to only be called if the rest of parsing succeeded + """ + if self.raw_json is None: log.debug("Error body was not parsed as JSON") - return - if not isinstance(json_data, dict): + return False + if not isinstance(self.raw_json, dict): log.warning( # type: ignore[unreachable] "Error body could not be parsed as JSON because it was not a dict" ) - return - - # if there appears to be a list of errors in the response data, grab the - # first error from that list for parsing - # this is only done if we determine that - # - 'errors' is present and is a non-empty list - # - 'errors[0]' is a dict - # - # this gracefully handles other uses of the key 'errors', e.g. as an int: - # {"message": "foo", "errors": 6} - if ( - isinstance(json_data.get("errors"), list) - and len(json_data["errors"]) > 0 - and isinstance(json_data["errors"][0], dict) + return False + + error_format = self._detect_error_format() + subparse_result: bool + if error_format == _ErrorFormat.jsonapi: + subparse_result = self._parse_jsonapi_error_format() + elif error_format == _ErrorFormat.type_zero: + subparse_result = self._parse_type_zero_error_format() + else: + subparse_result = self._parse_undefined_error_format() + + if not subparse_result: + return False + return self._post_parse_hook() + + def _post_parse_hook(self) -> bool: + """ + An internal callback for extra customizations after fully successful parsing. + + By default, does nothing. + """ + return True + + def _detect_error_format(self) -> _ErrorFormat: + # if the JSON:API mimetype was used, inspect the data to make sure it is + # well-formed + if self._jsonapi_mimetype(): + errors = self._dict_data.get("errors") + if not isinstance(errors, list): + return _ErrorFormat.undefined + elif len(errors) < 1: + return _ErrorFormat.undefined + elif not all(isinstance(error_doc, dict) for error_doc in errors): + return _ErrorFormat.undefined + + # only use 'jsonapi' if everything checked out + return _ErrorFormat.jsonapi + + # now evaluate attributes for Type Zero errors under the same paradigm + # check each attribute and only return 'type_zero' if nothing failed + + if not isinstance(self._dict_data.get("code"), str): + return _ErrorFormat.undefined + elif not isinstance(self._dict_data.get("message"), str): + return _ErrorFormat.undefined + # request_id is not required, but must be a string if present + elif "request_id" in self._dict_data and not isinstance( + self._dict_data["request_id"], str ): - # log a warning only when there is more than one error in the list - # if an API sends back an error of the form - # {"errors": [{"foo": "bar"}]} - # then the envelope doesn't matter and there's only one error to parse - if len(json_data["errors"]) != 1: - log.warning( - "Doing JSON load of error response with multiple " - "errors. Exception data will only include the " - "first error, but there are really %d errors", - len(json_data["errors"]), - ) - # try to grab the first error in the list, but also check - # if it isn't a dict - json_data = json_data["errors"][0] - self._load_from_json(json_data) - - def _load_from_json(self, data: dict[str, t.Any]) -> None: - # rewrite 'code' if present and correct type - if isinstance(data.get("code"), str): - self.code = data["code"] - - for f in self.MESSAGE_FIELDS: - if isinstance(data.get(f), str): - log.debug("Loaded message from '%s' field", f) - self.message = data[f] - break + return _ErrorFormat.undefined + + return _ErrorFormat.type_zero + + def _parse_jsonapi_error_format(self) -> bool: + """ + Parsing a JSON:API Error + + This is only called after the field type for 'errors' has been checked. + However, the nested/underlying fields will not have been checked yet. + """ + self.errors = [ErrorSubdocument(e) for e in self._dict_data["errors"]] + self.code = self._extract_code_from_error_array(self.errors) + self.messages = self._extract_messages_from_error_array(self.errors) + return True + + def _parse_type_zero_error_format(self) -> bool: + """ + Parsing a Type Zero Error + + This is only called after Type Zero has been detected. Therefore, we already + have assurances about the values in 'code' and 'message'. + Note that 'request_id' could be absent but *must* be a string if present. + """ + self.code = self._dict_data["code"] + self.messages = [self._dict_data["message"]] + self.request_id = self._dict_data.get("request_id") + if isinstance(self._dict_data.get("errors"), list) and all( + isinstance(subdoc, dict) for subdoc in self._dict_data["errors"] + ): + raw_errors = self._dict_data["errors"] else: - log.debug("No message found in parsed error body") + raw_errors = [self._dict_data] + self.errors = [ + ErrorSubdocument(e, message_fields=self.MESSAGE_FIELDS) for e in raw_errors + ] + return True + + def _parse_undefined_error_format(self) -> bool: + """ + Undefined Parsing: best effort support for unknown data shapes + + This is also a great place for custom parsing to hook in for different APIs + if we know that there's an unusual format in use + """ + + # attempt to pull out errors if possible and valid + if isinstance(self._dict_data.get("errors"), list) and all( + isinstance(subdoc, dict) for subdoc in self._dict_data["errors"] + ): + raw_errors = self._dict_data["errors"] + # if no 'errors' were found, or 'errors' is invalid, then + # 'errors' should be set to contain the root document + else: + raw_errors = [self._dict_data] + self.errors = [ + ErrorSubdocument(e, message_fields=self.MESSAGE_FIELDS) for e in raw_errors + ] + + # use 'code' if present and correct type + if isinstance(self._dict_data.get("code"), str): + self.code = t.cast(str, self._dict_data["code"]) + # otherwise, pull 'code' from the sub-errors array + elif self.errors: + # in undefined parse cases, the code will be left as `"Error"` for + # a higher degree of backwards compatibility + maybe_code = self._extract_code_from_error_array(self.errors) + if maybe_code is not None: + self.code = maybe_code + + # either there is an array of subdocument errors or there is not, + # in which case we load only from the root doc + self.messages = self._extract_messages_from_error_array( + self.errors + or [ErrorSubdocument(self._dict_data, message_fields=self.MESSAGE_FIELDS)] + ) + + return True + + def _extract_messages_from_error_array( + self, errors: list[ErrorSubdocument] + ) -> list[str]: + """ + Extract 'messages' from an array of errors (JSON:API or otherwise) + + Each subdocument *may* define its messages, so this is the aggregate of messages + from those documents which had messages. + + Note that subdocuments may be instructed about their `message_fields` by the + error class and parsing path which they take. Therefore, this may be extracting + `"message"`, `"detail"`, or `"title"` in the base implementation and other + fields if a subclass customizes this further. + """ + ret: list[str] = [] + for doc in errors: + if doc.message is not None: + ret.append(doc.message) + return ret + + def _extract_code_from_error_array( + self, errors: list[ErrorSubdocument] + ) -> str | None: + """ + Extract a 'code' field from an array of errors (JSON:API or otherwise) + + This is done by checking if each error document has the same 'code' + """ + codes: set[str] = set() + for error in errors: + if error.code is not None: + codes.add(error.code) + if len(codes) == 1: + return codes.pop() + return None + + +class ErrorSubdocument: + """ + Error subdocuments as returned by Globus APIs. + + :ivar dict raw: The unparsed error subdocument + """ + + # the default set of fields to use for message extraction, in order + # selected to match the fields defined by JSON:API by default + DEFAULT_MESSAGE_FIELDS: tuple[str, ...] = ("detail", "title") + + def __init__( + self, data: dict[str, t.Any], *, message_fields: t.Sequence[str] | None = None + ) -> None: + self.raw = data + self._message_fields: tuple[str, ...] + + if message_fields is not None: + self._message_fields = tuple(message_fields) + else: + self._message_fields = self.DEFAULT_MESSAGE_FIELDS + + @property + def message(self) -> str | None: + """ + The 'message' string of this subdocument, derived from its data based on the + parsing context. + + May be `None` if no message is defined. + """ + return _extract_message_from_dict(self.raw, self._message_fields) + + @property + def code(self) -> str | None: + """ + The 'code' string of this subdocument, derived from its data based on the + parsing context. + + May be `None` if no code is defined. + """ + if isinstance(self.raw.get("code"), str): + return t.cast(str, self.raw["code"]) + return None + + def get(self, key: str, default: t.Any = None) -> t.Any | None: + """ + A dict-like getter for the raw data. + """ + return self.raw.get(key, default) + + +def _extract_message_from_dict( + data: dict[str, t.Any], message_fields: tuple[str, ...] +) -> str | None: + """ + Extract a single message string from a dict if one is present. + """ + for f in message_fields: + if isinstance(data.get(f), str): + return t.cast(str, data[f]) + return None diff --git a/src/globus_sdk/services/auth/errors.py b/src/globus_sdk/services/auth/errors.py index 3d1d466ae..188284afa 100644 --- a/src/globus_sdk/services/auth/errors.py +++ b/src/globus_sdk/services/auth/errors.py @@ -10,8 +10,4 @@ class AuthAPIError(exc.GlobusAPIError): """ Error class for the API components of Globus Auth. - - Customizes message parsing to support the field named 'error'. """ - - MESSAGE_FIELDS = ["detail", "title"] diff --git a/src/globus_sdk/services/flows/errors.py b/src/globus_sdk/services/flows/errors.py index 58d0f10e1..e7ab08079 100644 --- a/src/globus_sdk/services/flows/errors.py +++ b/src/globus_sdk/services/flows/errors.py @@ -1,7 +1,39 @@ -from globus_sdk.exc import GlobusAPIError +from __future__ import annotations + +from globus_sdk.exc import ErrorSubdocument, GlobusAPIError class FlowsAPIError(GlobusAPIError): """ Error class to represent error responses from Flows. """ + + def _parse_undefined_error_format(self) -> bool: + """ + Treat any top-level "error" key as an "array of size 1". + Meaning that we'll see a single subdocument for data shaped like + { + "error": { + "foo": "bar" + } + } + """ + # if there is not a top-level 'error' key, no special behavior is defined + # fall-back to the base class implementation + if not isinstance(self._dict_data.get("error"), dict): + return super()._parse_undefined_error_format() + + self.errors = [ErrorSubdocument(self._dict_data["error"])] + self.code = self._extract_code_from_error_array(self.errors) + + details = self._dict_data["error"].get("detail") + if isinstance(details, list): + self.messages = [ + error_detail["msg"] + for error_detail in details + if isinstance(error_detail.get("msg"), str) + ] + else: + self.messages = self._extract_messages_from_error_array(self.errors) + + return True diff --git a/src/globus_sdk/services/gcs/errors.py b/src/globus_sdk/services/gcs/errors.py index cba57244b..c9dcdf975 100644 --- a/src/globus_sdk/services/gcs/errors.py +++ b/src/globus_sdk/services/gcs/errors.py @@ -26,10 +26,10 @@ def _get_args(self) -> list[t.Any]: args.append(self.detail) return args - def _load_from_json(self, data: dict[str, t.Any]) -> None: - super()._load_from_json(data) + def _post_parse_hook(self) -> bool: # detail can be a full document, so fetch, then look for a DATA_TYPE # and expose it as a top-level attribute for easy access - self.detail = data.get("detail") + self.detail = self._dict_data.get("detail") if isinstance(self.detail, dict) and "DATA_TYPE" in self.detail: self.detail_data_type = self.detail["DATA_TYPE"] + return True diff --git a/src/globus_sdk/services/search/errors.py b/src/globus_sdk/services/search/errors.py index 7bd2003bc..a3a710892 100644 --- a/src/globus_sdk/services/search/errors.py +++ b/src/globus_sdk/services/search/errors.py @@ -10,19 +10,16 @@ class SearchAPIError(exc.GlobusAPIError): """ Error class for the Search API client. In addition to the - inherited ``code`` and ``message`` instance variables, provides ``error_data``. + inherited instance variables, provides ``error_data``. :ivar error_data: Additional object returned in the error response. May be a dict, list, or None. """ - # the Search API always and only returns 'message' for string messages - MESSAGE_FIELDS = ["message"] - def __init__(self, r: requests.Response) -> None: - self.error_data = None + self.error_data: dict[str, t.Any] | None = None super().__init__(r) - def _load_from_json(self, data: dict[str, t.Any]) -> None: - super()._load_from_json(data) - self.error_data = data.get("error_data") + def _post_parse_hook(self) -> bool: + self.error_data = self._dict_data.get("error_data") + return True diff --git a/src/globus_sdk/services/timer/errors.py b/src/globus_sdk/services/timer/errors.py index c00c05a62..5b04bcb8a 100644 --- a/src/globus_sdk/services/timer/errors.py +++ b/src/globus_sdk/services/timer/errors.py @@ -2,7 +2,7 @@ import typing as t -from globus_sdk.exc import GlobusAPIError +from globus_sdk.exc import ErrorSubdocument, GlobusAPIError class TimerAPIError(GlobusAPIError): @@ -11,29 +11,20 @@ class TimerAPIError(GlobusAPIError): Has no particular additions to the base ``GlobusAPIError``, but implements a different method for parsing error responses from Timer due to the differences - between pydantic-generated errors and other errors. + between various error formats used. """ - MESSAGE_FIELDS: list[str] = [] # we are overriding `_load_from_json` instead - - def _load_from_json(self, data: dict[str, t.Any]) -> None: + def _parse_undefined_error_format(self) -> bool: """ - Errors generated by Timer itself look like this: - - .. code-block:: json - + Treat any top-level "error" key as an "array of size 1". + Meaning that we'll see a single subdocument for data shaped like { - "error": { - "code": "ERROR", - "detail": "Request failed successfully", - "status": 500 - } + "error": { + "foo": "bar" + } } - - While errors from Timer's pydantic implementation look like this: - - .. code-block:: json + Error shapes also include validation errors in a 'details' array: { "detail": [ @@ -46,22 +37,58 @@ def _load_from_json(self, data: dict[str, t.Any]) -> None: "loc": ["body", "callback_url"], "msg": "field required", "type": "value_error.missing" - }, - ... - - The ``msg`` might be like the above, or "value is not a valid ", and so - on. ``loc`` however is always a list which indicates the location of the error - within the request. It might say ``["body", "start"]`` for example, or it might - start with "query" to point to a value in the query string. + } + ] + } """ - if "error" in data: - self.code = data["error"].get("code", "ERROR") - self.message = data["error"].get("detail", "") - elif "detail" in data: + # if there is not a top-level 'error' key and no top-level + # 'detail' key, no special behavior is defined + # fall-back to the base class implementation + # but before that fallback, try the two relevant branches + + # if 'error' is present, use it to populate the errors array + # extract 'code' from it + # and extract 'messages' from it + if isinstance(self._dict_data.get("error"), dict): + self.errors = [ErrorSubdocument(self._dict_data["error"])] + self.code = self._extract_code_from_error_array(self.errors) + self.messages = self._extract_messages_from_error_array(self.errors) + return True + elif isinstance(self._dict_data.get("detail"), list): + # FIXME: + # the 'code' is currently being set explicitly by the + # SDK in this case even though none was provided by + # the service + # in a future version of the SDK, the code should be `None` self.code = "Validation Error" - self.message = "; ".join( - e["msg"] + ": " + ".".join(k for k in e["loc"]) for e in data["detail"] - ) + + # collect the errors array from details + self.errors = [ + ErrorSubdocument(d) + for d in self._dict_data["detail"] + if isinstance(d, dict) + ] + + # drop error objects which don't have the relevant fields + # and then build custom 'messages' for Globus Timers errors + details = list(_details_from_errors(self.errors)) + self.messages = [ + f"{e['msg']}: {'.'.join(k for k in e['loc'])}" for e in details + ] + return True else: - self.code = "Unknown Error" - self.message = "Could not parse error details from the response" + return super()._parse_undefined_error_format() + + +def _details_from_errors( + errors: list[ErrorSubdocument], +) -> t.Iterator[dict[str, t.Any]]: + for d in errors: + if not isinstance(d.get("msg"), str): + continue + loc_list = d.get("loc") + if not isinstance(loc_list, list): + continue + if not all(isinstance(path_item, str) for path_item in loc_list): + continue + yield d.raw diff --git a/src/globus_sdk/services/transfer/errors.py b/src/globus_sdk/services/transfer/errors.py index b13e56d6a..4dc1d825f 100644 --- a/src/globus_sdk/services/transfer/errors.py +++ b/src/globus_sdk/services/transfer/errors.py @@ -1,30 +1,9 @@ from __future__ import annotations -import typing as t - -import requests - from globus_sdk import exc class TransferAPIError(exc.GlobusAPIError): """ - Error class for the Transfer API client. In addition to the - inherited ``code`` and ``message`` instance variables, provides ``request_id``. - - :ivar request_id: Unique identifier for the request, which should be - provided when contacting support@globus.org. + Error class for the Transfer API client. """ - - def __init__(self, r: requests.Response) -> None: - self.request_id = None - super().__init__(r) - - def _get_args(self) -> list[t.Any]: - args = super()._get_args() - args.append(self.request_id) - return args - - def _load_from_json(self, data: dict[str, t.Any]) -> None: - super()._load_from_json(data) - self.request_id = data.get("request_id") diff --git a/tests/functional/services/flows/test_flow_crud.py b/tests/functional/services/flows/test_flow_crud.py index 5463681f2..e6dc08ff4 100644 --- a/tests/functional/services/flows/test_flow_crud.py +++ b/tests/functional/services/flows/test_flow_crud.py @@ -1,3 +1,6 @@ +import pytest + +from globus_sdk import FlowsAPIError from globus_sdk._testing import load_response @@ -8,6 +11,28 @@ def test_create_flow(flows_client): assert resp.data["title"] == "Multi Step Transfer" +def test_create_flow_error_parsing(flows_client): + metadata = load_response( + flows_client.create_flow, case="bad_admin_principal_error" + ).metadata + with pytest.raises(FlowsAPIError) as excinfo: + flows_client.create_flow(**metadata["params"]) + err = excinfo.value + assert err.code == "UNPROCESSABLE_ENTITY" + assert err.messages == [ + ( + 'Unrecognized principal string: "ae341a98-d274-11e5-b888-dbae3a8ba545". ' + 'Allowed principal types in role "FlowAdministrator": ' + "[, ]" + ), + ( + 'Unrecognized principal string: "4fab4345-6d20-43a0-9a25-16b2e3bbe765". ' + 'Allowed principal types in role "FlowAdministrator": ' + "[, ]" + ), + ] + + def test_get_flow(flows_client): meta = load_response(flows_client.get_flow).metadata resp = flows_client.get_flow(meta["flow_id"]) diff --git a/tests/functional/services/flows/test_run_flow.py b/tests/functional/services/flows/test_run_flow.py index e3966ad71..4f6dbd7c1 100644 --- a/tests/functional/services/flows/test_run_flow.py +++ b/tests/functional/services/flows/test_run_flow.py @@ -1,13 +1,33 @@ -import typing as t +from __future__ import annotations -from globus_sdk import SpecificFlowClient +import pytest + +from globus_sdk import FlowsAPIError, SpecificFlowClient from globus_sdk._testing import load_response -def test_run_flow(specific_flow_client_class: t.Type[SpecificFlowClient]): +def test_run_flow(specific_flow_client_class: type[SpecificFlowClient]): metadata = load_response(SpecificFlowClient.run_flow).metadata flow_client = specific_flow_client_class(flow_id=metadata["flow_id"]) resp = flow_client.run_flow(**metadata["request_params"]) assert resp.http_status == 200 + + +def test_run_flow_missing_scope(specific_flow_client_class: type[SpecificFlowClient]): + metadata = load_response( + SpecificFlowClient.run_flow, case="missing_scope_error" + ).metadata + + flow_client = specific_flow_client_class(flow_id=metadata["flow_id"]) + + with pytest.raises(FlowsAPIError) as excinfo: + flow_client.run_flow(**metadata["request_params"]) + + err = excinfo.value + assert err.http_status == 403 + assert err.code == "MISSING_SCOPE" + assert ( + err.message == "This action requires the following scope: frobulate[demuddle]" + ) diff --git a/tests/functional/services/timer/test_jobs.py b/tests/functional/services/timer/test_jobs.py index 40abd526f..bea69fc9e 100644 --- a/tests/functional/services/timer/test_jobs.py +++ b/tests/functional/services/timer/test_jobs.py @@ -3,7 +3,7 @@ import pytest -from globus_sdk import TimerClient, TimerJob, TransferData +from globus_sdk import TimerAPIError, TimerClient, TimerJob, TransferData from globus_sdk._testing import get_last_request, load_response from globus_sdk.config import get_service_url from globus_sdk.utils import slash_join @@ -29,6 +29,16 @@ def test_get_job(timer_client): assert response.data.get("job_id") == meta["job_id"] +def test_get_job_errors(timer_client): + meta = load_response(timer_client.get_job, case="simple_500_error").metadata + with pytest.raises(TimerAPIError) as excinfo: + timer_client.get_job(meta["job_id"]) + err = excinfo.value + assert err.http_status == 500 + assert err.code == "ERROR" + assert err.message == "Request failed terribly" + + @pytest.mark.parametrize("start", [datetime.utcnow(), "2022-04-05T06:00:00"]) @pytest.mark.parametrize( "interval", [timedelta(days=1), timedelta(minutes=60), 600, None] @@ -60,6 +70,22 @@ def test_create_job(timer_client, start, interval): ) +def test_create_job_validation_error(timer_client): + meta = load_response(timer_client.create_job, case="validation_error").metadata + transfer_data = TransferData( + source_endpoint=GO_EP1_ID, destination_endpoint=GO_EP2_ID + ) + timer_job = TimerJob.from_transfer_data(transfer_data, "2022-04-05T06:00:00", 1800) + + with pytest.raises(TimerAPIError) as excinfo: + timer_client.create_job(timer_job) + + err = excinfo.value + assert err.http_status == 422 + assert err.code == "Validation Error" + assert err.messages == meta["expect_messages"] + + def test_update_job(timer_client): meta = load_response(timer_client.update_job).metadata response = timer_client.update_job(meta["job_id"], {"name": meta["name"]}) diff --git a/tests/unit/errors/conftest.py b/tests/unit/errors/conftest.py index 89e774b7c..4458b7edc 100644 --- a/tests/unit/errors/conftest.py +++ b/tests/unit/errors/conftest.py @@ -63,13 +63,13 @@ def success_response(): @pytest.fixture def default_json_response(make_json_response): json_data = { + "code": "Json Error", "errors": [ { "message": "json error message", "title": "json error message", - "code": "Json Error", } - ] + ], } return make_json_response(json_data, 400) diff --git a/tests/unit/errors/test_auth_errors.py b/tests/unit/errors/test_auth_errors.py index 5b9a57e52..79a57a28f 100644 --- a/tests/unit/errors/test_auth_errors.py +++ b/tests/unit/errors/test_auth_errors.py @@ -15,7 +15,7 @@ def nested_auth_response(make_json_response): "errors": [ {"detail": "nested auth error message", "code": "Auth Error"}, { - "title": "some other error which will not be seen", + "title": "some secondary error", "code": "HiddenError", }, ] @@ -28,13 +28,18 @@ def nested_auth_response(make_json_response): ( # normal auth error data ("simple_auth_response", "404", "Error", "simple auth error message"), - ("nested_auth_response", "404", "Auth Error", "nested auth error message"), + ( + "nested_auth_response", + "404", + "Error", + "nested auth error message; some secondary error", + ), # wrong format (but still parseable) ("default_json_response", "400", "Json Error", "json error message"), # defaults for non-json data - ("default_text_response", "401", "Error", "error message"), + ("default_text_response", "401", "Error", "Unauthorized"), # malformed data is at least rendered successfully into an error - ("malformed_response", "403", "Error", "{"), + ("malformed_response", "403", "Error", "Forbidden"), ), ) def test_get_args_auth(request, response_fixture_name, status, code, message): diff --git a/tests/unit/errors/test_common_functionality.py b/tests/unit/errors/test_common_functionality.py index efac7cf0a..3a503a667 100644 --- a/tests/unit/errors/test_common_functionality.py +++ b/tests/unit/errors/test_common_functionality.py @@ -4,7 +4,7 @@ import pytest import requests -from globus_sdk import GlobusAPIError, RemovedInV4Warning, exc +from globus_sdk import ErrorSubdocument, GlobusAPIError, RemovedInV4Warning, exc def _strmatch_any_order(inputstr, prefix, midfixes, suffix, sep=", "): @@ -70,7 +70,7 @@ def test_get_args(default_json_response, default_text_response, malformed_respon None, "401", "Error", - "error message", + "Unauthorized", ] err = GlobusAPIError(malformed_response.r) assert err._get_args() == [ @@ -79,7 +79,7 @@ def test_get_args(default_json_response, default_text_response, malformed_respon None, "403", "Error", - "{", + "Forbidden", ] @@ -92,7 +92,7 @@ def test_get_args_on_unknown_json(make_json_response): None, "400", "Error", - '{"foo": "bar"}', + "Bad Request", ] @@ -105,7 +105,7 @@ def test_get_args_on_non_dict_json(make_json_response): None, "400", "Error", - '["foo", "bar"]', + "Bad Request", ] @@ -369,7 +369,7 @@ def test_http_header_exposure(make_response): "bearer", ), ("PATCH", 500, None, "https://bogus.example.org/bar", "", "unknown-token"), - ("PUT", 501, None, "https://bogus.example.org/bar", None, None), + ("PUT", 501, None, "https://bogus.example.org/bar", "Not Implemented", None), ], ) def test_error_repr_has_expected_info( @@ -429,3 +429,188 @@ def test_error_repr_has_expected_info( assert error_message in stringified else: assert http_reason in stringified + + +@pytest.mark.parametrize( + "content_type", + ("application/json", "application/unknown+json", "application/vnd.api+json"), +) +def test_loads_jsonapi_error_subdocuments(make_response, content_type): + res = make_response( + { + "errors": [ + { + "code": "TooShort", + "title": "Password data too short", + "detail": "password was only 3 chars long, must be at least 8", + }, + { + "code": "MissingSpecial", + "title": "Password data missing special chars", + "detail": "password must have non-alphanumeric characters", + }, + { + "code": "ContainsCommonDogName", + "title": "Password data has a popular dog name", + "detail": "password cannot contain 'spot', 'woofy', or 'clifford'", + }, + ] + }, + 422, + data_transform=json.dumps, + headers={"Content-Type": content_type}, + ) + err = GlobusAPIError(res.r) + + # code is not taken from any of the subdocuments (inherently too ambiguous) + # behavior will depend on which parsing path was taken + if content_type.endswith("vnd.api+json"): + # code becomes None because we saw "true" JSON:API and can opt-in to + # better behavior + assert err.code is None + else: + # code remains 'Error' for backwards compatibility in the non-JSON:API case + assert err.code == "Error" + + # but messages can be extracted, and they prefer detail to title + assert err.messages == [ + "password was only 3 chars long, must be at least 8", + "password must have non-alphanumeric characters", + "password cannot contain 'spot', 'woofy', or 'clifford'", + ] + + +@pytest.mark.parametrize( + "content_type", + ("application/json", "application/unknown+json", "application/vnd.api+json"), +) +def test_loads_jsonapi_error_subdocuments_with_common_code(make_response, content_type): + res = make_response( + { + "errors": [ + { + "code": "MissingClass", + "title": "Must contain capital letter", + "detail": "password must contain at least one capital letter", + }, + { + "code": "MissingClass", + "title": "Must contain special chars", + "detail": "password must have non-alphanumeric characters", + }, + ] + }, + 422, + data_transform=json.dumps, + headers={"Content-Type": content_type}, + ) + err = GlobusAPIError(res.r) + # code is taken because all subdocuments have the same code + assert err.code == "MissingClass" + + +@pytest.mark.parametrize( + "content_type", + ("application/json", "application/unknown+json", "application/vnd.api+json"), +) +def test_loads_jsonapi_error_messages_from_various_fields(make_response, content_type): + res = make_response( + { + "errors": [ + { + "message": "invalid password value", + }, + { + "title": "Must contain capital letter", + }, + { + "detail": "password must have non-alphanumeric characters", + }, + ] + }, + 422, + data_transform=json.dumps, + headers={"Content-Type": content_type}, + ) + err = GlobusAPIError(res.r) + # messages are extracted, and they use whichever field is appropriate for + # each sub-error + # note that 'message' will *not* be extracted if the Content-Type indicated JSON:API + # because JSON:API does not define such a field + if content_type.endswith("vnd.api+json"): + # code becomes None because we saw "true" JSON:API and can opt-in to + # better behavior + assert err.code is None + assert err.messages == [ + "Must contain capital letter", + "password must have non-alphanumeric characters", + ] + else: + # code remains 'Error' for backwards compatibility in the non-JSON:API case + assert err.code == "Error" + + assert err.messages == [ + "invalid password value", + "Must contain capital letter", + "password must have non-alphanumeric characters", + ] + + +@pytest.mark.parametrize( + "error_doc", + ( + # Type Zero Error Format + {"code": "FooCode", "message": "FooMessage"}, + # Undefined Error Format + {"message": "FooMessage"}, + ), +) +def test_non_jsonapi_parsing_uses_root_as_errors_array_by_default( + make_response, error_doc +): + res = make_response( + error_doc, + 422, + data_transform=json.dumps, + headers={"Content-Type": "application/json"}, + ) + err = GlobusAPIError(res.r) + + # errors is the doc root wrapped in a list, but converted to a subdocument error + assert len(err.errors) == 1 + subdoc = err.errors[0] + assert isinstance(subdoc, ErrorSubdocument) + assert subdoc.raw == error_doc + # note that 'message' is supported for error message extraction + # vs 'detail' and 'title' for JSON:API data + assert subdoc.message == error_doc["message"] + + +@pytest.mark.parametrize( + "error_doc", + ( + # Type Zero Error Format with sub-error data + {"code": "FooCode", "message": "FooMessage", "errors": [{"bar": "baz"}]}, + # Type Zero Error Format with *empty* sub-error data + {"code": "FooCode", "message": "FooMessage", "errors": []}, + # Undefined Error Format with sub-error data + {"message": "FooMessage", "errors": [{"bar": "baz"}]}, + # Undefined Error Format with *empty* sub-error data + {"message": "FooMessage", "errors": []}, + ), +) +def test_non_jsonapi_parsing_uses_errors_array_if_present(make_response, error_doc): + res = make_response( + error_doc, + 422, + data_transform=json.dumps, + headers={"Content-Type": "application/json"}, + ) + err = GlobusAPIError(res.r) + + # errors is the 'errors' list converted to error subdocs + # first some sanity checks... + assert len(err.errors) == len(error_doc["errors"]) + assert all(isinstance(subdoc, ErrorSubdocument) for subdoc in err.errors) + # ...and then a true equivalence test + assert [e.raw for e in err.errors] == error_doc["errors"] diff --git a/tests/unit/errors/test_timer_errors.py b/tests/unit/errors/test_timer_errors.py index 8581dff26..d2b62038c 100644 --- a/tests/unit/errors/test_timer_errors.py +++ b/tests/unit/errors/test_timer_errors.py @@ -36,9 +36,9 @@ def test_timer_error_load_nested(make_json_response): assert err.message == "field required: body.start; field required: body.end" -def test_timer_error_load_unrecognize_format(make_json_response): +def test_timer_error_load_unrecognized_format(make_json_response): response = make_json_response({}, 400) err = TimerAPIError(response.r) - assert err.code == "Unknown Error" - assert err.message == "Could not parse error details from the response" + assert err.code == "Error" + assert err.message is None diff --git a/tests/unit/errors/test_transfer_errors.py b/tests/unit/errors/test_transfer_errors.py index fb1e1a52a..55a5183e9 100644 --- a/tests/unit/errors/test_transfer_errors.py +++ b/tests/unit/errors/test_transfer_errors.py @@ -8,7 +8,7 @@ def transfer_response(make_json_response): transfer_data = { "message": "transfer error message", "code": "Transfer Error", - "request_id": 123, + "request_id": "123", } return make_json_response(transfer_data, 404) @@ -17,13 +17,13 @@ def transfer_response(make_json_response): "response_fixture_name, status, code, message, request_id", ( # normal transfer error data - ("transfer_response", "404", "Transfer Error", "transfer error message", 123), + ("transfer_response", "404", "Transfer Error", "transfer error message", "123"), # wrong format (but still parseable) ("default_json_response", "400", "Json Error", "json error message", None), # defaults for non-json data - ("default_text_response", "401", "Error", "error message", None), + ("default_text_response", "401", "Error", "Unauthorized", None), # malformed data is at least rendered successfully into an error - ("malformed_response", "403", "Error", "{", None), + ("malformed_response", "403", "Error", "Forbidden", None), ), ) def test_get_args_transfer( @@ -31,12 +31,14 @@ def test_get_args_transfer( ): response = request.getfixturevalue(response_fixture_name) err = TransferAPIError(response.r) - assert err._get_args() == [ + expected_args = [ response.method, response.url, None, status, code, message, - request_id, ] + if request_id is not None: + expected_args.append(request_id) + assert err._get_args() == expected_args