Change HttpClientBase.ExecutePost to match upstream better and drop Newtonsoft.Json dependency (#1212)

* Change HttpClientBase.ExecutePost to match upstream better and drop Newtonsoft.Json dependency

* Code cleanup of HttpClientBase and HttpReplicator, remove NewtonsoftJson dependency version

---------

Co-authored-by: Paul Irwin <paulirwin@gmail.com>

Author: AnastasiaJacson

.build/dependencies.props

@@ -65,7 +65,6 @@
     <MorfologikPolishPackageVersion>$(MorfologikFsaPackageVersion)</MorfologikPolishPackageVersion>
     <MorfologikStemmingPackageVersion>$(MorfologikFsaPackageVersion)</MorfologikStemmingPackageVersion>
     <NETStandardLibrary20PackageVersion>2.0.3</NETStandardLibrary20PackageVersion>
-    <NewtonsoftJsonPackageVersion>13.0.1</NewtonsoftJsonPackageVersion>
     <NUnit3TestAdapterPackageVersion>4.6.0</NUnit3TestAdapterPackageVersion>
     <NUnitPackageVersion>3.14.0</NUnitPackageVersion>
     <RandomizedTestingGeneratorsPackageVersion>2.7.8</RandomizedTestingGeneratorsPackageVersion>

src/Lucene.Net.Replicator/Http/HttpClientBase.cs

@@ -1,15 +1,13 @@
 using Lucene.Net.Diagnostics;
 using Lucene.Net.Support;
-using Newtonsoft.Json;
-using Newtonsoft.Json.Linq;
 using System;
 using System.IO;
 using System.Linq;
 using System.Net;
 using System.Net.Http;
-using System.Text;
 using System.Threading;
 using System.Threading.Tasks;
+#nullable enable
 
 namespace Lucene.Net.Replicator.Http
 {
@@ -47,7 +45,7 @@ public abstract class HttpClientBase : IDisposable
         /// Default request timeout for this client (100 seconds).
         /// <see cref="Timeout"/>.
         /// </summary>
-        public readonly static TimeSpan DEFAULT_TIMEOUT = TimeSpan.FromSeconds(100); // LUCENENET: This was DEFAULT_SO_TIMEOUT in Lucene, using .NET's default timeout value of 100 instead of 61 seconds
+        public static readonly TimeSpan DEFAULT_TIMEOUT = TimeSpan.FromSeconds(100); // LUCENENET: This was DEFAULT_SO_TIMEOUT in Lucene, using .NET's default timeout value of 100 instead of 61 seconds
 
         // TODO compression?
 
@@ -56,7 +54,8 @@ public abstract class HttpClientBase : IDisposable
         /// </summary>
         protected string Url { get; private set; }
 
-        private volatile bool isDisposed = false;
+        private volatile bool isDisposed;
+
         private readonly HttpClient httpc;
 
         /// <summary>
@@ -73,7 +72,7 @@ public abstract class HttpClientBase : IDisposable
         /// <param name="port">The port to be used to connect on.</param>
         /// <param name="path">The path to the replicator on the host.</param>
         /// <param name="messageHandler">Optional, The HTTP handler stack to use for sending requests, defaults to <c>null</c>.</param>
-        protected HttpClientBase(string host, int port, string path, HttpMessageHandler messageHandler = null)
+        protected HttpClientBase(string host, int port, string path, HttpMessageHandler? messageHandler = null)
             : this(NormalizedUrl(host, port, path), messageHandler)
         {
         }
@@ -88,7 +87,7 @@ protected HttpClientBase(string host, int port, string path, HttpMessageHandler
         /// <param name="url">The full url, including with host, port and path.</param>
         /// <param name="messageHandler">Optional, The HTTP handler stack to use for sending requests.</param>
         //Note: LUCENENET Specific
-        protected HttpClientBase(string url, HttpMessageHandler messageHandler = null)
+        protected HttpClientBase(string url, HttpMessageHandler? messageHandler = null)
             : this(url, new HttpClient(messageHandler ?? new HttpClientHandler()) { Timeout = DEFAULT_TIMEOUT })
         {
         }
@@ -97,7 +96,7 @@ protected HttpClientBase(string url, HttpMessageHandler messageHandler = null)
         /// Creates a new <see cref="HttpClientBase"/> with the given <paramref name="url"/> and <see cref="HttpClient"/>.
         /// </summary>
         /// <remarks>
-        /// This allows full controll over how the <see cref="HttpClient"/> is created,
+        /// This allows full control over how the <see cref="HttpClient"/> is created,
         /// prefer the <see cref="HttpClientBase(string, HttpMessageHandler)"/> over this unless you know you need the control of the <see cref="HttpClient"/>.
         /// </remarks>
         /// <param name="url"></param>
@@ -138,8 +137,11 @@ protected void EnsureOpen()
         private static string NormalizedUrl(string host, int port, string path)
         {
             if (string.IsNullOrEmpty(path))
+            {
                 path = "/";
-            return string.Format("http://{0}:{1}{2}", host, port, path);
+            }
+
+            return $"http://{host}:{port}{path}";
         }
 
         /// <summary>
@@ -174,15 +176,13 @@ protected virtual void ThrowKnownError(HttpResponseMessage response)
         /// <b>Internal:</b> Execute a request and return its result.
         /// The <paramref name="parameters"/> argument is treated as: name1,value1,name2,value2,...
         /// </summary>
-        protected virtual HttpResponseMessage ExecutePost(string request, object entity, params string[] parameters)
+        protected virtual HttpResponseMessage ExecutePost(string request, HttpContent content, params string[]? parameters)
         {
             EnsureOpen();
 
-            //.NET Note: No headers? No ContentType?... Bad use of Http?
-            HttpRequestMessage req = new HttpRequestMessage(HttpMethod.Post, QueryString(request, parameters));
+            var req = new HttpRequestMessage(HttpMethod.Post, QueryString(request, parameters));
 
-            req.Content = new StringContent(JToken.FromObject(entity, JsonSerializer.Create())
-                .ToString(Formatting.None), Encoding.UTF8, "application/json");
+            req.Content = content;
 
             return Execute(req);
         }
@@ -191,30 +191,29 @@ protected virtual HttpResponseMessage ExecutePost(string request, object entity,
         /// <b>Internal:</b> Execute a request and return its result.
         /// The <paramref name="parameters"/> argument is treated as: name1,value1,name2,value2,...
         /// </summary>
-        protected virtual HttpResponseMessage ExecuteGet(string request, params string[] parameters)
+        protected virtual HttpResponseMessage ExecuteGet(string request, params string[]? parameters)
         {
             EnsureOpen();
 
-            //Note: No headers? No ContentType?... Bad use of Http?
-            HttpRequestMessage req = new HttpRequestMessage(HttpMethod.Get, QueryString(request, parameters));
+            var req = new HttpRequestMessage(HttpMethod.Get, QueryString(request, parameters));
+
             return Execute(req);
         }
 
         private HttpResponseMessage Execute(HttpRequestMessage request)
         {
             //.NET Note: Bridging from Async to Sync, this is not ideal and we could consider changing the interface to be Async or provide Async overloads
             //      and have these Sync methods with their caveats.
-            HttpResponseMessage response = httpc.SendAsync(request, HttpCompletionOption.ResponseHeadersRead).ConfigureAwait(false).GetAwaiter().GetResult();
+            var response = httpc.SendAsync(request, HttpCompletionOption.ResponseHeadersRead).ConfigureAwait(false).GetAwaiter().GetResult();
             VerifyStatus(response);
             return response;
         }
 
-        private string QueryString(string request, params string[] parameters)
+        private string QueryString(string request, params string[]? parameters)
         {
-            return parameters is null
-                ? string.Format("{0}/{1}", Url, request)
-                : string.Format("{0}/{1}?{2}", Url, request, string
-                .Join("&", parameters.Select(WebUtility.UrlEncode).InPairs((key, val) => string.Format("{0}={1}", key, val))));
+            return parameters is null || parameters.Length == 0
+                ? $"{Url}/{request}"
+                : $"{Url}/{request}?{string.Join("&", parameters.Select(WebUtility.UrlEncode).InPairs((key, val) => $"{key}={val}"))}";
         }
 
         /// <summary>
@@ -257,8 +256,12 @@ public virtual Stream GetResponseStream(HttpResponseMessage response) // LUCENEN
         public virtual Stream GetResponseStream(HttpResponseMessage response, bool consume) // LUCENENET: This was ResponseInputStream in Lucene
         {
             Stream result = response.Content.ReadAsStreamAsync().ConfigureAwait(false).GetAwaiter().GetResult();
+
             if (consume)
+            {
                 result = new ConsumingStream(result);
+            }
+
             return result;
         }
 
@@ -284,7 +287,7 @@ protected virtual T DoAction<T>(HttpResponseMessage response, Func<T> call)
         /// </summary>
         protected virtual T DoAction<T>(HttpResponseMessage response, bool consume, Func<T> call)
         {
-            Exception th = null;
+            Exception? th /* = null */;
             try
             {
                 return call();
@@ -303,20 +306,13 @@ protected virtual T DoAction<T>(HttpResponseMessage response, bool consume, Func
                 {
                     if (consume)
                     {
-                        try
-                        {
-                            ConsumeQuietly(response);
-                        }
-                        finally
-                        {
-                            // ignoring on purpose
-                        }
+                        ConsumeQuietly(response);
                     }
                 }
             }
             if (Debugging.AssertsEnabled) Debugging.Assert(th != null); // extra safety - if we get here, it means the Func<T> failed
             Util.IOUtils.ReThrow(th);
-            return default; // silly, if we're here, IOUtils.reThrow always throws an exception
+            return default!; // silly, if we're here, IOUtils.reThrow always throws an exception
         }
 
         /// <summary>
@@ -346,7 +342,7 @@ private static void ConsumeQuietly(HttpResponseMessage response)
         {
             try
             {
-                response.Content?.Dispose(); // LUCENENET: Force a flush and and dispose the underlying stream
+                response.Content?.Dispose(); // LUCENENET: Force a flush and dispose the underlying stream
             }
             catch (Exception ioe) when (ioe.IsIOException())
             {
@@ -418,8 +414,11 @@ public override int EndRead(IAsyncResult asyncResult)
                 Consume(res);
                 return res;
             }
+
             public override long Seek(long offset, SeekOrigin origin) => input.Seek(offset, origin);
+
             public override void SetLength(long value) => input.SetLength(value);
+
             public override void Write(byte[] buffer, int offset, int count) => input.Write(buffer, offset, count);
 
             private void Consume(int zeroOrMinusOne)

src/Lucene.Net.Replicator/Http/HttpReplicator.cs

@@ -2,6 +2,7 @@
 using System;
 using System.IO;
 using System.Net.Http;
+#nullable enable
 
 namespace Lucene.Net.Replicator.Http
 {
@@ -34,7 +35,7 @@ public class HttpReplicator : HttpClientBase, IReplicator
         /// Creates a new <see cref="HttpReplicator"/> with the given host, port and path.
         /// <see cref="HttpClientBase(string, int, string, HttpMessageHandler)"/> for more details.
         /// </summary>
-        public HttpReplicator(string host, int port, string path, HttpMessageHandler messageHandler = null)
+        public HttpReplicator(string host, int port, string path, HttpMessageHandler? messageHandler = null)
             : base(host, port, path, messageHandler)
         {
         }
@@ -44,7 +45,7 @@ public HttpReplicator(string host, int port, string path, HttpMessageHandler mes
         /// <see cref="HttpClientBase(string, HttpMessageHandler)"/> for more details.
         /// </summary>
         //Note: LUCENENET Specific
-        public HttpReplicator(string url, HttpMessageHandler messageHandler = null)
+        public HttpReplicator(string url, HttpMessageHandler? messageHandler = null)
             : this(url, new HttpClient(messageHandler ?? new HttpClientHandler()) { Timeout = DEFAULT_TIMEOUT })
         {
         }
@@ -62,26 +63,28 @@ public HttpReplicator(string url, HttpClient client)
         /// <summary>
         /// Checks for updates at the remote host.
         /// </summary>
-        public virtual SessionToken CheckForUpdate(string currentVersion)
+        public virtual SessionToken? CheckForUpdate(string? currentVersion)
         {
-            string[] parameters = null;
+            string[]? parameters = null;
             if (currentVersion != null)
+            {
                 parameters = new[] { ReplicationService.REPLICATE_VERSION_PARAM, currentVersion };
+            }
 
-            HttpResponseMessage response = base.ExecuteGet(ReplicationService.ReplicationAction.UPDATE.ToString(), parameters);
+            var response = base.ExecuteGet(nameof(ReplicationService.ReplicationAction.UPDATE), parameters);
             return DoAction(response, () =>
             {
-                using DataInputStream inputStream = new DataInputStream(GetResponseStream(response));
+                using var inputStream = new DataInputStream(GetResponseStream(response));
                 return inputStream.ReadByte() == 0 ? null : new SessionToken(inputStream);
             });
         }
 
         /// <summary>
-        /// Obtains the given file from it's source at the remote host.
+        /// Obtains the given file from its source at the remote host.
         /// </summary>
         public virtual Stream ObtainFile(string sessionId, string source, string fileName)
         {
-            HttpResponseMessage response = ExecuteGet(ReplicationService.ReplicationAction.OBTAIN.ToString(),
+            var response = ExecuteGet(nameof(ReplicationService.ReplicationAction.OBTAIN),
                 ReplicationService.REPLICATE_SESSION_ID_PARAM, sessionId,
                 ReplicationService.REPLICATE_SOURCE_PARAM, source,
                 ReplicationService.REPLICATE_FILENAME_PARAM, fileName);
@@ -102,9 +105,9 @@ public virtual void Publish(IRevision revision)
         /// </summary>
         public virtual void Release(string sessionId)
         {
-            HttpResponseMessage response = ExecuteGet(ReplicationService.ReplicationAction.RELEASE.ToString(), ReplicationService.REPLICATE_SESSION_ID_PARAM, sessionId);
+            var response = ExecuteGet(nameof(ReplicationService.ReplicationAction.RELEASE), ReplicationService.REPLICATE_SESSION_ID_PARAM, sessionId);
             // do not remove this call: as it is still validating for us!
-            DoAction<object>(response, () => null);
+            DoAction<object?>(response, () => null);
         }
     }
 }

src/Lucene.Net.Replicator/Http/ReplicationService.cs

@@ -2,7 +2,6 @@
 using J2N.Text;
 using Lucene.Net.Diagnostics;
 using Lucene.Net.Replicator.Http.Abstractions;
-using Newtonsoft.Json;
 using System;
 using System.Collections.Generic;
 using System.IO;

src/Lucene.Net.Replicator/Lucene.Net.Replicator.csproj

@@ -47,7 +47,6 @@
 
   <ItemGroup>
     <PackageReference Include="J2N" Version="$(J2NPackageVersion)" />
-    <PackageReference Include="Newtonsoft.Json" Version="$(NewtonsoftJsonPackageVersion)" />
   </ItemGroup>
 
   <ItemGroup Condition=" '$(TargetFramework)' == 'net462' ">

src/Lucene.Net.Tests.Replicator/Lucene.Net.Tests.Replicator.csproj

@@ -63,10 +63,6 @@
 
   <Import Project="$(SolutionDir).build/TestReferences.Common.targets" />
 
-  <ItemGroup>
-    <PackageReference Include="Newtonsoft.Json" Version="$(NewtonsoftJsonPackageVersion)" />
-  </ItemGroup>
-
   <ItemGroup Condition="$(TargetFramework.StartsWith('net8.')) Or $(TargetFramework.StartsWith('net9.'))">
     <PackageReference Include="Microsoft.AspNetCore.TestHost" Version="$(MicrosoftAspNetCoreTestHostPackageVersion)" />
   </ItemGroup>