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

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

Can't Edit
Can't Publish+Mail
Start Review

Created:
11 years, 4 months ago by ngmiceli

Modified:
11 years, 3 months ago

Reviewers:
peleyal

CC:
google-api-dotnet-client_googlegroups.com

Visibility:
Public.

More Reviews

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 4 : Move MaxUrlLengthInterceptor into its own class #

Total comments: 19

Patch Set 5 : So many comments #

Patch Set 6 : Refactor namespace #

Created: 11 years, 3 months ago

Download [raw] [tar.bz2]

	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

Expand All Messages | Collapse All Messages

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 ...

11 years, 4 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 ...

11 years, 4 months ago (2013-08-23 21:18:43 UTC) #3

https://codereview.appspot.com/13096044/diff/1/Src/GoogleApis.Tests/Apis/Serv...
File Src/GoogleApis.Tests/Apis/Services/BaseClientServiceTest.cs (right):

https://codereview.appspot.com/13096044/diff/1/Src/GoogleApis.Tests/Apis/Serv...
Src/GoogleApis.Tests/Apis/Services/BaseClientServiceTest.cs:37: using
System.Net.Http.Headers;
On 2013/08/23 19:37:34, peleyal wrote:
> fix the usings order

Done.

https://codereview.appspot.com/13096044/diff/1/Src/GoogleApis.Tests/Apis/Serv...
Src/GoogleApis.Tests/Apis/Services/BaseClientServiceTest.cs:42: [TestFixture,
System.Runtime.InteropServices.GuidAttribute("B63A3B60-ACFE-4F96-B611-BE24FA4B48B2")]
On 2013/08/23 19:37:34, peleyal wrote:
> revert...

Done.

https://codereview.appspot.com/13096044/diff/1/Src/GoogleApis.Tests/Apis/Serv...
Src/GoogleApis.Tests/Apis/Services/BaseClientServiceTest.cs:358: // one execute
interceptor (for adding the "Authenticate" header
On 2013/08/23 19:37:34, peleyal wrote:
> two execute interceptors? explain...

Done.

https://codereview.appspot.com/13096044/diff/1/Src/GoogleApis.Tests/Apis/Serv...
Src/GoogleApis.Tests/Apis/Services/BaseClientServiceTest.cs:373: protected
override Task<HttpResponseMessage> SendAsync(HttpRequestMessage request,
CancellationToken cancellationToken)
On 2013/08/23 19:37:34, peleyal wrote:
> line length is 120

Done.

https://codereview.appspot.com/13096044/diff/1/Src/GoogleApis.Tests/Apis/Serv...
Src/GoogleApis.Tests/Apis/Services/BaseClientServiceTest.cs:377: return null;
On 2013/08/23 19:37:34, peleyal wrote:
> return response here.
> You can do so by:
> 
> TaskCompleteionSource<HttpResponseMessage> tcs = new
> TaskCompleteionSource<HttpResponseMessage>();
> tcs.SetResult(new HttpResponseMessage());
> return tcs.Task;

Why? It just clutters the code. I don't like to add anything to my test classes
that aren't actually being used for the test.

https://codereview.appspot.com/13096044/diff/1/Src/GoogleApis.Tests/Apis/Serv...
Src/GoogleApis.Tests/Apis/Services/BaseClientServiceTest.cs:387: var query =
"q=" + new String('x', 1020) + "&x=" + new String('y', 1000);
On 2013/08/23 19:37:34, peleyal wrote:
> I prefer:
> var uri = http://www.example.com;
> var requestUri = uri + "/?" + query;
> 
> and then on line 406 you can check 
> Assert.That(sentRequest.RequestUri, Is.EqualTo(new Uri(uri)));

Done.

https://codereview.appspot.com/13096044/diff/1/Src/GoogleApis.Tests/Apis/Serv...
Src/GoogleApis.Tests/Apis/Services/BaseClientServiceTest.cs:389: var request =
new HttpRequestMessage(HttpMethod.Get, requestUri);
On 2013/08/23 19:37:34, peleyal wrote:
> Question - Should it be only for GET or it should be for DELETE and PUT as
> well...?
There's no spec on this, so I went for minimum change. We know the use-case that
needs this feature, so I kept it to only affect the use-case we have confirmed.
> Please add test for Post or Delete as well...
Done.

https://codereview.appspot.com/13096044/diff/1/Src/GoogleApis.Tests/Apis/Serv...
Src/GoogleApis.Tests/Apis/Services/BaseClientServiceTest.cs:397: var sentRequest
= messageHandler.LastRequest;
On 2013/08/23 19:37:34, peleyal wrote:
> check if the request itself is the one that changes. I don't think that you
need
> to save it

No, you're correct, though I do need to save the content as it gets disposed
otherwise. I would prefer to in that case just save both since it makes the
tests a little more explicit in my opinion, but I'm happy just to save the one
if you'd prefer.

https://codereview.appspot.com/13096044/diff/1/Src/GoogleApis.Tests/Apis/Serv...
Src/GoogleApis.Tests/Apis/Services/BaseClientServiceTest.cs:399:
Assert.That(requestUri.Length, Is.EqualTo(2049));
On 2013/08/23 19:37:34, peleyal wrote:
> 2049? 
> magic number???
I'm happy to get rid of it but I wasn't sure how. It seems weird to me to expose
the DEFAULT_MAX_URL_LENGTH variable in BaseClientService. Is this better?
> 
> I prefer 
> var expectedLength = query.Length + requestUri.Length
> Assert.That(requestUri.Length, Is.EqualTo(expectedLength));
> 
> That test looks weird. You are testing that your input data is correct. That's
> weird.
I believe tests should be documentation. They should be 100% self explanatory
and be useful to future developers for understanding how a feature works. Why
not Assert the validity of both the input and output? In addition, it explains
the reason behind the magic numbers "1020" and "1000". Finally, it ensures if
the test URL is fiddled with that the user recognizes what they're doing and is
explicit about it.

https://codereview.appspot.com/13096044/diff/1/Src/GoogleApis.Tests/Apis/Serv...
Src/GoogleApis.Tests/Apis/Services/BaseClientServiceTest.cs:402:
Assert.That(sentRequest.Headers.GetValues("X-HTTP-Method-Override"),
On 2013/08/23 19:37:34, peleyal wrote:
> [optional]
> Assert.That(sentRequest.Headers.GetValues("X-HTTP-Method-Override").Single(),
> "GET");

Done.

https://codereview.appspot.com/13096044/diff/1/Src/GoogleApis.Tests/Apis/Serv...
Src/GoogleApis.Tests/Apis/Services/BaseClientServiceTest.cs:415: public void
TestGetWithUrlOkayLengthByDefault()
On 2013/08/23 19:37:34, peleyal wrote:
> [optional]
> you can another test with less than 2048

Done.

https://codereview.appspot.com/13096044/diff/1/Src/GoogleApis.Tests/Apis/Serv...
Src/GoogleApis.Tests/Apis/Services/BaseClientServiceTest.cs:421: {
On 2013/08/23 19:37:34, peleyal wrote:
> I like to indent this {} section

Done for consistency, though that looks really weird to me.

https://codereview.appspot.com/13096044/diff/1/Src/GoogleApis.Tests/Apis/Serv...
Src/GoogleApis.Tests/Apis/Services/BaseClientServiceTest.cs:442: var query =
"q=" + new String('x', 900) + "&x=" + new String('y', 72);
On 2013/08/23 19:37:34, peleyal wrote:
> can you also test a uri that contains encoding characters, like space or #?
I don't see a need. Encoding the characters is not part of the functionality of
this test, so it shouldn't be tested in this unit test. The characters should
already be encoded by your library when they're added to the query parameters,
and that behavior should be tested there.

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

https://codereview.appspot.com/13096044/diff/1/Src/GoogleApis/Apis/Services/B...
Src/GoogleApis/Apis/Services/BaseClientService.cs:55: private const int
DefaultMaxUrlLength = 2048;
On 2013/08/23 19:37:34, peleyal wrote:
> doc

Done.

https://codereview.appspot.com/13096044/diff/1/Src/GoogleApis/Apis/Services/B...
Src/GoogleApis/Apis/Services/BaseClientService.cs:120: public int MaxUrlLength {
get; set; }
On 2013/08/23 19:37:34, peleyal wrote:
> Do you want to have minimum and maximum values that you will check on setting
> the value?
No. Why bother?
> can we change the type to uint?
Done.

https://codereview.appspot.com/13096044/diff/1/Src/GoogleApis/Apis/Services/B...
Src/GoogleApis/Apis/Services/BaseClientService.cs:120: public int MaxUrlLength {
get; set; }
On 2013/08/23 19:37:34, peleyal wrote:
> doc here :)

Done.

https://codereview.appspot.com/13096044/diff/1/Src/GoogleApis/Apis/Services/B...
Src/GoogleApis/Apis/Services/BaseClientService.cs:180: MaxUrlLength =
initializer.MaxUrlLength;
On 2013/08/23 19:37:34, peleyal wrote:
> I'll remove it and won't save it in the BaseClientService class.
> What do you think?
> I think that it's enough to have it on the interceptor only

Done.

https://codereview.appspot.com/13096044/diff/1/Src/GoogleApis/Apis/Services/B...
Src/GoogleApis/Apis/Services/BaseClientService.cs:255: public int MaxUrlLength {
get; private set; }
On 2013/08/23 19:37:34, peleyal wrote:
> do we need that property?

Yup.

https://codereview.appspot.com/13096044/diff/1/Src/GoogleApis/Apis/Services/B...
Src/GoogleApis/Apis/Services/BaseClientService.cs:429: /// <summary> Intercepts
if a GET request URL is too long, and changes the request accordingly.
</summary>
On 2013/08/23 19:37:34, peleyal wrote:
> I'll explain further...
> 
> Intercepts a HTTP request, and change the method to POST in case it's a GET
> request and the URI is bigger then <see cref="MaxUrlLength" />. It also adds
> <c>X-HTTP-Method-Override</c> header and change the content type to
> <c>application/x-www-form-urlencoded</c>.

You want more documentation? Have some more documentation.

https://codereview.appspot.com/13096044/diff/1/Src/GoogleApis/Apis/Services/B...
Src/GoogleApis/Apis/Services/BaseClientService.cs:430: internal class
MaxUrlLengthInterceptor : IHttpExecuteInterceptor
On 2013/08/23 19:37:34, peleyal wrote:
> [optional] (and I prefer)
> Move to a new file

I was trying to keep consistent. Your existing Interceptor is an internal class
in AuthenticatorHelpers. I figured this too should be an internal class here.
I don't care where you organize things but I'm not yet familiar with the
structure of the library so just tell me where you'd prefer it.

https://codereview.appspot.com/13096044/diff/1/Src/GoogleApis/Apis/Services/B...
Src/GoogleApis/Apis/Services/BaseClientService.cs:434: public
MaxUrlLengthInterceptor(int maxUrlLength)
On 2013/08/23 19:37:34, peleyal wrote:
> ///<summary>
> ///Constructs a new Max URL length interceptor with the given max length.
> ///</summary>

What useless documentation. Done.

https://codereview.appspot.com/13096044/diff/1/Src/GoogleApis/Apis/Services/B...
Src/GoogleApis/Apis/Services/BaseClientService.cs:445: if (!
String.IsNullOrEmpty(query))
On 2013/08/23 19:37:34, peleyal wrote:
> add a test for uri without query which its length is bigger than 2048

There's no need, as you can't. The URI class throws an exception that it cannot
parse the URI if you give it one that long (I tried).

https://codereview.appspot.com/13096044/diff/1/Src/GoogleApis/Apis/Services/B...
Src/GoogleApis/Apis/Services/BaseClientService.cs:445: if (!
String.IsNullOrEmpty(query))
On 2013/08/23 19:37:34, peleyal wrote:
> remove extra space here.

Done.

https://codereview.appspot.com/13096044/diff/1/Src/GoogleApis/Apis/Services/B...
Src/GoogleApis/Apis/Services/BaseClientService.cs:451: request.RequestUri = new
Uri(requestString.Remove(requestString.IndexOf("?") - 1));
On 2013/08/23 19:37:34, peleyal wrote:
> isn't it just 
> requestString.Substring(1)? (remove only the first character)?

Nope.
You're thinking of line 448: request.Content = new
StringContent(query.Substring(1));
This line takes the URI and removes the query parameters. So for example
www.foo.com/?q=blah -> www.foo.com/
So find the first "?", and remove everything from it forward INCLUDING it (thus
the -1, substring is exclusive).

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 ...

11 years, 4 months ago (2013-08-26 01:28:47 UTC) #4

https://codereview.appspot.com/13096044/diff/1/Src/GoogleApis.Tests/Apis/Serv...
File Src/GoogleApis.Tests/Apis/Services/BaseClientServiceTest.cs (right):

https://codereview.appspot.com/13096044/diff/1/Src/GoogleApis.Tests/Apis/Serv...
Src/GoogleApis.Tests/Apis/Services/BaseClientServiceTest.cs:377: return null;
ACK
It's weird to me why it's working. null is weird here. I want to debug it and
see how the ConfigurableMessageHandler acts in this case
On 2013/08/23 21:18:44, ngmiceli wrote:
> On 2013/08/23 19:37:34, peleyal wrote:
> > return response here.
> > You can do so by:
> > 
> > TaskCompleteionSource<HttpResponseMessage> tcs = new
> > TaskCompleteionSource<HttpResponseMessage>();
> > tcs.SetResult(new HttpResponseMessage());
> > return tcs.Task;
> 
> Why? It just clutters the code. I don't like to add anything to my test
classes
> that aren't actually being used for the test.

https://codereview.appspot.com/13096044/diff/1/Src/GoogleApis.Tests/Apis/Serv...
Src/GoogleApis.Tests/Apis/Services/BaseClientServiceTest.cs:397: var sentRequest
= messageHandler.LastRequest;
I'll remove LastRequest and only keep a RequestContent variable on the message
handler
On 2013/08/23 21:18:44, ngmiceli wrote:
> On 2013/08/23 19:37:34, peleyal wrote:
> > check if the request itself is the one that changes. I don't think that you
> need
> > to save it
> 
> No, you're correct, though I do need to save the content as it gets disposed
> otherwise. I would prefer to in that case just save both since it makes the
> tests a little more explicit in my opinion, but I'm happy just to save the one
> if you'd prefer.

https://codereview.appspot.com/13096044/diff/1/Src/GoogleApis.Tests/Apis/Serv...
Src/GoogleApis.Tests/Apis/Services/BaseClientServiceTest.cs:399:
Assert.That(requestUri.Length, Is.EqualTo(2049));
you wrote before: "They should be 100% self explanatory and be useful to future
developers for understanding how.."
But that's no the case - maybe just explain 2049 = 1000y + 120x + 6 (query
setters) + 20 char in uri....) ...how do we get to 2049???

On 2013/08/23 21:18:44, ngmiceli wrote:
> On 2013/08/23 19:37:34, peleyal wrote:
> > 2049? 
> > magic number???
> I'm happy to get rid of it but I wasn't sure how. It seems weird to me to
expose
> the DEFAULT_MAX_URL_LENGTH variable in BaseClientService. Is this better?
> > 
> > I prefer 
> > var expectedLength = query.Length + requestUri.Length
> > Assert.That(requestUri.Length, Is.EqualTo(expectedLength));
> > 
> > That test looks weird. You are testing that your input data is correct.
That's
> > weird.
> I believe tests should be documentation. They should be 100% self explanatory
> and be useful to future developers for understanding how a feature works. Why
> not Assert the validity of both the input and output? In addition, it explains
> the reason behind the magic numbers "1020" and "1000". Finally, it ensures if
> the test URL is fiddled with that the user recognizes what they're doing and
is
> explicit about it.

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

https://codereview.appspot.com/13096044/diff/1/Src/GoogleApis/Apis/Services/B...
Src/GoogleApis/Apis/Services/BaseClientService.cs:430: internal class
MaxUrlLengthInterceptor : IHttpExecuteInterceptor
I'm not sure either. 
I think that we can move it to separate class and file (and it can be public as
well)

The AuthenticatorHelpers is not a good example cause we will remove it once we
will rewrite the OAuth library
On 2013/08/23 21:18:44, ngmiceli wrote:
> On 2013/08/23 19:37:34, peleyal wrote:
> > [optional] (and I prefer)
> > Move to a new file
> 
> I was trying to keep consistent. Your existing Interceptor is an internal
class
> in AuthenticatorHelpers. I figured this too should be an internal class here.
> I don't care where you organize things but I'm not yet familiar with the
> structure of the library so just tell me where you'd prefer it.

https://codereview.appspot.com/13096044/diff/1/Src/GoogleApis/Apis/Services/B...
Src/GoogleApis/Apis/Services/BaseClientService.cs:434: public
MaxUrlLengthInterceptor(int maxUrlLength)
I know but it's good for our online documentation.
I agree with you :)
On 2013/08/23 21:18:44, ngmiceli wrote:
> On 2013/08/23 19:37:34, peleyal wrote:
> > ///<summary>
> > ///Constructs a new Max URL length interceptor with the given max length.
> > ///</summary>
> 
> What useless documentation. Done.

https://codereview.appspot.com/13096044/diff/1/Src/GoogleApis/Apis/Services/B...
Src/GoogleApis/Apis/Services/BaseClientService.cs:451: request.RequestUri = new
Uri(requestString.Remove(requestString.IndexOf("?") - 1));
[optional]
I think that a comment with an example could be great here
On 2013/08/23 21:18:44, ngmiceli wrote:
> On 2013/08/23 19:37:34, peleyal wrote:
> > isn't it just 
> > requestString.Substring(1)? (remove only the first character)?
> 
> Nope.
> You're thinking of line 448: request.Content = new
> StringContent(query.Substring(1));
> This line takes the URI and removes the query parameters. So for example
> http://www.foo.com/?q=blah -> http://www.foo.com/
> So find the first "?", and remove everything from it forward INCLUDING it
(thus
> the -1, substring is exclusive).

https://codereview.appspot.com/13096044/diff/7001/Src/GoogleApis.Tests/Apis/S...
File Src/GoogleApis.Tests/Apis/Services/BaseClientServiceTest.cs (right):

https://codereview.appspot.com/13096044/diff/7001/Src/GoogleApis.Tests/Apis/S...
Src/GoogleApis.Tests/Apis/Services/BaseClientServiceTest.cs:384: private const
uint DefaultMaxUrlLength = 2048;
You can change BaseClientService.DefaultMaxUrlLength to internal and add the
[VisibleToTestOnly] and remove this variable

https://codereview.appspot.com/13096044/diff/7001/Src/GoogleApis.Tests/Apis/S...
Src/GoogleApis.Tests/Apis/Services/BaseClientServiceTest.cs:421:
TestGetWithUrlOkayLengthByDefaultHelper(2048, HttpMethod.Get);
rename to SubtestGetWithUrlOkayLengthByDefault (that's the convention)

https://codereview.appspot.com/13096044/diff/7001/Src/GoogleApis.Tests/Apis/S...
Src/GoogleApis.Tests/Apis/Services/BaseClientServiceTest.cs:427: /// Regression
test to verify that non-GET requests with URLs shorter or greater than 2048
characters are not modified.
too long

https://codereview.appspot.com/13096044/diff/7001/Src/GoogleApis.Tests/Apis/S...
Src/GoogleApis.Tests/Apis/Services/BaseClientServiceTest.cs:432:
TestGetWithUrlOkayLengthByDefaultHelper(2049, HttpMethod.Post);
change to
 DefaultMaxUrlLength + 1
 DefaultMaxUrlLength
 DefaultMaxUrlLength - 1

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

https://codereview.appspot.com/13096044/diff/7001/Src/GoogleApis/Apis/Service...
Src/GoogleApis/Apis/Services/BaseClientService.cs:258: public int MaxUrlLength {
get; private set; }
but you don't set it now :)
So you can remove it from this class and keep it only in the initializer.

https://codereview.appspot.com/13096044/diff/7001/Src/GoogleApis/Apis/Service...
Src/GoogleApis/Apis/Services/BaseClientService.cs:433: /// Intercepts HTTP GET
requests with a URLs longer than <see cref="MaxUrlLength" />.
<see cref="Initializer.MaxUrlLength"

https://codereview.appspot.com/13096044/diff/7001/Src/GoogleApis/Apis/Service...
Src/GoogleApis/Apis/Services/BaseClientService.cs:436: /// <item>The request
type will be changed to POST</item>
I think you mean "request's method" and not "request type"

https://codereview.appspot.com/13096044/diff/7001/Src/GoogleApis/Apis/Service...
Src/GoogleApis/Apis/Services/BaseClientService.cs:454: if (request.Method ==
HttpMethod.Get && request.RequestUri.AbsoluteUri.Length > maxUrlLength) {
[optional]
I think that you are the one that actually like that stile:
if (request.Method != HttpMethod.Get || request.RequestUri.AbsoluteUri.Length <=
maxUrlLength) {
 return
}

// change the method to POST
....

https://codereview.appspot.com/13096044/diff/7001/Src/GoogleApis/Apis/Service...
Src/GoogleApis/Apis/Services/BaseClientService.cs:458: if (!
String.IsNullOrEmpty(query))
if (! String.IsNullOrEmpty(query))
=>
if (!String.IsNullOrEmpty(query))

[remove extra space]

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: ...

11 years, 4 months ago (2013-08-29 19:17:05 UTC) #5

https://codereview.appspot.com/13096044/diff/1/Src/GoogleApis.Tests/Apis/Serv...
File Src/GoogleApis.Tests/Apis/Services/BaseClientServiceTest.cs (right):

https://codereview.appspot.com/13096044/diff/1/Src/GoogleApis.Tests/Apis/Serv...
Src/GoogleApis.Tests/Apis/Services/BaseClientServiceTest.cs:397: var sentRequest
= messageHandler.LastRequest;
On 2013/08/26 01:28:47, peleyal wrote:
> I'll remove LastRequest and only keep a RequestContent variable on the message
> handler
> On 2013/08/23 21:18:44, ngmiceli wrote:
> > On 2013/08/23 19:37:34, peleyal wrote:
> > > check if the request itself is the one that changes. I don't think that
you
> > need
> > > to save it
> > 
> > No, you're correct, though I do need to save the content as it gets disposed
> > otherwise. I would prefer to in that case just save both since it makes the
> > tests a little more explicit in my opinion, but I'm happy just to save the
one
> > if you'd prefer.
> 

Done.

https://codereview.appspot.com/13096044/diff/1/Src/GoogleApis.Tests/Apis/Serv...
Src/GoogleApis.Tests/Apis/Services/BaseClientServiceTest.cs:399:
Assert.That(requestUri.Length, Is.EqualTo(2049));
On 2013/08/26 01:28:47, peleyal wrote:
> you wrote before: "They should be 100% self explanatory and be useful to
future
> developers for understanding how.."
> But that's no the case - maybe just explain 2049 = 1000y + 120x + 6 (query
> setters) + 20 char in uri....) ...how do we get to 2049???

I felt that's the advantage of the assert line - it tells you the length of the
thing I'm building, but I'll add a comment too

> 
> On 2013/08/23 21:18:44, ngmiceli wrote:
> > On 2013/08/23 19:37:34, peleyal wrote:
> > > 2049? 
> > > magic number???
> > I'm happy to get rid of it but I wasn't sure how. It seems weird to me to
> expose
> > the DEFAULT_MAX_URL_LENGTH variable in BaseClientService. Is this better?
> > > 
> > > I prefer 
> > > var expectedLength = query.Length + requestUri.Length
> > > Assert.That(requestUri.Length, Is.EqualTo(expectedLength));
> > > 
> > > That test looks weird. You are testing that your input data is correct.
> That's
> > > weird.
> > I believe tests should be documentation. They should be 100% self
explanatory
> > and be useful to future developers for understanding how a feature works.
Why
> > not Assert the validity of both the input and output? In addition, it
explains
> > the reason behind the magic numbers "1020" and "1000". Finally, it ensures
if
> > the test URL is fiddled with that the user recognizes what they're doing and
> is
> > explicit about it.
>

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

https://codereview.appspot.com/13096044/diff/1/Src/GoogleApis/Apis/Services/B...
Src/GoogleApis/Apis/Services/BaseClientService.cs:430: internal class
MaxUrlLengthInterceptor : IHttpExecuteInterceptor
On 2013/08/26 01:28:47, peleyal wrote:
> I'm not sure either. 
> I think that we can move it to separate class and file (and it can be public
as
> well)
> 
> The AuthenticatorHelpers is not a good example cause we will remove it once we
> will rewrite the OAuth library
> On 2013/08/23 21:18:44, ngmiceli wrote:
> > On 2013/08/23 19:37:34, peleyal wrote:
> > > [optional] (and I prefer)
> > > Move to a new file
> > 
> > I was trying to keep consistent. Your existing Interceptor is an internal
> class
> > in AuthenticatorHelpers. I figured this too should be an internal class
here.
> > I don't care where you organize things but I'm not yet familiar with the
> > structure of the library so just tell me where you'd prefer it.
> 

Done.

https://codereview.appspot.com/13096044/diff/1/Src/GoogleApis/Apis/Services/B...
Src/GoogleApis/Apis/Services/BaseClientService.cs:451: request.RequestUri = new
Uri(requestString.Remove(requestString.IndexOf("?") - 1));
On 2013/08/26 01:28:47, peleyal wrote:
> [optional]
> I think that a comment with an example could be great here
> On 2013/08/23 21:18:44, ngmiceli wrote:
> > On 2013/08/23 19:37:34, peleyal wrote:
> > > isn't it just 
> > > requestString.Substring(1)? (remove only the first character)?
> > 
> > Nope.
> > You're thinking of line 448: request.Content = new
> > StringContent(query.Substring(1));
> > This line takes the URI and removes the query parameters. So for example
> > http://www.foo.com/?q=blah -> http://www.foo.com/
> > So find the first "?", and remove everything from it forward INCLUDING it
> (thus
> > the -1, substring is exclusive).
> 

Done.

https://codereview.appspot.com/13096044/diff/7001/Src/GoogleApis.Tests/Apis/S...
File Src/GoogleApis.Tests/Apis/Services/BaseClientServiceTest.cs (right):

https://codereview.appspot.com/13096044/diff/7001/Src/GoogleApis.Tests/Apis/S...
Src/GoogleApis.Tests/Apis/Services/BaseClientServiceTest.cs:384: private const
uint DefaultMaxUrlLength = 2048;
On 2013/08/26 01:28:48, peleyal wrote:
> You can change BaseClientService.DefaultMaxUrlLength to internal and add the
> [VisibleToTestOnly] and remove this variable

Done.

https://codereview.appspot.com/13096044/diff/7001/Src/GoogleApis.Tests/Apis/S...
Src/GoogleApis.Tests/Apis/Services/BaseClientServiceTest.cs:421:
TestGetWithUrlOkayLengthByDefaultHelper(2048, HttpMethod.Get);
On 2013/08/26 01:28:48, peleyal wrote:
> rename to SubtestGetWithUrlOkayLengthByDefault (that's the convention)

Done.

https://codereview.appspot.com/13096044/diff/7001/Src/GoogleApis.Tests/Apis/S...
Src/GoogleApis.Tests/Apis/Services/BaseClientServiceTest.cs:427: /// Regression
test to verify that non-GET requests with URLs shorter or greater than 2048
characters are not modified.
On 2013/08/26 01:28:48, peleyal wrote:
> too long

Done.

https://codereview.appspot.com/13096044/diff/7001/Src/GoogleApis.Tests/Apis/S...
Src/GoogleApis.Tests/Apis/Services/BaseClientServiceTest.cs:432:
TestGetWithUrlOkayLengthByDefaultHelper(2049, HttpMethod.Post);
On 2013/08/26 01:28:48, peleyal wrote:
> change to
>  DefaultMaxUrlLength + 1
>  DefaultMaxUrlLength
>  DefaultMaxUrlLength - 1

Done.

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

https://codereview.appspot.com/13096044/diff/7001/Src/GoogleApis/Apis/Service...
Src/GoogleApis/Apis/Services/BaseClientService.cs:258: public int MaxUrlLength {
get; private set; }
On 2013/08/26 01:28:48, peleyal wrote:
> but you don't set it now :)
> So you can remove it from this class and keep it only in the initializer. 

...oh right.
Done :)

https://codereview.appspot.com/13096044/diff/7001/Src/GoogleApis/Apis/Service...
Src/GoogleApis/Apis/Services/BaseClientService.cs:433: /// Intercepts HTTP GET
requests with a URLs longer than <see cref="MaxUrlLength" />.
On 2013/08/26 01:28:48, peleyal wrote:
> <see cref="Initializer.MaxUrlLength"

Done.

https://codereview.appspot.com/13096044/diff/7001/Src/GoogleApis/Apis/Service...
Src/GoogleApis/Apis/Services/BaseClientService.cs:436: /// <item>The request
type will be changed to POST</item>
On 2013/08/26 01:28:48, peleyal wrote:
> I think you mean "request's method" and not "request type"

Done.

https://codereview.appspot.com/13096044/diff/7001/Src/GoogleApis/Apis/Service...
Src/GoogleApis/Apis/Services/BaseClientService.cs:454: if (request.Method ==
HttpMethod.Get && request.RequestUri.AbsoluteUri.Length > maxUrlLength) {
On 2013/08/26 01:28:48, peleyal wrote:
> [optional]
> I think that you are the one that actually like that stile:
> if (request.Method != HttpMethod.Get || request.RequestUri.AbsoluteUri.Length
<=
> maxUrlLength) {
>  return
> }
> 
> // change the method to POST
> ....

Hey, you're right! I do prefer that :)
Done.

https://codereview.appspot.com/13096044/diff/7001/Src/GoogleApis/Apis/Service...
Src/GoogleApis/Apis/Services/BaseClientService.cs:458: if (!
String.IsNullOrEmpty(query))
On 2013/08/26 01:28:48, peleyal wrote:
> if (! String.IsNullOrEmpty(query))
> =>
> if (!String.IsNullOrEmpty(query))
> 
> [remove extra space]

Done.

peleyal

Small stuff (mainly comments and standards) Let me know if you want me to review ...

11 years, 4 months ago (2013-08-29 20:34:22 UTC) #6

ngmiceli

11 years, 3 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.

Expand All Messages | Collapse All Messages