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

Ensure Content-Length is Sent #170

Merged
merged 2 commits into from
Jun 11, 2022
Merged

Conversation

Matt-Pesce
Copy link
Contributor

Added specification of Content-Length during creation of the Request Body for URL Encoded and JSON Encoded content.

The existing implementation uses the ioutil.NopCloser method to create the Request body, which triggers ShouldSendChunkedRequestBody in https://go.dev/src/net/http/transfer.go that results in the Request Header specifying Transfer-Encoding: Chunked.

This change enables compatibility with Endpoints (such as Microsoft Graph APIs) that do not support Transfer-Encoding: Chunked

Testing: I've rebuilt the new http.go library into the Pixlet application: https://github.com/tidbyt/pixlet and hit multiple endpoints at Google and MSFT cloud with successful result (about 10 different API calls)

Added Content-Length for JSON and URL Encoded Request bodies.   This ensures that Content-Length is provided in the Request header.   This prevents Transfer-Encoding: chunked from being specified by https://go.dev/src/net/http/transfer.go    In particular, the Microsoft Graph API Token Endpoint doesn't permit Transfer-Encoding: chunked and this is the genesis of the fix.
@Matt-Pesce Matt-Pesce changed the title Add files via upload Ensure Content-Length is Sent Jun 7, 2022
@betterengineering
Copy link

cc @dustmop @b5 can you take a look?

Copy link
Member

@b5 b5 left a comment

Choose a reason for hiding this comment

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

Looks good! Just want to get the reason for setting ContentLength captured within the codebase, then this should be good to go

http/http.go Show resolved Hide resolved
Copy link
Member

@b5 b5 left a comment

Choose a reason for hiding this comment

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

LGTM!

@b5 b5 merged commit 7fb7ff9 into qri-io:master Jun 11, 2022
betterengineering added a commit to tidbyt/pixlet that referenced this pull request Jul 27, 2022
This commit bumps starlib to the main branch to pull in this change: qri-io/starlib#170
betterengineering added a commit to tidbyt/pixlet that referenced this pull request Jul 27, 2022
This commit bumps starlib to the main branch to pull in this change: qri-io/starlib#170
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.

None yet

3 participants