Make httpPrefixHeaders be coupled with required

[david-perez]

Consider this model from the protocol tests:

https://github.com/awslabs/smithy/blob/1863d672dde488ccd81d9c00c64f57a26728f75d/smithy-aws-protocol-tests/model/restJson1/http-prefix-headers.smithy#L81-L88

If a server receives a request with no headers prefixed with X-Foo-, should it give to the service owner an empty map for fooMap, or None? The spec does not say. However, this test:

https://github.com/awslabs/smithy/blob/1863d672dde488ccd81d9c00c64f57a26728f75d/smithy-aws-protocol-tests/model/restJson1/http-prefix-headers.smithy#L43-L57

when interpreted from a server perspective, is saying that the request should deserialize into an empty fooMap. So why allow fooMap to be optional? Why not make httpPrefixHeaders be coupled with required?

[mtdowling]

We don't want people to have to provide an empty map on input.

[mtdowling]

We could require that the member is marked as @required or @default though in IDL v2, and automatically add the @default trait if needed when migrating 1.0 -> 2.0 models. I don't think it makes sense to change this for v1 models though since that would be a breaking change.

[JordonPhillips]

That test case is intended to assert that clients are not serializing anything when given an empty headers map, it doesn't really make any sense from a server perspective. So it should just be marked as client only. We could also add a server-only variant that omits the parameter.

Making it required/default doesn't make sense. It does make sense for streaming since streams will always be there because that's just how the http interfaces work. That's not the case here though, except in the fringe case of an empty prefix.

[david-perez]

I think the behavior of the client and the server should be symmetric in this aspect.

Making it required/default doesn't make sense.

Why? The crux of the matter is in the expressiveness of the wire format. How can HTTP headers convey the different semantics ascribed to a null/absent collection vs an empty collection? You need distinct syntaxes to convey different semantics. With JSON there's no problem (null or absence of a field vs a []). With HTTP headers it's not clear (with streaming, as you point out, it's also not even clear with simple shapes). You could say that a header name with no value indicates an empty collection but that is calling for all sorts of bugs in the implementations. Personally I would just make it @required or @default in IDL v2 and avoid them.

[mtdowling]

I think Jordon's suggestion is fine if we don't make changes to the requirements of @httpPrefixHeaders, and the client/server behavior would be symmetrical. There's nothing a client can serialize when they're given an empty map, but there's no way to test that assertion without making the test client-specific. A server-only test could be added to make a similar assertion for responses (i.e., given an empty map, don't serialize any headers).

As for coupling @httpPrefixHeaders to @required or @default, I'm not sure we have a strong reason here, and I'm worried that we'll be excluding valid HTTP headers that can be empty. For example, Accept and Accept-Encoding can be empty (Accept-Encoding set to an empty header has an example in the related RFC). There are also lots of examples in other tools where they shipped without support for empty headers, then were asked to support it, and eventually did support it. For example:

• How to send empty request headers? guzzle/guzzle#1882
• https://bugzilla.mozilla.org/show_bug.cgi?id=474845
• https://curl-library.cool.haxx.narkive.com/RgEqnf3j/adding-empty-http-headers-to-curl-requests
• etc

To address your specific concerns: httpPrefixHeaders can be bound to Map<String, String>. An empty map is unambiguous since nothing is ever sent on the wire. But what if someone sends an empty header? I propose we treat it as an empty string (""). HTTP client/server libraries can represent empty headers however they want (e.g., null, using a semicolon like what cURL does, etc.), but we could use an empty string during deserialization and in protocol tests. So if you got X-Foo: , then it would become {"Foo": ""}. If we had to account for nulls, then that would be problematic since the difference between "" and null is non-existent on the wire. So to fix that, we could forbid targeting a map marked as @sparse when using @httpPrefixHeaders (remember that maps don't support null by default).