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

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

Closed
5 tasks done
doktor500 opened this issue Jul 25, 2020 · 10 comments
Closed
5 tasks done

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

doktor500 opened this issue Jul 25, 2020 · 10 comments

Comments

@doktor500
Copy link

Issue Classification

  • Bug report

Bug Report

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

Software versions

  • OS: Mac OSX 10.15.5
  • Consumer Pact library: @pact-foundation/pact": "^9.11.0
  • Provider Pact library: @pact-foundation/pact": "^9.11.0
  • Node Version: v12.10.0

Issue Checklist

Confirm the following:

  • I have upgraded to the latest
  • I have created a reproducable git repository (see below) to illuminate the problem
  • I have the read the FAQs in the Readme
  • I have triple checked, that there are no unhandled promises in my code

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
@doktor500 doktor500 changed the title HTTP DELETE requests with body doesn't work HTTP DELETE requests with body don't work Jul 25, 2020
@doktor500 doktor500 changed the title HTTP DELETE requests with body don't work HTTP DELETE requests with JSON body don't work Jul 25, 2020
@mefellows mefellows added bug Indicates an unexpected problem or unintended behavior Triage labels Jul 25, 2020
@mefellows
Copy link
Member

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

@TimothyJones
Copy link
Contributor

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
Copy link
Member

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
Copy link
Contributor

TimothyJones commented Jul 27, 2020

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: pact-foundation/pact-js-core#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): pact-foundation/pact-js-core#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
Copy link
Member

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
Copy link
Member

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 mefellows removed the bug Indicates an unexpected problem or unintended behavior label Mar 3, 2023
@mefellows
Copy link
Member

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

@mefellows mefellows added bug Indicates an unexpected problem or unintended behavior Awaiting feedback labels Mar 3, 2023
@TimothyJones
Copy link
Contributor

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

@mefellows mefellows added enhancement and removed bug Indicates an unexpected problem or unintended behavior labels Mar 3, 2023
@mefellows
Copy link
Member

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.

@YOU54F YOU54F closed this as completed Nov 18, 2024
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

No branches or pull requests

5 participants