mock-server / mockserver

MockServer enables easy mocking of any system you integrate with via HTTP or HTTPS with clients written in Java, JavaScript and Ruby. MockServer also includes a proxy that introspects all proxied traffic including encrypted SSL traffic and supports Port Forwarding, Web Proxying (i.e. HTTP proxy), HTTPS Tunneling Proxying (using HTTP CONNECT) and SOCKS Proxying (i.e. dynamic port forwarding).
http://mock-server.com
Apache License 2.0
4.54k stars 1.06k forks source link

MockServerClient::retrieveRecordedRequests returning RequestDefinition instead of HttpRequest #939

Closed pnsantos closed 2 years ago

pnsantos commented 3 years ago

Describe the issue

Between 5.10 and 5.11 (https://github.com/mock-server/mockserver/commit/b17f09796d6e645bce603deb02e7603977a35ace#diff-9ef4b74a33d39e5857ebb8bc5b9b5e2776b6db042587cd523a47d6062e95e39c) there seems to have been a change in several method signatures of MockServerClient that kinda broke the API...

Previously the retrieveRecordedRequest returned org.mockserver.model.HttpRequest but in 5.11.x the return type was changed to org.mockserver.model.RequestDefinition (this class doesn't give any access to the request data...)

The example on the website at https://www.mock-server.com/proxy/record_and_replay.html still shows this:

HttpRequest[] recordedRequests = new MockServerClient("localhost", 1080)
    .retrieveRecordedRequests(
        request()
            .withPath("/some/path")
            .withMethod("POST")
    );

However the return type in 5.11 is now RequestDefinition. Are we suppose to cast it to HttpRequest? (I tried and it works but seems a bit awkward...)

What you are trying to do

Trying to retrieve a recorded request

MockServer version

5.11.2

To Reproduce

Expected behaviour

MockServer Log

JamJar00 commented 3 years ago

Would love to see an official response to this. There's hardly any information in RequestDefinition so it's impossible to assert on the requests made without a cast to HttpRequest which isn't type safe.

Mockserver is supposed to follow SemVer but this is a breaking change released as a minor version, how has this happened?

I guess we're sticking on 5.10.0

davidvuletas commented 3 years ago

Hello, any official response about this ?

javiertoja commented 3 years ago

I faced today this issue breaking all our test, we will stick to 5.10.0, until a solution, or an official response if there is any

pjroth commented 2 years ago

We have the same issue where this breaking change was made and I'm not sure how/if I can update our code calling retrieveRecordedRequests to use this new version. Any advice would be appreciated.

Veske commented 2 years ago

What is the progress on this ?

hustlerman commented 2 years ago

Adding my voice here, this needs to be resolved since it's a breaking change; I wouldn't care as much about following sem-ver if there was at least some documentation about how to accommodate this.

EDIT: Btw, casting for me is not sufficient. Tests throw a ClassCastException when trying to do this.

pjroth commented 2 years ago

I figured out one workaround for this issue. Here is the code. You have to be sure to cast each element of the array, not the full array (which was my initial failure when I posed previously). This works for us:

Optional<HttpRequest[]> result = Optional.of(Arrays.stream(mockServerClient
                .retrieveRecordedRequests(
                        request()
                                .withPath("/my/path")
                                .withMethod("POST")))
        // Cast is required after breaking change in mockserver-client around 5.11.2.  Perhaps future
        // versions will improve this and remove need for cast.
        .map(x -> (HttpRequest) x)
        .toArray(HttpRequest[]::new)
);
jamesdbloom commented 2 years ago

This will be fixed next and released in the next release due in a couple of weeks