|
|
DescriptionIssue 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 #MessagesTotal messages: 8
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/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; fix the usings order 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")] revert... 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 two execute interceptors? explain... 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) line length is 120 https://codereview.appspot.com/13096044/diff/1/Src/GoogleApis.Tests/Apis/Serv... Src/GoogleApis.Tests/Apis/Services/BaseClientServiceTest.cs:377: return null; return response here. You can do so by: TaskCompleteionSource<HttpResponseMessage> tcs = new TaskCompleteionSource<HttpResponseMessage>(); tcs.SetResult(new HttpResponseMessage()); return tcs.Task; 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); 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))); 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); Question - Should it be only for GET or it should be for DELETE and PUT as well...? Please add test for Post or Delete as well... https://codereview.appspot.com/13096044/diff/1/Src/GoogleApis.Tests/Apis/Serv... Src/GoogleApis.Tests/Apis/Services/BaseClientServiceTest.cs:397: var sentRequest = messageHandler.LastRequest; check if the request itself is the one that changes. I don't think that you need to save it 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)); 2049? magic number??? 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. 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"), [optional] Assert.That(sentRequest.Headers.GetValues("X-HTTP-Method-Override").Single(), "GET"); https://codereview.appspot.com/13096044/diff/1/Src/GoogleApis.Tests/Apis/Serv... Src/GoogleApis.Tests/Apis/Services/BaseClientServiceTest.cs:415: public void TestGetWithUrlOkayLengthByDefault() [optional] you can another test with less than 2048 https://codereview.appspot.com/13096044/diff/1/Src/GoogleApis.Tests/Apis/Serv... Src/GoogleApis.Tests/Apis/Services/BaseClientServiceTest.cs:421: { I like to indent this {} section 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); can you also test a uri that contains encoding characters, like space or #? 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; doc https://codereview.appspot.com/13096044/diff/1/Src/GoogleApis/Apis/Services/B... Src/GoogleApis/Apis/Services/BaseClientService.cs:120: public int MaxUrlLength { get; set; } doc here :) https://codereview.appspot.com/13096044/diff/1/Src/GoogleApis/Apis/Services/B... Src/GoogleApis/Apis/Services/BaseClientService.cs:120: public int MaxUrlLength { get; set; } Do you want to have minimum and maximum values that you will check on setting the value? can we change the type to uint? https://codereview.appspot.com/13096044/diff/1/Src/GoogleApis/Apis/Services/B... Src/GoogleApis/Apis/Services/BaseClientService.cs:180: MaxUrlLength = initializer.MaxUrlLength; 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 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; } do we need that property? 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> 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>. https://codereview.appspot.com/13096044/diff/1/Src/GoogleApis/Apis/Services/B... Src/GoogleApis/Apis/Services/BaseClientService.cs:430: internal class MaxUrlLengthInterceptor : IHttpExecuteInterceptor [optional] (and I prefer) Move to a new file https://codereview.appspot.com/13096044/diff/1/Src/GoogleApis/Apis/Services/B... Src/GoogleApis/Apis/Services/BaseClientService.cs:434: public MaxUrlLengthInterceptor(int maxUrlLength) ///<summary> ///Constructs a new Max URL length interceptor with the given max length. ///</summary> https://codereview.appspot.com/13096044/diff/1/Src/GoogleApis/Apis/Services/B... Src/GoogleApis/Apis/Services/BaseClientService.cs:445: if (! String.IsNullOrEmpty(query)) add a test for uri without query which its length is bigger than 2048 https://codereview.appspot.com/13096044/diff/1/Src/GoogleApis/Apis/Services/B... Src/GoogleApis/Apis/Services/BaseClientService.cs:445: if (! String.IsNullOrEmpty(query)) remove extra space here. 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)); isn't it just requestString.Substring(1)? (remove only the first character)?
Sign in to reply to this message.
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).
Sign in to reply to this message.
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]
Sign in to reply to this message.
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.
Sign in to reply to this message.
Small stuff (mainly comments and standards) Let me know if you want me to review it next time with you. 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; Please add the "Google" header https://codereview.appspot.com/13096044/diff/16001/Src/GoogleApis.Tests/Apis/... Src/GoogleApis.Tests/Apis/Http/MaxUrlLengthInterceptorTest.cs:17: class MaxUrlLengthInterceptorTest /// <summary> Test for <seealso cref="MaxUrlLengthInterceptor"/>. </summary> https://codereview.appspot.com/13096044/diff/16001/Src/GoogleApis.Tests/Apis/... Src/GoogleApis.Tests/Apis/Http/MaxUrlLengthInterceptorTest.cs:19: internal class MockMessageHandler : HttpMessageHandler I would add a comment here as well. https://codereview.appspot.com/13096044/diff/16001/Src/GoogleApis.Tests/Apis/... Src/GoogleApis.Tests/Apis/Http/MaxUrlLengthInterceptorTest.cs:19: internal class MockMessageHandler : HttpMessageHandler 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%2FSrc%2F... 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, add async before Task https://codereview.appspot.com/13096044/diff/16001/Src/GoogleApis.Tests/Apis/... Src/GoogleApis.Tests/Apis/Http/MaxUrlLengthInterceptorTest.cs:26: RequestContent = request.Content.ReadAsStringAsync().Result; and now you can use await RequestContent = await request.Content.ReadAsStringAsync(); https://codereview.appspot.com/13096044/diff/16001/Src/GoogleApis.Tests/Apis/... Src/GoogleApis.Tests/Apis/Http/MaxUrlLengthInterceptorTest.cs:37: public void TestGetWithUrlTooLongByDefault() That one should be remove from here, because you left it in the Service level, right? 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 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 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> 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?
Sign in to reply to this message.
My first and last LGTM to you :(
Sign in to reply to this message.
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.
|