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

fix: missing HTTP method in cronet client #1035

Closed

Conversation

kientux
Copy link

@kientux kientux commented Oct 25, 2023

In send function of CronetClient, HTTP method in request is never set to the builder. With default behavior, Cronet will use GET when there's no body and POST otherwise, so every PUT request will become POST.

This PR just set the missing HTTP method to the builder.

@google-cla
Copy link

google-cla bot commented Oct 25, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@kientux kientux force-pushed the fix-missing-cronet-http-method branch from c7592f0 to 38eb88f Compare October 25, 2023 08:11
@kevmoo kevmoo requested a review from brianquinlan November 21, 2023 20:56
@@ -332,6 +332,7 @@ class CronetClient extends BaseClient {
builder.setUploadDataProvider(
jb.UploadDataProviders.create2(body.toJByteBuffer()), _executor);
}
builder.setHttpMethod(request.method.toJString());
Copy link
Member

Choose a reason for hiding this comment

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

Weird this is not caught in an existing test.

@brianquinlan ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Because I accidentally deleted the test while also breaking the code :-(

@brianquinlan
Copy link
Collaborator

I've incorporated your change into #1058 - thanks for the PR!

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.

3 participants