Handle HTTP headers as case insensitive

[tinovyatkin]

By HTTP standard headers are case insensitive, so checking for existing Idempotency-Key like headers.hasOwnProperty('Idempotency-Key') is wrong. Should we fix that?

[paulasjes-stripe]

We always set the header name to use the format Idempotency-Keys when a key is provided in a request, however it can't hurt to be extra safe.

Could you add tests for this change? They should go under the Network Retry section here.

[paulasjes-stripe]

If we're doing this for Idempotency-Key, then we should do the same for Stripe-Version and Stripe-Account

stripe-node/lib/utils.js

Lines 141 to 146 in a5885d7

	if (params.stripe_account) {
	opts.headers['Stripe-Account'] = params.stripe_account;
	}
	if (params.stripe_version) {
	opts.headers['Stripe-Version'] = params.stripe_version;
	}

Can you move this functionality to a helper function in utils and reuse it for the version and account headers?

[tinovyatkin]

There is no problem with setter of headers, only getter of headers must be case insensitive.

[tinovyatkin]

What I see weird here, is that you are using hardcoded string like this at setters and then lowecased hardcoded strings at getters:

stripe-node/lib/StripeResource.js

Line 127 in a5885d7

api_version: headers['stripe-version'],

It looks like a bad style and human errors prone. I would prefer move all private headers to a http-headers.js file as lowercase constants and use constants everywhere. Are you OK with that?

[rattrayalex-stripe]

While I don't think a separate constants file is necessary, I think that's fine!

[rattrayalex-stripe]

I do think we should have a getHeader(header, 'Header-Name') function in utils, and use that to check whether idempotency-key is already defined, and also to get the stripe-version/stripe-account values.

[rattrayalex-stripe]

(Alternatively, we could normalize the headers hash to be all lower-case keys prior to this processing)

[tinovyatkin]

While I don't think a separate constants file is necessary, I think that's fine!

You already have around 10 private HTTP headers sprinkled all around SDK. Some of them are prefixed with X-, like X-Stripe-Client-Telemetry, some doesn't like Stripe-Signature or Stripe-Version. Having them all in the same file will set good conventions how to add new one and enforce constants use to prevent any errors in future version regarding them. If you are OK, I'll try to do that today in this PR.

[rattrayalex-stripe]

Sure, if you think it would help readability to users, please do!

[rattrayalex-stripe]

Hey Tino, Just wanted to say thanks again for all your work here. I'm pretty busy today but hope to give this and #661 a thorough review Monday or Tuesday; appreciate your patience!

[rattrayalex-stripe]

Trying to give this a thorough, careful review made my head spin because our headers were just all over the place (our fault, not yours) so I threw together #667 which ended up obviating this, so closing.

I know you were looking forward to exporting constants for the headers, but on second thought I'd rather not treat this as a public API, and with #667 it's at least easy to see them in one place. I hope you understand.

Thanks for all your terrific work here @tinovyatkin !