[core-rest-pipeline] Preserve header casing by xirzec · Pull Request #18517 · Azure/azure-sdk-for-js

xirzec · 2021-11-03T22:18:21Z

Preserve original casing of HTTP header names when iterating over stored name/value pairs and optionally when converting HttpHeader collections back to JSON.

Fixes #18367

Preserve original casing of HTTP header names when converting HttpHeader collections back to JSON and when iterating over stored name/value pairs. Fixes Azure#18367

azure-sdk · 2021-11-03T22:33:35Z

API changes have been detected in @azure-rest/core-client. You can review API changes here

xirzec · 2021-11-03T22:56:43Z

API changes have been detected in @azure-rest/core-client. You can review API changes here

This looks like a bug - it's pulling changes from core-client... @praveenkuttappan any thoughts?

jeremymeng · 2021-11-04T00:15:06Z

sdk/core/core-rest-pipeline/src/httpHeaders.ts

-    for (const [key, value] of this._headersMap) {
-      result[key] = value;
+    for (const entry of this._headersMap.values()) {
+      result[entry.name] = entry.value;


@deyaaeldeen mentioned that this change of behavior could break existing code that assumes lower case key

The LRO engine assumes the retry header has lower case so perhaps we will need to explicitly lower the case there?

yeah I think it's better to fix the incorrect assumption.

Updating core-lro sounds good to me.

@deyaaeldeen I looked into this a bit more and would like your feedback.

Since the public surface of core-lro declares that RawResponse.headers are lowercase, perhaps we should preserve this behavior?

azure-sdk-for-js/sdk/core/core-lro/src/lroEngine/models.ts

Line 67 in 58beb3e

/** A HttpHeaders collection in the response represented as a simple JSON object where all header names have been normalized to be lower-case. */

The downside is that core-lro doesn't actually produce RawResponse objects, it only consumes them via generated code. So this is tricky to fix. I see some options:

We change core-lro wherever it reads headers to look for a lowercased version of each property name (kinda messy)

We update the codegen to lowercase all header names before passing them into core-lro

For the second one, I could add an optional parameter to toJSON() that lowercases the header names, to make the codegen very similar with toJSON({lowercase: true}) being the only change.

What do you think? Should we change the contract or update codegen?

@xirzec I think the main trade-off here is that normalization makes it easy to lookup standard headers but makes looking up custom headers inconvenient because these lookups are usually case-sensitive.

I think the best way to handle this is for core to provide two APIs, one for each case. This way, the generated code does not need to be updated.

@deyaaeldeen that's an interesting idea. Maybe we go the other way and have toJSON({preserveCase: true}) for this? @jeremymeng would that work for Storage?

@xirzec Storage doesn't use the headers directly. In core if we send the request using the original casing the it should be fine

azure-sdk-for-js/sdk/core/core-rest-pipeline/src/nodeHttpClient.ts

Line 211 in 61331e6

headers: request.headers.toJSON()

OK I ended up splitting the difference. I kept the original casing when iterating over the collection, since then you're not hunting for a particular value (and you can easily toLowercase each thing as you iterate), but I switched toJSON back to lowercasing unless a flag is passed in.

Now xhrHttpClient and nodeHttpClient should both do the right thing, which is send original case for header names. @deyaaeldeen @jeremymeng please take another look when you have time 😄

xirzec · 2021-11-04T22:14:23Z

@sadasant would love you to take a look at some changes needed to unbreak identity tests: 6306d66

For the Secret casing change I made it consistent with the MSI credential, but I'm also happy to leave it inconsistent and only change the tests.

sdk/identity/identity/src/credentials/managedIdentityCredential/fabricMsi.ts

sadasant

Looks good 👍

This still leaves the iterator with the original casing, so that we don't have to do special handling in xhrHttpClient in order to send the original casing.

jeremymeng

Looks good! Please update CHANGELOG. I am porting this fix to core-http and will open a PR soon.

deyaaeldeen · 2021-11-05T19:04:44Z

sdk/core/core-rest-pipeline/src/httpHeaders.ts

+      if (options.preserveCase) {
+        result[entry.name] = entry.value;
+      } else {
+        result[normalizedName] = entry.value;
+      }


NIT: perhaps factor out the preserveCase check out the loop so it fires only once?

Hm yeah I'm not sure if the runtime will optimize this correctly, so I'm going to do as you suggest

deyaaeldeen

Looks great!

azure-sdk · 2021-11-05T19:07:46Z

API changes have been detected in @azure/core-rest-pipeline. You can review API changes here

API changes

-     toJSON(): RawHttpHeaders;
+     toJSON(options?: {
+             preserveCase?: boolean;
+         }): RawHttpHeaders;

Since identity tests spy on the headers being sent (which have casing preserved now), we do need these changes even though the public interface is maintained.

xirzec · 2021-11-10T00:17:36Z

/check-enforcer override

[core-rest-pipeline] Preserve header casing

ae9fd9d

Preserve original casing of HTTP header names when converting HttpHeader collections back to JSON and when iterating over stored name/value pairs. Fixes Azure#18367

xirzec added Client This issue points to a problem in the data-plane of the library. Azure.Core labels Nov 3, 2021

xirzec requested review from deyaaeldeen, jeremymeng and joheredi November 3, 2021 22:18

xirzec self-assigned this Nov 3, 2021

jeremymeng reviewed Nov 4, 2021

View reviewed changes

xirzec added 2 commits November 4, 2021 13:26

Merge branch 'main' into preserveHeaderCase

39fb265

Fix some header casing breaks in identity

6306d66

xirzec requested review from sadasant, schaabs and witemple-msft as code owners November 4, 2021 22:12

sadasant reviewed Nov 4, 2021

View reviewed changes

sdk/identity/identity/src/credentials/managedIdentityCredential/fabricMsi.ts Show resolved Hide resolved

sadasant approved these changes Nov 4, 2021

View reviewed changes

xirzec added 2 commits November 5, 2021 10:29

undo identity changes

8299544

Change toJSON to lowercase by default.

f311cba

This still leaves the iterator with the original casing, so that we don't have to do special handling in xhrHttpClient in order to send the original casing.

jeremymeng approved these changes Nov 5, 2021

View reviewed changes

Update CHANGELOG and ApiView

2d3de2a

xirzec requested review from bterlson and ramya-rao-a as code owners November 5, 2021 18:57

Merge branch 'main' into preserveHeaderCase

a188abd

deyaaeldeen reviewed Nov 5, 2021

View reviewed changes

deyaaeldeen approved these changes Nov 5, 2021

View reviewed changes

xirzec added 2 commits November 5, 2021 13:26

Add identity changes back

f82f4fd

Since identity tests spy on the headers being sent (which have casing preserved now), we do need these changes even though the public interface is maintained.

PR feedback

82cc9a9

xirzec enabled auto-merge (squash) November 5, 2021 20:33

jeremymeng mentioned this pull request Nov 6, 2021

[core-http] preserve header key casing #18561

Merged

xirzec disabled auto-merge November 6, 2021 00:54

Update tables batch tests

7bfd41f

joheredi approved these changes Nov 9, 2021

View reviewed changes

xirzec enabled auto-merge (squash) November 9, 2021 22:28

xirzec added 2 commits November 9, 2021 14:34

Merge branch 'main' into preserveHeaderCase

7577a48

Fix more tests

1e382d3

xirzec merged commit 638d5eb into Azure:main Nov 10, 2021

xirzec deleted the preserveHeaderCase branch November 10, 2021 00:17

Conversation

xirzec commented Nov 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

azure-sdk commented Nov 3, 2021

Uh oh!

xirzec commented Nov 3, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xirzec commented Nov 4, 2021

Uh oh!

Uh oh!

sadasant left a comment

Choose a reason for hiding this comment

Uh oh!

jeremymeng left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deyaaeldeen left a comment

Choose a reason for hiding this comment

Uh oh!

azure-sdk commented Nov 5, 2021

Uh oh!

xirzec commented Nov 10, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

xirzec commented Nov 3, 2021 •

edited

Loading