Skip to content

Commit

Permalink
Minor but breaking changes (#15)
Browse files Browse the repository at this point in the history
* Minor but breaking changes

A quick internet search reveals that the two properties that derive from ServiceClient
are the now-deprecated Video API and sunsetting Emotion API, so should mostly go
unnoticed.

* Fix protected object name `UrlReqeust` to `UrlRequest`
* Expose the HttpClient instance to a derived class, giving it a chance to dispose it
* Beef up error handling to allow both old/new styles of error JSON.

* Better messages in exceptions

* Fix comments based on CR

* Fix comments based on CR (2)
  • Loading branch information
cthrash authored May 6, 2018
1 parent 10b0875 commit 6f1c953
Show file tree
Hide file tree
Showing 2 changed files with 20 additions and 14 deletions.
1 change: 1 addition & 0 deletions ClientLibrary/ClientException.cs
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,7 @@ public ClientException(string message, string errorCode, HttpStatusCode httpStat
/// <param name="error">The error entity.</param>
/// <param name="httpStatus">The http status.</param>
public ClientException(ClientError error, HttpStatusCode httpStatus)
: base(error?.Message)
{
this.Error = error;
this.HttpStatus = httpStatus;
Expand Down
33 changes: 19 additions & 14 deletions ClientLibrary/ServiceClient.cs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
//
//
// Copyright (c) Microsoft. All rights reserved.
// Licensed under the MIT license.
//
Expand Down Expand Up @@ -47,7 +47,7 @@ namespace Microsoft.ProjectOxford.Common
{
public abstract class ServiceClient : IDisposable
{
protected class UrlReqeust
protected class UrlRequest
{
public string url { get; set; }
}
Expand All @@ -71,7 +71,7 @@ public class WrappedClientError
ContractResolver = s_defaultResolver
};

private readonly HttpClient _httpClient;
protected HttpClient HttpClient { get; }

private readonly bool _ownHttpClient;

Expand Down Expand Up @@ -99,7 +99,7 @@ protected ServiceClient(HttpClient httpClient) : this(httpClient, false)
/// <param name="ownHttpClient">True if this object owns the HttpClient, false if the caller owns it.</param>
private ServiceClient(HttpClient httpClient, bool ownHttpClient)
{
_httpClient = httpClient;
HttpClient = httpClient;
_ownHttpClient = ownHttpClient;
}

Expand All @@ -121,7 +121,7 @@ protected virtual void Dispose(bool disposing)

if (disposing && _ownHttpClient)
{
_httpClient.Dispose();
HttpClient.Dispose();
}

_disposed = true;
Expand Down Expand Up @@ -150,7 +150,7 @@ protected virtual void Dispose(bool disposing)

#region the JSON client
/// <summary>
/// Helper method executing a GET REST request.
/// Helper method executing a POST REST request.
/// </summary>
/// <typeparam name="TRequest">Type of request.</typeparam>
/// <typeparam name="TResponse">Type of response.</typeparam>
Expand All @@ -165,18 +165,16 @@ protected Task<TResponse> PostAsync<TRequest, TResponse>(string apiUrl, TRequest
}

/// <summary>
/// Helper method executing a POST REST request.
/// Helper method executing a GET REST request.
/// </summary>
/// <typeparam name="TRequest">Type of request.</typeparam>
/// <typeparam name="TResponse">Type of response.</typeparam>
/// <param name="apiUrl">API URL relative to the apiRoot</param>
/// <param name="requestBody">Content of the HTTP request.</param>
/// <param name="cancellationToken">Async cancellation token</param>
/// <returns>TResponse</returns>
/// <exception cref="ClientException">Service exception</exception>
protected Task<TResponse> GetAsync<TRequest, TResponse>(string apiUrl, TRequest requestBody, CancellationToken cancellationToken)
protected Task<TResponse> GetAsync<TResponse>(string apiUrl, CancellationToken cancellationToken)
{
return SendAsync<TRequest, TResponse>(HttpMethod.Get, apiUrl, requestBody, cancellationToken);
return SendAsync<Object, TResponse>(HttpMethod.Get, apiUrl, null, cancellationToken);
}

/// <summary>
Expand Down Expand Up @@ -211,7 +209,7 @@ protected Task<TResponse> SendAsync<TRequest, TResponse>(HttpMethod method, stri
}
}

var task = _httpClient.SendAsync(request, cancellationToken)
var task = HttpClient.SendAsync(request, cancellationToken)
.ContinueWith(t => GetContent(t.Result)
.ContinueWith(u => GetResponse<TResponse>(t.Result, u.Result)));

Expand All @@ -230,7 +228,7 @@ private Task<string> GetContent(HttpResponseMessage response)
return response.Content.ReadAsStringAsync();
}
}
else if (response.Content != null && response.Content.Headers.ContentType.MediaType.Contains("application/json"))
else if (response.Content != null && response.Content.Headers.ContentType?.MediaType.Contains("application/json") == true)
{
return response.Content.ReadAsStringAsync();
}
Expand All @@ -251,13 +249,20 @@ private TResponse GetResponse<TResponse>(HttpResponseMessage response, string re
}
else
{
if (response.Content != null && response.Content.Headers.ContentType.MediaType.Contains("application/json"))
if (response?.Content?.Headers?.ContentType?.MediaType.Contains("application/json") == true)
{
// Some of the endpoints return an top-level 'error' field (WrappedClientError) whereas some will simply
// return the plain ClientError.
var wrappedClientError = JsonConvert.DeserializeObject<WrappedClientError>(responseContent);
if (wrappedClientError?.Error != null)
{
throw new ClientException(wrappedClientError.Error, response.StatusCode);
}
var clientError = JsonConvert.DeserializeObject<ClientError>(responseContent);
if (clientError != null)
{
throw new ClientException(clientError, response.StatusCode);
}
}

response.EnsureSuccessStatusCode();
Expand Down

0 comments on commit 6f1c953

Please sign in to comment.