Skip to content

GET with body sends as query string #185

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

Closed
Nexith opened this issue Feb 4, 2019 · 8 comments
Closed

GET with body sends as query string #185

Nexith opened this issue Feb 4, 2019 · 8 comments

Comments

@Nexith
Copy link

Nexith commented Feb 4, 2019

I have the following contract for a consumer. The request works when sending it to the server using Postman.

When the pact provider tries to verify it fails. The body is sent in the query string and not in the body of the GET request as expected.

Using the latest version of PactNet.

{
  "consumer": {
    "name": "Source System"
  },
  "provider": {
    "name": "Event API"
  },
  "interactions": [
    {
      "description": "A GET request with event filters",
      "providerState": "default state",
      "request": {
        "method": "get",
        "path": "/events",
        "headers": {
          "Content-Type": "application/json; charset=utf-8"
        },
        "body": {
          "filters": [
            {
              "type": "state",
              "source": "logs"
            },
            {
              "type": "information",
              "source": "memory"
            }
          ]
        }
      },
      "response": {
        "status": 200,
        "headers": {
          "Content-Type": "application/json; charset=utf-8"
        },
        "body": {
          "data": [
            {
              "id": 1,
              "event": "init",
            },
            {
              "id": 2,
              "event": "start",
            }
          ]
        }
      }
    }
  ],
  "metadata": {
    "pactSpecification": {
      "version": "2.0.0"
    }
  }
}
@neilcampbell
Copy link
Member

@Nexith That's going to be the ruby core doing that. Typically a body in a GET requests is not a good idea, purely because the HTTP 1.1 spec had language to indicate it should not be supported on the server side. I know now the spec has been amended, but still you run into compatibility issues like this one. I'd recommend changing your API to accept query params instead and updating the pact to use query, rather than body.

@bethesque
Copy link
Member

There is no way to tell the underlying library that in this case, the body should go in the body. For all history (until recently, it seems) GETs did not support bodies, so if we were to change that, it would break a lot of existing tests that put the "params" in the query string.

@Nexith
Copy link
Author

Nexith commented Feb 5, 2019

@neilcampbell I know the HTTP 1.1 spec was unclear on this, but as you say with the amended HTTP 1.1 spec there might be a compatibility issue as you say.

However I think this is a test case that should be supported as more services/products go to include a body in the GET requests as converting a complex object into a query can be hard seeing there is a limit to how long a query can be and they want to stay RESTful.

@bethesque You have the pactSpecification version included in metadata, so maybe a new version could support that and change how you consume the pact json based on what version it is without breaking existing implementations?

There is the query property to that set under the request in the json if a query string is specified already.

@bethesque
Copy link
Member

The problem is that the underlying library that makes the request (rack test) will only allow either a body or query parameters, and it will automatically put the body in the parameters for a GET request. See this: https://github.com/rack-test/rack-test/blob/master/lib/rack/test.rb#L235

I don't think there is a way to force it to put data in the body.

@Nexith
Copy link
Author

Nexith commented Feb 5, 2019

Ah, that makes things a bit harder. They seem to have a proposed solution for updating their library to make it possible see: rack/rack-test#225

There is no set release date for it, but possible a good place to make a suggestion on how this can be supported so Pact can support a body with a GET request.

@neilcampbell
Copy link
Member

neilcampbell commented Feb 5, 2019

This is also worth a read swagger-api/swagger-ui#2136 for some thoughts on this. This comment in particular about the OpenAPI 3.0 take on it.

The requestBody is only supported in HTTP methods where the HTTP 1.1 specification RFC7231 has explicitly defined semantics for request bodies. In other cases where the HTTP spec is vague, requestBody SHALL be ignored by consumers.

@Nexith
Copy link
Author

Nexith commented Feb 7, 2019

There seems to be a split between people thinking that it should be supported and not. I can see a case for both but, but until the RFC for HTTP 1.1 is a bit more clear it probably won't be supported by OpenAPI and Swagger at least.

However I think Pact should not lift the body parameter in the GET request in put it on the query string and instead ignore it completely and only use the query property in the json schema to avoid confusion and potential errors with the query string having unexpected values.

@adamrodger
Copy link
Contributor

Should be fixed in 4.x with the move to Rust core instead of Ruby (or at least it'll be broken in a differnt way 😁 )

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

4 participants