Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[core-http] use the original header name when returning raw headers #18321

Closed
wants to merge 3 commits into from

Conversation

jeremymeng
Copy link
Member

@jeremymeng jeremymeng commented Oct 22, 2021

so that the casing of header names is preserved in the result of rawHeaders().

Relevant issue: #8117

Raw headers are copied to proxy options
@ghost ghost added the Azure.Core label Oct 22, 2021
@jeremymeng
Copy link
Member Author

/azp run js - storage-blob - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jeremymeng jeremymeng marked this pull request as ready for review October 22, 2021 22:40
@jeremymeng
Copy link
Member Author

/azp run js - storage-file-share - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jeremymeng
Copy link
Member Author

/azp run js - storage-file-datalake - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jeremymeng
Copy link
Member Author

/azp run js - service-bus - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@deyaaeldeen deyaaeldeen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Also, all tests passed so I think we do not have any tests sensitive to headers casing.

@jeremymeng
Copy link
Member Author

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

Latest core-lro depends on core-rest-pipeline, which doesn't keep information about the original header names (I will log a separate issue for that). This PR only fixes core-http.

Core-http's rawHeaders() is impacted, thus toJson() and toString() too. We use rawHeaders() to

I have not found real usage of toJson() or toString() in our repo. Customers may have used the raw headers from the response. I would still think this is fixing a buggy behavior though.

@deyaaeldeen
Copy link
Member

@jeremymeng core-lro is core client agnostic and should work with either core-http or core-client.

@jeremymeng
Copy link
Member Author

@jeremymeng core-lro is core client agnostic and should work with either core-http or core-client.

@deyaaeldeen Thanks! I see it now. Then this PR would would break it since it is calling toJson()

@jeremymeng jeremymeng marked this pull request as draft October 25, 2021 18:54
@jeremymeng
Copy link
Member Author

The following approach may work

  • update toJson() to not wrap around rawHeaders. Have it return an object with all lowercase header keys so the behavior is unchanged. Any existing consumers of toJson() will continue to work
  • update rawHeaders() to use the original header keys. This would have a different behavior but I think is more correct. or
  • we could update FetchHttpClient to re-assemble the original headers using HttpHeaders.headersArray() since it has the original information. However I feel fixing rawHeaders() is better.

I opened another PR 18348 to fix clone() behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants