andrzejchm / RESTMock

HTTP Server for Android Instrumentation tests
https://medium.com/@andrzejchm/ittd-instrumentation-ttd-for-android-4894cbb82d37#.uir3loxu7
Apache License 2.0
756 stars 58 forks source link

Allow multiple matchings for requests #73

Closed Sloy closed 6 years ago

Sloy commented 6 years ago

Hi!

We've been using WireMock for a while now on our app, and we're happy with it. But it's sometimes too heavy for Android development, and this library seems like an awesome lightweight replacement for it. Congratulations!

There's just one feature missing that stops us from adopting it, and it's having multiple matchings for a given request. For example having a very specific matching and a default fallback:

/api/login -> login_error_response.json
/api/login?user=me&pass=123 -> login_success_response.json

In this case the first matching would also match the second request, and RESTMock will return a MORE_THAN_ONE_RESPONSE_ERROR.

In WireMock the user can specify a priority value, and it will pick the highest one (the lower number means more priority). In my example, I would set a priority 2 for the default error, and priority 1 for the success case.

Another approach would be to just pick the last one added. Both solutions are not mutually exclusive, although I think the one with priorities is more robust.

What do you think about having this feature? I think it could be implemented as a withPriority(int) method on MatchableCall.

andrzejchm commented 6 years ago

definetely something to consider! will come up with approach and let you know here and will be a part of the next version. Thanks!

andrzejchm commented 6 years ago

I've had a thought on this and am rather reluctant on introducing the priority, since it will make matching responses more complex by adding one additional layer which will make it way harder for debug if you specify priorities incorectly. We here are dealing with hamcrest matchers that let us do very complex matching rules themselves. This particular issue you've mentioned could be solved with matchers, like:

/api/login -> allOf(pathEndsWithIgnoringQueryParams("login"), not(hasQueryParameters())) /api/login?user=me&pass=123 -> allOf(pathEndsWithIgnoringQueryParams("login"), hasQueryParameterNames("user", "pass"))).

Both of those matchers will be introduced in the upcoming release pretty shortly.

Please share your thoughts

Sloy commented 6 years ago

I don't really see much complexity in having a priority parameter. In my head, it would not be part of the hamcrest matchers, so without adding priorities everything would work like right now. Priorities would only be considered when there are multiple matching responses (in MatchableCallsRequestDispatcher#dispatch() -> else if (matchedCalls.size() > 1) {). I imagine something like this:

if (matchedCalls.size() == 1) {
    return onOneResponseMatched(recordedRequest, matchedCalls);
} else if (matchedCalls.size() > 1) {
   // -> New logic
    List<MatchableCall> highestPriorityCalls = takeHighestPriority(matchedCalls)
    if (highestPriorityCalls.size() == 1) {
        return onOneResponseMatched(recordedRequest, highestPriorityCalls);
    } else {
        return onTooManyResponsesMatched(recordedRequest, matchedCalls);
    }
    // <- New logic
} else {
    return onNoResponsesMatched(recordedRequest);
}

Where the priority would be an optional attribute of MatchableCall, and not part of its Matcher<RecordedRequest>. (The code could probably be more clean, it's just to show the idea)

Is this also what you thought? What do you think of that approach? Does it still add too much complexity?

My login request scenario was just one example, we have more scenarios where we need some way to overwrite the same request for particular cases.

Anyways, if you don't feel this is something you'd like to have in the library it's totally OK. We can always fork the code or implement a custom dispatcher for ourselves :)

andrzejchm commented 6 years ago

By complexity I mean actually the purpose of the library. I want the Hamcrest matchers be the only source of truth for the end user, while more complex logic should be a part of his own code, this has 2 benefits:

I think that better approach than prioritizing responses is to provide you with a mechanism of determining response on per-request basis in the following way:

RESTMockServer.whenGET(pathStartsWith("login"))
                .thenAnswer((request) -> {
                    if (not(hasQueryParameters()).matches(request)) {
                        return new MockResponse().setBody("NOT OK").setResponseCode(401);
                    } else if (hasQueryParameterNames("user","pass").matches(request)) {
                        return new MockResponse().setBody("OK").setResponseCode(200);
                    }
                    return null;
                });

This mechanism is a part of the next release #74

Sloy commented 6 years ago

Aha, ok I understand. You're right, priorities would mean introducing a new concept for matchings.

I will try to satisfy all of my requirements with your suggestions.

Thanks a lot @andrzejchm for the new mechanism and for all your work on this library :)