Issue 361: MediaDownloader can't download drive export list (which includes query parameters)

Issue 361: MediaDownloader can't download drive export list (which includes query parameters)

[ngmiceli, 2013-10-04 20:12:22]

https://codereview.appspot.com/13480044/diff/12001/Src/GoogleApis/Apis/Http/C...
File Src/GoogleApis/Apis/Http/ConfigurableMessageHandler.cs (right):

https://codereview.appspot.com/13480044/diff/12001/Src/GoogleApis/Apis/Http/C...
Src/GoogleApis/Apis/Http/ConfigurableMessageHandler.cs:97: /// only the allowed
tries of >=400 responses, or in a case an exception was thrown.
This is fine on a technical basis, but it's a little confusing to the user if
they haven't read the source code. What happens if the numTries = 0, but
numRedirects  = 10? While I know that means it won't retry OR redirect, the user
may expect that means ONLY handle redirects, don't do any other retries. The
interactions between the two are not well explained here.

https://codereview.appspot.com/13480044/diff/12001/Src/GoogleApis/Apis/[Media...
File Src/GoogleApis/Apis/[Media]/Download/MediaDownloader.cs (right):

https://codereview.appspot.com/13480044/diff/12001/Src/GoogleApis/Apis/[Media...
Src/GoogleApis/Apis/[Media]/Download/MediaDownloader.cs:205: var pairs = from
parameter in query.Split('&')
This block of code is rather generic. Are you sure there isn't a built in method
to do this? If not, perhaps you could make it a utility method?

https://codereview.appspot.com/13480044/diff/12001/Src/GoogleApis/Apis/[Media...
Src/GoogleApis/Apis/[Media]/Download/MediaDownloader.cs:247: // content range is
null in case the server doesn't adhere the media download protocol, in
"in the case" -> "when"

https://codereview.appspot.com/13480044/diff/12001/Src/GoogleApis/Apis/[Media...
Src/GoogleApis/Apis/[Media]/Download/MediaDownloader.cs:249:
currentRequestFirstBytePos = mediaContentLength =
Is this logic tested?

[peleyal, 2013-10-10 20:06:37]

https://codereview.appspot.com/13480044/diff/12001/Src/GoogleApis/Apis/Http/C...
File Src/GoogleApis/Apis/Http/ConfigurableMessageHandler.cs (right):

https://codereview.appspot.com/13480044/diff/12001/Src/GoogleApis/Apis/Http/C...
Src/GoogleApis/Apis/Http/ConfigurableMessageHandler.cs:97: /// only the allowed
tries of >=400 responses, or in a case an exception was thrown.
numTries can't be 0. it should be more than 1.
so if numTries = 1 and numRedirect = 10, I think it's obvious, am I right?


On 2013/10/04 20:12:23, ngmiceli wrote:
> This is fine on a technical basis, but it's a little confusing to the user if
> they haven't read the source code. What happens if the numTries = 0, but
> numRedirects  = 10? While I know that means it won't retry OR redirect, the
user
> may expect that means ONLY handle redirects, don't do any other retries. The
> interactions between the two are not well explained here.

https://codereview.appspot.com/13480044/diff/12001/Src/GoogleApis/Apis/[Media...
File Src/GoogleApis/Apis/[Media]/Download/MediaDownloader.cs (right):

https://codereview.appspot.com/13480044/diff/12001/Src/GoogleApis/Apis/[Media...
Src/GoogleApis/Apis/[Media]/Download/MediaDownloader.cs:205: var pairs = from
parameter in query.Split('&')
ParseQueryString is not PCL!
I don't need that in any other place, so I don't think to add a utility now,
unless it's important for you, then I'll do that :)
On 2013/10/04 20:12:23, ngmiceli wrote:
> This block of code is rather generic. Are you sure there isn't a built in
method
> to do this? If not, perhaps you could make it a utility method?

https://codereview.appspot.com/13480044/diff/12001/Src/GoogleApis/Apis/[Media...
Src/GoogleApis/Apis/[Media]/Download/MediaDownloader.cs:247: // content range is
null in case the server doesn't adhere the media download protocol, in
On 2013/10/04 20:12:23, ngmiceli wrote:
> "in the case" -> "when"

Done.

https://codereview.appspot.com/13480044/diff/12001/Src/GoogleApis/Apis/[Media...
Src/GoogleApis/Apis/[Media]/Download/MediaDownloader.cs:249:
currentRequestFirstBytePos = mediaContentLength =
I ran it with excel sheet in the drive API. 
I didn't add test for it yet. All the trick is to stop after the first request,
because range header is empty, so this is the first and last chunk. nothing
special

On 2013/10/04 20:12:23, ngmiceli wrote:
> Is this logic tested?

[ngmiceli, 2013-10-14 19:27:10]

LGTM

Just patch up documentation

https://codereview.appspot.com/13480044/diff/22001/Src/GoogleApis/Apis/Http/C...
File Src/GoogleApis/Apis/Http/ConfigurableMessageHandler.cs (right):

https://codereview.appspot.com/13480044/diff/22001/Src/GoogleApis/Apis/Http/C...
Src/GoogleApis/Apis/Http/ConfigurableMessageHandler.cs:98: /// <see
cref="NumTries"/> to 1 and <see cref="NumRedirect"/> to 5, a retry will happen
only for 5 redirects.
Try to be as explicit as possible. Here's what we came up with together:

"to 5, the library will send up to five redirect requests, but will not send any
retry requests due to an error HTTP status code."