Match requests on query params and request body by default

[jsteiner]

It seems to me that use_cassette should default to using the options match_requests_on: [:query, :request_body]. If you are submitting different data, it's likely that the server will respond differently. exvcr implicitly hides those different responses, which is both unexpected and likely to lead to false test coverage.

I'm suggesting that the options for match_requests_on: [:query, :request_body] should be opt-out instead of opt-in. At the very least, there should be a global config option to override this behavior, but I'd rather push people in the right direction by default.

I'm happy to write some code for this, but I wanted to get your response before starting anything.

[parroty]

Thanks for the feedback! Regarding the default, I'm not so confident about adding them by default (yet). I think your point about false test coverage makes sense, but I was assuming that these params can fluctuate depending on the servers/services and can cause the error. I'm seeing the ruby's VCR library docs, and seems not going into these strict matches.

https://relishapp.com/vcr/vcr/v/3-0-1/docs/request-matching

In order to properly replay previously recorded requests, VCR must match new HTTP requests to a previously recorded one. By default, it matches on HTTP method and URI, since that is usually deterministic and fully identifies the resource and action for typical RESTful APIs.

However,

At the very least, there should be a global config option to override this behavior, ...

This part is just a missing functionality and I don't have concern about adding one. Maybe having default setting (ex. in config.exs). I would like to add this setting, having some difficulties to spare time. PR is appreciated :smile:

What do you think?

[smdern]

I created a PR to address this. https://github.com/parroty/exvcr/pull/47 You can override the default match_requests_on to something like this:

use Mix.Config

config :exvcr, [
  match_requests_on: [:query, :request_body],
]

[TMSCH]

I agree with @jsteiner that the default should match against URL, query params and body. All these pieces of data will trigger a different behavior of the server, it should matter to the developers when they change.

Some other reasons:

• avoid issues where the reply doesn't behave as the original, b/c query params/body info are dropped and incorrect requests are matched.
• avoid regression where changing the query params or body of the request still pass the test, when it doesn't against the real servers.
• stricter by default is just better. It should be a conscious decision of the developers to NOT use strict mode.
• not all servers are RESTful servers. In fact, a lot aren't.

As you quoted, VCR explains their decisions was made so that it works against RESTful API. I don't think this is a valid assumption, that every developer will work against RESTful API. Also, being stricter in the context of RESTful API will not be detrimental.

I'm happy to discuss further, I don't see any valid reason to keep the current behavior of VCR in ExVCR, also happy to make a PR to update the default.