Fixed bug in ParseOkHttpClient which didn't handle ParseFile File uploads properly.

[mgarnerdev]

This check was unnecessary and in the event of a ParseFileHttpBody it would result in a null body for the Http POST which throws an exception.
Alternatively it could be replaced with the following:

if(parseBody instanceof ParseByteArrayHttpBody || parseBody instanceof ParseFileHttpBody) {
  okHttpRequestBody = new ParseOkHttpRequestBody(parseBody);
}

However both of those classes extend ParseHttpBody and they are the only classes that do so and the constructor for ParseOkHttpRequestBody is expecting type ParseHttpBody which they both extend so it seems as though the check is unnecessary.

This fixes File uploads as part of a ParseFile. Previously it was throwing the exception:

com.parse.ParseException: java.lang.IllegalArgumentException: method POST must have a request body.
01-22 14:03:12.982 24657-24657/com.plus11.ncredible W/System.err:     at com.parse.ParseTaskUtils$2$1.run(ParseTaskUtils.java:114)
01-22 14:03:12.983 24657-24657/com.plus11.ncredible W/System.err:     at android.os.Handler.handleCallback(Handler.java:739)
01-22 14:03:12.983 24657-24657/com.plus11.ncredible W/System.err:     at android.os.Handler.dispatchMessage(Handler.java:95)
01-22 14:03:12.983 24657-24657/com.plus11.ncredible W/System.err:     at android.os.Looper.loop(Looper.java:148)
01-22 14:03:12.983 24657-24657/com.plus11.ncredible W/System.err:     at android.app.ActivityThread.main(ActivityThread.java:5417)
01-22 14:03:12.983 24657-24657/com.plus11.ncredible W/System.err:     at java.lang.reflect.Method.invoke(Native Method)
01-22 14:03:12.983 24657-24657/com.plus11.ncredible W/System.err:     at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:726)
01-22 14:03:12.983 24657-24657/com.plus11.ncredible W/System.err:     at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:616)
01-22 14:03:12.984 24657-24657/com.plus11.ncredible W/System.err: Caused by: java.lang.IllegalArgumentException: method POST must have a request body.
01-22 14:03:12.984 24657-24657/com.plus11.ncredible W/System.err:     at com.squareup.okhttp.Request$Builder.method(Request.java:256)
01-22 14:03:12.984 24657-24657/com.plus11.ncredible W/System.err:     at com.squareup.okhttp.Request$Builder.post(Request.java:229)
01-22 14:03:12.985 24657-24657/com.plus11.ncredible W/System.err:     at com.parse.ParseOkHttpClient.getRequest(ParseOkHttpClient.java:157)
01-22 14:03:12.985 24657-24657/com.plus11.ncredible W/System.err:     at com.parse.ParseOkHttpClient.executeInternal(ParseOkHttpClient.java:66)
01-22 14:03:12.985 24657-24657/com.plus11.ncredible W/System.err:     at com.parse.ParseHttpClient$ParseNetworkInterceptorChain.proceed(ParseHttpClient.java:158)
01-22 14:03:12.985 24657-24657/com.plus11.ncredible W/System.err:     at com.parse.ParsePlugins$1.intercept(ParsePlugins.java:115)
01-22 14:03:12.985 24657-24657/com.plus11.ncredible W/System.err:     at com.parse.ParseHttpClient$ParseNetworkInterceptorChain.proceed(ParseHttpClient.java:147)
01-22 14:03:12.985 24657-24657/com.plus11.ncredible W/System.err:     at com.parse.ParseHttpClient.execute(ParseHttpClient.java:122)
01-22 14:03:12.986 24657-24657/com.plus11.ncredible W/System.err:     at com.parse.ParseRequest$3.then(ParseRequest.java:136)
01-22 14:03:12.986 24657-24657/com.plus11.ncredible W/System.err:     at com.parse.ParseRequest$3.then(ParseRequest.java:133)
01-22 14:03:12.986 24657-24657/com.plus11.ncredible W/System.err:     at bolts.Task$15.run(Task.java:839)
01-22 14:03:12.986 24657-24657/com.plus11.ncredible W/System.err:     at bolts.BoltsExecutors$ImmediateExecutor.execute(BoltsExecutors.java:105)
01-22 14:03:12.986 24657-24657/com.plus11.ncredible W/System.err:     at bolts.Task.completeAfterTask(Task.java:830)
01-22 14:03:12.986 24657-24657/com.plus11.ncredible W/System.err:     at bolts.Task.continueWithTask(Task.java:642)
01-22 14:03:12.987 24657-24657/com.plus11.ncredible W/System.err:     at bolts.Task.continueWithTask(Task.java:653)
01-22 14:03:12.987 24657-24657/com.plus11.ncredible W/System.err:     at bolts.Task$13.then(Task.java:745)
01-22 14:03:12.987 24657-24657/com.plus11.ncredible W/System.err:     at bolts.Task$13.then(Task.java:733)
01-22 14:03:12.987 24657-24657/com.plus11.ncredible W/System.err:     at bolts.Task$15.run(Task.java:839)
01-22 14:03:12.987 24657-24657/com.plus11.ncredible W/System.err:     at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1113)
01-22 14:03:12.987 24657-24657/com.plus11.ncredible W/System.err:     at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:588)
01-22 14:03:12.988 24657-24657/com.plus11.ncredible W/System.err:     at java.lang.Thread.run(Thread.java:818)

[facebook-github-bot]

By analyzing the blame information on this pull request, we identified @grantland and @wangmengyan95 to be potential reviewers.

[facebook-github-bot]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[wangmengyan95]

Hi @michaelgarnerdev, good catch. This change make sense to me. Is it possible for you to add a unit test case to cover this case? Thanks!

[mgarnerdev]

Hi @wangmengyan95,

I looked into adding a test case, but it would seem there is already a test case for creating a ParseFile with a File and saveAsync. It would appear to me, that it is covered, but if not I'm not too sure how I would write one to test something this deep in the APIs.

[mgarnerdev]

Also, where is the appropriate place for feature requests? I would like an additional constructor in ParseFile to create ParseFile like this:
It might require almost nothing to add this, but at worst it means copying the File to a new destination before creating the ParseFile.
File file = new File("PATH");
ParseFile parseFile = new ParseFile("new-desired-filename", file);

[grantland]

I think this will cause NPE if parseBody is null when actually executing the request since all the methods on `ParseOkHttpRequestBody use it: https://github.com/ParsePlatform/Parse-SDK-Android/blob/master/Parse/src/main/java/com/parse/ParseOkHttpClient.java#L281

[wangmengyan95]

Ideally, I do not think it will cause NPE sicne we only use okHttpRequestBody for POST and PUT request. In that situation parseBody should not be null.
If we add the null checking, when the POST or PUT has a null body, okHttpRequestBuilder will throw IllegalArgumentException.
If we do not add the null checking, when the POST or PUT has a null body, the exception will be thrown when we actually write the body.
In this situation, I think probably it is better to add the null checking. Let's prevent the error as early as possible.

[grantland]

@michaelgarnerdev Mind adding the null check?

[grantland]

You can add unit tests here: https://github.com/ParsePlatform/Parse-SDK-Android/blob/master/Parse/src/test/java/com/parse/ParseOkHttpClientTest.java

[grantland]

@wangmengyan95 any idea why integration tests didn't catch this issue?

[wangmengyan95]

@grantland @michaelgarnerdev our test cases related to httpClient is here. In the test cases, we just use the ParseByteArrayHttpBody to build the request. It seems we should also use ParseByteArrayHttpBody to build the request.

[facebook-github-bot]

@michaelgarnerdev updated the pull request.

[dan-dr]

Why isn't this merged?
I'm getting the "method POST must have a request body." error when saving a ParseFile only is some cases. not sure why it works in some cases but not in others

Any known workaround?

[grantland]

Waiting on a revision to the PR to add null check.

[mgarnerdev]

I'm no longer developing with Parse so I recommend someone else does this.

On Mon, Nov 7, 2016, 14:39 Grantland Chew notifications@github.com wrote:

Waiting on a revision to the PR to add null check.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#364 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AMuLNOINTRDOIB2yCjM3gKWtewupbaNUks5q76itgaJpZM4HKtXk
.

[dan-dr]

PR'd #542

[rogerhu]

This PR can be closed in lieu of #542