diff --git a/BTCPayServer.Data/ApplicationDbContext.cs b/BTCPayServer.Data/ApplicationDbContext.cs index 071d3ea77..6a6ce84fe 100644 --- a/BTCPayServer.Data/ApplicationDbContext.cs +++ b/BTCPayServer.Data/ApplicationDbContext.cs @@ -93,7 +93,7 @@ namespace BTCPayServer.Data ApplicationUser.OnModelCreating(builder, Database); AddressInvoiceData.OnModelCreating(builder); APIKeyData.OnModelCreating(builder, Database); - AppData.OnModelCreating(builder); + AppData.OnModelCreating(builder, Database); CustodianAccountData.OnModelCreating(builder, Database); //StoredFile.OnModelCreating(builder); InvoiceEventData.OnModelCreating(builder); diff --git a/BTCPayServer.Data/Data/AppData.cs b/BTCPayServer.Data/Data/AppData.cs index 0cc7af150..784f9162c 100644 --- a/BTCPayServer.Data/Data/AppData.cs +++ b/BTCPayServer.Data/Data/AppData.cs @@ -1,5 +1,6 @@ using System; using Microsoft.EntityFrameworkCore; +using Microsoft.EntityFrameworkCore.Infrastructure; using Newtonsoft.Json; namespace BTCPayServer.Data @@ -16,13 +17,20 @@ namespace BTCPayServer.Data public string Settings { get; set; } public bool Archived { get; set; } - internal static void OnModelCreating(ModelBuilder builder) + internal static void OnModelCreating(ModelBuilder builder, DatabaseFacade databaseFacade) { builder.Entity() .HasOne(o => o.StoreData) .WithMany(i => i.Apps).OnDelete(DeleteBehavior.Cascade); builder.Entity() .HasOne(a => a.StoreData); + + if (databaseFacade.IsNpgsql()) + { + builder.Entity() + .Property(o => o.Settings) + .HasColumnType("JSONB"); + } } // utility methods diff --git a/BTCPayServer.Data/Migrations/20231219031609_appssettingstojson.cs b/BTCPayServer.Data/Migrations/20231219031609_appssettingstojson.cs new file mode 100644 index 000000000..e56ed3c03 --- /dev/null +++ b/BTCPayServer.Data/Migrations/20231219031609_appssettingstojson.cs @@ -0,0 +1,26 @@ +using BTCPayServer.Data; +using Microsoft.EntityFrameworkCore; +using Microsoft.EntityFrameworkCore.Infrastructure; +using Microsoft.EntityFrameworkCore.Migrations; + +#nullable disable + +namespace BTCPayServer.Migrations +{ + [DbContext(typeof(ApplicationDbContext))] + [Migration("20231219031609_appssettingstojson")] + public partial class appssettingstojson : Migration + { + protected override void Up(MigrationBuilder migrationBuilder) + { + if (migrationBuilder.IsNpgsql()) + { + migrationBuilder.Sql("ALTER TABLE \"Apps\" ALTER COLUMN \"Settings\" TYPE JSONB USING \"Settings\"::JSONB"); + } + } + + protected override void Down(MigrationBuilder migrationBuilder) + { + } + } +} diff --git a/BTCPayServer.Data/Migrations/ApplicationDbContextModelSnapshot.cs b/BTCPayServer.Data/Migrations/ApplicationDbContextModelSnapshot.cs index 7ce34c053..3d5f484c7 100644 --- a/BTCPayServer.Data/Migrations/ApplicationDbContextModelSnapshot.cs +++ b/BTCPayServer.Data/Migrations/ApplicationDbContextModelSnapshot.cs @@ -1,4 +1,4 @@ -// +// using System; using BTCPayServer.Data; using Microsoft.EntityFrameworkCore; @@ -89,7 +89,7 @@ namespace BTCPayServer.Migrations .HasColumnType("TEXT"); b.Property("Settings") - .HasColumnType("TEXT"); + .HasColumnType("JSONB"); b.Property("StoreDataId") .HasColumnType("TEXT"); diff --git a/BTCPayServer.Tests/AltcoinTests/AltcoinTests.cs b/BTCPayServer.Tests/AltcoinTests/AltcoinTests.cs index b8ea83f37..1c5605114 100644 --- a/BTCPayServer.Tests/AltcoinTests/AltcoinTests.cs +++ b/BTCPayServer.Tests/AltcoinTests/AltcoinTests.cs @@ -5,6 +5,7 @@ using System.Linq; using System.Threading.Tasks; using BTCPayServer.Controllers; using BTCPayServer.Data; +using BTCPayServer.Events; using BTCPayServer.HostedServices; using BTCPayServer.Hosting; using BTCPayServer.Lightning; @@ -757,24 +758,26 @@ noninventoryitem: vmpos.Template = AppService.SerializeTemplate(MigrationStartupTask.ParsePOSYML(vmpos.Template)); Assert.IsType(pos.UpdatePointOfSale(app.Id, vmpos).Result); - //inventoryitem has 1 item available - await tester.WaitForEvent(() => + async Task AssertCanBuy(string choiceKey, bool expected) { - Assert.IsType(publicApps - .ViewPointOfSale(app.Id, PosViewType.Cart, 1, choiceKey: "inventoryitem").Result); - return Task.CompletedTask; - }); + var redirect = Assert.IsType(await publicApps + .ViewPointOfSale(app.Id, PosViewType.Cart, 1, choiceKey: choiceKey)); + if (expected) + Assert.Equal("UIInvoice", redirect.ControllerName); + else + Assert.NotEqual("UIInvoice", redirect.ControllerName); + } + + //inventoryitem has 1 item available + await AssertCanBuy("inventoryitem", true); //we already bought all available stock so this should fail await Task.Delay(100); - Assert.IsType(publicApps - .ViewPointOfSale(app.Id, PosViewType.Cart, 1, choiceKey: "inventoryitem").Result); + await AssertCanBuy("inventoryitem", false); //inventoryitem has unlimited items available - Assert.IsType(publicApps - .ViewPointOfSale(app.Id, PosViewType.Cart, 1, choiceKey: "noninventoryitem").Result); - Assert.IsType(publicApps - .ViewPointOfSale(app.Id, PosViewType.Cart, 1, choiceKey: "noninventoryitem").Result); + await AssertCanBuy("noninventoryitem", true); + await AssertCanBuy("noninventoryitem", true); //verify invoices where created invoices = user.BitPay.GetInvoices(); @@ -808,34 +811,8 @@ normal: price: 1.0"; vmpos.Template = AppService.SerializeTemplate(MigrationStartupTask.ParsePOSYML(vmpos.Template)); Assert.IsType(pos.UpdatePointOfSale(app.Id, vmpos).Result); - try - { - Assert.IsType(publicApps - .ViewPointOfSale(app.Id, PosViewType.Cart, 1, choiceKey: "btconly").Result); - } - catch (IsTypeException) - { - TestLogs.LogInformation("This test sometimes fails, so we try to find the issue here..."); - TestLogs.LogInformation("Template: " + vmpos.Template); - var retryOk = publicApps.ViewPointOfSale(app.Id, PosViewType.Cart, 1, choiceKey: "btconly").Result is RedirectToActionResult; - var noChoiceKey = publicApps.ViewPointOfSale(app.Id, PosViewType.Cart, 1).Result is RedirectToActionResult; - TestLogs.LogInformation("RetryOk: " + retryOk); - TestLogs.LogInformation("NoChoiceKey: " + retryOk); - var appService = tester.PayTester.GetService(); - var found = await appService.GetApp(app.Id, PointOfSaleAppType.AppType); - TestLogs.LogInformation("Found: " + (found != null)); - if (found is not null) - { - var settings = found.GetSettings(); - TestLogs.LogInformation("settings Found: " + (settings != null)); - if (settings is not null) - { - TestLogs.LogInformation("template Found: " + (settings.Template)); - TestLogs.LogInformation("parsed template Found: " + (AppService.SerializeTemplate(AppService.Parse(settings.Template, false)))); - } - } - throw; - } + Assert.IsType(publicApps + .ViewPointOfSale(app.Id, PosViewType.Cart, 1, choiceKey: "btconly").Result); Assert.IsType(publicApps .ViewPointOfSale(app.Id, PosViewType.Cart, 1, choiceKey: "normal").Result); invoices = user.BitPay.GetInvoices(); diff --git a/BTCPayServer/HostedServices/AppInventoryUpdaterHostedService.cs b/BTCPayServer/HostedServices/AppInventoryUpdaterHostedService.cs index fc22734f0..18f9a0053 100644 --- a/BTCPayServer/HostedServices/AppInventoryUpdaterHostedService.cs +++ b/BTCPayServer/HostedServices/AppInventoryUpdaterHostedService.cs @@ -20,7 +20,6 @@ namespace BTCPayServer.HostedServices protected override void SubscribeToEvents() { Subscribe(); - Subscribe(); } public AppInventoryUpdaterHostedService(EventAggregator eventAggregator, AppService appService, Logs logs) : base(eventAggregator, logs) @@ -31,77 +30,18 @@ namespace BTCPayServer.HostedServices protected override async Task ProcessEvent(object evt, CancellationToken cancellationToken) { - if (evt is UpdateAppInventory updateAppInventory) - { - //get all apps that were tagged that have manageable inventory that has an item that matches the item code in the invoice - var apps = (await _appService.GetApps(updateAppInventory.AppId)).Select(data => - { - switch (data.AppType) - { - case PointOfSaleAppType.AppType: - var possettings = data.GetSettings(); - return (Data: data, Settings: (object)possettings, - Items: AppService.Parse(possettings.Template)); - case CrowdfundAppType.AppType: - var cfsettings = data.GetSettings(); - return (Data: data, Settings: (object)cfsettings, - Items: AppService.Parse(cfsettings.PerksTemplate)); - default: - return (null, null, null); - } - }).Where(tuple => tuple.Data != null && tuple.Items.Any(item => - item.Inventory.HasValue && - updateAppInventory.Items.FirstOrDefault(i => i.Id == item.Id) != null)); - foreach (var app in apps) - { - foreach (var cartItem in updateAppInventory.Items) - { - var item = app.Items.FirstOrDefault(item => item.Id == cartItem.Id); - if (item == null) continue; - - if (updateAppInventory.Deduct) - { - item.Inventory -= cartItem.Count; - } - else - { - item.Inventory += cartItem.Count; - } - } - - switch (app.Data.AppType) - { - case PointOfSaleAppType.AppType: - ((PointOfSaleSettings)app.Settings).Template = - AppService.SerializeTemplate(app.Items); - break; - case CrowdfundAppType.AppType: - ((CrowdfundSettings)app.Settings).PerksTemplate = - AppService.SerializeTemplate(app.Items); - break; - default: - throw new InvalidOperationException(); - } - - app.Data.SetSettings(app.Settings); - await _appService.UpdateOrCreateApp(app.Data); - } - - - } - else if (evt is InvoiceEvent invoiceEvent) + if (evt is InvoiceEvent invoiceEvent) { List cartItems = null; - bool deduct; + int deduct; switch (invoiceEvent.Name) { case InvoiceEvent.Expired: - case InvoiceEvent.MarkedInvalid: - deduct = false; + deduct = 1; break; case InvoiceEvent.Created: - deduct = true; + deduct = -1; break; default: return; @@ -112,11 +52,6 @@ namespace BTCPayServer.HostedServices { var appIds = AppService.GetAppInternalTags(invoiceEvent.Invoice); - if (!appIds.Any()) - { - return; - } - var items = cartItems?.ToList() ?? new List(); if (!string.IsNullOrEmpty(invoiceEvent.Invoice.Metadata.ItemCode)) { @@ -128,27 +63,13 @@ namespace BTCPayServer.HostedServices }); } - _eventAggregator.Publish(new UpdateAppInventory + var changes = items.Select(i => new AppService.InventoryChange(i.Id, i.Count * deduct)).ToArray(); + foreach (var appId in appIds) { - Deduct = deduct, - Items = items, - AppId = appIds - }); - + await _appService.UpdateInventory(appId, changes); + } } } } - - public class UpdateAppInventory - { - public string[] AppId { get; set; } - public List Items { get; set; } - public bool Deduct { get; set; } - - public override string ToString() - { - return string.Empty; - } - } } } diff --git a/BTCPayServer/Plugins/PluginService.cs b/BTCPayServer/Plugins/PluginService.cs index bc45db44d..7812e0157 100644 --- a/BTCPayServer/Plugins/PluginService.cs +++ b/BTCPayServer/Plugins/PluginService.cs @@ -37,7 +37,7 @@ namespace BTCPayServer.Plugins public IEnumerable LoadedPlugins { get; } public BTCPayServerEnvironment Env { get; } - public Version? GetVersionOfPendingInstall(string plugin) + public Version GetVersionOfPendingInstall(string plugin) { var dirName = Path.Combine(_dataDirectories.Value.PluginDir, plugin); var manifestFileName = dirName + ".json"; diff --git a/BTCPayServer/Services/Apps/AppService.cs b/BTCPayServer/Services/Apps/AppService.cs index 90b0e8c85..bf31b063c 100644 --- a/BTCPayServer/Services/Apps/AppService.cs +++ b/BTCPayServer/Services/Apps/AppService.cs @@ -15,6 +15,7 @@ using BTCPayServer.Plugins.PointOfSale.Models; using BTCPayServer.Services.Invoices; using BTCPayServer.Services.Rates; using BTCPayServer.Services.Stores; +using Dapper; using Ganss.Xss; using Microsoft.EntityFrameworkCore; using NBitcoin; @@ -46,6 +47,7 @@ namespace BTCPayServer.Services.Apps private readonly StoreRepository _storeRepository; private readonly HtmlSanitizer _HtmlSanitizer; public CurrencyNameTable Currencies => _Currencies; + public AppService( IEnumerable apps, ApplicationDbContextFactory contextFactory, @@ -380,6 +382,42 @@ namespace BTCPayServer.Services.Apps return app; } + record AppSettingsWithXmin(string apptype, string settings, uint xmin); + public record InventoryChange(string ItemId, int Delta); + public async Task UpdateInventory(string appId, InventoryChange[] changes) + { + await using var ctx = _ContextFactory.CreateContext(); + // We use xmin to make sure we don't override changes made by another process +retry: + var connection = ctx.Database.GetDbConnection(); + var row = connection.QueryFirstOrDefault( + "SELECT \"AppType\" AS apptype, \"Settings\" AS settings, xmin FROM \"Apps\" WHERE \"Id\"=@appId", new { appId } + ); + if (row?.settings is null) + return; + var templatePath = row.apptype switch + { + CrowdfundAppType.AppType => "PerksTemplate", + _ => "Template" + }; + var settings = JObject.Parse(row.settings); + var items = JArray.Parse(settings[templatePath]!.Value()!); + foreach (var change in changes) + { + var item = items.FirstOrDefault(i => i["id"]?.Value() == change.ItemId && i["inventory"] is not null); + if (item is null) + continue; + var inventory = item["inventory"]!.Value(); + inventory += change.Delta; + item["inventory"] = inventory; + } + settings[templatePath] = items.ToString(Formatting.None); + var updated = await connection.ExecuteAsync("UPDATE \"Apps\" SET \"Settings\"=@v::JSONB WHERE \"Id\"=@appId AND xmin=@xmin", new { appId, xmin = (int)row.xmin, v = settings.ToString(Formatting.None) }) == 1; + // If we can't update, it means someone else updated the row, so we need to retry + if (!updated) + goto retry; + } + public async Task UpdateOrCreateApp(AppData app) { await using var ctx = _ContextFactory.CreateContext(); diff --git a/Changelog.md b/Changelog.md index 05f3279c0..460e722d1 100644 --- a/Changelog.md +++ b/Changelog.md @@ -31,6 +31,7 @@ * Do not activate Blazor in Wizard screens (#5435) @NicolasDorier * Pull Payment: Display the amount of claims (#5427) @NicolasDorier * Dashboard: LND limbo balance had the wrong unit (a 1 BTC limbo balance would show as 0.001 BTC) @NicolasDorier +* Fix occasional concurrency issue which would result in app settings change not being properly saved (#5565) @NicolasDorier ### Improvements