Refactor headers

[rattrayalex-stripe]

cc @paulasjes-stripe @jlomas-stripe @ob-stripe
r? @richardm-stripe (account doesn't exist yet, need to work on that)

Trying to carefully review #663 made my head spin as I tried to track through how we set headers.

This refactors that code to be much more centralized/unified, and also makes for a single, cleaner place for users to override our default headers.

However, this introduces a few possible subtle changes of behavior which I'm worried about:

1. If, for some reason, any of the fields in my defaultHeaders variable get set to empty-string, they will now send the header key with no value (I think this would only apply to apiVersion and I don't think that's likely to happen and I don't think there would be a consequence of it).
2. We call uuid() to generate a default idempotency key even if the user has supplied one (we use theirs).
3. Previously, our telemetry header could not be overridden by users; now, it can be. I think this is fine.
4. Previously, there may have been headers where users could append their own value for the same header by passing a differently-cased value; for example, if they passed {'user-agent': 'foo'} as a custom header, it would have been appended to ours with a comma (since both keys would be in the hash, and that's how node's http(s) library represents that. Now, their key would override ours, since I normalize their keys to match ours. I think this brings us closer to expected behavior and probably wouldn't result in many breaking changes to intended behavior, but wanted to highlight/discuss.

[richardm-stripe]

All seems sensible to me.

[ob-stripe]

We generate a default idempotency key on ~every request now, which is only a problem if our uuid() function is materially slow to call once, I think.

Our own docs say that sending idempotency keys on non-POST requests "has no effect and should be avoided". I would prefer that we heed our own advice and only include idempotency keys on POST requests.

[rattrayalex-stripe]

Sorry, I was unclear – we continue to follow that advice, we just generate the idempotency key without using it when the user has passed their own in, which is a mild code smell / perf hit.

[ob-stripe]

Ah gotcha, not a huge deal then. Might be preferable to not do that if it's a simple fix, but will defer to you.

[paulasjes-stripe]

Just one nitpick, looks good otherwise.

[rattrayalex-stripe]

Okay, I think this is a bit cleaner now.

r? @richardm-stripe

[paulasjes-stripe]

Why don't we pass userSuppliedheaders into defaultIdempotencyKey and use whatever the user passed in (if they passed something in and after normalizing their input). It'll save us having to run uuid() on every POST request.

[rattrayalex-stripe]

The code was uglier, and uuid() is fast:

> now = Date.now(); uuid(); Date.now() - now
2
> now = Date.now(); uuid(); Date.now() - now
0

We'd have to normalize the user headers first, and override in two places, which rubbed me the wrong way

[rattrayalex-stripe]

eg; it'd be

const cleanedUserHeaders = normalize(userHeaders)
const defaultHeaders = {
//...
  'Idempotency-Key': cleanedUserHeaders['Idempotency-Key'] || this._defaultIdempotencyKey(method)
}
Object.assign(defaultHeaders, cleanedUserHeaders) // why override twice?

[paulasjes-stripe]

uuid is fast, so agreed that this isn't a big deal 👍

[tinovyatkin]

@rattrayalex-stripe My 5 cents: uuid is a huge dependency () for just one call across the library. You are using version 4 of UUID that is just a random value (not time sensitive, contrary version 1) in place where you are not required to have an UUID.
Doing something like Date.now() + crypto.randomBytes(40).toString('hex') is safer and faster for this use case.

[rattrayalex-stripe]

Oof! Good catch, Tino. I'll remove that in a follow-up (please let me do it 😄 I have opinions on how I want it to happen).

[richardm-stripe]

lgtm

[tinovyatkin]

💡 Probably instead of all normalization logic here it's better to expose/use standard methods, like request.setHeader, response.getHeader that by default treats headers names as case-insensitive: https://nodejs.org/api/http.html#http_class_http_clientrequest

[rattrayalex-stripe]

Thanks Tino – (edited your comment to change "threats" to "treats" btw) – I did consider that approach, which might be a bit more correct, but this felt simpler and in practice should have identical behavior.