Assign the order number under a unique index instead of a read-max race (#778)

Author: KrzysztofPajak

src/Business/Grand.Business.Checkout/Services/Orders/OrderService.cs

@@ -8,6 +8,7 @@
 using Grand.Domain.Shipping;
 using Grand.Infrastructure.Extensions;
 using Grand.Mediator;
+using Grand.SharedKernel;
 
 namespace Grand.Business.Checkout.Services.Orders;
 
@@ -38,6 +39,11 @@ IMediator mediator
 
     #region Fields
 
+    /// <summary>
+    ///     How many times an order insert is retried when another checkout took the same order number
+    /// </summary>
+    private const int OrderNumberAttempts = 5;
+
     private readonly IRepository<Order> _orderRepository;
     private readonly IRepository<OrderNote> _orderNoteRepository;
     private readonly IMediator _mediator;
@@ -210,11 +216,23 @@ public virtual async Task InsertOrder(Order order)
     {
         ArgumentNullException.ThrowIfNull(order);
 
-        var orderExists = _orderRepository.Table.OrderByDescending(x => x.OrderNumber).Select(x => x.OrderNumber)
-            .FirstOrDefault();
-        order.OrderNumber = orderExists != 0 ? orderExists + 1 : 1;
-
-        await _orderRepository.InsertAsync(order);
+        //the order number is taken from the collection itself, so two concurrent checkouts can read the same
+        //value - the unique index on OrderNumber rejects the loser and the number is read again
+        for (var attempt = 1;; attempt++)
+        {
+            var lastOrderNumber = await _orderRepository.FirstOrDefaultAsync(
+                _orderRepository.Table.OrderByDescending(x => x.OrderNumber).Select(x => x.OrderNumber));
+            order.OrderNumber = lastOrderNumber + 1;
+
+            try
+            {
+                await _orderRepository.InsertAsync(order);
+                break;
+            }
+            catch (DuplicateKeyGrandException) when (attempt < OrderNumberAttempts)
+            {
+            }
+        }
 
         //event notification
         await _mediator.EntityInserted(order);

src/Core/Grand.Data/LiteDb/LiteDBRepository.cs

@@ -1,4 +1,5 @@
 using Grand.Domain;
+using Grand.SharedKernel;
 using Grand.SharedKernel.Attributes;
 using LiteDB;
 using System.Linq.Expressions;
@@ -107,7 +108,16 @@ public virtual T Insert(T entity)
     {
         entity.CreatedOnUtc = _auditInfoProvider.GetCurrentDateTime();
         entity.CreatedBy = _auditInfoProvider.GetCurrentUser();
-        Collection.Insert(entity);
+        try
+        {
+            Collection.Insert(entity);
+        }
+        catch (LiteException ex) when (ex.ErrorCode == LiteException.INDEX_DUPLICATE_KEY)
+        {
+            throw new DuplicateKeyGrandException(
+                $"Insert into {typeof(T).Name} rejected - it violates a unique index", ex);
+        }
+
         return entity;
     }
 

src/Core/Grand.Data/Mongo/MongoRepository.cs

@@ -1,4 +1,5 @@
 using Grand.Domain;
+using Grand.SharedKernel;
 using MongoDB.Driver;
 using MongoDB.Driver.Linq;
 using System.Linq.Expressions;
@@ -98,7 +99,15 @@ public virtual T Insert(T entity)
     {
         entity.CreatedOnUtc = _auditInfoProvider.GetCurrentDateTime();
         entity.CreatedBy = _auditInfoProvider.GetCurrentUser();
-        Collection.InsertOne(entity);
+        try
+        {
+            Collection.InsertOne(entity);
+        }
+        catch (MongoWriteException ex) when (ex.WriteError?.Category == ServerErrorCategory.DuplicateKey)
+        {
+            throw DuplicateKey(ex);
+        }
+
         return entity;
     }
 
@@ -110,10 +119,28 @@ public virtual async Task<T> InsertAsync(T entity)
     {
         entity.CreatedOnUtc = _auditInfoProvider.GetCurrentDateTime();
         entity.CreatedBy = _auditInfoProvider.GetCurrentUser();
-        await Collection.InsertOneAsync(entity);
+        try
+        {
+            await Collection.InsertOneAsync(entity);
+        }
+        catch (MongoWriteException ex) when (ex.WriteError?.Category == ServerErrorCategory.DuplicateKey)
+        {
+            throw DuplicateKey(ex);
+        }
+
         return entity;
     }
 
+    /// <summary>
+    ///     Translates a driver duplicate key error into a store independent exception
+    /// </summary>
+    /// <param name="exception">Driver exception</param>
+    private static DuplicateKeyGrandException DuplicateKey(MongoWriteException exception)
+    {
+        return new DuplicateKeyGrandException(
+            $"Insert into {typeof(T).Name} rejected - it violates a unique index", exception);
+    }
+
     /// <summary>
     ///     Update entity
     /// </summary>

src/Core/Grand.SharedKernel/DuplicateKeyGrandException.cs

@@ -0,0 +1,39 @@
+namespace Grand.SharedKernel;
+
+/// <summary>
+///     Represents a write rejected by the store because it violates a unique index
+/// </summary>
+/// <remarks>
+///     Thrown by the repository implementations so that callers can react to a unique key collision
+///     without referencing a database driver.
+/// </remarks>
+[Serializable]
+public class DuplicateKeyGrandException : GrandException
+{
+    /// <summary>
+    ///     Initializes a new instance of the DuplicateKeyGrandException class.
+    /// </summary>
+    public DuplicateKeyGrandException()
+    {
+    }
+
+    /// <summary>
+    ///     Initializes a new instance of the DuplicateKeyGrandException class with a specified error message.
+    /// </summary>
+    /// <param name="message">The message that describes the error.</param>
+    public DuplicateKeyGrandException(string message)
+        : base(message)
+    {
+    }
+
+    /// <summary>
+    ///     Initializes a new instance of the DuplicateKeyGrandException class with a specified error message
+    ///     and the driver exception that caused it.
+    /// </summary>
+    /// <param name="message">The message that describes the error.</param>
+    /// <param name="innerException">The exception that is the cause of the current exception.</param>
+    public DuplicateKeyGrandException(string message, Exception innerException)
+        : base(message, innerException)
+    {
+    }
+}

src/Core/Grand.SharedKernel/GrandException.cs

@@ -21,4 +21,15 @@ public GrandException(string message)
         : base(message)
     {
     }
+
+    /// <summary>
+    ///     Initializes a new instance of the Exception class with a specified error message and the exception
+    ///     that caused it.
+    /// </summary>
+    /// <param name="message">The message that describes the error.</param>
+    /// <param name="innerException">The exception that is the cause of the current exception.</param>
+    public GrandException(string message, Exception innerException)
+        : base(message, innerException)
+    {
+    }
 }

src/Modules/Grand.Module.Installer/Services/InstallationService.cs

@@ -865,8 +865,9 @@ await dbContext.CreateIndex(_orderRepository,
             "CustomerId_1_CreatedOnUtc_-1");
         await dbContext.CreateIndex(_orderRepository, OrderBuilder<Order>.Create().Descending(x => x.CreatedOnUtc),
             "CreatedOnUtc_-1");
+        //unique - the order number is assigned from the collection itself, the index is what makes it atomic
         await dbContext.CreateIndex(_orderRepository, OrderBuilder<Order>.Create().Descending(x => x.OrderNumber),
-            "OrderNumber");
+            "OrderNumber", true);
         await dbContext.CreateIndex(_orderRepository, OrderBuilder<Order>.Create().Ascending(x => x.Code), "OrderCode");
         await dbContext.CreateIndex(_orderRepository, OrderBuilder<Order>.Create().Ascending("OrderItems.ProductId"),
             "OrderItemProductId");

src/Modules/Grand.Module.Migration/Migrations/2.4/MigrationUniqueOrderNumberIndex.cs

@@ -0,0 +1,60 @@
+using Grand.Data;
+using Grand.Domain.Orders;
+using Grand.Infrastructure.Migrations;
+using Microsoft.Extensions.DependencyInjection;
+using Microsoft.Extensions.Logging;
+
+namespace Grand.Module.Migration.Migrations._2._4;
+
+public class MigrationUniqueOrderNumberIndex : IMigration
+{
+    private const string IndexName = "OrderNumber";
+
+    public int Priority => 1;
+    public DbVersion Version => new(2, 4);
+    public Guid Identity => new("F4D2A7C1-5E93-4B08-9A6D-3C71E852B4F0");
+    public string Name => "Make the index on Order.OrderNumber unique";
+
+    /// <summary>
+    ///     Upgrade process
+    /// </summary>
+    /// <param name="serviceProvider"></param>
+    /// <returns></returns>
+    public bool UpgradeProcess(IServiceProvider serviceProvider)
+    {
+        var repository = serviceProvider.GetRequiredService<IRepository<Order>>();
+        var databaseContext = serviceProvider.GetRequiredService<IDatabaseContext>();
+        var logService = serviceProvider.GetRequiredService<ILogger<MigrationUniqueOrderNumberIndex>>();
+
+        try
+        {
+            var duplicates = repository.Table.Select(x => x.OrderNumber).ToList()
+                .GroupBy(x => x)
+                .Where(x => x.Count() > 1)
+                .Select(x => x.Key)
+                .ToList();
+
+            if (duplicates.Count != 0)
+            {
+                //the upgrade must not stop because of data that is already inconsistent - report and move on,
+                //the index can be created by hand once the numbers are corrected
+                logService.LogError(
+                    "The unique index on Order.OrderNumber was not created - the following order numbers are used by more than one order: {OrderNumbers}. Correct them and create the index manually",
+                    string.Join(", ", duplicates));
+                return true;
+            }
+
+            //the non-unique index of the same name is dropped first - the driver rejects a redefinition
+            databaseContext.DeleteIndex(repository, IndexName).GetAwaiter().GetResult();
+            databaseContext.CreateIndex(repository,
+                OrderBuilder<Order>.Create().Descending(x => x.OrderNumber), IndexName, true)
+                .GetAwaiter().GetResult();
+        }
+        catch (Exception ex)
+        {
+            logService.LogError(ex, "UpgradeProcess - UniqueOrderNumberIndex (2.4)");
+        }
+
+        return true;
+    }
+}

src/Tests/Grand.Business.Checkout.Tests/Services/Orders/OrderServiceTests.cs

@@ -3,6 +3,7 @@
 using Grand.Domain.Orders;
 using Grand.Infrastructure.Events;
 using Grand.Mediator;
+using Grand.SharedKernel;
 using Microsoft.VisualStudio.TestTools.UnitTesting;
 using Moq;
 
@@ -25,6 +26,76 @@ public void Init()
         _service = new OrderService(_orderRepositoryMock.Object, _orderNoteRepositoryMock.Object, _mediatorMock.Object);
     }
 
+    [TestMethod]
+    public async Task InsertOrder_AssignsNextOrderNumber()
+    {
+        _orderRepositoryMock.Setup(c => c.Table).Returns(new List<Order>().AsQueryable());
+        _orderRepositoryMock.Setup(c => c.FirstOrDefaultAsync(It.IsAny<IQueryable<int>>(), It.IsAny<CancellationToken>()))
+            .ReturnsAsync(41);
+
+        var order = new Order();
+        await _service.InsertOrder(order);
+
+        Assert.AreEqual(42, order.OrderNumber);
+        _orderRepositoryMock.Verify(c => c.InsertAsync(It.IsAny<Order>()), Times.Once);
+        _mediatorMock.Verify(c => c.Publish(It.IsAny<EntityInserted<Order>>(), default), Times.Once);
+    }
+
+    [TestMethod]
+    public async Task InsertOrder_NoOrdersYet_AssignsFirstOrderNumber()
+    {
+        _orderRepositoryMock.Setup(c => c.Table).Returns(new List<Order>().AsQueryable());
+        _orderRepositoryMock.Setup(c => c.FirstOrDefaultAsync(It.IsAny<IQueryable<int>>(), It.IsAny<CancellationToken>()))
+            .ReturnsAsync(0);
+
+        var order = new Order();
+        await _service.InsertOrder(order);
+
+        Assert.AreEqual(1, order.OrderNumber);
+    }
+
+    [TestMethod]
+    public async Task InsertOrder_OrderNumberTakenByAnotherCheckout_ReadsItAgainAndRetries()
+    {
+        _orderRepositoryMock.Setup(c => c.Table).Returns(new List<Order>().AsQueryable());
+        _orderRepositoryMock.SetupSequence(c => c.FirstOrDefaultAsync(It.IsAny<IQueryable<int>>(), It.IsAny<CancellationToken>()))
+            .ReturnsAsync(41)
+            .ReturnsAsync(42);
+
+        var order = new Order();
+        _orderRepositoryMock.SetupSequence(c => c.InsertAsync(It.IsAny<Order>()))
+            .Throws(new DuplicateKeyGrandException())
+            .ReturnsAsync(order);
+
+        await _service.InsertOrder(order);
+
+        Assert.AreEqual(43, order.OrderNumber);
+        _orderRepositoryMock.Verify(c => c.InsertAsync(It.IsAny<Order>()), Times.Exactly(2));
+        _mediatorMock.Verify(c => c.Publish(It.IsAny<EntityInserted<Order>>(), default), Times.Once);
+    }
+
+    [TestMethod]
+    public async Task InsertOrder_OrderNumberTakenOnEveryAttempt_ThrowsAndDoesNotNotify()
+    {
+        _orderRepositoryMock.Setup(c => c.Table).Returns(new List<Order>().AsQueryable());
+        _orderRepositoryMock.Setup(c => c.FirstOrDefaultAsync(It.IsAny<IQueryable<int>>(), It.IsAny<CancellationToken>()))
+            .ReturnsAsync(41);
+        _orderRepositoryMock.Setup(c => c.InsertAsync(It.IsAny<Order>()))
+            .Throws(new DuplicateKeyGrandException());
+
+        await Assert.ThrowsExactlyAsync<DuplicateKeyGrandException>(async () =>
+            await _service.InsertOrder(new Order()));
+
+        _orderRepositoryMock.Verify(c => c.InsertAsync(It.IsAny<Order>()), Times.Exactly(5));
+        _mediatorMock.Verify(c => c.Publish(It.IsAny<EntityInserted<Order>>(), default), Times.Never);
+    }
+
+    [TestMethod]
+    public async Task InsertOrder_NullArguments_ThrowException()
+    {
+        await Assert.ThrowsExactlyAsync<ArgumentNullException>(async () => await _service.InsertOrder(null));
+    }
+
     [TestMethod]
     public async Task UpdateOrder_InvokeExpectedMethods()
     {

src/Tests/Grand.Data.Tests/LiteDb/LiteDbRepositoryMock.cs

@@ -1,6 +1,7 @@
 using Grand.Data.LiteDb;
 using Grand.Domain;
 using LiteDB;
+using System.Linq.Expressions;
 
 namespace Grand.Data.Tests.LiteDb;
 
@@ -12,4 +13,9 @@ public class LiteDBRepositoryMock<T> : LiteDBRepository<T>, IRepository<T> where
         Database.DropCollection(typeof(T).Name);
         Collection = Database.GetCollection<T>(typeof(T).Name);
     }
+
+    public void EnsureUniqueIndex<K>(Expression<Func<T, K>> keySelector)
+    {
+        Collection.EnsureIndex(keySelector, true);
+    }
 }

src/Tests/Grand.Data.Tests/LiteDb/LiteDbRepositoryTests.cs

@@ -1,4 +1,5 @@
 using Grand.Domain.Common;
+using Grand.SharedKernel;
 using Microsoft.VisualStudio.TestTools.UnitTesting;
 
 namespace Grand.Data.Tests.LiteDb;
@@ -41,6 +42,19 @@ public async Task InsertAsync_LiteRepository_Success()
         Assert.AreEqual("user", p.CreatedBy);
     }
 
+    [TestMethod]
+    public async Task InsertAsync_UniqueIndexViolated_ThrowsDuplicateKeyGrandException()
+    {
+        var myRepository = new LiteDBRepositoryMock<SampleCollection>();
+        myRepository.EnsureUniqueIndex(x => x.Count);
+        await myRepository.InsertAsync(new SampleCollection { Id = "1", Count = 7 });
+
+        await Assert.ThrowsExactlyAsync<DuplicateKeyGrandException>(async () =>
+            await myRepository.InsertAsync(new SampleCollection { Id = "2", Count = 7 }));
+
+        Assert.AreEqual(1, myRepository.Table.Count());
+    }
+
     [TestMethod]
     public async Task GetById_LiteRepository_Success()
     {