LukeMathWalker / wiremock-rs

HTTP mocking to test Rust applications.
Apache License 2.0
631 stars 73 forks source link

Add PathRegexMatcher #4

Closed exul closed 4 years ago

exul commented 4 years ago

This matcher matches a path against a regular expression.

This is useful if the request is sent to a path where the exact string of the path is not known, for example if the path contains any kind of ID that is randomly generated for each request.

LukeMathWalker commented 4 years ago

Thanks for the PR! A question: do you think we need the full flexibility of regexes here? Or would it be enough to be able to match paths with the syntax generally used when writing endpoints on a server (e.g. /hello/{:id}/do_something)?

On one side, regex syntax is unambiguous and well-known/easy to look up. On the other side, it feels more complex than what is needed in most cases.

exul commented 4 years ago

A question: do you think we need the full flexibility of regexes here? Or would it be enough to be able to match paths with the syntax generally used when writing endpoints on a server (e.g. /hello/{:id}/do_something)?

I've used a regex because some routers support regular expressions in their path (e.g. hyper-router). I agree that most cases don't need it. On the other hand I'm not sure if having a custom syntax is the better option: /hello/{:id}/do_something, vs. /hello/:id/do_something vs. /hello/<id>/do_something. If it's added though, it will be hard to remove/change it without a breaking change. So we can also wait until there's more user feedback.

LukeMathWalker commented 4 years ago

I think it's fine to have a powerful regex matcher and we can then introduce a simplified route matcher if we feel like it makes sense. Would you mind adding an example with a regex that matches an arbitrary path segment? The equivalent of /somestuff/{a_segment}/someotherstuff. I assume it's what most people will look out for.

exul commented 4 years ago

I've changed the unwrap to an expect and added another example. The example uses a simple .+, to catch anything, but it could also be something like [a-z0-9-]{36}, which one do you perfer?

P.S. Sorry I've amended to my previous commit, which means you don't see the diff since the last review. Do you prefer fixup commits or completely separate commits for such updates?

LukeMathWalker commented 4 years ago

I've changed the unwrap to an expect and added another example. The example uses a simple .+, to catch anything, but it could also be something like [a-z0-9-]{36}, which one do you perfer?

I would prefer [a-z0-9-~_]{1,}, because it does not make any assumption on the length of the segment but it's also not going to swallow multiple segments (as .* does because it matches on slashes).

P.S. Sorry I've amended to my previous commit, which means you don't see the diff since the last review. Do you prefer fixup commits or completely separate commits for such updates?

It doesn't matter - I stick to one commit per PR, so I am going to squash and merge :+1:

exul commented 4 years ago

I would prefer [a-z0-9-~_]{1,}, because it does not make any assumption on the length of the segment but it's also not going to swallow multiple segments (as .* does because it matches on slashes).

Updated.

LukeMathWalker commented 4 years ago

Nice, merged :grin: Thank you!