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 ...
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).
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, 12 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]
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, 11 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.
Issue 13096044: Issue 146: Pass override HTTP header when request URI too long
(Closed)
Created 11 years ago by ngmiceli
Modified 10 years, 10 months ago
Reviewers: peleyal
Base URL:
Comments: 95