Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(991)

Issue 13096044: Issue 146: Pass override HTTP header when request URI too long (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 7 months ago by ngmiceli
Modified:
10 years, 5 months ago
Reviewers:
peleyal
CC:
google-api-dotnet-client_googlegroups.com
Visibility:
Public.

Description

Issue 146: Pass override HTTP header when request URI too long https://code.google.com/p/google-api-dotnet-client/issues/detail?id=146

Patch Set 1 #

Total comments: 58

Patch Set 2 : Comments #

Total comments: 18

Patch Set 3 : More comments #

Patch Set 4 : Move MaxUrlLengthInterceptor into its own class #

Total comments: 19

Patch Set 5 : So many comments #

Patch Set 6 : Refactor namespace #

Unified diffs Side-by-side diffs Delta from patch set Stats (+321 lines, -19 lines) Patch
M Src/GoogleApis.DotNet4/Apis/Logging/Log4NetLogger.cs View 1 2 3 4 5 1 chunk +0 lines, -1 line 0 comments Download
M Src/GoogleApis.Tests/Apis/Download/MediaDownloaderTest.cs View 1 2 3 4 5 1 chunk +0 lines, -1 line 0 comments Download
M Src/GoogleApis.Tests/Apis/Http/ConfigurableMessageHandlerTest.cs View 1 2 3 4 5 1 chunk +0 lines, -1 line 0 comments Download
A Src/GoogleApis.Tests/Apis/Http/MaxUrlLengthInterceptorTest.cs View 1 2 3 4 5 1 chunk +120 lines, -0 lines 0 comments Download
M Src/GoogleApis.Tests/Apis/Requests/ClientServiceRequestTest.cs View 1 2 3 4 5 1 chunk +0 lines, -1 line 0 comments Download
M Src/GoogleApis.Tests/Apis/Services/BaseClientServiceTest.cs View 1 2 3 4 5 3 chunks +69 lines, -3 lines 0 comments Download
M Src/GoogleApis.Tests/Apis/Upload/ResumableUploadTest.cs View 1 2 3 4 5 1 chunk +0 lines, -1 line 0 comments Download
M Src/GoogleApis.Tests/Apis/Utils/UtilitiesTest.cs View 1 2 3 4 5 1 chunk +0 lines, -6 lines 0 comments Download
M Src/GoogleApis.Tests/GoogleApis.Tests.csproj View 1 2 3 4 2 chunks +2 lines, -0 lines 0 comments Download
M Src/GoogleApis.Tests/[Mock]/MockBackOffHandler.cs View 1 2 3 4 5 1 chunk +1 line, -1 line 0 comments Download
M Src/GoogleApis.Tests/[Mock]/MockClientService.cs View 1 2 3 4 5 1 chunk +1 line, -1 line 0 comments Download
M Src/GoogleApis.Tests/[Mock]/MockHttpClientFactory.cs View 1 2 3 4 5 1 chunk +1 line, -1 line 0 comments Download
A Src/GoogleApis.Tests/[Mock]/MockMessageHandler.cs View 1 2 3 4 5 1 chunk +35 lines, -0 lines 0 comments Download
A Src/GoogleApis/Apis/Http/MaxUrlLengthInterceptor.cs View 1 2 3 4 1 chunk +71 lines, -0 lines 0 comments Download
M Src/GoogleApis/Apis/Services/BaseClientService.cs View 1 2 3 4 5 chunks +20 lines, -2 lines 0 comments Download
M Src/GoogleApis/GoogleApis.csproj View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 8
ngmiceli
10 years, 7 months ago (2013-08-23 19:08:48 UTC) #1
peleyal
Please also test with Translate. Let me know if you need help, or anything. https://codereview.appspot.com/13096044/diff/1/Src/GoogleApis.Tests/Apis/Services/BaseClientServiceTest.cs ...
10 years, 7 months ago (2013-08-23 19:37:34 UTC) #2
ngmiceli
https://codereview.appspot.com/13096044/diff/1/Src/GoogleApis.Tests/Apis/Services/BaseClientServiceTest.cs File Src/GoogleApis.Tests/Apis/Services/BaseClientServiceTest.cs (right): https://codereview.appspot.com/13096044/diff/1/Src/GoogleApis.Tests/Apis/Services/BaseClientServiceTest.cs#newcode37 Src/GoogleApis.Tests/Apis/Services/BaseClientServiceTest.cs:37: using System.Net.Http.Headers; On 2013/08/23 19:37:34, peleyal wrote: > fix ...
10 years, 7 months ago (2013-08-23 21:18:43 UTC) #3
peleyal
https://codereview.appspot.com/13096044/diff/1/Src/GoogleApis.Tests/Apis/Services/BaseClientServiceTest.cs File Src/GoogleApis.Tests/Apis/Services/BaseClientServiceTest.cs (right): https://codereview.appspot.com/13096044/diff/1/Src/GoogleApis.Tests/Apis/Services/BaseClientServiceTest.cs#newcode377 Src/GoogleApis.Tests/Apis/Services/BaseClientServiceTest.cs:377: return null; ACK It's weird to me why it's ...
10 years, 7 months ago (2013-08-26 01:28:47 UTC) #4
ngmiceli
https://codereview.appspot.com/13096044/diff/1/Src/GoogleApis.Tests/Apis/Services/BaseClientServiceTest.cs File Src/GoogleApis.Tests/Apis/Services/BaseClientServiceTest.cs (right): https://codereview.appspot.com/13096044/diff/1/Src/GoogleApis.Tests/Apis/Services/BaseClientServiceTest.cs#newcode397 Src/GoogleApis.Tests/Apis/Services/BaseClientServiceTest.cs:397: var sentRequest = messageHandler.LastRequest; On 2013/08/26 01:28:47, peleyal wrote: ...
10 years, 7 months ago (2013-08-29 19:17:05 UTC) #5
peleyal
Small stuff (mainly comments and standards) Let me know if you want me to review ...
10 years, 7 months ago (2013-08-29 20:34:22 UTC) #6
peleyal
My first and last LGTM to you :(
10 years, 5 months ago (2013-10-02 19:15:14 UTC) #7
ngmiceli
10 years, 5 months ago (2013-10-02 19:15:18 UTC) #8
https://codereview.appspot.com/13096044/diff/16001/Src/GoogleApis.Tests/Apis/...
File Src/GoogleApis.Tests/Apis/Http/MaxUrlLengthInterceptorTest.cs (right):

https://codereview.appspot.com/13096044/diff/16001/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Http/MaxUrlLengthInterceptorTest.cs:1: using System;
On 2013/08/29 20:34:22, peleyal wrote:
> Please add the "Google" header

Done.

https://codereview.appspot.com/13096044/diff/16001/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Http/MaxUrlLengthInterceptorTest.cs:1: using System;
On 2013/08/29 20:34:22, peleyal wrote:
> Please add the "Google" header

Done.

https://codereview.appspot.com/13096044/diff/16001/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Http/MaxUrlLengthInterceptorTest.cs:17: class
MaxUrlLengthInterceptorTest
On 2013/08/29 20:34:22, peleyal wrote:
> /// <summary> Test for <seealso cref="MaxUrlLengthInterceptor"/>. </summary>

Done.

https://codereview.appspot.com/13096044/diff/16001/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Http/MaxUrlLengthInterceptorTest.cs:19: internal class
MockMessageHandler : HttpMessageHandler
On 2013/08/29 20:34:22, peleyal wrote:
> Maybe it should be added to our Mocking classes because you just copied pasted
> it here (it exists also in BaseClientServiceTest...)
> 
> I think you should move it to
>
https://code.google.com/p/google-api-dotnet-client/source/browse/#hg%252FSrc%...

Done.

https://codereview.appspot.com/13096044/diff/16001/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Http/MaxUrlLengthInterceptorTest.cs:19: internal class
MockMessageHandler : HttpMessageHandler
On 2013/08/29 20:34:22, peleyal wrote:
> I would add a comment here as well.
Removed as per below request

https://codereview.appspot.com/13096044/diff/16001/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Http/MaxUrlLengthInterceptorTest.cs:23: protected
override Task<HttpResponseMessage> SendAsync(HttpRequestMessage request,
On 2013/08/29 20:34:22, peleyal wrote:
> add async before Task

Done.

https://codereview.appspot.com/13096044/diff/16001/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Http/MaxUrlLengthInterceptorTest.cs:26: RequestContent
= request.Content.ReadAsStringAsync().Result;
On 2013/08/29 20:34:22, peleyal wrote:
> and now you can use await
> 
> RequestContent = await request.Content.ReadAsStringAsync();

Doing so causes a test to fail, as the value remains null. I don't know enough
about .NET's async to understand why but this code seems to work fine for a
test/mock.

https://codereview.appspot.com/13096044/diff/16001/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Http/MaxUrlLengthInterceptorTest.cs:37: public void
TestGetWithUrlTooLongByDefault()
On 2013/08/29 20:34:22, peleyal wrote:
> That one should be remove from here, because you left it in the Service level,
> right?

It is duplicated, but it feels incomplete without it...
How's this?

https://codereview.appspot.com/13096044/diff/16001/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Http/MaxUrlLengthInterceptorTest.cs:112: /// Verifies
that URLs over user-specified max number of characters on GET requests are
correctly translated
On 2013/08/29 20:34:22, peleyal wrote:
> I would change it to something like - Verifies that URLs over user-specified
max
> number of characters (1000 in this test) on GET requests are correctly
> translated

Done.

https://codereview.appspot.com/13096044/diff/16001/Src/GoogleApis/Apis/Servic...
File Src/GoogleApis/Apis/Services/BaseClientService.cs (right):

https://codereview.appspot.com/13096044/diff/16001/Src/GoogleApis/Apis/Servic...
Src/GoogleApis/Apis/Services/BaseClientService.cs:55: /// <summary>The default
maximum allowed length of a URL string for GET requests.</summary>
On 2013/08/29 20:34:22, peleyal wrote:
> maybe we should add the following behavior as well:
> For avoiding the behavior of converting a too long GET request into POST, set
> this variable to 0
> add also check that in the constructor of BaseClientService.
> 
> What do you think?

Not this variable, since this is the default, but MaxUrlLength below, yes I
agree. Done.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b