Skip to content
This repository was archived by the owner on Jan 21, 2026. It is now read-only.

fix: don't let trace context injection throw#989

Merged
kjin merged 2 commits intogoogleapis:masterfrom
kjin:expect-more
Apr 3, 2019
Merged

fix: don't let trace context injection throw#989
kjin merged 2 commits intogoogleapis:masterfrom
kjin:expect-more

Conversation

@kjin
Copy link
Copy Markdown
Contributor

@kjin kjin commented Mar 26, 2019

It's possible for setHeader to throw if an outgoing request was created with a header Expect but not capitalized exactly that way. This change makes checking for the expect header a little more robust, and also swallows an error that would be thrown for further unexpected capitalizations.

It's possible that hasExpectHeader can either (1) stay the same or (2) be even more robust and actually iterate through all header keys to do a case-insensitive check. I'm definitely OK with the first option, the second option might have too much overhead to accomadate for a rare issue.

@kjin kjin requested a review from a team March 26, 2019 23:05
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Mar 26, 2019
@yoshi-automation yoshi-automation added the 🚨 This issue needs some love. label Apr 2, 2019
Copy link
Copy Markdown
Contributor

@ofrobots ofrobots left a comment

Choose a reason for hiding this comment

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

Can we have a test for this?

Comment thread src/plugins/plugin-http.ts
@kjin
Copy link
Copy Markdown
Contributor Author

kjin commented Apr 2, 2019

@ofrobots Thanks for the suggestions, this PR is ready for another look.

@kjin kjin force-pushed the expect-more branch 2 times, most recently from 0e92504 to e2fb08f Compare April 3, 2019 20:03
@kjin kjin merged commit 50421a5 into googleapis:master Apr 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla: yes This human has signed the Contributor License Agreement. 🚨 This issue needs some love.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants