Add content headers to script headers object #1344
Conversation
There was a problem hiding this comment.
Do we have other tests passing multi-value headers, headers with quality, etc.? Would be good to make sure those are handled as expected.
There was a problem hiding this comment.
Another great comment, all of our multiple-value tests use User-Agent, which as mentioned in the other comment is its own special snowflake as it uses " " separator. I'll add some tests with the accept header which uses standard comma separators.
There was a problem hiding this comment.
Why using an empty space and not a comma here? This is a bit confusing. How is this being interpreted and why are we not using a comma?
There was a problem hiding this comment.
Great question, after looking at this httpheaders comment I'm going to go back to the original serializing code. Probably not worth it trying to simplify if there are weird edge cases like this - though I think that beyond User-Agent, , separator is the standard
There was a problem hiding this comment.
When concatenating these to ToString() values, is the first guaranteed to have a separator so they don't munge together?
There was a problem hiding this comment.
will add a newline separator to ensure that the headers are split
There was a problem hiding this comment.
The accept header is separated by commas + space - those are all parsed out the header collection ToString?
There was a problem hiding this comment.
We have logic here https://github.com/Azure/azure-webjobs-sdk-script/blob/658d4d0c9dce9af940bd1a4fd8d5fb0f2fb85e90/src/WebJobs.Script/Binding/HttpBinding.cs#L290 that goes the other way - takes top level headers and applies them to a response. So what you're doing makes this symmetric for request content.
There was a problem hiding this comment.
Can you add like 2 more content headers and verify them below to test the multiplicity? I.e. symmetric with https://github.com/Azure/azure-webjobs-sdk-script/blob/658d4d0c9dce9af940bd1a4fd8d5fb0f2fb85e90/src/WebJobs.Script/Binding/HttpBinding.cs#L290
|
Thanks! |
resolves #1314