search

pact-foundation / pact-js

JS version of Pact. Pact is a contract testing framework for HTTP APIs and non-HTTP asynchronous messaging systems.
https://pact.io
Other
1.63k stars 346 forks source link

HTTP DELETE requests with JSON body don't work #484

Open doktor500 opened 4 years ago

doktor500 commented 4 years ago

Issue Classification

Bug Report

Contracts for HTTP DELETE requests that contain a JSON body can't be verified in the provider.

Software versions

Issue Checklist

Confirm the following:

Expected behaviour

A contract for an HTTP DELETE request with a JSON body can be verified in the provider

Actual behaviour

A contract for an HTTP DELETE request with a JSON body can't be verified in the provider

Steps to reproduce

https://github.com/doktor500/pact-delete-bug

Relevant log files

    Verifying a pact between Consumer and Provider

      Given delete-user-1
        a request for deleting user 1 with HTTP DELETE
          with DELETE /users/1
            returns a response which

              has status code 200 (FAILED - 1)

      Given delete-user-1
        a request for deleting user 1 with HTTP POST
          with POST /users/1
            returns a response which

              has status code 200

    Failures:

      1) Verifying a pact between Consumer and Provider Given delete-user-1 a request for deleting user 1 with HTTP DELETE with DELETE /users/1 returns a response which has status code 200
         Failure/Error: replay_interaction interaction, options[:request_customizer]

         EOFError:
           end of file reached

    2 interactions, 1 failure

    Failed interactions:

    * A request for deleting user 1 with http delete given delete-user-1
mefellows commented 4 years ago

Thanks @doktor500, it might be an issue with the underlying Ruby process. Will need to investigate.

TimothyJones commented 4 years ago

Last time I checked this, I thought HTTP DELETE wasn't valid with a body (I believe HTTP 1.1 says that they may have a body, but the server might refuse the request).

mefellows commented 4 years ago

That was my thoughts briefly, but a cursory google seems to indicate its up to the server implementation.

Further digging needed but it seems likely it should be supported.

TimothyJones commented 4 years ago

So, here's the current delete spec: https://tools.ietf.org/html/rfc7231#section-4.3.5

A payload within a DELETE request message has no defined semantics; sending a payload body on a DELETE request might cause some existing implementations to reject the request.

This is like the body in GET - you're allowed to include one, but it doesn't mean anything, and it might mean a spec-compliant server rejects the request.

We had a similar discussion about GET, where I argued that we shouldn't support this. (see here: https://github.com/pact-foundation/pact-node/issues/183 )

We also had this same issue reported earlier, where I thought it was fixed upstream (but didn't check the spec at that time): https://github.com/pact-foundation/pact-node/issues/183

I'm starting to change my mind on this one, since we get this question a lot, and it's clear what the user intends (even if it's not spec compliant). I think the biggest challenge is cross-language support, since implementations are allowed to reject deletes with bodies.

I think the ideal behaviour is to allow the user to specify DELETE with body (and to have a mock server that allows this), but to warn that it's not valid. What do you think?

bethesque commented 4 years ago

Ideally it would be nice to offer that functionality, but unless Rack Test supports it, it can't actually be done in the Ruby Implementation. It might need to wait until we move to Rust.

mefellows commented 4 years ago

I haven't tried myself, but if you're game, you could try https://github.com/pact-foundation/pact-js/#pact-js-v3 a go (it's in beta, but is supported so we will fix bugs etc.).

mefellows commented 1 year ago

This could be tested with the latest 10.x.x library. I suspect it will suffer the same fate.

TimothyJones commented 1 year ago

I think you could close this one. There isn't any meaning to a delete body.

mefellows commented 1 year ago

I think you could close this one. There isn't any meaning to a delete body.

I agree with you that's how it should be interpreted, but my reading of the RFC is that it's undefined - so a server can interpret that how it wishes. As a framework, we should be cautious expressing too strong of an opinion there. I've dropped the bug label, because I don't think we should call it a bug, but if it can be supported in one way or improved (your suggestion above on a "warn" might be one way to do that) then I'm open to that.