Validation callback supresses receive HTTP message actions

[Syllero]

Citrus Version 2.7.6

Expected behavior In the java DSL, when setting e.g. status, in HttpClientReceiveActionBuilder, the value should be validated once the action is executed, even if a validation callback is added.

Actual behavior When adding a validation callback, values that are validated via the "controlMessage"/MessageValidator mechanism in the ReceiveMessageAction class are skipped.

Test case sample The following testcase should fail as the mocked response is returned with INTERNAL_SERVER_ERROR status, but the test expects OK. However, it will not fail.

    @Test
    public void testShouldFail() {
        reset(httpClient, messageConsumer, configuration);
        when(httpClient.createConsumer()).thenReturn(messageConsumer);
        when(httpClient.getEndpointConfiguration()).thenReturn(configuration);
        when(httpClient.getActor()).thenReturn(null);
        when(messageConsumer.receive(any(TestContext.class), anyLong())).thenReturn(new HttpMessage("<TestRequest><Message>Hello World!</Message></TestRequest>")
                .method(HttpMethod.GET)
                .uri("/test")
                .status(HttpStatus.INTERNAL_SERVER_ERROR)
                .version("HTTP/1.1"));
        new MockTestRunner(getClass().getSimpleName(), applicationContext, context) {
            @Override
            public void execute() {
                http(action -> action.client(httpClient)
                        .receive()
                        .response(HttpStatus.OK)
                        .version("HTTP/1.1")
                        .validationCallback(((message, ctx) -> Assert.assertEquals(message.getPayload(String.class), "<TestRequest><Message>Hello World!</Message></TestRequest>"))));
            }
        };
    }

[svettwer]

Hi!

You're right, currently the validation callback is substituting the validation logic of the ReceiveMessageAction.

You'd have to add the header validation by yourself in the callback.

public class TodoListIT extends TestNGCitrusTestRunner {

    @Autowired
    private HttpClient httpClient;

    @Autowired
    private HttpServer httpServer;

    @Test
    @CitrusTest
    public void testGet() {
        http(action -> action
                .client(httpClient)
                .send()
                .get("")
                .fork(true));

        http(action ->action
                .server(httpServer)
                .receive()
                .get());

        http(action -> action
                .server(httpServer)
                .respond()
                .version("HTTP/1.1")
                .payload("<TestRequest><Message>Hello World!</Message></TestRequest>"));

        http(action -> action.client(httpClient)
                .receive()
                .response()
                .validationCallback(fancyValidationCallback()));
    }

    private AbstractValidationCallback<String> fancyValidationCallback() {
        return new AbstractValidationCallback<String>() {
            @Override
            public void validate(final String payload, final Map<String, Object> headers, final TestContext context) {
                assertEquals(payload, "<TestRequest><Message>Hello World!</Message></TestRequest>");
                assertEquals(headers.get(HttpMessageHeaders.HTTP_VERSION), "HTTP/1.0");
            }
        };
    }
}

This test will fail because of the HTTP version. I've replaced the status code validation because of #616.

As the documentation mentions the validation callback as a "Callback able to additionally validate received message", we have two tasks here in my opinion.

1. Improve the documentation to point out the substitution. (I'll open a issue for this)
2. Discuss whether a validation callback should be an addition or a substitution in general.

If we agree that a validation callback should be an addition, we would have to check whether this might break current validation callback implementations. If it does, this is something to change in a major version update. I'd have to think a little bit more about this just to be sure.

BR, Sven

[Syllero]

Hello,

Thanks for the reply. I completely agree that this needs to be documented for the current version. I also understand, and respect the decision, that the header verification can/should be performed in the validation callback if one is used.

However, I am also of the opinion, that if the validation callback is a replacement, the action builder should not allow a validation callback to be set after calling header/body verification methods on it. The current behavior can easily mislead into believing that certain validation is done, while in fact it is not.

This particular false positive test scenario is very easy to fall into. Especially if more advanced validation (beyond XPath, JSONPath) is needed, as it is in my work place. In my opinion, these types of issues should be prevented at compilation, by having the builders prevent mis-configuration of the validation. E.g. any method that add validation on a message, should not remove validation that has previously been configured.

I totaly understand that this sort of feature would be breaking the existing API, and that it is not in scope until a major version upgrade. However, if this is something that is wanted in the long run, I can perhaps start looking into the issue.

Best regards, Alex