fix: don't update Data when body is empty

[llimllib]

When a request to /anything has an empty body, the data field of the output should be the empty string. Currently, go-httpbin is returning data:application/octet-stream;base64,`.

• Should the output actually be null? That would match files, form, and json, but also would require deeper changes since Data is stored as a string not a pointer, and I didn't want to mess with things any more than I had to. An empty string works fine for my purposes
• how can I test this PR? I tried but I got confused by the TestAnything method
• all the current tests pass under this change

This PR closes #124. Before:

$ curl -s 'http://0.0.0.0:8080/anything?one=two'
{
  "args": {
    "one": [
      "two"
    ]
  },
  "headers": {
    "Accept": [
      "*/*"
    ],
    "Host": [
      "0.0.0.0:8080"
    ],
    "User-Agent": [
      "curl/7.88.1"
    ]
  },
  "method": "GET",
  "origin": "127.0.0.1:60402",
  "url": "http://0.0.0.0:8080/anything?one=two",
  "data": "data:application/octet-stream;base64,",
  "files": null,
  "form": null,
  "json": null
}

After:

$ curl -s 'http://0.0.0.0:8080/anything?one=two'
{
  "args": {
    "one": [
      "two"
    ]
  },
  "headers": {
    "Accept": [
      "*/*"
    ],
    "Host": [
      "0.0.0.0:8080"
    ],
    "User-Agent": [
      "curl/7.88.1"
    ]
  },
  "method": "GET",
  "origin": "127.0.0.1:60595",
  "url": "http://0.0.0.0:8080/anything?one=two",
  "data": "",
  "files": null,
  "form": null,
  "json": null
}

[codecov]

Codecov Report

Merging #125 (2cec1a1) into main (7e81a93) will decrease coverage by 0.21%.
The diff coverage is 25.00%.

@@            Coverage Diff             @@
##             main     #125      +/-   ##
==========================================
- Coverage   98.41%   98.20%   -0.21%     
==========================================
  Files           8        8              
  Lines        1448     1450       +2     
==========================================
- Hits         1425     1424       -1     
- Misses         17       19       +2     
- Partials        6        7       +1     

Impacted Files	Coverage Δ
httpbin/helpers.go	96.26% <25.00%> (-1.23%)	⬇️

[mccutchen]

+1, I agree that this change makes sense.

• Should the output actually be null? That would match files, form, and json, but also would require deeper changes since Data is stored as a string not a pointer, and I didn't want to mess with things any more than I had to. An empty string works fine for my purposes

That's a good question, let me think about it a little bit this morning. My first instinct is that an empty string is a reasonable enough compromise that, even if null would be more consistent with other optional elements of these responses, it's probably not worth the effort it would take to coerce the golang type system & json serializer into handling this for us.

I might take a quick pass locally just to see how feasible something like a type OptionalData string w/ custom marshal/unmarshal methods might be, but I also feel like every time I try to do this a) it takes me forever to figure out and b) it doesn't end up working quite the way I want it to.

• how can I test this PR? I tried but I got confused by the TestAnything method

Also a good question, and I'm sorry for any confusion! I know the test suite is a bit of a mess, and I have no idea off the top of my head how best to test this, but lemme take a look.

[mccutchen]

That's a good question, let me think about it a little bit this morning. My first instinct is that an empty string is a reasonable enough compromise that, even if null would be more consistent with other optional elements of these responses, it's probably not worth the effort it would take to coerce the golang type system & json serializer into handling this for us.

Okay, took a quick pass at this, and I don't think it's worth all of the additional overhead that would be required to marshal empty bodies as null in the response.

The custom type approach feels like a whole lot of complexity (including a particularly heavyweight custom MarshalJSON method that would have to allocate an extra temporary buffer in order to use a json.Encoder in order to disable HTML escaping in order to maintain compatibility with this other compatibility-related change), and for now I don't particularly want to get into the business of working with string pointers everywhere.

[mccutchen]

• how can I test this PR? I tried but I got confused by the TestAnything method

Also a good question, and I'm sorry for any confusion! I know the test suite is a bit of a mess, and I have no idea off the top of my head how best to test this, but lemme take a look.

Oh boy, I just spent wayyyy too long trying to figure out why the existing test cases for requests with empty bodies were showing a different behavior from the curl test (either the tests should have failed, or the curl should have returned an empty string for the "data" field).

Turns out it's down to a couple of things:

• Most/all of the tests in the test suite use httptest.NewRecorder and just call the ServeHTTP method directly, rather than spinning up a live HTTP server using httptest.NewServer and making an actual HTTP request, like so:

  go-httpbin/httpbin/handlers_test.go

  Lines 633 to 636 in 1223a73

  	r, _ := http.NewRequest(verb, path, nil)
  	r.Header.Set("Content-Type", test.contentType)
  	w := httptest.NewRecorder()
  	app.ServeHTTP(w, r)

• After adding some logging, it became clear that this conditional is true when using the test recorder and false when making a real HTTP request:

  go-httpbin/httpbin/helpers.go

  Lines 115 to 118 in 1223a73

  	func parseBody(w http.ResponseWriter, r *http.Request, resp *bodyResponse) error {
  	if r.Body == nil {
  	return nil
  	}

So, weirdly, we already have unit tests in place that describe the behavior we're actually implementing in this pull request, they just weren't testing the actual live behavior in the face of an actual HTTP request.

My inclination is to land this change as-is and then I'll follow up with an update to the test suite to stop using the recorder approach. I've considered that a number of times in the past for vague notions of correctness, but this is the first time I can remember running into an actual behavior difference that mattered quite so much.

[mccutchen]

FYI, this was released in v2.9.0

[llimllib]

Thanks so much! I want to add that I've found this project really useful, thanks for all the work