Fix conflation of () which serializes to the JSON value null and a return that produces no content

[ahl]

We've used () as a sentinel value that means that an endpoint produces no response body. In fact, however, () serializes to the JSON value null. We ran into this in progenitor because some endpoints (inadvertently) produced null and would therefore deserialize into (), but the endpoints that produce no data can't be properly deserialized into () because there is no data.

This PR distinguishes between endpoints that return () and those that actually return nothing.

I could easily be persuaded that a better way to address this disparity would be simply to special case response bodies that are simply () and have them return zero-length response bodies. This may, however, be non-intuitive (in a different way that the proposed solution is non-intuitive, perhaps), and may be hard to reliably in all cases such as if one had a newtype alias for ().

[davepacheco]

Dang, things for digging into this.

How would you summarize the impact for Dropshot users? Is it that: if you previously were returning HttpResponseOk<()>, that endpoint would have returned null at runtime, but the OpenAPI spec would incorrectly say ... what, exactly? Now, the OpenAPI spec will say it returns null? And if you want to return an empty body, you should return HttpResponseOk<Empty>?

And were we previously returning a 4-byte null response when you used HttpResponseUpdatedNoContent or HttpResponseDeleted?

---

I found it surprising that () serializes to null, especially since a None can already be used to do that. It looks like it's because serializing it to the empty string wouldn't be valid JSON. That makes me realize that these empty response bodies aren't valid JSON at all. Should we be using a different set of response types? That is, you'd use HttpResponseOk<T> when you wanted a JSON response (which is necessarily non-empty), and you'd use something else (HttpResponseOkEmpty?) if you want an empty response?

[ahl]

How would you summarize the impact for Dropshot users? Is it that: if you previously were returning HttpResponseOk<()>, that endpoint would have returned null at runtime, but the OpenAPI spec would incorrectly say ... what, exactly? Now, the OpenAPI spec will say it returns null? And if you want to return an empty body, you should return HttpResponseOk<Empty>?

Yeah, I had planned an entry to the changelog ;-)

If your endpoint returns HttpResponseOk<()> the OpenAPI description for that operation would have no responseBody field. This isn't quite right because the () is serialized to null. After this change, the schema for the response body will be this:

{
  "title": "Null",
  "type": "string",
  "enum": [
    null
  ]
}

While this schema is a little awkward, OpenAPI 3.0.x doesn't provide a representation for a null type (though it does provide a representation of a type that may be null or some other value via nullable... which is an extension to JSON Schema). In OpenAPI 3.1.0 the spec has more closely aligned with recent JSON Schema drafts such that the null type can be more simply represented ({ "type": "null" }), but until 3.1 becomes more broadly used, Dropshot will stick to OpenAPI 3.0.3.

And were we previously returning a 4-byte null response when you used HttpResponseUpdatedNoContent or HttpResponseDeleted?

No! We special-cased those types:

impl From<HttpResponseUpdatedNoContent> for HttpHandlerResult {
    fn from(_: HttpResponseUpdatedNoContent) -> HttpHandlerResult {
        Ok(Response::builder()
            .status(HttpResponseUpdatedNoContent::STATUS_CODE)
            .body(Body::empty())?)
    }
}

Thinking this through more, I added some code to handle the case where a user specifies, say, HttpResponseOk<Empty> as the return type... I'm not sure I like it though:

    fn for_object(body_object: &Self::Body) -> HttpHandlerResult {
        let response = Response::builder().status(Self::STATUS_CODE);

        if <dyn std::any::Any>::is::<Empty>(body_object) {
            Ok(response.body(Body::empty())?)
        } else {
            let serialized = serde_json::to_string(&body_object)
                .map_err(|e| HttpError::for_internal_error(e.to_string()))?;
            Ok(response
                .header(http::header::CONTENT_TYPE, CONTENT_TYPE_JSON)
                .body(serialized.into())?)
        }
    }

Thoughts?

I found it surprising that () serializes to null, especially since a None can already be used to do that. It looks like it's because serializing it to the empty string wouldn't be valid JSON. That makes me realize that these empty response bodies aren't valid JSON at all. Should we be using a different set of response types? That is, you'd use HttpResponseOk<T> when you wanted a JSON response (which is necessarily non-empty), and you'd use something else (HttpResponseOkEmpty?) if you want an empty response?

Picayune as it is, I think that's right.

[ahl]

@davepacheco I'd love to get your eyes on this in particular; I don't love it, but I think maybe it's better than not doing it. In particular, I think casting is icky, but I don't know how else to see if we've got an Empty type. Alternatively, perhaps we don't make Empty pub.

[davepacheco]

I kind of like not making Empty pub and saying that you should use a different HttpResponseOkEmpty...but I hope someone will push back if that seems excessively pedantic. I'd even consider renaming HttpResponseOk to HttpResponseOkBody, though that does seem pretty annoying for existing consumers.

[ahl]

Well it turns out that Empty must be pub because it's used as the Body type param for some pub response types. But I made it #[doc(hidden)] to discourage its use... which would otherwise fail at runtime. I didn't rename the type because--as you said--it would be annoying to consumers.