Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature: Allow passing a closure to validate requests #95

Closed
zacps opened this issue Jan 5, 2020 · 10 comments · Fixed by #206
Closed

Feature: Allow passing a closure to validate requests #95

zacps opened this issue Jan 5, 2020 · 10 comments · Fixed by #206

Comments

@zacps
Copy link

zacps commented Jan 5, 2020

To support more complex validation I would like the ability to use a closure (probably Fn) as a matcher.

@lipanski
Copy link
Owner

lipanski commented Jan 9, 2020

@zacps I'm a bit sceptical about exposing the request object. I changed the request parsing implementation quite a few times in the past and I still don't consider the interface stable enough to expose it. on top of that, I think it would add a way too complex use case to the library.

@zacps
Copy link
Author

zacps commented Jan 10, 2020

I can understand the hesitation to expose a potentially unstable API. However, the current matching system is quite limited.

As for complexity, eh, the current API surface is small enough I don't think it's a concern.

@chess4ever
Copy link

We have an use case where we would like to partially match a binary body. This would be possible if this feature would be available.

@lipanski
Copy link
Owner

@chess4ever I think this could be solved with a new matcher, similar to #43 (but as a matcher). we could introduce it as Matcher::Partial(String) or Matcher::Partial(Vec<u8>)

@torrefatto
Copy link
Contributor

torrefatto commented Oct 19, 2020

Hi @lipanski! Thanks for replying! I was preparing a PR for this. Right now that commit is a stub, and I am working on the tests now. Do you like the approach?

@huuff
Copy link

huuff commented Nov 9, 2024

I'm currently needing this too! My use case is trying to match a multipart, but reqwest sets a random boundary string (and there doesn't seem to be any way to change it) so I'm a bit stuck at this.

Given that exposing the request object is undesired, perhaps providing a proxy struct just for this kind of validation works? The request object could be transformed to this proxy and then the proxy struct may be provided to the closure, so the user can run any logic on it for validation. Maybe the proxy could only expose the request as a str?

This way the internal request object can evolve independently of the matcher and only the (internal) transformation would need to be updated.

@huuff
Copy link

huuff commented Nov 11, 2024

I've got a working PoC here: https://github.com/huuff/mockito/tree/predicate

This is an usage example:

#[test]
fn test_match_predicate() {
    let mut s = Server::new();
    s.mock("POST", "/")
        .match_body(|body: &str| body.contains("world"))
        .create();

    let (status, _, _) = request_with_body(s.host_with_port(), "POST /", "", "hello world");
    assert_eq!("HTTP/1.1 200 OK\r\n", status);
}

I honestly don't know why the closure input type can't be inferred.

I didn't need any proxy object (I guess concerns about exposing the request object may no longer apply to newer versions?), However, I think this may be a breaking change? Since some downstream crate might be matching on the Matcher enum (though it seems unlikely).

If this is a breaking change, it may be too much hassle for relatively little gain. Though this makes the matching system extensible, I think an approach that makes Matcher a trait instead would be much more powerful. Then any downstream crates could provide any Matchers and no further changes (especially breaking ones) would be necessary to the main crate to extend the matching system.

@lipanski
Copy link
Owner

@huuff Thanks for that. I think it's time to expose the Request struct so I took your idea and turned it into this #206 since it also covers headers and other use cases where matchers might be more limited. This should also cover your use case, right?

@huuff
Copy link

huuff commented Nov 11, 2024

This is even better, since it covers more use-cases and isn't a breaking change! Looking forward to the merge!

@lipanski
Copy link
Owner

Released as 1.6.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants