[CONTINT-685] - initial tests for validation of OpenApi request

[azakrzewski-hy]

CONTINT-685

Initial tests for OpenApi request validation.
This PR does not contain final version of the work needed for the ticket.
If you have any advice / idea what could be the best way to cover the scope just let me know in commands.

Test is not finished yet:
testRequestToIngestionEvents()

it fails as string is not successfully validated agains OpenApi specification
expectedBody string is exactly the same as in CreateRequestIntegrationTest where we use it as the reference body

[krdabrowski]

I'm not sure if we should go this path - I'm not in favor of adding another static (json or yaml) requests - I think we should introduce some Java DTOs and use them. With introduction of these DTOs and new Mappers we could get rid of UpdateNodeEventSerializer, DeleteNodeEventSerializer and some static requests from our tests - WDYT @tpage-alfresco and @Pawel-608 ?

[tpage-alfresco]

The idea here is just to extract the static json from e.g. CreateRequestIntegrationTest and put it in a file where two tests can access it. I think what you're suggesting is to combine the tests so that we validate the output from the Connector directly against the remote schema, but I'm concerned that if the test starts failing then we won't know if it's due to our code changes or changes in the remote schema.

[tpage-alfresco]

Additionally I think we want to validate the exact output of the Connector (rather than just its validity), because otherwise we would pass tests by e.g. only producing delete requests.

[krdabrowski]

Ok, I get your point - the idea is to perform validation in two steps:

1. verify if connector output meats "our requirements",
2. verify if "our requirements" comply with OpenAPI spec

One downside what I see here is: maintain complexity - if someone will forget to update this static yaml request we might end up in situation where Connector will be sending not valid requests and we might notice it quite late

[tpage-alfresco]

Yes - I agree. It's important that all static requests are also included in e2e tests so that we know they can be produced by the application. I can't think of a neat way to verify that.

[tpage-alfresco]

I made a suggestion of how we could ensure we test all requests in the folder against the schema here: #767 (comment)

[krdabrowski]

I think, that we could leave CreateRequestIntegrationTest and others usecase.e2e tests as they are and introduce new test verifying if our code complies with OpenAPI spec. If only this new one test would fail, it would mean, that the spec has changed. While when more tests will start to fail, then someone messed something up in the code.
Our Endpoint-to-Endpoint tests are very handy and bring a lot of value presenting in a simple way what will be the outcome for different repo events, so we shouldn't change them.

[tpage-alfresco]

We are very close to a generic test here. If we add in a check based on whether the request contains a properties field then we can do something like:

for (requestFile : expected-hxinsight-requests folder)
{
    HxInsightRequest hxInsightRequest = RequestLoader.load(requestFile);

    Request request = makeRequest(hxInsightRequest);

    assertThat(classUnderTest.validateRequest(request).getMessages()).isEmpty();
    if (hxInsightRequest.hasField("properties")
    {
        validatePropertiesField(hxInsightRequest.body(), propertiesSchema);
    }
}

Then we never need to update this file and we automatically check any new requests stored in the expected requests folder.

[azakrzewski-hy]

It could be done in separate pr

[tpage-alfresco]

For this method of testing to be valid we need to ensure that we're also checking the application can output these requests. We are doing this for create-document-request.yml in CreateRequestIntegrationTest.java, but I don't think we're doing it for the other three yml files.

[azakrzewski-hy]

I've updated testUpdateRequest() in UpdateRequestIntegrationTest.java class with "/expected-hxinsight-requests/update-document-request.yml". In UpdateRequestIntegrationTest.java there are test cases which use different request body - we do not use those in OpenApi tests. DeleteRequestIntegrationTest.java class was updated with "/expected-hxinsight-requests/delete-document-request.yml". There is no body for PresignedUrls case so no need for update there.

[krdabrowski]

Please put this declaration in parent pom's dependenyManagemnt section and specify here only:

<dependency>
            <groupId>com.atlassian.oai</groupId>
            <artifactId>swagger-request-validator-core</artifactId>
            <scope>test</scope>
</dependency>

[krdabrowski]

Files could be a bit different organized, like:

resources
|_ rest
  |_ hxinsight
     |_ requests
        |_ create-document.yml
        |_ create-document-without-file.yml
        .
        .
        .
     |_ responses
        |_ sth.yml

WDYT @azakrzewski-hy ?