[uss_qualifier] fragments+validation code for operational intent references (CRUD & synchronization)

[Shastick]

This adds test step fragments related to Op Intent References in the context of DSS0210,2 (OIR synchronization)

These will be used for the actual scenario implementation being added in #530

[BenjaminPelletier]

Is this check checking format? It seems like format is checked in the check after this one

[Shastick]

This check covers both format and content (meaning, that the OIR's fields correspond to what was requested).

The check below will cover only the format, as specified by the OpenAPI spec.

This is due to the way the OIRValidator is implemented: its constructor takes a main_check (in this case the one from line 10), and the one from line 15 (Create operational intent reference response format conforms to spec) is one of the checks run by the validator.

#530 shows how these are used, notably here

The structure of this file is inherited from the structure I picked for the subscription fragments(*): some scenarios may elect to only check the format, and not the content, of responses (thus not requiring the whole validation fragment). This is why the check for conformance with the OpenAPI spec is here, and not in one of the fragments present in the validate folder.

I could update the wording to make it clear that both checks focus on different things (and would welcome suggestions to that effect)

(*): the subscription fragments suffer from the same redundancy in some checks, I'll do a cleanup pass in an upcoming PR

[BenjaminPelletier]

If we have two logicals checks A ("make sure the format of the response is correct") and B ("make sure the content of the response is correct"), we should declare those checks as A and B rather than A+B and A even if it makes the code slightly less convenient (though clear encapsulation like that should usually force cleaner code).

The primary purpose of test documentation is to serve as the interface between InterUSS developers writing the tests and users consuming the tests (especially regulators understanding the tests to verify coverage and USSs understanding how a failure came about), so the documentation should be able to stand alone regardless of exactly how the implementation works (or whether one exists yet). Regardless of implementation, it doesn't seem to make sense to check the format of the response in the check following this one after the format of the response was already checked in this check.

some scenarios may elect to only check the format, and not the content, of responses (thus not requiring the whole validation fragment)

That's probably something we want to address when possible -- if a test step will never perform a check, that check should not be documented for that test step. Linking to a fragment includes the fragment's contents in the test step. Note that fragments are kind of like documentary subroutines and while it often makes sense to have them map to programmatic subroutines, there is no need for a 1:1 correspondence there. Instead, the primary goal of documentation is to be maximally readable and intelligible.

I could update the wording to make it clear that both checks focus on different things (and would welcome suggestions to that effect)

The check following this one seems pretty clear -- the format is checked according to the OpenAPI spec. It seems like this check could apply only to the content (does the content match what we put in), and in that case, I would expect it to be checked following the format check (can't check the content of an improperly-formatted response). The actual implementation could still go about the checks in different phases; e.g.,:

1. Decode response to ImplicitDict object; upon decoding failure, indicate format check failure and skip following two checks
2. Check content is as was provided
3. Perform rigorous format validation against OpenAPI (failure attributed to same check as phase 1)

The documentation for a content-only check would presumably be something like

The operational intent reference created is expected to have content which reflects the creation request. If the content of the response does not match the request, the DSS is failing to implement astm.f3548.v21.DSS0005,1

[Shastick]

The check following this one seems pretty clear -- the format is checked according to the OpenAPI spec. It seems like this check could apply only to the content (does the content match what we put in), and in that case, I would expect it to be checked following the format check (can't check the content of an improperly-formatted response)

That makes sense. I'll reword things so that the check of line 10 (and any equivalents in other fragments) becomes about the content only.

some scenarios may elect to only check the format, and not the content, of responses (thus not requiring the whole validation fragment)

That's probably something we want to address when possible [...]

I believe that is currently limited, but I'll make sure to avoid that in the scenarios I currently work on: the solution is probably to either accept some duplication of checks in Markdown files, or have a few more very small fragments.

[BenjaminPelletier]

It seems like this check could apply only to the content (does the content match what we put in), and in that case, I would expect it to be checked following the format check (can't check the content of an improperly-formatted response).

The documentation is still indicating that content is going to be checked before format, but it seems like we can't check whether the values (content) are as expected before we determine that we actually have values that we can check (format is correct). I tried to find where the "Create operational intent reference response content is correct" check is performed in code (to see if it's before or after the "Create operational intent reference response format conforms to spec" check) and I don't see it in this PR -- will it be added in a separate PR?

[Shastick]

My bad, I did not switch the order of the checks (that's now corrected)

Code using the fragments is in #530 (I probably chose the wrong way to slice these PRs :/)

[BenjaminPelletier]

It seems like the previous check checks the body of the response; in what way is this check different than the previous one?

[Shastick]

The check from line 21 is redundant (and equivalent in its intention) to the check on line 10: I'll remove it. See my other answer for more context.

[BenjaminPelletier]

Is the format checked in this check? It seems like that check is performed in the following check

[Shastick]

Same situation as for the comment on create.md

[BenjaminPelletier]

Is the format checked in this check? It seems like that check is performed in the following check

[Shastick]

Same situation as for the comment on create.md

[BenjaminPelletier]

If new_oir doesn't have a valid response format, line 314 will nonetheless return (since the check is only Medium severity), so parsing it as an ImplicitDict below could raise a TypeError

[Shastick]

Good catch.

[Shastick]

Wording updated, removed redundant checks and code fix, which have been tested locally on #530 (that PR has been updated as well)

[BenjaminPelletier]

I don't think this PR needs to be reworked, but in the future, the best way to make small PRs is to add small but fully-exercised pieces of functionality in each PR. In this PR, many functions are created but then not called so the content of this PR is not tested by the CI. A better split, for instance, would have been to complete all the create stuff (and perhaps some or all the delete stuff) and move all of the read and update stuff to a later PR. Or, if a PR with all create and delete would be too big, a PR with just create and delete documentation would be great, followed by the implementation of either all that stuff, or just the basic stuff with the remainder of the create and delete in a follow-up PR.

Small PRs have a number of advantages -- in this case specifically, for instance, there is the question of order of checks regarding format and content of responses, and what the severity of the format check should be (since some format errors preclude content checks). Since this is effectively a breadth-first PR rather than a depth-first PR, we end up needing to check that discussion in 4 different places instead of 1 or 2, and changes need to be made in 4 different places rather than 1 or 2.

[BenjaminPelletier]

Since the format check is medium severity and therefore does not stop test execution, we don't know if this will succeed (it may throw a QueryError). If the caller of validate_searched_oir needs to handle certain expected exceptions, they need to be documented in the method docstring. But, it seems like we should just make the format check stop execution if it fails. Alternately, we could catch QueryError here and return early if we can't parse into something that can have its content checked.

[Shastick]

I think it makes most sense to move the severity of this to high, the current severity is a leftover from when the check was done last (and where, if the scenario had reached it without failing, it medium severity might have made more sense)

I'll adapt this for all crud fragments.

[BenjaminPelletier]

It looks like this check is performed before the previous check. To have the documentation better match the procedure, this check should be declared before the previous check. That also improves readability since we can't really check content (whether the op intent is in the search results) if we don't have valid content to check (the response doesn't contain valid search results).

This comment may also apply to the other CRUD operations.

[BenjaminPelletier]

Certainly failing this check is high severity since none of the following checks can be performed if this check fails. However, why are any of the checks following this one high severity? It seems like the test can easily proceed if any of the other checks in this fragment fail