Header APIs consistency

[romain-grecourt]

Helidon Version: 4.0.0-ALPHA6

Header name type

We are forcing the use of HeaderName instead of String to ensure best practice for performance.
Thus we are explicitly not overloading the header related methods to support String as a type for the header name.

The following method exists:

Header.createCached("h1", "v");

• We should consider removing it for consistency.

Header value types

Also we have multiple variants of the header methods in order to support different value types, however there are some inconsistencies where some classes only have certain variants.

• We should ensure that all classes that provide header related methods have the same variants for header value types.

Header.create(HeaderName, ...)
Header.createCached(HeaderName, ...)
WritableHeaders.set(HeaderName, ...)
WritableHeaders.add(HeaderName, ...)
WebClient.Builder.headers(HeaderName, ...)
ClientRequest.headers(HeaderName, ...)

HeaderValue

The creation of an instance of HeaderValue is odd:

Header.create(Header.create("h"), "v");

Both HeaderName and HeaderValue are created via Header.create()

• We should consider something like this instead:

Header.create(HeaderName.create("foo"), "bar")

Also HeaderValue does not model a header value, but an actual pair of header name and header value, which is just a header.

• We should consider using the existing Http.Header class to model that or renaming HeaderValue to something else

MediaType

MediaType is now more abstract than it was and has been superseded in practice with HttpMediaType.

On one hand, MediaTypes defines constants for MediaType, but HttpMediaType defines its own constants.
Also, there is a discrepancy in the constants available, E.g. HttpMediaType.APPLICATION_XML is not defined.

This can be confusing for APIs that require the use of HttpMediaType as MediaTypes.

• We should probably create HttpMediaTypes for the constants of HttpMediatype to provide some symmetry and also reduce the gap of the constants available.

Both WritableHeaders.contentType(HttpMediaType mediaType) and WritableHeaders.contentType(MediaType mediaType) have the exact same implementation even though HttpMediaType extends Mediatype.

• We should consider removing WritableHeaders.contentType(HttpMediaType mediaType)

Both ServerResponseHeaders.addAcceptPatches(HttpMediaType... acceptableMediaTypes) and ServerResponseHeaders.addAcceptPatches(MediaType... acceptableMediaTypes) have the exact same implementation event though HttpMediaType extends Mediatype.

• We should consider removing ServerResponseHeaders.addAcceptPatches(HttpMediaType... acceptableMediaTypes)

Bulk

WritableHeaders does not have methods add headers of a given Headers instance.

• We should consider having a variant for WritableHeaders.add(Headers)

Not sure if WritableHeaders.set(Headers) makes sense.

[romain-grecourt]

CC @tomas-langer

[tomas-langer]

First proposal:

HTTP Header:

1. Rename Http.Header to Http.HeaderNames
2. Rename all methods that create header names to create (no createName method)
3. Move methods that create values to Http.HeaderValues (Http.Headers if we rename HeaderValue as mentioned below)
4. Rename all methods that create header values to create (no createValue method)
5. Consider adding a builder for values (to support changing, sensitive etc.)
6. Create all variants for creating values for (with String, String..., int, long, Iterable<String>)
   1. HeaderValues.createCached(String name, ...)
   2. HeaderValues.createCached(HeaderName name, ...)
   3. HeaderValues.create(String name, ...)
   4. HeaderValues.create(HeaderName name, ...)
   5. HeaderValues.create(HeaderName name, boolean, boolean, ...)
7. Rename HeaderValue to HttpHeader (or Header, but that would be confusing for backward compatibility)

HTTP Headers:

1. WritableHeaders - there is a factory method to create WritableHeaders from another instance, nevertheless we sometimes need to add/set all headers from another instance on an existing instance. We should add from(Headers) or update(Headers) or `set(HttpHeaders)
2. Consider which "shortcut" methods are needed on WritableHeaders for set/add (set(HeaderValue), set(HeaderName, String value)...)
3. Consider which "shortcut" methods are needed on ClientRequest and ServerResponse (if any) - either remove existing and require update through header consumer, or add consistent
4. Consider renaming Headers to HttpHeaders (or just make sure we do not use the same name for this class as for something that is an inner class of Http)

Media types:

1. Remove methods on headers that accept HttpMediaType, unless they behave differently (they should, as HttpMediaType can add parameters)
2. Constants on HttpMediaTypes should only contain constants with additional parameters and be named accordingly, never duplicate MediaTypes constants (to make the distinction clear - HttpMediaType is a MediaType with parameters)

[tomas-langer]

Option if HeaderValue and HeaderValues is not OK:
HTTP Header

headers.add(Http.HeaderNames.ACCEPT, MediaTypes.TEXT_PLAIN.text()); (1)
Http.HeaderName name = Http.HeaderNames.create("accept", "Accept"); (2)
Http.Header header = Http.Headers.create(name, "value"); (3) (7)
Http.Header header2 = Http.Headers.createCached("Accept", "value"); (6) (7)

HTTP Headers

HttpHeaders requestHeaders = request.headers(); (4)
WritableHeaders<?> headers = WritableHeaders.create();
headers.update(requestHeaders); (1) // copy all headers from requestHeaders to headers

[spericas]

@tomas-langer Regarding initial comment.

First section:

• 1-6 LGTM
• 7 seems like Http.Header is better than Http.HttpHeader if I understand the options

Don't have a strong opinion on the changes proposed in the other two sections.

[tomas-langer]

• 7 seems like Http.Header is better than Http.HttpHeader if I understand the options

Agreed, Header seems to be the right one. This will mean that Headers will contain the "known" values.
This is the only problem I see:

• Http.Headers - known headers with values (such as CONNECTION_CLOSE)
• Headers (or HttpHeaders) - the container of Headers used in request and response

If anybody has a great idea here, please share, I am just not sure.

[tomas-langer]

The PR is merged, the problems raised in this issue are all addressed (sometimes not in the way proposed here).
If there is further concern regarding Header or MediaType APIs, please raise a new issue.