Fix HTTP Body Generation for JSON Requests

[Ben-G]

I ran into a tricky issue which I believe is a bug in PMHTTP, though I'm not entirely I'm using the API correctly.

When generating and performing a network request as following, the provided JSON is not transmitted as part of the HTTP body, instead the HTTP body is empty:

let request = HTTP.request(
    POST: "test",
    json: [
        "email" : user.email
    ]
)

request?.performRequest { task, result in
...
}

This can be fixed by extending the code that generates the upload body in HTTPManager to also check for a .json body type.

This code also seems to exist in the preparedURLRequest property (

PMHTTP/Sources/HTTPManagerRequest.swift

Lines 483 to 503 in 19e9c57

	public var preparedURLRequest: URLRequest {
	var request = _preparedURLRequest
	switch uploadBody {
	case .data(let data)?:
	request.httpBody = data
	case .formUrlEncoded(let queryItems)?:
	request.httpBody = FormURLEncoded.data(for: queryItems)
	case .json(let json)?:
	request.httpBody = JSON.encodeAsData(json)
	case let .multipartMixed(boundary, parameters, bodyParts)?:
	// We have at least one Pending value, we need to wait for them to evaluate (otherwise we can't
	// accurately implement the `canRead` stream callback).
	for case .pending(let deferred) in bodyParts {
	deferred.wait()
	}
	request.httpBodyStream = HTTPBody.createMultipartMixedStream(boundary, parameters: parameters, bodyParts: bodyParts)
	case nil:
	break
	}
	return request
	}

), but that code isn't called form anywhere except from the test suite.

I would assume a long term fix would be to incorporate this code into the actual HTTP body generation.

The test suite itself did not expose this bug, since it was calling preparedURLRequest, which had the side effect of generating the HTTP body correctly. As soon as that line was removed from the tests, they failed expectedly.

---

This change is 

[lilyball]

The test case does actually test the HTTP body on the real upload request too. There is a line that tests to make sure the preparedURLRequest.httpBody is populated, but the rest of that test case also tests making a full request to the test server and validating that the JSON was uploaded.

How are you testing that the HTTP body is empty with your upload? If you're reading the code, the httpBody field on the request won't be set, but that's because it uses a stream request and implements urlSession(_:task:needNewBodyStream:) to provide the JSON upload.

What I'm wondering right now is if the behavior you're seeing is actually because this upload doesn't include a Content-Length header (since it's a streamed upload, it uses the Transfer-Encoding: chunked instead).

[Ben-G]

but the rest of that test case also tests making a full request to the test server and validating that the JSON was uploaded.

That is correct, but the rest of the test only passes if .preparedURLRequest is called, since that code actually sets the .httpBody (reading details below it seems that a set httpBody is not relevant here).

How are you testing that the HTTP body is empty with your upload? If you're reading the code, the httpBody field on the request won't be set, but that's because it uses a stream request and implements urlSession(_:task:needNewBodyStream:) to provide the JSON upload.

I ran into this communicating with my own backend, so I could see the empty JSON. I could also reproduce this using the test suite by removing the assertion that accesses .preparedURLRequest (since this causes the body to no longer be set).

[lilyball]

preparedURLRequest sets httpBody on the URLRequest value that it returns, but that value is unrelated to the rest of the test. Every time you access preparedURLRequest it creates a fresh URLRequest value, populates it, and returns it.

I ran into this communicating with my own backend, so I could see the empty JSON.

Does your backend support HTTP 1.1? If it's an HTTP 0.9 or even HTTP 1.0, then it probably doesn't understand Transfer-Encoding: chunked (which is a requirement of HTTP 1.1), and would therefore report the body as empty since there's no Content-Length.

I could also reproduce this using the test suite by removing the assertion that accesses .preparedURLRequest (since this causes the body to no longer be set).

Are you sure about this? As I said above, that line doesn't actually affect the request that is sent to the server since it's a fresh value. In addition, if I comment out that line, the test case still passes.

[Ben-G]

Are you sure about this? As I said above, that line doesn't actually affect the request that is sent to the server since it's a fresh value. In addition, if I comment out that line, the test case still passes.

Hah, no idea what I did, but apparently my failing tests were due to some other intermediate changes. Your explanation makes sense. The server does support HTTP 1.1, but given the info you provided here it seems like it's a server side issue anyway.

I'll close for now and verify with another server. I'll reopen in case this turns out to be an actual issue (which seems unlikely now). Thanks for the help!