OpIntentValidator raises exception before recording query

[BenjaminPelletier]

report.json is not attached because
s2_redacted.html attached instead. Codebase version used for test was 51ef122.
s2_redacted.html.zip

Test check
None; scenario effectively crashes when it is not at a check.

Difference from expected behavior
uss_qualifier should never have exceptions to report to the user under nominal circumstances. If exceptions are raised under expected conditions (e.g., raising QueryError), they should be caught and applied to a check or normal flow control logic. In this case, failure to do so made it so the query critical to identifying and resolving the problem (the call to get op intent details from the USS under test) was not recoded in the report.

Additional context
This also affects OpIntentValidator, so we probably want to finish merging #428 before addressing this issue.

[mickmis]

Indeed this crash is not good, and I believe it may happen in many different spots of the codebase if I'm not mistaken: every time we do a query, try to parse it with ImplicitDict.parse, and that we do not catch the exceptions raised, a crash may occur if the response does not match the ImplicitDict model.

Fixing this one issue would be easy, but individually handling the exceptions at callsites might be tedious and error-prone.
I'm thinking about adding something like this in monitoring/monitorlib/fetch/__init__.py:

ResponseType = TypeVar('ResponseType', bound=ImplicitDict)


def query_describe_and_parse(
    client: Optional[infrastructure.UTMClientSession],
    verb: str,
    url: str,
    parse_type: Type[ResponseType],
    query_type: Optional[QueryType] = None,
    participant_id: Optional[str] = None,
    **kwargs,
) -> Tuple[Optional[ResponseType], Query]:
    query = query_and_describe(client, verb, url, query_type, participant_id, **kwargs)
    try:
        parsed = parse_type(
            ImplicitDict.parse(
                query.response.json, parse_type
            )
        )
        return parsed, query
    except (ValueError, TypeError):
        return None, query

And everywhere in the codebase where we currently do a fetch_and_describe followed by a ImplicitDict.parse we use this query_describe_and_parse instead.

What do you think about such a solution?

[mickmis]

When all the PRs that mentioned this issue are merged (at the time of writing this #501, 502 and #503 are still open) I think this issue can be closed as those changes ensure that queries are recorded when some functions of DSSInstance are used.
Now the same issue is very probably present in other parts of the codebase as the error handling for querying USSes/DSS is quite inconsistent.
I believe the approach I used in DSSInstance enables flexibility in the error handling while being able to ensure queries are recorder, at the price of a slightly heavier call site. It could probably be extended to other calls but I'm not sure of what kind of priorities this would have now.

[BenjaminPelletier]

All mentioned PRs are merged. We'll keep in mind error resilience as we develop and improve, but I believe this specific issue can be closed.