Replace synthetic headers with request attributes

[aruediger]

They are confusing in logs or when debugging and have to be manually filtered out when proxing reuqests.

[ktoso]

I think it would be nice to make the .headers not include the "internal" headers, however add some additional method which would show all headers - WDYT about that way of going about this? It causes not much changes in how we use them nowadays and would solve the problem of people seeing those.

[jlprat]

So your proposal @ktoso is hide the synthetic headers and only retrieve them when is explicitly said, right?
Doesn't sound bad. Replacing them with attributes, I'm not so sure, it's a pretty common thing to use custom headers to pass some extra info to the server...

[aruediger]

@jlprat

it's a pretty common thing to use custom headers to pass some extra info to the server...

Yes. But these synthetic headers are added after the request was received by the server. IMO users should be confident that what they see is what was actually sent by the client (or. intermediate proxies).

@ktoso

I think it would be nice to make the .headers not include the "internal" headers, however add some additional method which would show all headers - WDYT about that way of going about this?

When users log requests/responses they'll see them anyway.

I'm just saying that treating message headers as a store for everything but the kitchen sink might not scale as there will always be demand for attaching additional infos to messages.

This discussion comes up every once in a while (partly because of me bringing this up) so I thought opening a ticket might be best to discuss improvements.

[ktoso]

When users log requests/responses they'll see them anyway.

No, the intent is to completely hide them from users unless accessed directly using a header[SpecialThingy] or other internal allEvenTheInternalHeaders etc. Headers would get some internal marker, be it a trait or something else by which we decide which ones not to show

We can after all override the toString of the case class.
And change the def headers to do the filtering.

I think this should be possible to pull of in a binary compatible way.

[jlprat]

I was confused because you commented on #888 and that one is about custom headers sent by the client (at least in the example there)

[aruediger]

Yes. I was just reacting to @jypma's comment over there.

[jlprat]

Ah, then I said nothing

…

On Tue, Feb 28, 2017, 11:20 André Rüdiger ***@***.***> wrote: Yes. I was just reacting to @jypma <https://github.com/jypma>'s comment <#888 (comment)> over there. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#911 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADLuC8dh3zF8_ZdQuZu-IpZRKyylzB0Nks5rg_TvgaJpZM4MOLVZ> .

[jypma]

From my point of view, the "internal" headers only exist because no arbitrary request attribute feature existed yet. There are two strong arguments here for refactoring the synthetic headers to become request attributes:

• Users need this all the time, e.g. custom auth data, custom request token tracing, ...
• Calling things "headers" that aren't HTTP headers is extremely confusing (as shown above in this same PR) if it's not always stuff that the client sends.

However, given binary compatibility, hiding the current internal ones as best we can (e.g. using the flag) improve things.

I feel that in the end, request.headers really should ONLY show what as actually given by the client, even down to Host: and friends. Everything else should move into request.attributes, which we can still keep in a typed/safe way like the DSL on headers does. Since it'd be an addition, it actually wouldn't break binary compat immediately, would it?

[jlprat]

I agree except for the auth ones, I still think those are fine as headers, as in my opinion, they are not synthetic but just custom ones and are definitely send by the client. Am I missing or misinterpreting something?

[jypma]

Oh sure you're right about parsing and interpreting the actual Authorization: header and custom authorization headers. When I said "custom auth", I meant user-specific UserInformation-like classes that would cache authorization rights, roles, favourite cat, etc. for a real user. So application-specific. Those'd go into request.attributes.

[ktoso]

I'll get back here after some more pondering, but tracing for example would end up being a header anyway since that's how all x-trace-like tech propagates the trace anyway. We also instrument things into the request anyway, so I would like to not conflate the tracing story with this use case where we have the timeouts etc inside here.

[jlprat]

OK, now I understand your concerns and I tend to agree, conceptually, they belong to attributes.

[jrudolph]

The set of headers to use is not fixed to any particular use case and scope. Some headers are only used for proxies, some are only allowed to hop-by-hop, some are user-defined, etc. It's generic enough to carry all kinds of metadata for HTTP messages.

Also, spray and akka-http never represented the request or response headers as seen on the wire. Our model is already an interpretation of the raw data we read.

Regardless whether it's our internal headers or whatever headers proxies or reverse proxies added, you cannot assume to understand all headers that are in the list as you are not in control of generating the headers.

In summary, I don't share your fear that additional headers confuse people or are inappropriate to store arbitrary metadata. (I actually haven't never seen someone complaining about internal headers but that doesn't have to say anything, but maybe, @ktoso, have you heard about reported issues?)

That said, I'm not strongly opposing the idea of introducing something like request / response attributes. What I wouldn't like to see is having to duplicate the header infrastructure to access attributes (Java/Scala model and directives). Also, there's the practical problem that we cannot compatibly evolve the Scala HttpRequest / HttpResponse classes to introduce attributes in a proper way.

[aruediger]

The set of headers to use is not fixed to any particular use case and scope. Some headers are only used for proxies, some are only allowed to hop-by-hop, some are user-defined, etc. It's generic enough to carry all kinds of metadata for HTTP messages.

I don't want to limit what can be put into headers. Users should be able to add anything they want. I'm just saying that we're exposing implementation details that are of no value to the users.

Also, spray and akka-http never represented the request or response headers as seen on the wire. Our model is already an interpretation of the raw data we read.

But it's a rather close representation of what was transferred.

Regardless whether it's our internal headers or whatever headers proxies or reverse proxies added, you cannot assume to understand all headers that are in the list as you are not in control of generating the headers.

Agreed.

[nrktkt]

Poking an old issue here, but I still run up against it pretty often (particularly the custom user information case @jypma mentioned). We also see some fairly elaborate articles around the web to add this functionality indicating there is demand for it.

What are everyone's thoughts on adding something like

def attributes: immutable.Seq[RequestAttribute]

on RequestContext rather than on HttpMessage or HttpRequest? This could be added for users to leverage today, and migrating synthetic headers to attributes could be done later.