From 60ed4d52e8edfae2b35b15923110d9b2ccbdd6e7 Mon Sep 17 00:00:00 2001 From: Matthew Riley Date: Tue, 19 Apr 2016 17:45:23 -0700 Subject: [PATCH 1/2] Update NUnit framework from v2 to v3 --- .../GoogleApis.Auth.PlatformServices.Tests_Net45.csproj | 4 ++-- Src/Support/GoogleApis.Auth.DotNet4.Tests/packages.config | 2 +- .../GoogleApis.Auth.Tests/GoogleApis.Auth.Tests.csproj | 4 ++-- .../GoogleApis.Auth.Tests/OAuth2/BearerTokenTests.cs | 6 +++--- Src/Support/GoogleApis.Auth.Tests/packages.config | 2 +- .../Apis/Requests/ClientServiceRequestTest.cs | 2 +- .../GoogleApis.Tests/Apis/Upload/ResumableUploadTest.cs | 2 +- Src/Support/GoogleApis.Tests/GoogleApis.Tests.csproj | 4 ++-- Src/Support/GoogleApis.Tests/packages.config | 2 +- travis.sh | 4 ++-- 10 files changed, 16 insertions(+), 16 deletions(-) diff --git a/Src/Support/GoogleApis.Auth.DotNet4.Tests/GoogleApis.Auth.PlatformServices.Tests_Net45.csproj b/Src/Support/GoogleApis.Auth.DotNet4.Tests/GoogleApis.Auth.PlatformServices.Tests_Net45.csproj index 3414b9b4f8..99d2e3c7b4 100644 --- a/Src/Support/GoogleApis.Auth.DotNet4.Tests/GoogleApis.Auth.PlatformServices.Tests_Net45.csproj +++ b/Src/Support/GoogleApis.Auth.DotNet4.Tests/GoogleApis.Auth.PlatformServices.Tests_Net45.csproj @@ -56,8 +56,8 @@ ..\packages\Newtonsoft.Json.7.0.1\lib\net45\Newtonsoft.Json.dll True - - ..\packages\NUnit.2.6.3\lib\nunit.framework.dll + + ..\packages\NUnit.3.2.1\lib\net45\nunit.framework.dll True diff --git a/Src/Support/GoogleApis.Auth.DotNet4.Tests/packages.config b/Src/Support/GoogleApis.Auth.DotNet4.Tests/packages.config index 185befd88e..eb857ae50b 100644 --- a/Src/Support/GoogleApis.Auth.DotNet4.Tests/packages.config +++ b/Src/Support/GoogleApis.Auth.DotNet4.Tests/packages.config @@ -2,5 +2,5 @@ - + \ No newline at end of file diff --git a/Src/Support/GoogleApis.Auth.Tests/GoogleApis.Auth.Tests.csproj b/Src/Support/GoogleApis.Auth.Tests/GoogleApis.Auth.Tests.csproj index fd2fea78fc..bd5258df5b 100644 --- a/Src/Support/GoogleApis.Auth.Tests/GoogleApis.Auth.Tests.csproj +++ b/Src/Support/GoogleApis.Auth.Tests/GoogleApis.Auth.Tests.csproj @@ -56,8 +56,8 @@ ..\packages\Newtonsoft.Json.7.0.1\lib\net45\Newtonsoft.Json.dll True - - ..\packages\NUnit.2.6.3\lib\nunit.framework.dll + + ..\packages\NUnit.3.2.1\lib\net45\nunit.framework.dll True diff --git a/Src/Support/GoogleApis.Auth.Tests/OAuth2/BearerTokenTests.cs b/Src/Support/GoogleApis.Auth.Tests/OAuth2/BearerTokenTests.cs index 33933fe11c..dc8bbf9549 100644 --- a/Src/Support/GoogleApis.Auth.Tests/OAuth2/BearerTokenTests.cs +++ b/Src/Support/GoogleApis.Auth.Tests/OAuth2/BearerTokenTests.cs @@ -52,7 +52,7 @@ public void AuthorizationHeaderAccessMethod_GetAccessToken() var request = new HttpRequestMessage(); request.Headers.Authorization = new AuthenticationHeaderValue("a", "1"); var accessToken = new BearerToken.AuthorizationHeaderAccessMethod().GetAccessToken(request); - Assert.IsNullOrEmpty(accessToken); + Assert.That(accessToken, Is.Null.Or.Empty); request.Headers.Authorization = new AuthenticationHeaderValue("Bearer", "abc"); accessToken = new BearerToken.AuthorizationHeaderAccessMethod().GetAccessToken(request); @@ -81,12 +81,12 @@ public void QueryParameterAccessMethod_GetAccessToken() // No query parameter at all. var request = new HttpRequestMessage(HttpMethod.Get, new Uri("https://sample.com")); var accessToken = new BearerToken.QueryParameterAccessMethod().GetAccessToken(request); - Assert.IsNullOrEmpty(accessToken); + Assert.That(accessToken, Is.Null.Or.Empty); // Different query parameter. request = new HttpRequestMessage(HttpMethod.Get, new Uri("https://sample.com?a=1")); accessToken = new BearerToken.QueryParameterAccessMethod().GetAccessToken(request); - Assert.IsNullOrEmpty(accessToken); + Assert.That(accessToken, Is.Null.Or.Empty); // One query parameter and it's access_token. request = new HttpRequestMessage(HttpMethod.Get, new Uri("https://sample.com?a=1&access_token=abc")); diff --git a/Src/Support/GoogleApis.Auth.Tests/packages.config b/Src/Support/GoogleApis.Auth.Tests/packages.config index 4d48ac946a..f0e45e2095 100644 --- a/Src/Support/GoogleApis.Auth.Tests/packages.config +++ b/Src/Support/GoogleApis.Auth.Tests/packages.config @@ -2,5 +2,5 @@ - + \ No newline at end of file diff --git a/Src/Support/GoogleApis.Tests/Apis/Requests/ClientServiceRequestTest.cs b/Src/Support/GoogleApis.Tests/Apis/Requests/ClientServiceRequestTest.cs index a80fac1ec4..661da3ced6 100644 --- a/Src/Support/GoogleApis.Tests/Apis/Requests/ClientServiceRequestTest.cs +++ b/Src/Support/GoogleApis.Tests/Apis/Requests/ClientServiceRequestTest.cs @@ -41,7 +41,7 @@ namespace Google.Apis.Tests.Apis.Requests [TestFixture] public class ClientServiceRequestTest { - [TestFixtureSetUp] + [OneTimeSetUp] public void SetUp() { // Uncomment to enable logging during tests diff --git a/Src/Support/GoogleApis.Tests/Apis/Upload/ResumableUploadTest.cs b/Src/Support/GoogleApis.Tests/Apis/Upload/ResumableUploadTest.cs index f18887adff..c9d110f2bd 100644 --- a/Src/Support/GoogleApis.Tests/Apis/Upload/ResumableUploadTest.cs +++ b/Src/Support/GoogleApis.Tests/Apis/Upload/ResumableUploadTest.cs @@ -46,7 +46,7 @@ nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum."; - [TestFixtureSetUp] + [OneTimeSetUp] public void SetUp() { // Change the false parameter to true if you want to enable logging during tests. diff --git a/Src/Support/GoogleApis.Tests/GoogleApis.Tests.csproj b/Src/Support/GoogleApis.Tests/GoogleApis.Tests.csproj index 1ab1078061..62f282e441 100644 --- a/Src/Support/GoogleApis.Tests/GoogleApis.Tests.csproj +++ b/Src/Support/GoogleApis.Tests/GoogleApis.Tests.csproj @@ -84,8 +84,8 @@ ..\packages\Newtonsoft.Json.7.0.1\lib\net45\Newtonsoft.Json.dll True - - ..\packages\NUnit.2.6.3\lib\nunit.framework.dll + + ..\packages\NUnit.3.2.1\lib\net45\nunit.framework.dll True diff --git a/Src/Support/GoogleApis.Tests/packages.config b/Src/Support/GoogleApis.Tests/packages.config index 225a5b2418..8fc44f3b69 100644 --- a/Src/Support/GoogleApis.Tests/packages.config +++ b/Src/Support/GoogleApis.Tests/packages.config @@ -3,6 +3,6 @@ - + \ No newline at end of file diff --git a/travis.sh b/travis.sh index 50cbb8a5ee..238b43f837 100755 --- a/travis.sh +++ b/travis.sh @@ -4,8 +4,8 @@ set -e # grab nunit runners -nuget install NUnit.Runners -Version 2.6.4 -OutputDirectory testrunner -NUNIT="${PWD}/testrunner/NUnit.Runners.2.6.4/tools/nunit-console.exe" +nuget install NUnit.ConsoleRunner -Version 3.2.1 -OutputDirectory testrunner +NUNIT="${PWD}/testrunner/NUnit.ConsoleRunner.3.2.1/tools/nunit3-console.exe" cd Src/Support From 0307285f1abe603dfab62d978d94e15909a25e69 Mon Sep 17 00:00:00 2001 From: Matthew Riley Date: Tue, 12 Apr 2016 15:01:54 -0700 Subject: [PATCH 2/2] Overhaul of MediaDownloader: simpler, handles `Content-Encoding: gzip` Today, MediaDownloader downloads content in chunks by sending one HTTP request per chunk. Each request has an appropriate `Range` header set. One consequence of this approach is that when users try to download content served with `Content-Encoding: gzip` and that content happens to be more than one chunk large, the download will fail with an error like: `System.IO.InvalidDataException: The magic number in GZip header is not correct. Make sure you are passing in a GZip stream.` `HttpClientHandler` is trying to do the decoding for us because we set its `AutomaticDecompression` property, but the decoding fails because we've requested a range of data in the middle of a GZip stream. Users can encounter this when downloading files from GCS that were uploaded with `gsutil -Z`. I considered asking the server to send us already-decompressed content by not setting the `Accept-Encoding` header in our requests. Aside from wasting customers' bandwidth, this turns out not to work: GCS ignores the header and returns gzipped data regardless. I also briefly considered special case handling for GZip downloads, e.g. setting a huge ChunkSize or buffering intermediate results. Instead, I decided to make things simpler and more flexible by only making one request per download. As far as I can tell, there wasn't any benefit to the multiple-request approach. It probably hurt performance because we interrupted established transfers. To avoid any impact on clients, ChunkSize is retained. Now it indicates the granularity at which content should be written to the output stream and our callers will be notified of progress. I've verified that the progress events the caller sees are the same for both implementations. I could not find a way to test this in MediaDownloaderTest as it was. We were mocking out the request side of HttpClient and not exercising any of the code that is actually responsible for transport concerns like compression. My solution was to rewrite MediaDownloaderTest to use a local HTTP server that serves the responses we need to exercise the behavior under test. A lot of code changed, but I really think the resulting test is easier to read and less fragile than before. The old MediaDownloader implementation passes the new tests (except the one that returns GZip content). Only minimal modifications to the old tests are necessary for them to pass on the new MediaDownloader -- mostly removing those tests' baked-in assumptions about how many requests would be made and for what ranges. While I was there, I replaced some fiddly Query string manipulation code in MediaDownloader that took apart the Query string only to immediately reassembly it. That removed MediaDownloader's use of RequestBuilder. Since the latter is what we trusted to bring in our URI hacks, I made `PatchUriQuirks` public and added a class initializer to MediaDownloader to use it. --- .../GoogleApis.Core/Apis/Util/UriPatcher.cs | 4 +- .../Apis/Download/MediaDownloaderTest.cs | 365 +++++++++++------- .../Apis/[Media]/Download/MediaDownloader.cs | 202 ++++++---- 3 files changed, 348 insertions(+), 223 deletions(-) diff --git a/Src/Support/GoogleApis.Core/Apis/Util/UriPatcher.cs b/Src/Support/GoogleApis.Core/Apis/Util/UriPatcher.cs index 15d3657aa6..f046a5d19f 100644 --- a/Src/Support/GoogleApis.Core/Apis/Util/UriPatcher.cs +++ b/Src/Support/GoogleApis.Core/Apis/Util/UriPatcher.cs @@ -54,9 +54,9 @@ namespace Google.Apis.Util // As a class library, we can't control app.config or the entry assembly, so we can't take // either approach. Instead, we resort to reflection trickery to try to solve these problems // if we detect they exist. Sorry. - internal static class UriPatcher + public static class UriPatcher { - internal static void PatchUriQuirks() + public static void PatchUriQuirks() { var uriParser = typeof(System.Uri).Assembly.GetType("System.UriParser"); if (uriParser == null) { return; } diff --git a/Src/Support/GoogleApis.Tests/Apis/Download/MediaDownloaderTest.cs b/Src/Support/GoogleApis.Tests/Apis/Download/MediaDownloaderTest.cs index 1d024c96ee..47ad94596f 100644 --- a/Src/Support/GoogleApis.Tests/Apis/Download/MediaDownloaderTest.cs +++ b/Src/Support/GoogleApis.Tests/Apis/Download/MediaDownloaderTest.cs @@ -17,10 +17,12 @@ limitations under the License. using System; using System.Collections.Generic; using System.IO; +using System.IO.Compression; using System.Linq; using System.Net; using System.Net.Http; using System.Net.Http.Headers; +using System.Net.Sockets; using System.Text; using System.Threading; using System.Threading.Tasks; @@ -29,7 +31,6 @@ limitations under the License. using Google.Apis.Download; using Google.Apis.Json; -using Google.Apis.Services; using Google.Apis.Requests; using Google.Apis.Util; @@ -39,95 +40,175 @@ namespace Google.Apis.Tests.Apis.Download [TestFixture] class MediaDownloaderTest { - /// The content the "server" uses to send to the client. + /// A content string that will be returned and looked for. private static readonly byte[] MediaContent = Encoding.UTF8.GetBytes("Media content goes here. This is an example of test content."); - /// An handler which demonstrate a client download which contains multiple chunks. - private class MultipleChunksMessageHandler : CountableMessageHandler + /// + /// An error object that will be returned and looked for. + /// + private static readonly RequestError BadRequestError = new RequestError { - /// Gets or sets the chunk size to send. - public int ChunkSize { get; set; } + Code = 12345, + Message = "Bad Request!", + Errors = new[] { new SingleError { Message = "The request was malformed." } } + }; - /// Gets or sets the status code in case we simulate an error. - public HttpStatusCode StatusCode { get; set; } + /// + /// An error string that will be returned and looked for. + /// + private const string NotFoundError = "No resource found by that name."; - /// Gets or sets the cancellation token used to cancel a request in the middle. - public CancellationTokenSource CancellationTokenSource { get; set; } + /// + /// Get an available port. This is best-effort since the port may immediately be + /// used by another process. + /// + /// A port number + private static int GetOpenPort() + { + var tcpListener = new TcpListener(IPAddress.Loopback, 0); + try + { + tcpListener.Start(); + return ((IPEndPoint)tcpListener.LocalEndpoint).Port; + } + finally + { + tcpListener.Stop(); + } + } - /// Gets or sets the request index we are going to cancel. - public int CancelRequestNum { get; set; } + /// + /// Prefix on which the test server will listen. {0} is replaced by port. + /// + private const string PrefixFormat = "http://localhost:{0}/download/"; - /// Gets or sets the download Uri. - public Uri DownloadUri { get; set; } + /// + /// Prefix on which the test server is listening, e.g. "http://localhost:12345/path/". + /// + private string _httpPrefix; - /// Content to stream for success requests. - internal Stream StreamContent { get; set; } + /// + /// The Task running the test server. + /// + private Task _httpServerTask; - /// Content to return (in UTF-8) for error requests. - internal string ErrorResponse { get; set; } + /// + /// Run a simple HTTP server that listens for a few test URIs. + /// The server exits when a client requests "/Quit". + /// + /// Prefix at which the server should listen + private async Task RunTestServer(string prefix) + { + using (var httpListener = new HttpListener()) + { + httpListener.Prefixes.Add(prefix); + httpListener.Start(); - /// The number of bytes this "server" has sent so far. - private long bytesRead; + while (httpListener.IsListening) + { + var context = await httpListener.GetContextAsync(); - /// - /// Constructor which uses the given content for StreamContent. - /// - internal MultipleChunksMessageHandler(byte[] content) - { - StreamContent = new MemoryStream(content); - } + var requestUri = context.Request.Url; + var response = context.Response; - /// - /// Constructor which sets StreamContent to an initially-empty stream. - /// - internal MultipleChunksMessageHandler() - { - StreamContent = new MemoryStream(); - } + if (requestUri.AbsolutePath.EndsWith("/Quit")) + { + // Shut down the HTTP server. + response.Close(); + httpListener.Stop(); + continue; + } - protected override Task SendAsyncCore(HttpRequestMessage request, - CancellationToken cancellationToken) - { - TaskCompletionSource tcs = new TaskCompletionSource(); + // All downloader requests should include ?alt=media. + Assert.AreEqual("media", context.Request.QueryString["alt"]); - if (Calls == CancelRequestNum && CancellationTokenSource != null) - { - CancellationTokenSource.Cancel(); - } + response.ContentType = "text/plain"; + response.SendChunked = true; // Avoid having to set Content-Length. - // validate the request - Assert.That(request.RequestUri, Is.EqualTo(DownloadUri)); - Assert.That(request.Headers.Range, Is.EqualTo(new RangeHeaderValue(bytesRead, - bytesRead + ChunkSize - 1))); + Stream outStream = new MemoryStream(); - // prepare the response to send back - var response = new HttpResponseMessage(); - response.StatusCode = StatusCode; - if (StatusCode == HttpStatusCode.OK) - { - // read the current data from the stream - var contentLength = StreamContent.Length; - byte[] buffer = new byte[ChunkSize]; - int currentRead = StreamContent.Read(buffer, 0, ChunkSize); - var readStream = new MemoryStream(buffer, 0, currentRead); - - response.Content = new StreamContent(readStream); - response.Content.Headers.ContentType = new MediaTypeHeaderValue("image/png"); - response.Content.Headers.ContentLength = currentRead; - response.Content.Headers.ContentRange = new ContentRangeHeaderValue(bytesRead, - bytesRead + currentRead - 1, contentLength); - - bytesRead += currentRead; - } - else - { - response.Content = new StreamContent(new MemoryStream(Encoding.UTF8.GetBytes(ErrorResponse))); + if (requestUri.AbsolutePath.EndsWith("/EchoUrl")) + { + // Return the URL that we saw. + byte[] uriBytes = Encoding.UTF8.GetBytes(requestUri.AbsoluteUri); + outStream.Write(uriBytes, 0, uriBytes.Length); + } + else if (requestUri.AbsolutePath.EndsWith("/BadRequestJson")) + { + // Return 400 with a JSON-encoded error. + var apiResponse = new StandardResponse { Error = BadRequestError }; + var apiResponseText = new NewtonsoftJsonSerializer().Serialize(apiResponse); + byte[] apiResponseBytes = Encoding.UTF8.GetBytes(apiResponseText); + + response.StatusCode = (int)HttpStatusCode.BadRequest; + outStream.Write(apiResponseBytes, 0, apiResponseBytes.Length); + } + else if (requestUri.AbsolutePath.EndsWith("/NotFoundPlainText")) + { + // Return 404 with a plaintext error. + response.StatusCode = (int)HttpStatusCode.NotFound; + byte[] errorBytes = Encoding.UTF8.GetBytes(NotFoundError); + outStream.Write(errorBytes, 0, errorBytes.Length); + } + else if (requestUri.AbsolutePath.EndsWith("/GzipContent")) + { + // Return gzip-compressed content. + using (var gzipStream = new GZipStream(outStream, CompressionMode.Compress, true)) + { + gzipStream.Write(MediaContent, 0, MediaContent.Length); + } + response.AddHeader("Content-Encoding", "gzip"); + } + else + { + // Return plaintext content. + outStream.Write(MediaContent, 0, MediaContent.Length); + } + + outStream.Position = 0; + + // Provide rudimentary, non-robust support for Range. + // MediaDownloader doesn't exercise this code anymore, but it was useful for + // testing previous implementations that did. It remains for posterity. + string rangeHeader = context.Request.Headers["Range"]; + if (rangeHeader != null && response.StatusCode == (int)HttpStatusCode.OK) + { + var range = RangeHeaderValue.Parse(rangeHeader); + var firstRange = range.Ranges.First(); + + long from = firstRange.From ?? 0; + long to = Math.Min(outStream.Length - 1, firstRange.To ?? long.MaxValue); + + var contentRangeHeader = new ContentRangeHeaderValue(from, to, outStream.Length); + response.AddHeader("Content-Range", contentRangeHeader.ToString()); + + outStream.Position = from; + outStream.SetLength(to + 1); + } + + await outStream.CopyToAsync(response.OutputStream); + + response.Close(); } - tcs.SetResult(response); - return tcs.Task; } } + [OneTimeSetUp] + public void SetUp() + { + // Start up the test HTTP server. + _httpPrefix = String.Format(PrefixFormat, GetOpenPort()); + _httpServerTask = RunTestServer(_httpPrefix); + } + + [OneTimeTearDown] + public async Task TearDown() + { + // Shut down the test HTTP server. + await new HttpClient().GetAsync(_httpPrefix + "Quit"); + await _httpServerTask; + } + /// Tests that download works in case the server returns multiple chunks to the client. [Test] public void Download_MultipleChunks() @@ -136,6 +217,13 @@ public void Download_MultipleChunks() Subtest_Download_Chunks(MediaContent.Length - 1); } + /// Tests that download works in case the data is retrieved in multiple chunks and is gzipped. + [Test] + public void Download_MultipleChunksGzipped() + { + Subtest_Download_Chunks(2, true, 0, "GzipContent"); + } + /// Tests that download works in case the server returns a single chunk to the client. [Test] public void Download_SingleChunk() @@ -149,21 +237,24 @@ public void Download_SingleChunk() [Test] public void Download_SingleChunk_UriContainsQueryParameters() { - Subtest_Download_Chunks(MediaContent.Length, true, 0, "https://www.sample.com?a=1&b=2"); + string url = _httpPrefix + "EchoUrl?a=1&b=2"; + Assert.AreEqual(SimpleDownload(url), url + "&alt=media"); } /// Tests that download works in case the URI download contains query parameters. [Test] public void Download_SingleChunk_UriContainsEncodedQueryParameters() { - Subtest_Download_Chunks(MediaContent.Length, true, 0, "https://www.sample.com?a=foo%2Fbar"); + string url = _httpPrefix + "EchoUrl?a=foo%2Fbar"; + Assert.AreEqual(SimpleDownload(url), url + "&alt=media"); } /// Tests that download works in case the URI download contains query parameters. [Test] public void Download_SingleChunk_UriContainsValuelessQueryParameters() { - Subtest_Download_Chunks(MediaContent.Length, true, 0, "https://www.sample.com?a&b=1"); + string url = _httpPrefix + "EchoUrl?a&b=1"; + Assert.AreEqual(SimpleDownload(url), url + "&alt=media"); } /// @@ -197,36 +288,59 @@ public void DownloadAsync_Cancel() Subtest_Download_Chunks(MediaContent.Length - 1, false, 1); } + /// + /// Uses MediaDownloader to download the contents of a URI. + /// Asserts that the download succeeded and returns the resulting content as a string. + /// + /// Uri to download + /// + private string SimpleDownload(string uri) + { + using (var service = new MockClientService()) + { + var downloader = new MediaDownloader(service); + var outputStream = new MemoryStream(); + var result = downloader.Download(uri, outputStream); + + Assert.AreEqual(result.Status, DownloadStatus.Completed); + Assert.IsNull(result.Exception); + Assert.AreEqual(result.BytesDownloaded, outputStream.Position); + + return Encoding.UTF8.GetString(outputStream.GetBuffer(), 0, (int)outputStream.Position); + } + } + /// A helper test to test sync and async downloads. /// The chunk size for each part. /// Indicates if this download should be synchronously or asynchronously. - /// Defines the request index to cancel the download request. - /// The URI which contains the media to download. - private void Subtest_Download_Chunks(int chunkSize, bool sync = true, int cancelRequest = 0, - string downloadUri = "http://www.sample.com") + /// Defines the chunk at which to cancel the download request. + /// Last component of the Uri to download + private void Subtest_Download_Chunks(int chunkSize, bool sync = true, int cancelChunk = 0, string target = "content") { - var handler = new MultipleChunksMessageHandler(MediaContent); - handler.StatusCode = HttpStatusCode.OK; - handler.ChunkSize = chunkSize; - handler.DownloadUri = new Uri(downloadUri + - (downloadUri.Contains("?") ? "&" : "?") + "alt=media"); - - // support cancellation - if (cancelRequest > 0) - { - handler.CancelRequestNum = cancelRequest; - } - handler.CancellationTokenSource = new CancellationTokenSource(); + string downloadUri = _httpPrefix + target; + var cts = new CancellationTokenSource(); - int expectedCalls = (int)Math.Ceiling((double) MediaContent.Length / chunkSize); - using (var service = CreateMockClientService(handler)) + using (var service = new MockClientService()) { var downloader = new MediaDownloader(service); downloader.ChunkSize = chunkSize; IList progressList = new List(); + int progressUpdates = 0; + long lastBytesDownloaded = 0; downloader.ProgressChanged += (p) => { + if (p.Status != DownloadStatus.Failed) + { + // We shouldn't receive duplicate notifications for the same range. + Assert.That(p.BytesDownloaded, Is.GreaterThan(lastBytesDownloaded)); + } + lastBytesDownloaded = p.BytesDownloaded; + progressList.Add(p); + if (++progressUpdates == cancelChunk) + { + cts.Cancel(); + } }; var outputStream = new MemoryStream(); @@ -238,11 +352,10 @@ private void Subtest_Download_Chunks(int chunkSize, bool sync = true, int cancel { try { - var result = downloader.DownloadAsync(downloadUri, outputStream, - handler.CancellationTokenSource.Token).Result; + var result = downloader.DownloadAsync(downloadUri, outputStream, cts.Token).Result; if (result.Exception == null) { - Assert.AreEqual(0, handler.CancelRequestNum); + Assert.AreEqual(0, cancelChunk); } else { @@ -256,20 +369,16 @@ private void Subtest_Download_Chunks(int chunkSize, bool sync = true, int cancel } var lastProgress = progressList.LastOrDefault(); - if (cancelRequest > 0) + if (cancelChunk > 0) { - // the download was interrupted in the middle - Assert.That(handler.Calls, Is.EqualTo(cancelRequest)); // last request should fail Assert.NotNull(lastProgress); Assert.NotNull(lastProgress.Exception); Assert.That(lastProgress.Status, Is.EqualTo(DownloadStatus.Failed)); - Assert.That(lastProgress.BytesDownloaded, Is.EqualTo(chunkSize * cancelRequest)); + Assert.That(lastProgress.BytesDownloaded, Is.EqualTo(chunkSize * cancelChunk)); } else { - // the download succeeded - Assert.That(handler.Calls, Is.EqualTo(expectedCalls)); Assert.NotNull(lastProgress); Assert.Null(lastProgress.Exception); Assert.That(lastProgress.Status, Is.EqualTo(DownloadStatus.Completed)); @@ -285,22 +394,9 @@ private void Subtest_Download_Chunks(int chunkSize, bool sync = true, int cancel [Test] public void Download_Error_JsonResponse() { - var downloadUri = "http://www.sample.com"; - var chunkSize = 100; - var error = new RequestError { Code = 12345, Message = "Text", Errors = new[] { new SingleError { Message = "Nested error" } } }; - var response = new StandardResponse { Error = error }; - var responseText = new NewtonsoftJsonSerializer().Serialize(response); - - var handler = new MultipleChunksMessageHandler { ErrorResponse = responseText }; - handler.StatusCode = HttpStatusCode.BadRequest; - handler.ChunkSize = chunkSize; - // The media downloader adds the parameter... - handler.DownloadUri = new Uri(downloadUri + "?alt=media"); - - using (var service = CreateMockClientService(handler)) + using (var service = new MockClientService()) { var downloader = new MediaDownloader(service); - downloader.ChunkSize = chunkSize; IList progressList = new List(); downloader.ProgressChanged += (p) => { @@ -308,36 +404,24 @@ public void Download_Error_JsonResponse() }; var outputStream = new MemoryStream(); - downloader.Download(downloadUri, outputStream); + downloader.Download(_httpPrefix + "BadRequestJson", outputStream); var lastProgress = progressList.LastOrDefault(); Assert.That(lastProgress.Status, Is.EqualTo(DownloadStatus.Failed)); GoogleApiException exception = (GoogleApiException) lastProgress.Exception; - Assert.That(exception.HttpStatusCode, Is.EqualTo(handler.StatusCode)); + Assert.That(exception.HttpStatusCode, Is.EqualTo(HttpStatusCode.BadRequest)); // Just a smattering of checks - if these two pass, it's surely okay. - Assert.That(exception.Error.Code, Is.EqualTo(error.Code)); - Assert.That(exception.Error.Errors[0].Message, Is.EqualTo(error.Errors[0].Message)); + Assert.That(exception.Error.Code, Is.EqualTo(BadRequestError.Code)); + Assert.That(exception.Error.Errors[0].Message, Is.EqualTo(BadRequestError.Errors[0].Message)); } } [Test] - [TestCase("Not Found")] - [TestCase("")] - public void Download_Error_PlaintextResponse(string responseText) + public void Download_Error_PlaintextResponse() { - var downloadUri = "http://www.sample.com"; - var chunkSize = 100; - - var handler = new MultipleChunksMessageHandler { ErrorResponse = responseText }; - handler.StatusCode = HttpStatusCode.NotFound; - handler.ChunkSize = chunkSize; - // The media downloader adds the parameter... - handler.DownloadUri = new Uri(downloadUri + "?alt=media"); - - using (var service = CreateMockClientService(handler)) + using (var service = new MockClientService()) { var downloader = new MediaDownloader(service); - downloader.ChunkSize = chunkSize; IList progressList = new List(); downloader.ProgressChanged += (p) => { @@ -345,26 +429,15 @@ public void Download_Error_PlaintextResponse(string responseText) }; var outputStream = new MemoryStream(); - downloader.Download(downloadUri, outputStream); + downloader.Download(_httpPrefix + "NotFoundPlainText", outputStream); var lastProgress = progressList.LastOrDefault(); Assert.That(lastProgress.Status, Is.EqualTo(DownloadStatus.Failed)); GoogleApiException exception = (GoogleApiException) lastProgress.Exception; - Assert.That(exception.HttpStatusCode, Is.EqualTo(handler.StatusCode)); - Assert.That(exception.Message, Is.EqualTo(responseText)); + Assert.That(exception.HttpStatusCode, Is.EqualTo(HttpStatusCode.NotFound)); + Assert.That(exception.Message, Is.EqualTo(NotFoundError)); Assert.IsNull(exception.Error); } } - - /// - /// Creates a mock client service whose HTTP client factory uses the specified message handler. - /// - private static IClientService CreateMockClientService(HttpMessageHandler handler) - { - return new MockClientService(new BaseClientService.Initializer - { - HttpClientFactory = new MockHttpClientFactory(handler) - }); - } } } diff --git a/Src/Support/GoogleApis/Apis/[Media]/Download/MediaDownloader.cs b/Src/Support/GoogleApis/Apis/[Media]/Download/MediaDownloader.cs index b2ac958769..617c752922 100644 --- a/Src/Support/GoogleApis/Apis/[Media]/Download/MediaDownloader.cs +++ b/Src/Support/GoogleApis/Apis/[Media]/Download/MediaDownloader.cs @@ -15,26 +15,28 @@ limitations under the License. */ using System; -using System.Linq; using System.IO; -using System.Net.Http.Headers; +using System.Net.Http; using System.Threading; using System.Threading.Tasks; using Google.Apis.Logging; using Google.Apis.Media; -using Google.Apis.Requests; using Google.Apis.Services; using Google.Apis.Util; namespace Google.Apis.Download { /// - /// A media downloader implementation which handles media downloads. It supports downloading a media content in - /// chunks, when each chunk size is defined by . + /// A media downloader implementation which handles media downloads. /// public class MediaDownloader : IMediaDownloader { + static MediaDownloader() + { + UriPatcher.PatchUriQuirks(); + } + private static readonly ILogger Logger = ApplicationContext.Logger.ForType(); /// The service which this downloader belongs to. @@ -47,9 +49,10 @@ public class MediaDownloader : IMediaDownloader private int chunkSize = MaximumChunkSize; /// - /// Gets or sets the size of each chunk to download from the server. - /// Chunks can't be set to be a value bigger than . Default value is - /// . + /// Gets or sets the amount of data that will be downloaded before notifying the caller of + /// the download's progress. + /// Must not exceed . + /// Default value is . /// public int ChunkSize { @@ -148,8 +151,71 @@ public async Task DownloadAsync(string url, Stream stream, #endregion /// - /// The core download logic. It downloads the media in parts, where each part's size is defined by - /// (in bytes). + /// CountedBuffer bundles together a byte buffer and a count of valid bytes. + /// + private class CountedBuffer + { + public byte[] Data { get; set; } + + /// + /// How many bytes at the beginning of Data are valid. + /// + public int Count { get; private set; } + + public CountedBuffer(int size) + { + Data = new byte[size]; + Count = 0; + } + + /// + /// Returns true if the buffer contains no data. + /// + public bool IsEmpty { get { return Count == 0; } } + + /// + /// Read data from stream until the stream is empty or the buffer is full. + /// + /// Stream from which to read. + /// Cancellation token for the operation. + public async Task Fill(Stream stream, CancellationToken cancellationToken) + { + // ReadAsync may return if it has *any* data available, so we loop. + while (Count < Data.Length) + { + int read = await stream.ReadAsync(Data, Count, Data.Length - Count, cancellationToken).ConfigureAwait(false); + if (read == 0) { break; } + Count += read; + } + } + + /// + /// Remove the first n bytes of the buffer. Move any remaining valid bytes to the beginning. + /// Trying to remove more bytes than the buffer contains just clears the buffer. + /// + /// The number of bytes to remove. + public void RemoveFromFront(int n) + { + if (n >= Count) + { + Count = 0; + } + else + { + // Some valid data remains. + Array.Copy(Data, n, Data, 0, Count - n); + Count -= n; + } + } + } + + /// + /// The core download logic. We download the media and write it to an output stream + /// ChunkSize bytes at a time, raising the ProgressChanged event after each chunk. + /// + /// The chunking behavior is largely a historical artifact: a previous implementation + /// issued multiple web requests, each for ChunkSize bytes. Now we do everything in + /// one request, but the API and client-visible behavior are retained for compatibility. /// /// The URL of the resource to download. /// The download will download the resource into this stream. @@ -166,96 +232,82 @@ private async Task DownloadCoreAsync(string url, Stream strea throw new ArgumentException("stream doesn't support write operations"); } - RequestBuilder builder = null; - - var uri = new Uri(url); - if (string.IsNullOrEmpty(uri.Query)) + // Add alt=media to the query parameters. + var uri = new UriBuilder(url); + if (uri.Query == null || uri.Query.Length <= 1) { - builder = new RequestBuilder() { BaseUri = new Uri(url) }; + uri.Query = "alt=media"; } else { - builder = new RequestBuilder() { BaseUri = new Uri(url.Substring(0, url.IndexOf("?"))) }; - // Remove '?' at the beginning. - var query = uri.Query.Substring(1); - var pairs = from parameter in query.Split('&') - select parameter.Split('='); - // Add each query parameter. each pair contains the key [0] and then its value [1]. - foreach (var p in pairs) - { - var key = Uri.UnescapeDataString(p[0]); - var value = p.Length == 2 ? Uri.UnescapeDataString(p[1]) : ""; - builder.AddParameter(RequestParameterType.Query, key, value); - } + // Remove the leading '?'. UriBuilder.Query doesn't round-trip. + uri.Query = uri.Query.Substring(1) + "&alt=media"; } - builder.AddParameter(RequestParameterType.Query, "alt", "media"); - long currentRequestFirstBytePos = 0; + var request = new HttpRequestMessage(HttpMethod.Get, uri.ToString()); + + // Number of bytes sent to the caller's stream. + int bytesReturned = 0; try { - // This "infinite" loop stops when the "to" byte position in the "Content-Range" header is the last - // byte of the media ("length"-1 in the "Content-Range" header). - // e.g. "Content-Range: 200-299/300" - "to"(299) = "length"(300) - 1. - while (true) - { - var currentRequestLastBytePos = currentRequestFirstBytePos + ChunkSize - 1; - - // Create the request and set the Range header. - var request = builder.CreateRequest(); - request.Headers.Range = new RangeHeaderValue(currentRequestFirstBytePos, - currentRequestLastBytePos); + // Signal SendAsync to return as soon as the response headers are read. + // We'll stream the content ourselves as it becomes available. + var completionOption = HttpCompletionOption.ResponseHeadersRead; - using (var response = await service.HttpClient.SendAsync(request, cancellationToken). - ConfigureAwait(false)) + using (var response = await service.HttpClient.SendAsync(request, completionOption, cancellationToken).ConfigureAwait(false)) + { + if (!response.IsSuccessStatusCode) { - if (!response.IsSuccessStatusCode) - { - throw await MediaApiErrorHandling.ExceptionForResponseAsync(service, response).ConfigureAwait(false); - } - - // Read the content and copy to the parameter's stream. - var responseStream = await response.Content.ReadAsStreamAsync().ConfigureAwait(false); - responseStream.CopyTo(stream); - - // Read the headers and check if all the media content was already downloaded. - var contentRange = response.Content.Headers.ContentRange; - long mediaContentLength; - - if (contentRange == null) - { - // Content range is null when the server doesn't adhere the media download protocol, in - // that case we got all the media in one chunk. - currentRequestFirstBytePos = mediaContentLength = - response.Content.Headers.ContentLength.Value; - } - else - { - currentRequestFirstBytePos = contentRange.To.Value + 1; - mediaContentLength = contentRange.Length.Value; - } + throw await MediaApiErrorHandling.ExceptionForResponseAsync(service, response).ConfigureAwait(false); + } - if (currentRequestFirstBytePos == mediaContentLength) + using (var responseStream = await response.Content.ReadAsStreamAsync().ConfigureAwait(false)) + { + // We send ChunkSize bytes at a time to the caller, but we keep ChunkSize + 1 bytes + // buffered. That way we can tell when we've reached the end of the response, even if the + // response length is evenly divisible by ChunkSize, and we can avoid sending a Downloading + // event followed by a Completed event with no bytes downloaded in between. + // + // This maintains the client-visible behavior of a previous implementation. + var buffer = new CountedBuffer(ChunkSize + 1); + + while (true) { - var progress = new DownloadProgress(DownloadStatus.Completed, mediaContentLength); - UpdateProgress(progress); - return progress; + await buffer.Fill(responseStream, cancellationToken).ConfigureAwait(false); + + // Send one chunk to the caller's stream. + int bytesToReturn = Math.Min(ChunkSize, buffer.Count); + await stream.WriteAsync(buffer.Data, 0, bytesToReturn, cancellationToken).ConfigureAwait(false); + checked { bytesReturned += bytesToReturn; } + + buffer.RemoveFromFront(ChunkSize); + if (buffer.IsEmpty) + { + // We had <= ChunkSize bytes buffered, so we've read and returned the entire response. + // Skip sending a Downloading event. We'll send Completed instead. + break; + } + + UpdateProgress(new DownloadProgress(DownloadStatus.Downloading, bytesReturned)); } } - UpdateProgress(new DownloadProgress(DownloadStatus.Downloading, currentRequestFirstBytePos)); + var finalProgress = new DownloadProgress(DownloadStatus.Completed, bytesReturned); + UpdateProgress(finalProgress); + return finalProgress; } } catch (TaskCanceledException ex) { Logger.Error(ex, "Download media was canceled"); - UpdateProgress(new DownloadProgress(ex, currentRequestFirstBytePos)); - throw ex; + UpdateProgress(new DownloadProgress(ex, bytesReturned)); + throw; } catch (Exception ex) { Logger.Error(ex, "Exception occurred while downloading media"); - var progress = new DownloadProgress(ex, currentRequestFirstBytePos); + var progress = new DownloadProgress(ex, bytesReturned); UpdateProgress(progress); return progress; }