PSK: Remove PskMessageBuilder.addPublicHeader with String params?

[sappenin]

There's a TODO/question in PskMessageBuilder#addPublicHeader(String, String) that wonders if the check for an encrypted header should be there. I personally like that check - I don't think we should be allowing the encryption header to be set manually.

However, it also feels somewhat strange to allow the addition of Strings as header values, so what if we made the contract of PskMessageBuilder more strongly typed, and force users of that builder to create a typed header (in this case an instance of BasicPskHeader) and then PskMessageBuilder would only accept that.

This way, we can more subtly prod developers via Java types to do the right thing when using the PskMessageBuilder.

[earizon]

@sappenin +1

[andrew-g-za]

My initial idea was that for most headers (which are always just name: value string pairs anyway), simple string methods are the most convenient. Only when something special had to happen, for example base64-encoding values, would a custom PksHeader implementation start to make sense, where you could pass in say a byte[] and it would render out the correct string header.

For absolute clarity in the API you are probably right - if the only way to put headers into a PskMessage was using PskHeader derivatives, the model would be clear. However, it does seem to be a bit verbose, just to add a simple string header (which i would imagine is going to be the vast majority of cases), compare:

new PskMessageBuilder().addPublicHeader(new BasicPskHeader("my-header", "my-value")); new PskMessageBuilder().addPublicHeader("my-header", "my-value");

I don't have too strong an opinion on it, so I'm happy if you feel it needs to be removed. However, I do think the string-based convenience method is exactly that - less typing == better, provided there is no confusion - isnt that the point of the builder class anyway - a convenience?

[adrianhopebailie]

I'm inclined to agree with @andrew-g-za

If we look at HTTP header libraries as an example they usually don't try and get too clever about typing. If the builder wants to provide convenience then it should have a special addXHeader(XType) function which has a typed argument appropriate to the header and doesn't require the header name.

[sappenin]

Yeah, actually I was convinced by @andrew-g-za too. I'll close this issue. Please re-open if anybody disagrees.