mockttp icon indicating copy to clipboard operation
mockttp copied to clipboard

Negate matchers/function predicates

Open renataogarcia opened this issue 7 years ago • 6 comments

I think it would nice to be able to allow matching based on negation. In my case I would like to match when a header is not present, but I think it would make sense to allow it on the other parts of the requests as well. More specifically I would like to model when the user is not logged in and and therefore has no cookie.

Alternatively function predicates in the same style the nock supports would probably be more flexible and also cover this scenario.

renataogarcia avatar Nov 17 '18 02:11 renataogarcia

Nice idea! Yes, I'd definitely be open to both matcher negation and a function predicate matcher, though the API for negation is a little tricky. For function matching I think it's fairly simple, I'm imagining something like:

server.get('/').matching((request) => request.headers['content-type'] != 'text/plain').thenReply(200);

Would that work for you? The only slightly harder part here is making this work in a browser (you need live communication between the browser and server to check matches), but we already solve a very similar problem in thenCallback, so we could easily crib from that.

For negation, it's a bit more difficult to come up with a nice API. You could have a .not. step in the rule chain (server.get('/').not.withHeaders(...)...), but it's potentially confusing both as an API and in the implementation when you start thinking about multiple matches. In the below, is the body matcher negated or not?

server.get('/').not.withHeaders({ a: 1 }).withBody('123');

For either answer, I think there's a route to making it work, but it worries me a little that it's not 100% obvious, and for either one it's messy if you wanted the other option.

And how about these?

server.get('/').not.always().withHeaders({ a: 1 }).withBody('123')
server.get('/').not.thenReply(200)

Ick. I guess maybe there's a clearer function-based route here too, something more like:

server.get('/').not(r => r.withHeaders({ a: 1 })).withBody('123')

In that case, we could disallow anything except matchers inside the not() (so no always()) and it becomes super clear which matchers are negated and which aren't, and easy to negate multiple at once... I'm reasonably sure that would also lend itself to a clearer implementation as well.

What do you think?

pimterry avatar Nov 17 '18 13:11 pimterry

@pimterry Thanks for the prompt and detailed response. Thanks for mockttp as well!

I agree that coming up with a API for negation is trickier - I also wonder if it's common enough to justify having a specific API just for that, when you could do it with the matching API. Eventually you would also want things like: header not present (no matter what the value is), conditionals like or/and, or containing a specific value and so on.

With that said, I think the matching/predicate approach would handle all these cases since it's powerful and flexible. It's also easier to understand than a DSL in my opinion, since it is just JS/TS. The downside of this approach though is probably that it is less convenient and can lead to less readable code depending on how it's used. To contrast both approaches:

server.get('/').withHeaders({'content-type': 'text/plain'}).thenReply(200)
server.get('/').matching( request => request.headers['content-type'] === 'text/plain' ).thenReply(200)

While the first is more concise and readable, I still think the flexibility that the predicate approach provides makes it worth it. Also, it could be made more readable while still using predicates by using a named function instead:

server.get('/').matching(contentType('plain/text')).thenReply(200)

Whilst this is not necessarily a good example, it illustrates the syntax that is possible to achieve.

Some common predicates could be part of mockttp, the user could also extend to it's need for their most common cases, and can also use Lodash partials, matches and the predicate combinators like _.negate, _.overSome, overEvery. Not mandatory, but still possible.

I also think that it would make sense to allow matching on the URL and method. Assuming that this is feasible, it would mean that the API would have to be something like:

server.matching(...).thenReply(200)

Don't think this is ideal though because if I want to match on headers I would still need to handle the url and method. So perhaps if we apply best of the these two approaches, from API perspective (not sure if it's too hard to implement), I think having predicates on every part of the request would allow to achieve the necessary flexibility without compromising conciseness/readability and consistency.

server.matching( request => ['GET','POST'].includes(request.method) && !request.url.endsWith("api")).thenReply(404)
server.get(_.includes("/query")).thenReply(200)
server.get('/').withHeaders(_.matches({'content-type':'plain/text'})).thenReply(200)
server.post('/').withBody(_.matches({'username': foo, 'password': bar })).thenReply(200)

I would still like to have matching though when I need to the flexibility to match on more than one entry with some logic that is not easy to express by only combining the existing functions.

As an aside, I'm just wondering what would be the use case for supporting browser as well?

renataogarcia avatar Nov 18 '18 07:11 renataogarcia

Hey @pimterry, just wondering if got a chance to think about this? Do you think it would be feasible?

renataogarcia avatar Jan 09 '19 09:01 renataogarcia

Sure, ok, I think we can and should have a go at this. I do want to keep it simple to start with though, so I'd prefer to build just the .matching() matcher as the first step, and then go from there.

We can layer more predicates and other parts on later if need be, but it'd better to iterate our way there instead of doing it all up front.

By the way, there is also already a server.anyRequest() method, so once we have a predicate matcher you can just do server.anyRequest().matching(...) to manually match the method & path, if that's what you want.

As an aside, I'm just wondering what would be the use case for supporting browser as well?

Everything in Mockttp (very nearly) works in both node & browsers, with identical behaviour. The original motivation for the library was testing universal JS libraries that work in both, with one set of HTTP mocks. In the browser, the client is a wrapper that sends requests to a node process, which then runs a Mockttp server.

For most features that's very easy, and very useful.

For a couple of features like .thenStream and .thenCallback that dynamically generate responses it's more complicated, and we do it with websockets, and I think we'd need that for matching() too.

Right now with the existing handlers, each has serialize/deserialize methods used when passing them from the browser to the server. These methods set up the handler so that it sends messages when required (i.e. if thenCallback has matched a request on the server, it sends a websocket message to the browser asking what it should respond with) and registers matching listeners for websocket messages too (i.e. if the browser receives a websocket message asking what to respond with, it calls the JS callback in the browser, then sends back the result).

You can see how this works for .thenCallback here: https://github.com/httptoolkit/mockttp/blob/master/src/rules/handlers.ts#L133-L219.

Anyway, I'd be happy to add this. I'm unlikely to get to it myself in the short term, but I'd happily accept a PR for this if somebody wants to pick that up. Feel free to pick it up (you or anybody else) if you'd like, and let me know if you've got any other questions. If not, watch this space, and I'll try to get to it myself when I can!

pimterry avatar Jan 11 '19 16:01 pimterry

Ok, cool - if I get a change I will have a go at it. Thanks!

renataogarcia avatar Jan 20 '19 00:01 renataogarcia

As of Mockttp v2.3.0, predicate matching is now supported! See #56. In short, you can now do things like:

server.anyRequest().matching((req) => /* any check you like */).thenReply(200);

I'm going to leave this open to track negation support as a potential future extension, but I think this should solve all sorts of use cases already.

pimterry avatar Oct 12 '21 13:10 pimterry