Round sub-milliseconds up to the next milliseconds on transaction tim…

…eout (#720) * Round sub-milliseconds up to the next milliseconds on transaction timeout. * Minor tidying * Fixup * Fix failing tests * Added non-integer milliseconds to tests * blacklist test that is failing in .NET * xml docs * Fix mutating default Tx Config --------- Co-authored-by: grant lodge <[email protected]>
neo4j · Jul 13, 2023 · fecc2f0 · fecc2f0
1 parent 1a5e46d
commit fecc2f0
Show file tree

Hide file tree

Showing 7 changed files with 124 additions and 55 deletions.
diff --git a/Neo4j.Driver/Neo4j.Driver.Tests.TestBackend/TestBlackList.cs b/Neo4j.Driver/Neo4j.Driver.Tests.TestBackend/TestBlackList.cs
@@ -88,6 +88,8 @@ private static readonly (string Name, string Reason)[] BlackListNames =
             "driver does not report errors on RUN before consuming results"),
         ("tests.stub.tx_run.test_tx_run.TestTxRun.test_should_prevent_pull_after_tx_termination_on_run",
             "driver does not report errors on RUN before consuming results"),
+        ("stub.tx_run.test_tx_run.TestTxRun.test_should_prevent_commit_after_tx_termination",
+            "driver does not report errors on RUN before consuming results"),
 
         //TODO:
         ("stub.tx_run.test_tx_run.TestTxRun.test_should_prevent_pull_after_tx_termination_on_pull",

diff --git a/Neo4j.Driver/Neo4j.Driver.Tests/Internal/Util/ConfigBuildersTests.cs b/Neo4j.Driver/Neo4j.Driver.Tests/Internal/Util/ConfigBuildersTests.cs
@@ -23,30 +23,6 @@ namespace Neo4j.Driver.Internal.Util
 {
     public class ConfigBuildersTests
     {
-        public class BuildTransactionOptions
-        {
-            [Fact]
-            public void ShouldReturnEmptyTxOptionsWhenBuilderIsNull()
-            {
-                var options = ConfigBuilders.BuildTransactionConfig(null);
-                options.Should().Be(TransactionConfig.Default);
-            }
-
-            [Fact]
-            public void ShouldReturnNewTxOptions()
-            {
-                var options1 = ConfigBuilders.BuildTransactionConfig(o => o.WithTimeout(TimeSpan.FromSeconds(5)));
-                var options2 = ConfigBuilders.BuildTransactionConfig(o => o.WithTimeout(TimeSpan.FromSeconds(30)));
-                options1.Timeout.Should().Be(TimeSpan.FromSeconds(5));
-                options2.Timeout.Should().Be(TimeSpan.FromSeconds(30));
-
-                // When I reset to another value
-                options1.Timeout = TimeSpan.FromMinutes(1);
-                options1.Timeout.Should().Be(TimeSpan.FromMinutes(1));
-                options2.Timeout.Should().Be(TimeSpan.FromSeconds(30));
-            }
-        }
-
         public class BuildSessionOptions
         {
             [Fact]

diff --git a/Neo4j.Driver/Neo4j.Driver.Tests/Internal/Util/TransactionTimeoutTests.cs b/Neo4j.Driver/Neo4j.Driver.Tests/Internal/Util/TransactionTimeoutTests.cs
@@ -0,0 +1,72 @@
+// Copyright (c) "Neo4j"
+// Neo4j Sweden AB [http://neo4j.com]
+//
+// This file is part of Neo4j.
+//
+// Licensed under the Apache License, Version 2.0 (the "License").
+// You may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+using System;
+using FluentAssertions;
+using Moq;
+using Moq.AutoMock;
+using Xunit;
+
+namespace Neo4j.Driver.Internal.Util
+{
+    public class TransactionTimeoutTests
+    {
+        [Theory]
+        [InlineData(0, 0, 0)]
+        [InlineData(0, 1, 1)]
+        [InlineData(0.1, 0, 1)]
+        [InlineData(1, 0, 1)]
+        [InlineData(1, 1, 2)]
+        [InlineData(1.23, 0, 2)]
+        [InlineData(598, 1, 599)]
+        [InlineData(598.2, 0, 599)]
+        [InlineData(2000, 96, 2001)]
+        [InlineData(2000.8, 0, 2001)]
+        public void ShouldRoundSubmillisecondTimeoutsToMilliseconds(
+            double milliseconds,
+            int ticks,
+            int expectedMilliseconds)
+        {
+            var inputMilliseconds = TimeSpan.FromMilliseconds(milliseconds);
+            var inputTicks = TimeSpan.FromTicks(ticks);
+            var totalInput = inputMilliseconds + inputTicks;
+            var expectedTimeout = TimeSpan.FromMilliseconds(expectedMilliseconds);
+
+            var autoMocker = new AutoMocker(MockBehavior.Strict);
+            if (expectedTimeout != inputMilliseconds)
+            {
+                // only logs if it changes the timeout
+                autoMocker.GetMock<ILogger>()
+                    .Setup(
+                        x => x.Info(
+                            It.Is<string>(s => s.Contains("rounded up")),
+                            It.IsAny<object[]>()))
+                    .Verifiable();
+            }
+
+            var sut =
+                new TransactionConfigBuilder(autoMocker.GetMock<ILogger>().Object, TransactionConfig.Default)
+                .WithTimeout(totalInput);
+
+            var result = sut.Build().Timeout;
+
+            result.Should().Be(expectedTimeout);
+            (result?.Ticks % TimeSpan.TicksPerMillisecond).Should().Be(0);
+            autoMocker.VerifyAll();
+        }
+    }
+}
diff --git a/Neo4j.Driver/Neo4j.Driver.Tests/TransactionConfigTests.cs b/Neo4j.Driver/Neo4j.Driver.Tests/TransactionConfigTests.cs
@@ -36,7 +36,7 @@ public class TimeoutField
             {
                 new object[] { null },
                 new object[] { (TimeSpan?)TimeSpan.Zero },
-                new object[] { (TimeSpan?)TimeSpan.FromMilliseconds(0.1) },
+                new object[] { (TimeSpan?)TimeSpan.FromMilliseconds(1) },
                 new object[] { (TimeSpan?)TimeSpan.FromMinutes(30) },
                 new object[] { (TimeSpan?)TimeSpan.MaxValue }
             };
@@ -53,7 +53,7 @@ public void ShouldReturnDefaultValueAsNull()
             [MemberData(nameof(ValidTimeSpanValues))]
             public void ShouldAllowToSetToNewValue(TimeSpan? input)
             {
-                var builder = TransactionConfig.Builder;
+                var builder = new TransactionConfigBuilder(null, TransactionConfig.Default);
                 builder.WithTimeout(input);
 
                 var config = builder.Build();
@@ -65,7 +65,7 @@ public void ShouldAllowToSetToNewValue(TimeSpan? input)
             [MemberData(nameof(InvalidTimeSpanValues))]
             public void ShouldThrowExceptionIfAssigningValueLessThanZero(TimeSpan input)
             {
-                var error = Record.Exception(() => TransactionConfig.Builder.WithTimeout(input));
+                var error = Record.Exception(() => new TransactionConfigBuilder(null, TransactionConfig.Default).WithTimeout(input));
 
                 error.Should().BeOfType<ArgumentOutOfRangeException>();
                 error.Message.Should().Contain("not be negative");
@@ -85,8 +85,8 @@ public void ShouldReturnDefaultValueEmptyDictionary()
             [Fact]
             public void ShouldAllowToSetToNewValue()
             {
-                var builder = TransactionConfig.Builder;
-                builder.WithMetadata(new Dictionary<string, object> { { "key", "value" } });
+                var builder = new TransactionConfigBuilder(null, TransactionConfig.Default)
+                    .WithMetadata(new Dictionary<string, object> { { "key", "value" } });
 
                 var config = builder.Build();
 
@@ -98,7 +98,8 @@ public void ShouldAllowToSetToNewValue()
             [Fact]
             public void ShouldThrowExceptionIfAssigningNull()
             {
-                var error = Record.Exception(() => TransactionConfig.Builder.WithMetadata(null));
+                var error = Record.Exception(
+                    () => new TransactionConfigBuilder(null, TransactionConfig.Default).WithMetadata(null));
 
                 error.Should().BeOfType<ArgumentNullException>();
                 error.Message.Should().Contain("should not be null");

diff --git a/Neo4j.Driver/Neo4j.Driver/Internal/AsyncSession.cs b/Neo4j.Driver/Neo4j.Driver/Internal/AsyncSession.cs
@@ -193,6 +193,17 @@ public Task<IResultCursor> RunAsync(
         return result;
     }
 
+    private TransactionConfig BuildTransactionConfig(Action<TransactionConfigBuilder> action)
+    {
+        if (action == null)
+        {
+            return TransactionConfig.Default;
+        }
+        var builder = new TransactionConfigBuilder(_logger, new TransactionConfig());
+        action.Invoke(builder);
+        return builder.Build();
+    }
+
     public Task<T> ReadTransactionAsync<T>(
         Func<IAsyncTransaction, Task<T>> work,
         Action<TransactionConfigBuilder> action = null)

diff --git a/Neo4j.Driver/Neo4j.Driver/Internal/Util/ConfigBuilders.cs b/Neo4j.Driver/Neo4j.Driver/Internal/Util/ConfigBuilders.cs
@@ -21,23 +21,6 @@ namespace Neo4j.Driver.Internal.Util;
 
 internal static class ConfigBuilders
 {
-    public static TransactionConfig BuildTransactionConfig(Action<TransactionConfigBuilder> action)
-    {
-        TransactionConfig config;
-        if (action == null)
-        {
-            config = TransactionConfig.Default;
-        }
-        else
-        {
-            var builder = TransactionConfig.Builder;
-            action.Invoke(builder);
-            config = builder.Build();
-        }
-
-        return config;
-    }
-
     public static SessionConfig BuildSessionConfig(Action<SessionConfigBuilder> action)
     {
         SessionConfig config;

diff --git a/Neo4j.Driver/Neo4j.Driver/TransactionConfig.cs b/Neo4j.Driver/Neo4j.Driver/TransactionConfig.cs
@@ -49,8 +49,6 @@ internal TransactionConfig(IDictionary<string, object> metadata, TimeSpan? timeo
         _timeout = timeout;
     }
 
-    internal static TransactionConfigBuilder Builder => new(new TransactionConfig());
-
     /// <summary>
     /// Transaction timeout. Transactions that execute longer than the configured timeout will be terminated by the
     /// database. This functionality allows to limit query/transaction execution time. Specified timeout overrides the default
@@ -103,10 +101,14 @@ public override string ToString()
 /// <summary>The builder to create a <see cref="TransactionConfig"/></summary>
 public sealed class TransactionConfigBuilder
 {
+    private readonly ILogger _logger;
     private readonly TransactionConfig _config;
 
-    internal TransactionConfigBuilder(TransactionConfig config)
+    internal TransactionConfigBuilder(
+        ILogger logger,
+        TransactionConfig config)
     {
+        _logger = logger;
         _config = config;
     }
 
@@ -115,19 +117,41 @@ internal TransactionConfigBuilder(TransactionConfig config)
     /// by the database. This functionality allows to limit query/transaction execution time. Specified timeout overrides the
     /// default timeout configured in the database using <code>dbms.transaction.timeout</code> setting. Leave this field
     /// unmodified to use default timeout configured on database. Setting a zero timeout will result in no timeout.
+    /// <para/>
+    /// If the timeout is not an exact number of milliseconds, it will be rounded up to the next millisecond.
     /// </summary>
     /// <exception cref="ArgumentOutOfRangeException">
-    /// If the value given to transaction timeout in milliseconds is less than
-    /// zero
+    /// If the value given to transaction timeout in milliseconds is less than zero.
     /// </exception>
-    /// <param name="timeout">the new timeout</param>
-    /// <returns>this <see cref="TransactionConfigBuilder"/> instance</returns>
+    /// <param name="timeout">The new timeout.</param>
+    /// <returns>this <see cref="TransactionConfigBuilder"/> instance.</returns>
     public TransactionConfigBuilder WithTimeout(TimeSpan? timeout)
     {
-        _config.Timeout = timeout;
+        _config.Timeout = FixSubmilliseconds(timeout);
         return this;
     }
 
+    private TimeSpan? FixSubmilliseconds(TimeSpan? timeout)
+    {
+        if(timeout == null || timeout == TimeSpan.MaxValue)
+        {
+            return timeout;
+        }
+
+        if (timeout.Value.Ticks % TimeSpan.TicksPerMillisecond == 0)
+        {
+            return timeout;
+        }
+
+        var result = TimeSpan.FromMilliseconds(Math.Ceiling(timeout.Value.TotalMilliseconds));
+        _logger?.Info(
+            "Transaction timeout {timeout} contains sub-millisecond precision and will be rounded up to {result}.",
+            timeout,
+            result);
+
+        return result;
+    }
+
     /// <summary>
     /// The transaction metadata. Specified metadata will be attached to the executing transaction and visible in the
     /// output of <code>dbms.listQueries</code> and <code>dbms.listTransactions</code> procedures. It will also get logged to