Skip to content

1658 lines of validator exceptions differ mostly by two values #3389

Description

@adamtheturtle

What happens

src/mock_vws/_services_validators/exceptions.py is 889 lines for 23 exception classes, and src/mock_vws/_query_validators/exceptions.py is 769 lines for 21. That is 1658 lines and 44 classes, averaging 38 lines each, and almost all of it is the same three steps repeated:

        super().__init__()
        self.status_code = HTTPStatus.NOT_FOUND
        body = {
            "transaction_id": uuid.uuid4().hex,
            "result_code": ResultCodes.UNKNOWN_TARGET.value,
        }
        self.response_text = json_dump(body=body)
        date = email.utils.formatdate(timeval=None, localtime=False, usegmt=True)
        self.headers = {
            "Connection": "keep-alive",
            "Content-Type": "application/json",
            "server": "envoy",
            "Date": date,
            "x-envoy-upstream-service-time": "5",
            "Content-Length": str(object=len(self.response_text)),
            "strict-transport-security": "max-age=31536000",
            "x-aws-region": "us-east-2, us-west-2",
            "x-content-type-options": "nosniff",
        }

Grouping the classes by which headers they set:

_services_validators/exceptions.py: 23 classes
   20 classes: Connection, Content-Length, application/json, x-aws-region
    1 class:   Connection, Content-Length, text/plain
    1 class:   Connection, Content-Length, text/html
    1 class:   Content-Length

_query_validators/exceptions.py: 21 classes
    8 classes: Connection, Content-Length, application/json
    4 classes: Connection, Content-Length
    3 classes: Connection, Content-Length, WWW-Authenticate, text/plain
    2 classes: Connection, Content-Length, text/plain
    2 classes: Connection, Content-Length, WWW-Authenticate, application/json
    1 class:   Connection, Content-Length, text/html
    1 class:   Cache-Control, Connection, Content-Length, text/html

So 20 of the 23 services exceptions differ from each other in exactly two values: the status code and the result code. The query exceptions vary more, but still fall into a handful of shapes.

Why it matters

This is where header changes have to be made, and it is why they are expensive. #3365 opens by noting "There are 39 occurrences across src/" of the x-aws-region value, and most of those are here. Whatever is decided in that issue is a 39-site edit against the current structure and a one-line edit against a factored one. The same applies to server: envoy, x-envoy-upstream-service-time and strict-transport-security, which are equally hardcoded and equally likely to need revisiting.

The repetition also hides the interesting information. What is worth reading about UnknownTargetError is "404, UnknownTarget". That fact is currently surrounded by 30 lines which are identical to the previous class.

Suggested resolution

Move the response construction into the ValidatorError base class, so that the common case becomes a declaration:

class UnknownTargetError(ValidatorError):
    """..."""
    status_code = HTTPStatus.NOT_FOUND
    result_code = ResultCodes.UNKNOWN_TARGET

with the base building transaction_id, response_text and the standard header set, and hooks for the minority of classes which need a different content type, an extra header such as WWW-Authenticate, or a non-JSON body.

The two modules should keep their own base, since the services and query APIs genuinely have different standard header sets — the query API responses do not carry x-aws-region, for instance. Sharing the Date, Content-Length and content-type logic between the two bases is worthwhile; forcing one header set on both is not.

This is a pure refactor with no behaviour change, so it is well covered by the existing tests: any drift in a status code, result code or header would fail them.

Worth doing before #3365 rather than after.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions