Skip to content
This repository has been archived by the owner on Apr 26, 2023. It is now read-only.

Copy original HTTP request headers #433

Merged
merged 3 commits into from
Aug 21, 2017
Merged

Copy original HTTP request headers #433

merged 3 commits into from
Aug 21, 2017

Conversation

gavinr
Copy link
Contributor

@gavinr gavinr commented Aug 14, 2017

@bsvensson
Copy link
Member

Thanks @gavinr :)

This PR has quite a few changes and potential for breaking unexpected things with the header changes. I tested it successfully with my 26 test cases. @MikeTschudi @jgravois et al - it'd be great and if you could also review/test.

I plan to merge this PR if I don't hear anything the next few days...

@esoekianto
Copy link
Contributor

I remember we started passing thru all header value in the past during this issue #87 , Header, "Content-Disposition".

@bsvensson does any of your 26 test cases cover attachment scenario per issue #87 ?

If it does, we should be good.

@bsvensson
Copy link
Member

@bsvensson does any of your 26 test cases cover attachment scenario per issue #87

No.

@MikeTschudi
Copy link
Member

@gavinr, some questions and comments about the DotNet flavor, please.

  • In forwardToServer(), the older code postBody.Length > 0?... remains; it's commented out, but is not needed even as a comment--the new code is better.
  • In copyRequestHeaders(), how about executing headerKey.ToLower() once into a local variable? Even though the compiler can handle optimizing the 11 repeats of the call, it would make the code more readable.

@bsvensson

@gavinr
Copy link
Contributor Author

gavinr commented Aug 18, 2017

Thanks @MikeTschudi - those changes have been made. Please take a look to make sure I have completed them in alignment with what you were thinking.

@MikeTschudi
Copy link
Member

@gavinr, thanks!
@bsvensson, looks good to go from my perspective

@bsvensson bsvensson merged commit 3112776 into Esri:master Aug 21, 2017
@bsvensson
Copy link
Member

Thanks @jwoyame @gavinr @MikeTschudi

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

Successfully merging this pull request may close these issues.

4 participants