pact-foundation / pact-ruby

Enables consumer driven contract testing, providing a mock service and DSL for the consumer project, and interaction playback and verification for the service provider project.
https://pact.io
MIT License
2.17k stars 216 forks source link

Pact verifier doesn't send DELETE requests with body correctly #198

Closed jnazander closed 3 years ago

jnazander commented 5 years ago

(This is a copy of an issue from pact-node. I was advised that since pact-node is a wrapper around pact-ruby, the underlying problem is actually here)

Software versions

Expected behaviour

Pact mock client during pact provider tests should send HTTP DELETE requests with HTTP body correctly.

Actual behaviour

It doesn't send HTTP DELETE requests correctly.

I'm using Node.js with Express framework as the server, and I'm running pact provider tests / pact verifications. One of the pact interactions is a DELETE request that has a request body (The spec does not forbid it). All other requests go through fine, but the delete request seemingly makes the server freeze and not return anything until the timeout, at which it resets the HTTP connection (see the pact provider log attached).

Upon further investigation with Wireshark I found out the underlying problem: the pact mock client doesn't even send all of the TCP packets that make up the HTTP DELETE request - it stops short of sending the body. This makes the server wait for the remaining packet, which never comes, thus hanging the test until it times out.

image

Interestingly, it does send Content-Length: 15 in the first packet, indicating that there will be a body in the following packet(s), but it never sends it.

I assume that the issue is in the underlying Ruby HTTP client? As stated in the previous issue thread, many Ruby HTTP clients either don't support DELETE requests with body, or they do, but require a special way of calling them (stackoverflow thread)

Steps to reproduce

Try to verify a pact with the following interaction:

{
      "description": "A DELETE request to delete one reminder",
      "providerState": "User user1@example.com exists with 2 smart reminders",
      "request": {
        "method": "DELETE",
        "path": "/api/users/22/schedule",
        "headers": {
          "Content-Type": "application/json",
        },
        "body": {
          "items": [
            100
          ]
        }
      },
      "response": {
        "status": 204,
        "headers": {
        }
      }
    }

(or any pact that has a HTTP DELETE method with a request body).

Relevant log files

Log file generated by pact provider test attached. pact-provider.log

bethesque commented 5 years ago

I think it's most likely the Rack test library, but I haven't trawled through the code. We've had this question asked a couple of times over the last 6 years. Can you explain your usecase?

jnazander commented 5 years ago

@bethesque the use case is that we have a batch-delete endpoint, where we pass the list of IDs to delete in the request body.

Of course we can change the implementation by passing the IDs in the query string, but in any case, this should either be fixed, or if it's really forbidden to make HTTP DELETE requests with body, then it should be made obvious by throwing an appropriate error (preferably already during pact consumer test), instead of silently timing out and leaving it to the developer to figure out the cause.

TimothyJones commented 5 years ago

It looks like this bug was fixed in rack-test 0.8.3, but this brought in a regression, which was then fixed in 1.0.0. I think we could fix this by updating our version of rack-test to 1.0.0.

TimothyJones commented 5 years ago

Hm, though it looks like rack-test 1.1.0 was used in the most recent pact-ruby build, and rack-test 0.8.3 was used in the latest pact-standalone build (1.70.2, which is used by the latest pact-node). However, I don't know enough about the ruby ecosystem to determine whether these versions are the cause.

bethesque commented 3 years ago

@TimothyJones did someone verify this was fixed?

TimothyJones commented 3 years ago

I thought this was fixed, but I'm trying to verify it now with pact-ruby-standalone-e2e-example, and I can't seem to confirm it. Perhaps I don't understand how the mock provider works.

You can check it out here:

https://github.com/TimothyJones/pact-ruby-standalone-e2e-example/tree/test-delete-with-body

bethesque commented 3 years ago

Pretty sure this is working now @TimothyJones? Closing, but reopen if not.

TimothyJones commented 3 years ago

Which version is supposed to fix it? I checked the changelog, but it's not very useful for consumers:

https://github.com/pact-foundation/pact-ruby-standalone/blob/master/CHANGELOG.md

(it would be awesome if we could automate this, because then we could automate the changelog in pact-node, too).

My dev machine is in for repair, so I can't confirm with the repro repo above.

bethesque commented 3 years ago

The changelogs of each of the individual gems are automated. Automating the aggregated changelog for the standalone would be great, but is not likely to get to the top of my priority list ever, especially as we are trying to get rid of it.

I thought you were the one who had dealt with the pact-js issue related to to this, but now I'm thinking about it, I think it was the WEBDAV stuff.

mikiihuang commented 2 years ago

sorry to bother you,I'm facing the same problem in pact-python version 1.5.0. Is this bug fixed? @bethesque @mefellows

bethesque commented 2 years ago

I don't think so. I remember I tried to get this working locally a while ago, and it didn't work.