try/catch when attempting to deserialize ServerError from response

This commit adds a try/catch block around attempting to deserialize a ServerError
from the body, since IElasticsearchResponse types withn Elasticsearch.Net
set/attempt to set the response body, irrespective of the response status code.

Move BytesResponseTests and StringResponseTests from Tests.Reproduce into
main Tests project.
Allow byte array to be passed to FixedResponseClient.

Fixes #3378

Author: russcam

src/Elasticsearch.Net/Responses/ElasticsearchResponse.cs

@@ -10,6 +10,7 @@ namespace Elasticsearch.Net
 	/// </summary>
 	public abstract class ElasticsearchResponseBase : IApiCallDetails, IElasticsearchResponse
 	{
+		/// <inheritdoc />
 		public IApiCallDetails ApiCall { get; set; }
 
 		bool IElasticsearchResponse.TryGetServerErrorReason(out string reason) => this.TryGetServerErrorReason(out reason);
@@ -20,35 +21,52 @@ protected virtual bool TryGetServerErrorReason(out string reason)
 			return false;
 		}
 
+		// TODO: Remove?
 		//ignored
 		List<Audit> IApiCallDetails.AuditTrail { get; set; }
 
+		/// <inheritdoc cref="IApiCallDetails.Success"/>
 		public bool Success => this.ApiCall.Success;
+
+		/// <inheritdoc cref="IApiCallDetails.ResponseMimeType"/>
 		public string ResponseMimeType => this.ApiCall.ResponseMimeType;
+
+		/// <inheritdoc cref="IApiCallDetails.HttpMethod"/>
 		public HttpMethod HttpMethod => this.ApiCall.HttpMethod;
+
+		/// <inheritdoc cref="IApiCallDetails.Uri"/>
 		public Uri Uri => this.ApiCall.Uri;
+
+		/// <inheritdoc cref="IApiCallDetails.HttpStatusCode"/>
 		public int? HttpStatusCode => this.ApiCall.HttpStatusCode;
+
+		/// <inheritdoc cref="IApiCallDetails.AuditTrail"/>
 		public List<Audit> AuditTrail => this.ApiCall.AuditTrail;
+
+		/// <inheritdoc cref="IApiCallDetails.DeprecationWarnings"/>
 		public IEnumerable<string> DeprecationWarnings => this.ApiCall.DeprecationWarnings;
+
+		/// <inheritdoc cref="IApiCallDetails.SuccessOrKnownError"/>
 		public bool SuccessOrKnownError => this.ApiCall.SuccessOrKnownError;
+
+		/// <inheritdoc cref="IApiCallDetails.OriginalException"/>
 		public Exception OriginalException => this.ApiCall.OriginalException;
 
-		/// <summary>The raw byte request message body, only set when DisableDirectStreaming() is set on Connection configuration</summary>
+		/// <inheritdoc cref="IApiCallDetails.RequestBodyInBytes"/>
 		public byte[] RequestBodyInBytes => this.ApiCall.RequestBodyInBytes;
 
-		/// <summary>The raw byte response message body, only set when DisableDirectStreaming() is set on Connection configuration</summary>
+		/// <inheritdoc cref="IApiCallDetails.ResponseBodyInBytes"/>
 		public byte[] ResponseBodyInBytes => this.ApiCall.ResponseBodyInBytes;
 
+		/// <inheritdoc cref="IApiCallDetails.DebugInformation"/>
 		public string DebugInformation => this.ApiCall.DebugInformation;
 
 		public override string ToString() => this.ApiCall.ToString();
-
-
 	}
 
 	/// <summary>
 	/// A response from Elasticsearch including details about the request/response life cycle. Base class for the built in low level response
-	/// types: <see cref="StringResponse"/> <see cref="BytesResponse"/> and <see cref="DynamicResponse"/>
+	/// types, <see cref="StringResponse"/>, <see cref="BytesResponse"/>, <see cref="DynamicResponse"/> and <see cref="VoidResponse"/>
 	/// </summary>
 	public abstract class ElasticsearchResponse<T> : ElasticsearchResponseBase
 	{

src/Elasticsearch.Net/Responses/HttpDetails/IApiCallDetails.cs

@@ -4,19 +4,22 @@
 
 namespace Elasticsearch.Net
 {
+	/// <summary>
+	/// Details about the API call
+	/// </summary>
 	public interface IApiCallDetails
 	{
 		/// <summary>
 		/// The response status code is in the 200 range or is in the allowed list of status codes set on the request.
 		/// </summary>
 		bool Success { get; }
 
-		/// <summary>The response mime type </summary>
+		/// <summary>The response MIME type </summary>
 		string ResponseMimeType { get; }
 
 		/// <summary>
 		/// If <see cref="Success"/> is <c>false</c>, this will hold the original exception.
-		/// This will be the orginating CLR exception in most cases.
+		/// This will be the originating CLR exception in most cases.
 		/// </summary>
 		Exception OriginalException { get; }
 
@@ -55,7 +58,7 @@ public interface IApiCallDetails
         [DebuggerDisplay("{RequestBodyInBytes != null ? System.Text.Encoding.UTF8.GetString(RequestBodyInBytes) : null,nq}")]
 		byte[] RequestBodyInBytes { get; }
 
-		//TODO Get rid of setter
+		//TODO: Get rid of setter
         /// <summary>
         /// An audit trail of requests made to nodes within the cluster
         /// </summary>

src/Elasticsearch.Net/Responses/Special/BytesResponse.cs

@@ -1,4 +1,5 @@
-using System.IO;
+using System;
+using System.IO;
 using System.Text;
 
 namespace Elasticsearch.Net
@@ -12,9 +13,16 @@ public bool TryGetServerError(out ServerError serverError)
 		{
 			serverError = null;
 			if (this.Body == null || this.Body.Length == 0) return false;
-			using(var stream = new MemoryStream(this.Body))
-				serverError = ServerError.Create(stream);
-			return true;
+			try
+			{
+				using(var stream = new MemoryStream(this.Body))
+					serverError = ServerError.Create(stream);
+				return true;
+			}
+			catch
+			{
+				return false;
+			}
 		}
 
 		protected override bool TryGetServerErrorReason(out string reason)

src/Elasticsearch.Net/Responses/Special/StringResponse.cs

@@ -1,4 +1,5 @@
-using System.IO;
+using System;
+using System.IO;
 using System.Text;
 
 namespace Elasticsearch.Net
@@ -12,9 +13,16 @@ public bool TryGetServerError(out ServerError serverError)
 		{
 			serverError = null;
 			if (string.IsNullOrEmpty(this.Body)) return false;
-			using(var stream = new MemoryStream(Encoding.UTF8.GetBytes(this.Body)))
-				serverError = ServerError.Create(stream);
-			return true;
+			try
+			{
+				using(var stream = new MemoryStream(Encoding.UTF8.GetBytes(this.Body)))
+					serverError = ServerError.Create(stream);
+				return true;
+			}
+			catch (Exception)
+			{
+				return false;
+			}
 		}
 
 		protected override bool TryGetServerErrorReason(out string reason)

src/Tests/Tests.Core/Client/FixedResponseClient.cs

@@ -26,12 +26,25 @@ public static ConnectionSettings CreateConnectionSettings(
 			Exception exception = null)
 		{
 			var serializer = TestClient.Default.RequestResponseSerializer;
-			byte[] fixedResult = null;
-			if (response is string s) fixedResult = Encoding.UTF8.GetBytes(s);
-			else if (contentType == RequestData.MimeType) fixedResult = serializer.SerializeToBytes(response);
-			else fixedResult = Encoding.UTF8.GetBytes(response.ToString());
+			byte[] responseBytes;
+			switch (response)
+			{
+				case string s:
+					responseBytes = Encoding.UTF8.GetBytes(s);
+					break;
+				case byte[] b:
+					responseBytes = b;
+					break;
+				default:
+				{
+					responseBytes = contentType == RequestData.MimeType
+						? serializer.SerializeToBytes(response)
+						: Encoding.UTF8.GetBytes(response.ToString());
+					break;
+				}
+			}
 
-			var connection = new InMemoryConnection(fixedResult, statusCode, exception, contentType);
+			var connection = new InMemoryConnection(responseBytes, statusCode, exception, contentType);
 			var connectionPool = new SingleNodeConnectionPool(new Uri("http://localhost:9200"));
 			var defaultSettings = new ConnectionSettings(connectionPool, connection)
 				.DefaultIndex("default-index");

src/Tests/Tests.Reproduce/BytesResponseTests.cs

@@ -9,6 +9,7 @@
 using Nest;
 using Tests.Domain;
 using Tests.Framework;
+using Tests.Framework.SerializationTests;
 
 namespace Tests.ClientConcepts.ConnectionPooling.Exceptions
 {
@@ -99,14 +100,7 @@ [U] public async Task BadAuthenticationIsUnrecoverable()
 			);
 		}
 
-		private static byte[] HtmlNginx401Response = Encoding.UTF8.GetBytes(@"<html>
-<head><title>401 Authorization Required</title></head>
-<body bgcolor=""white"">
-<center><h1>401 Authorization Required</h1></center>
-<hr><center>nginx/1.4.6 (Ubuntu)</center>
-</body>
-</html>
-");
+		private static byte[] HtmlNginx401Response = Encoding.UTF8.GetBytes(StubResponse.NginxHtml401Response);
 
 		/**
 		 * When a bad authentication response occurs, the client attempts to deserialize the response body returned;

src/Tests/Tests/ClientConcepts/ConnectionPooling/Exceptions/UnrecoverableExceptions.doc.cs

@@ -0,0 +1,73 @@
+using System;
+using System.Linq;
+using System.Text;
+using Elastic.Xunit.XunitPlumbing;
+using Elasticsearch.Net;
+using FluentAssertions;
+using Tests.Core.Client;
+using Tests.Core.ManagedElasticsearch.Clusters;
+
+namespace Tests.Framework.SerializationTests
+{
+	public class BytesResponseTests : IClusterFixture<ReadOnlyCluster>
+	{
+		private readonly ReadOnlyCluster _cluster;
+
+		public BytesResponseTests(ReadOnlyCluster cluster) => _cluster = cluster;
+
+		[I] public void NonNullBytesResponse()
+		{
+			var client = _cluster.Client;
+
+			var bytesResponse = client.LowLevel.Search<BytesResponse>("project", "project", PostData.Serializable(new { }));
+			bytesResponse.Body.Should().NotBeNull();
+			bytesResponse.Body.Should().BeEquivalentTo(bytesResponse.ResponseBodyInBytes);
+		}
+
+		[I] public void NonNullBytesLowLevelResponse()
+		{
+			var settings  = new ConnectionConfiguration(new Uri($"http://localhost:{_cluster.Nodes.First().Port ?? 9200}"));
+			var lowLevelClient = new ElasticLowLevelClient(settings);
+
+			var bytesResponse = lowLevelClient.Search<BytesResponse>("project", "project", PostData.Serializable(new { }));
+
+			bytesResponse.Body.Should().NotBeNull();
+			bytesResponse.Body.Should().BeEquivalentTo(bytesResponse.ResponseBodyInBytes);
+		}
+
+		[U]
+		public void TryGetServerErrorDoesNotThrowException()
+		{
+			var responseBytes = Encoding.UTF8.GetBytes(StubResponse.NginxHtml401Response);
+
+			var client = FixedResponseClient.Create(responseBytes, 401,
+				modifySettings: s => s.DisableDirectStreaming(),
+				contentType: "text/html",
+				exception: new Exception("problem with the request as a result of 401")
+			);
+
+			var bytesResponse = client.LowLevel.Search<BytesResponse>("project", "project", PostData.Serializable(new { }));
+
+			bytesResponse.Body.Should().NotBeNull();
+			bytesResponse.Body.Should().BeEquivalentTo(responseBytes);
+			bytesResponse.TryGetServerError(out var serverError).Should().BeFalse();
+			serverError.Should().BeNull();
+		}
+
+		[U] public void SkipDeserializationForStatusCodesSetsBody()
+		{
+			var responseBytes = Encoding.UTF8.GetBytes(StubResponse.NginxHtml401Response);
+
+			var client = FixedResponseClient.Create(responseBytes, 401,
+				modifySettings: s => s.DisableDirectStreaming().SkipDeserializationForStatusCodes(401),
+				contentType: "text/html",
+				exception: new Exception("problem with the request as a result of 401")
+			);
+
+			var bytesResponse = client.LowLevel.Search<BytesResponse>("project", "project", PostData.Serializable(new { }));
+
+			bytesResponse.Body.Should().NotBeNull();
+			bytesResponse.Body.Should().BeEquivalentTo(responseBytes);
+		}
+	}
+}

src/Tests/Tests/Framework/SerializationTests/BytesResponseTests.cs

@@ -0,0 +1,42 @@
+using System;
+using System.Text;
+using Elastic.Xunit.XunitPlumbing;
+using Elasticsearch.Net;
+using FluentAssertions;
+using Tests.Core.Client;
+
+namespace Tests.Framework.SerializationTests
+{
+	public class StringResponseTests
+	{
+		[U] public void TryGetServerErrorDoesNotThrowException()
+		{
+			var client = FixedResponseClient.Create(StubResponse.NginxHtml401Response, 401,
+				modifySettings: s => s.DisableDirectStreaming(),
+				contentType: "text/html",
+				exception: new Exception("problem with the request as a result of 401")
+			);
+
+			var stringResponse = client.LowLevel.Search<StringResponse>("project", "project", PostData.Serializable(new { }));
+
+			stringResponse.Body.Should().NotBeNull();
+			stringResponse.Body.Should().Be(StubResponse.NginxHtml401Response);
+			stringResponse.TryGetServerError(out var serverError).Should().BeFalse();
+			serverError.Should().BeNull();
+		}
+
+		[U] public void SkipDeserializationForStatusCodesSetsBody()
+		{
+			var client = FixedResponseClient.Create(StubResponse.NginxHtml401Response, 401,
+				modifySettings: s => s.DisableDirectStreaming().SkipDeserializationForStatusCodes(401),
+				contentType: "text/html",
+				exception: new Exception("problem with the request as a result of 401")
+			);
+
+			var stringResponse = client.LowLevel.Search<StringResponse>("project", "project", PostData.Serializable(new { }));
+
+			stringResponse.Body.Should().NotBeNull();
+			stringResponse.Body.Should().Be(StubResponse.NginxHtml401Response);
+		}
+	}
+}

src/Tests/Tests/Framework/SerializationTests/StringResponseTests.cs

@@ -0,0 +1,19 @@
+namespace Tests.Framework.SerializationTests
+{
+	public class StubResponse
+	{
+		public const string NginxHtml401Response = @"<html>
+<head><title>401 Authorization Required</title></head>
+<body bgcolor=""white"">
+<center><h1>401 Authorization Required</h1></center>
+<hr><center>nginx/1.4.6 (Ubuntu)</center>
+</body>
+</html>
+<!-- a padding to disable MSIE and Chrome friendly error page -->
+<!-- a padding to disable MSIE and Chrome friendly error page -->
+<!-- a padding to disable MSIE and Chrome friendly error page -->
+<!-- a padding to disable MSIE and Chrome friendly error page -->
+<!-- a padding to disable MSIE and Chrome friendly error page -->
+<!-- a padding to disable MSIE and Chrome friendly error page -->";
+	}
+}