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

Use StringContent rather than StreamContent for text based Mime Types #122

Closed
wants to merge 5 commits into from

Conversation

arale61
Copy link

@arale61 arale61 commented Sep 30, 2024

Little improvement to proxy request with HttpRequestMessage.HttpContent as StringContent rather than StreamContent, only for content based requests (POST/PUT/PATCH) that have an "application/json" or "application/xml" (can be extended but my understanding is that this covers major use cases).

According to documentation, StringContent is recommended for text based content requests rather than StreamContent. StringContent is associated with text based Mime Type content requests while StreamContent seems to be convenient for octet-stream based requests.

This improvement should be transparent to the current functionality and enables further integration with other libraries that provide specific HttpClient implementations (such as AwsSignatureVersion4 nugget package).

arale61 and others added 2 commits September 29, 2024 16:30
…cation/xml as StringContent httpContent types
…-for-text-based-mime-types

Extend CreateProxiedHttpRequests to handle application/json and appli…
Copy link

@squidjam squidjam left a comment

Choose a reason for hiding this comment

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

Seems good to me!

@twitchax
Copy link
Owner

Should we also add text/html?

I'm not sure I 100% understand the impetus. What do those other libraries do that doesn't work with the regular StremContent?

@arale61
Copy link
Author

arale61 commented Oct 1, 2024

Good Morning,
Could pls check your email? I'd sent some details explaining the reasons 2 days ago.
Thanks,

@arale61
Copy link
Author

arale61 commented Oct 1, 2024

For text/html I believe it's not so convenient since you already manage the form type submit a part. This change would affect POST/PUT/PATCH for json or xml based requests to be handled as StringContent, allowing to read the resultant stream as seekable allowing to read the content properly.
When using StreamContent, which reading the dotnet docs and source, you can see it's convenient for octem-based streams, like uploading chunks (streaming), or zips and so on. At the other hand, StringContent produces seekable streams and other libraries can proceed to read the content normally for doing their job.
In my specific use case, I'm using AwsSignatureVersion4 as an http client injected that is used for signing requests with AWS creds before sending them. It will find when trying to send POST/PUT/PATCH for json for example, because the reasons mentioned here.
Please, make sure to try to read my email, or if it's not possible, let me know I will repeat here the details I can.

@arale61
Copy link
Author

arale61 commented Oct 1, 2024

Oh, sorry, "text/html" yes could be also. Potentially any text based mimy type could benefit from this.
The reason I just used json and xml is because at the moment the most common text based content requests are json or xml based, but you are right, in case we want to proxy other more legacy types like direct text, html or csv for post/put/patch then yes, you are right.

@twitchax
Copy link
Owner

twitchax commented Oct 5, 2024

Ok, sounds good. Any way we can get a treat added that exercises this, and would fail if it did not use StringContent properly?

Essentially, this is the type of case that needs to be protected by a test. Otherwise, someone is going to come along in 3 years and revert because noone remembers why it was changed to this implementation.

@arale61
Copy link
Author

arale61 commented Oct 6, 2024

Yes, I agree that a test can help to demonstrate the reasoning.
A test with a custom http client trying to read the content for doing something else, like adding a header with the md5 digest of the content.
In case a post/put/patch is used and no content-type as application/json or application/xml will fail (or any other that is not handled in code) since a StreamContent will be created as the HttpContent producing the issues mentioned previously.
I had a bit of time to detail and extend on details.
If you can, check this https://github.com/arale61/ProxyAndSignError/blob/main/README.md
And if you want to give it a try, here are some notes for testing it:
https://github.com/arale61/ProxyAndSignError/blob/main/RUN_SAMPLE.md

arale61 and others added 2 commits October 7, 2024 01:23
…-for-text-based-mime-types

Extend text based mime types and add the usesStreamContent to false f…
@arale61 arale61 closed this Oct 15, 2024
@arale61
Copy link
Author

arale61 commented Oct 15, 2024

Hi,
Finally I have decided to refuse to use ASPNETCore.Proxy since the integration with AwsSignatureVersion4 was not working well.
The signature done by the injected HttpClient from AwsSignatureVersion4 was not matching the verification on AWS side.
There are some HTTP headers involved on this problem.
I finally come up with a very light-weight proxy controller that for now covers my requirements for the proxy related functionality I need to have.

Thanks anyways for the time and effort.

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

Successfully merging this pull request may close these issues.

4 participants