diff --git a/src/WidgetWorks.Application/Abstractions/IOrderRepository.cs b/src/WidgetWorks.Application/Abstractions/IOrderRepository.cs index 927ec5d..7419c5d 100644 --- a/src/WidgetWorks.Application/Abstractions/IOrderRepository.cs +++ b/src/WidgetWorks.Application/Abstractions/IOrderRepository.cs @@ -7,13 +7,24 @@ public interface IOrderRepository /// Atomically inserts the order and reserves stock. Returns false (rolled back) if any line is short. Task TryPlaceAsync(Order order, CancellationToken ct); - /// Records the provider + reference and parks the order in AwaitingPayment (async settlement). - Task MarkAwaitingPaymentAsync(Guid orderId, string provider, string reference, DateTimeOffset now, CancellationToken ct); + /// + /// Records the provider + reference and parks the order in AwaitingPayment (async settlement). + /// Returns false when the order had already moved on, so the write was declined. + /// + Task MarkAwaitingPaymentAsync(Guid orderId, string provider, string reference, DateTimeOffset now, CancellationToken ct); - Task MarkPaidAsync(Guid orderId, string provider, string reference, DateTimeOffset now, CancellationToken ct); + /// + /// Settles the order. Returns false when it was no longer awaiting settlement — a duplicate or + /// out-of-order provider event, which must not overwrite a decided order. + /// + Task MarkPaidAsync(Guid orderId, string provider, string reference, DateTimeOffset now, CancellationToken ct); - /// Marks the order failed and releases its inventory reservations. - Task MarkPaymentFailedAsync(Order order, string reason, DateTimeOffset now, CancellationToken ct); + /// + /// Marks the order failed and releases its inventory reservations, atomically. Returns false + /// when the order had already moved on; the caller must treat that as "someone else handled it" + /// rather than retrying, because the stock has already been dealt with. + /// + Task MarkPaymentFailedAsync(Order order, string reason, DateTimeOffset now, CancellationToken ct); /// /// Persists a fulfilment transition together with the inventory movement it implies, in one @@ -23,6 +34,15 @@ public interface IOrderRepository /// Task UpdateStatusAsync(Order order, DateTimeOffset now, CancellationToken ct); + /// + /// Orders still parked in AwaitingPayment since before , items loaded + /// so their reservations can be released. A settlement webhook that never arrives would + /// otherwise hold that stock forever. + /// + /// Caps one sweep, so a large backlog is worked through over several passes + /// rather than in one long transaction. + Task> GetStaleAwaitingPaymentAsync(DateTimeOffset cutoff, int limit, CancellationToken ct); + Task GetByIdAsync(Guid id, CancellationToken ct); /// Finds an order by the payment provider + reference stored at authorization time (webhook correlation). diff --git a/src/WidgetWorks.Application/Abstractions/IWidgetRepository.cs b/src/WidgetWorks.Application/Abstractions/IWidgetRepository.cs index cf92d81..b21bcd9 100644 --- a/src/WidgetWorks.Application/Abstractions/IWidgetRepository.cs +++ b/src/WidgetWorks.Application/Abstractions/IWidgetRepository.cs @@ -3,11 +3,35 @@ namespace WidgetWorks.Application.Abstractions; /// Filter/paging criteria for catalog listing and search. -public sealed record WidgetQuery(string? Search, bool ActiveOnly, int Page, int PageSize) +/// +/// A catalogue listing request. Search and Category are independent narrowings combined with AND, +/// so "turbine" within Mega means both, not either. +/// +/// +/// One of . Never interpolated into SQL - the repository maps it through a +/// fixed set of order-by clauses, so an unrecognised value falls back to the default rather than +/// reaching the database. +/// +public sealed record WidgetQuery( + string? Search, + bool ActiveOnly, + int Page, + int PageSize, + string? Category = null, + string? Sort = null) { public int Offset => (Math.Max(1, Page) - 1) * PageSize; } +/// The orderings the catalogue offers. Values are part of the API contract. +public static class WidgetSort +{ + public const string Featured = "featured"; + public const string PriceAscending = "price-asc"; + public const string PriceDescending = "price-desc"; + public const string Name = "name"; +} + public interface IWidgetRepository { Task GetByIdAsync(Guid id, CancellationToken ct); diff --git a/src/WidgetWorks.Application/Carts/AddItem/AddCartItemHandler.cs b/src/WidgetWorks.Application/Carts/AddItem/AddCartItemHandler.cs index 58a8a63..80841f9 100644 --- a/src/WidgetWorks.Application/Carts/AddItem/AddCartItemHandler.cs +++ b/src/WidgetWorks.Application/Carts/AddItem/AddCartItemHandler.cs @@ -40,7 +40,10 @@ public async Task> Handle(AddCartItemCommand command, Cancellat private async Task ResolveCartAsync(Guid? cartId, Guid? userId, CancellationToken ct) { - if (cartId is { } id && await carts.GetAsync(id, ct) is { } existing) + // A supplied id is only honoured when the caller may actually use that cart. A foreign one + // falls through to the caller's own rather than erroring, so an attacker learns nothing about + // whether the id existed and an honest client with a stale id simply carries on. + if (cartId is { } id && await carts.GetAsync(id, ct) is { } existing && CartAccess.IsPermitted(existing, userId)) { return existing; } diff --git a/src/WidgetWorks.Application/Carts/CartAccess.cs b/src/WidgetWorks.Application/Carts/CartAccess.cs new file mode 100644 index 0000000..1e4a9c4 --- /dev/null +++ b/src/WidgetWorks.Application/Carts/CartAccess.cs @@ -0,0 +1,25 @@ +using WidgetWorks.Domain.Carts; + +namespace WidgetWorks.Application.Carts; + +/// +/// The single rule for who may touch a cart. +/// +/// A guest cart carries no owner and is reachable by anyone holding its id — that is what lets a +/// visitor fill a basket before signing in, and it is a capability model: the id is the credential. +/// The moment a cart belongs to a user, only that user may reach it, so signing in genuinely +/// protects the basket instead of leaving it as exposed as a guest's. +/// +/// Kept as one function rather than repeated per handler so a new cart operation cannot quietly ship +/// without the check, and so the rule can be tested on its own. +/// +public static class CartAccess +{ + /// The cart that was loaded by id. + /// The signed-in user, or null for an anonymous caller. + public static bool IsPermitted(Cart cart, Guid? requestedBy) + { + ArgumentNullException.ThrowIfNull(cart); + return cart.UserId is null || cart.UserId == requestedBy; + } +} diff --git a/src/WidgetWorks.Application/Carts/GetCart/GetCartHandler.cs b/src/WidgetWorks.Application/Carts/GetCart/GetCartHandler.cs index 61662e1..ad8684b 100644 --- a/src/WidgetWorks.Application/Carts/GetCart/GetCartHandler.cs +++ b/src/WidgetWorks.Application/Carts/GetCart/GetCartHandler.cs @@ -1,9 +1,10 @@ using WidgetWorks.Application.Abstractions; using WidgetWorks.Domain.Common; +using WidgetWorks.Application.Carts; namespace WidgetWorks.Application.Carts.GetCart; -public sealed record GetCartQuery(Guid CartId); +public sealed record GetCartQuery(Guid CartId, Guid? RequestedBy); public sealed class GetCartHandler(ICartRepository carts, IWidgetRepository widgets) { @@ -15,6 +16,13 @@ public async Task> Handle(GetCartQuery query, CancellationToken return Result.Fail("Cart not found."); } + if (!CartAccess.IsPermitted(cart, query.RequestedBy)) + { + // Deliberately the same answer as a missing cart: telling an unauthorized caller that + // the cart exists would confirm a guess. + return Result.Fail("Cart not found."); + } + return Result.Success(await CartAssembler.BuildAsync(cart, widgets, ct)); } } diff --git a/src/WidgetWorks.Application/Carts/RemoveItem/RemoveCartItemHandler.cs b/src/WidgetWorks.Application/Carts/RemoveItem/RemoveCartItemHandler.cs index 2db1f03..b02084a 100644 --- a/src/WidgetWorks.Application/Carts/RemoveItem/RemoveCartItemHandler.cs +++ b/src/WidgetWorks.Application/Carts/RemoveItem/RemoveCartItemHandler.cs @@ -1,9 +1,10 @@ using WidgetWorks.Application.Abstractions; using WidgetWorks.Domain.Common; +using WidgetWorks.Application.Carts; namespace WidgetWorks.Application.Carts.RemoveItem; -public sealed record RemoveCartItemCommand(Guid CartId, Guid WidgetId); +public sealed record RemoveCartItemCommand(Guid CartId, Guid WidgetId, Guid? RequestedBy); public sealed class RemoveCartItemHandler(ICartRepository carts, IWidgetRepository widgets, TimeProvider clock) { @@ -15,6 +16,13 @@ public async Task> Handle(RemoveCartItemCommand command, Cancel return Result.Fail("Cart not found."); } + if (!CartAccess.IsPermitted(cart, command.RequestedBy)) + { + // Deliberately the same answer as a missing cart: telling an unauthorized caller that + // the cart exists would confirm a guess. + return Result.Fail("Cart not found."); + } + await carts.RemoveItemAsync(command.CartId, command.WidgetId, ct); await carts.TouchAsync(command.CartId, clock.GetUtcNow(), ct); diff --git a/src/WidgetWorks.Application/Carts/UpdateItem/UpdateCartItemHandler.cs b/src/WidgetWorks.Application/Carts/UpdateItem/UpdateCartItemHandler.cs index ea7bca8..07639b1 100644 --- a/src/WidgetWorks.Application/Carts/UpdateItem/UpdateCartItemHandler.cs +++ b/src/WidgetWorks.Application/Carts/UpdateItem/UpdateCartItemHandler.cs @@ -1,10 +1,11 @@ using WidgetWorks.Application.Abstractions; using WidgetWorks.Domain.Common; +using WidgetWorks.Application.Carts; namespace WidgetWorks.Application.Carts.UpdateItem; /// Sets an absolute quantity for a line; a quantity of zero removes it. -public sealed record UpdateCartItemCommand(Guid CartId, Guid WidgetId, int Quantity); +public sealed record UpdateCartItemCommand(Guid CartId, Guid WidgetId, int Quantity, Guid? RequestedBy); public sealed class UpdateCartItemHandler(ICartRepository carts, IWidgetRepository widgets, TimeProvider clock) { @@ -16,6 +17,13 @@ public async Task> Handle(UpdateCartItemCommand command, Cancel return Result.Fail("Cart not found."); } + if (!CartAccess.IsPermitted(cart, command.RequestedBy)) + { + // Deliberately the same answer as a missing cart: telling an unauthorized caller that + // the cart exists would confirm a guess. + return Result.Fail("Cart not found."); + } + var now = clock.GetUtcNow(); if (command.Quantity <= 0) { diff --git a/src/WidgetWorks.Application/Catalog/Browse/BrowseWidgetsHandler.cs b/src/WidgetWorks.Application/Catalog/Browse/BrowseWidgetsHandler.cs index cc3ec81..77d0403 100644 --- a/src/WidgetWorks.Application/Catalog/Browse/BrowseWidgetsHandler.cs +++ b/src/WidgetWorks.Application/Catalog/Browse/BrowseWidgetsHandler.cs @@ -2,7 +2,13 @@ namespace WidgetWorks.Application.Catalog.Browse; -public sealed record BrowseWidgetsQuery(string? Search, bool IncludeInactive, int Page, int PageSize); +public sealed record BrowseWidgetsQuery( + string? Search, + bool IncludeInactive, + int Page, + int PageSize, + string? Category = null, + string? Sort = null); public sealed class BrowseWidgetsHandler(IWidgetRepository widgets) { @@ -18,7 +24,9 @@ public async Task> Handle(BrowseWidgetsQuery query, Canc string.IsNullOrWhiteSpace(query.Search) ? null : query.Search.Trim(), ActiveOnly: !query.IncludeInactive, page, - size); + size, + Category: string.IsNullOrWhiteSpace(query.Category) ? null : query.Category.Trim(), + Sort: query.Sort); var items = await widgets.SearchAsync(repoQuery, ct); var total = await widgets.CountAsync(repoQuery, ct); diff --git a/src/WidgetWorks.Application/Checkout/ConfirmPayment/ConfirmPaymentHandler.cs b/src/WidgetWorks.Application/Checkout/ConfirmPayment/ConfirmPaymentHandler.cs index 725189b..9da1842 100644 --- a/src/WidgetWorks.Application/Checkout/ConfirmPayment/ConfirmPaymentHandler.cs +++ b/src/WidgetWorks.Application/Checkout/ConfirmPayment/ConfirmPaymentHandler.cs @@ -38,7 +38,14 @@ public async Task> Handle(ConfirmPaymentCommand command, Cancella if (command.Type == PaymentEventType.Succeeded) { - await orders.MarkPaidAsync(order.Id, order.PaymentProvider ?? command.Provider, command.Reference, now, ct); + // The status check above is a courtesy; this is the decision. Two deliveries of the + // same event both pass that check, and the database picks exactly one winner. Only the + // winner sends a receipt, so a retried webhook cannot email the customer twice. + if (!await orders.MarkPaidAsync(order.Id, order.PaymentProvider ?? command.Provider, command.Reference, now, ct)) + { + return Result.Success(order.Status); + } + order.Status = OrderStatus.Paid; try @@ -58,7 +65,13 @@ public async Task> Handle(ConfirmPaymentCommand command, Cancella return Result.Success(OrderStatus.Paid); } - await orders.MarkPaymentFailedAsync(order, "Payment failed.", now, ct); + // Same contract on the failure path, and it matters more here: the losing caller must not + // release the reservation a second time. + if (!await orders.MarkPaymentFailedAsync(order, "Payment failed.", now, ct)) + { + return Result.Success(order.Status); + } + order.Status = OrderStatus.PaymentFailed; return Result.Success(OrderStatus.PaymentFailed); } diff --git a/src/WidgetWorks.Application/Checkout/PlaceOrder/CheckoutHandler.cs b/src/WidgetWorks.Application/Checkout/PlaceOrder/CheckoutHandler.cs index c0ca741..251a431 100644 --- a/src/WidgetWorks.Application/Checkout/PlaceOrder/CheckoutHandler.cs +++ b/src/WidgetWorks.Application/Checkout/PlaceOrder/CheckoutHandler.cs @@ -65,8 +65,10 @@ public async Task> Handle(CheckoutCommand command, Cancel } var cart = await carts.GetAsync(command.CartId, ct); - if (cart is null) + if (cart is null || !CartAccess.IsPermitted(cart, command.UserId)) { + // One answer for "no such cart" and "not yours": checking out someone else's basket + // would otherwise disclose its contents in the resulting order. return Fail("Cart not found."); } @@ -93,7 +95,10 @@ public async Task> Handle(CheckoutCommand command, Cancel if (payment.Status == PaymentStatus.Declined) { - await orders.MarkPaymentFailedAsync(order, payment.Error ?? "Payment failed.", clock.GetUtcNow(), ct); + // Discarded deliberately: this order was created moments ago and is still Pending, so + // the compare-and-set cannot decline. A webhook arriving later is the contended path, + // and ConfirmPaymentHandler is where the answer is acted on. + _ = await orders.MarkPaymentFailedAsync(order, payment.Error ?? "Payment failed.", clock.GetUtcNow(), ct); return Fail(payment.Error ?? "Payment failed."); } @@ -103,7 +108,7 @@ public async Task> Handle(CheckoutCommand command, Cancel { // Async settlement (redirect/BNPL): keep the reservation, park the order, and let the // provider webhook finalize it. The receipt email is sent on confirmation, not here. - await orders.MarkAwaitingPaymentAsync(order.Id, payment.Provider, reference, clock.GetUtcNow(), ct); + _ = await orders.MarkAwaitingPaymentAsync(order.Id, payment.Provider, reference, clock.GetUtcNow(), ct); order.Status = OrderStatus.AwaitingPayment; await carts.DeleteAsync(cart.Id, ct); @@ -113,7 +118,7 @@ public async Task> Handle(CheckoutCommand command, Cancel } // Synchronous success. - await orders.MarkPaidAsync(order.Id, payment.Provider, reference, clock.GetUtcNow(), ct); + _ = await orders.MarkPaidAsync(order.Id, payment.Provider, reference, clock.GetUtcNow(), ct); order.Status = OrderStatus.Paid; await carts.DeleteAsync(cart.Id, ct); diff --git a/src/WidgetWorks.Application/Checkout/ReleaseStale/ReleaseStaleReservationsHandler.cs b/src/WidgetWorks.Application/Checkout/ReleaseStale/ReleaseStaleReservationsHandler.cs new file mode 100644 index 0000000..762391e --- /dev/null +++ b/src/WidgetWorks.Application/Checkout/ReleaseStale/ReleaseStaleReservationsHandler.cs @@ -0,0 +1,99 @@ +using Microsoft.Extensions.Logging; +using WidgetWorks.Application.Abstractions; + +namespace WidgetWorks.Application.Checkout.ReleaseStale; + +/// +/// How long an unsettled order may hold stock, and how often to look. Bound from the +/// Reservations configuration section. +/// +public sealed class ReservationOptions +{ + /// + /// How long an order may sit in AwaitingPayment before its stock is handed back. + /// + /// This is a trade, not a tuning knob: too short and a slow but honest bank redirect loses a + /// customer's basket; too long and abandoned or abusive orders hold the catalogue hostage. + /// Fifteen minutes is longer than any interactive redirect and short enough that a shopper who + /// returns to an out-of-stock item is rare. + /// + public int ExpireAfterMinutes { get; set; } = 15; + + /// How often the sweep runs. + public int SweepIntervalMinutes { get; set; } = 5; + + /// + /// Most orders released in one pass. A backlog is worked through over several sweeps rather + /// than in one long pass, so a bad day cannot turn into a slow transaction storm. + /// + public int BatchSize { get; set; } = 100; + + /// Turns the sweep off — for a host that should not run background work. + public bool Enabled { get; set; } = true; +} + +/// +/// Hands back stock held by orders whose payment never settled. +/// +/// Checkout reserves stock the moment an order is placed. When settlement is asynchronous the order +/// parks in AwaitingPayment and waits for a provider webhook. If that webhook never arrives — a +/// provider outage, a dropped delivery, a shopper who closed the tab at the bank's redirect — the +/// reservation is held forever, and without this sweep the only route back is an administrator +/// editing inventory counts by hand. +/// +/// The release itself reuses : it already sets +/// the status and releases the reservation in one transaction, and its compare-and-set means a +/// webhook landing at the same moment as a sweep cannot both act. PaymentFailed is also the honest +/// description of a settlement that never came, so no new status is needed. +/// +/// Scheduling deliberately lives elsewhere. This type is a plain handler so the policy can be tested +/// without a timer, a host, or a clock that really waits. +/// +public sealed class ReleaseStaleReservationsHandler( + IOrderRepository orders, + TimeProvider clock, + ReservationOptions options, + ILogger logger) +{ + /// Runs one sweep. Returns how many orders had their stock returned to sale. + public async Task Handle(CancellationToken ct) + { + var now = clock.GetUtcNow(); + var cutoff = now.AddMinutes(-Math.Max(1, options.ExpireAfterMinutes)); + var stale = await orders.GetStaleAwaitingPaymentAsync(cutoff, Math.Max(1, options.BatchSize), ct); + + var released = 0; + foreach (var order in stale) + { + // Checked between orders rather than only at the top: a shutdown midway through a large + // batch should stop cleanly, and each release is already committed on its own. + ct.ThrowIfCancellationRequested(); + + if (await orders.MarkPaymentFailedAsync(order, "Payment was not completed in time.", now, ct)) + { + released++; + } + else + { + // The order moved on between the query and the write — almost always a webhook + // that landed first, which is the good outcome. Recorded at debug because it is + // expected, not a fault. + logger.LogDebug( + "Order {OrderNumber} settled before the sweep reached it; nothing released.", + order.OrderNumber); + } + } + + if (released > 0) + { + // Worth a real log line: it means customers or scripts are abandoning payments, and a + // rising count is the signal that something upstream is wrong. + logger.LogInformation( + "Released stock held by {Released} order(s) unsettled since before {Cutoff:o}.", + released, + cutoff); + } + + return released; + } +} diff --git a/src/WidgetWorks.Application/DependencyInjection.cs b/src/WidgetWorks.Application/DependencyInjection.cs index 387aae8..e0b0795 100644 --- a/src/WidgetWorks.Application/DependencyInjection.cs +++ b/src/WidgetWorks.Application/DependencyInjection.cs @@ -19,6 +19,7 @@ using WidgetWorks.Application.Catalog.Update; using WidgetWorks.Application.Checkout.ConfirmPayment; using WidgetWorks.Application.Checkout.PlaceOrder; +using WidgetWorks.Application.Checkout.ReleaseStale; using WidgetWorks.Application.Checkout.Quote; using WidgetWorks.Application.Orders.Admin; using WidgetWorks.Application.Orders.GetMine; @@ -67,6 +68,7 @@ public static IServiceCollection AddApplication(this IServiceCollection services services.AddScoped(); services.AddScoped(); services.AddScoped(); + services.AddScoped(); services.AddScoped(); services.AddScoped(); services.AddScoped(); diff --git a/src/WidgetWorks.Infrastructure/Persistence/OrderRepository.cs b/src/WidgetWorks.Infrastructure/Persistence/OrderRepository.cs index 6abbd5a..294de97 100644 --- a/src/WidgetWorks.Infrastructure/Persistence/OrderRepository.cs +++ b/src/WidgetWorks.Infrastructure/Persistence/OrderRepository.cs @@ -75,31 +75,56 @@ public async Task TryPlaceAsync(Order order, CancellationToken ct) } } - public async Task MarkAwaitingPaymentAsync(Guid orderId, string provider, string reference, DateTimeOffset now, CancellationToken ct) + /// + /// The statuses a settlement outcome may still be applied from. Anything else means the order + /// has already moved on and a late or repeated event must not touch it. + /// + private static readonly string[] AwaitingSettlement = [OrderStatus.Pending, OrderStatus.AwaitingPayment]; + + public async Task MarkAwaitingPaymentAsync(Guid orderId, string provider, string reference, DateTimeOffset now, CancellationToken ct) { using var db = await factory.OpenAsync(ct); - await db.ExecuteAsync( - "update orders set status = @Status, payment_provider = @Provider, payment_reference = @Reference, updated_at = @Now where id = @Id", - new { Id = orderId, Status = OrderStatus.AwaitingPayment, Provider = provider, Reference = reference, Now = now }); + var affected = await db.ExecuteAsync(new CommandDefinition( + @"update orders set status = @Status, payment_provider = @Provider, payment_reference = @Reference, updated_at = @Now + where id = @Id and status = @Expected", + new { Id = orderId, Status = OrderStatus.AwaitingPayment, Provider = provider, Reference = reference, Now = now, Expected = OrderStatus.Pending }, + cancellationToken: ct)); + return affected == 1; } - public async Task MarkPaidAsync(Guid orderId, string provider, string reference, DateTimeOffset now, CancellationToken ct) + public async Task MarkPaidAsync(Guid orderId, string provider, string reference, DateTimeOffset now, CancellationToken ct) { using var db = await factory.OpenAsync(ct); - await db.ExecuteAsync( - "update orders set status = @Status, payment_provider = @Provider, payment_reference = @Reference, updated_at = @Now where id = @Id", - new { Id = orderId, Status = OrderStatus.Paid, Provider = provider, Reference = reference, Now = now }); + var affected = await db.ExecuteAsync(new CommandDefinition( + @"update orders set status = @Status, payment_provider = @Provider, payment_reference = @Reference, updated_at = @Now + where id = @Id and status = any(@Expected)", + new { Id = orderId, Status = OrderStatus.Paid, Provider = provider, Reference = reference, Now = now, Expected = AwaitingSettlement }, + cancellationToken: ct)); + return affected == 1; } - public async Task MarkPaymentFailedAsync(Order order, string reason, DateTimeOffset now, CancellationToken ct) + public async Task MarkPaymentFailedAsync(Order order, string reason, DateTimeOffset now, CancellationToken ct) { using var db = await factory.OpenAsync(ct); using var tx = db.BeginTransaction(); try { - await db.ExecuteAsync(new CommandDefinition( - "update orders set status = @Status, updated_at = @Now where id = @Id", - new { Id = order.Id, Status = OrderStatus.PaymentFailed, Now = now }, tx, cancellationToken: ct)); + // Compare-and-set inside the transaction, so the row decides who wins rather than the + // caller. Two concurrent deliveries of the same failure both pass an application-level + // status check; only one can win this update, and only the winner releases the + // reservation. Without it a redelivered webhook decrements quantity_reserved a second + // time and eats stock still held by a different order. + var applied = await db.ExecuteAsync(new CommandDefinition( + @"update orders set status = @Status, updated_at = @Now + where id = @Id and status = any(@Expected)", + new { Id = order.Id, Status = OrderStatus.PaymentFailed, Now = now, Expected = AwaitingSettlement }, + tx, cancellationToken: ct)); + + if (applied != 1) + { + tx.Rollback(); + return false; + } foreach (var item in order.Items) { @@ -108,6 +133,7 @@ await db.ExecuteAsync(new CommandDefinition( } tx.Commit(); + return true; } catch { @@ -159,6 +185,27 @@ await db.ExecuteAsync(new CommandDefinition( } } + public async Task> GetStaleAwaitingPaymentAsync(DateTimeOffset cutoff, int limit, CancellationToken ct) + { + using var db = await factory.OpenAsync(ct); + var orders = (await db.QueryAsync(new CommandDefinition( + $@"select {OrderColumns} from orders + where status = @Status and updated_at < @Cutoff + order by updated_at + limit @Limit", + new { Status = OrderStatus.AwaitingPayment, Cutoff = cutoff, Limit = limit }, + cancellationToken: ct))).ToList(); + + foreach (var order in orders) + { + order.Items = (await db.QueryAsync(new CommandDefinition( + $"select {ItemColumns} from order_items where order_id = @id order by name", + new { id = order.Id }, cancellationToken: ct))).ToList(); + } + + return orders; + } + public async Task GetByIdAsync(Guid id, CancellationToken ct) { using var db = await factory.OpenAsync(ct); diff --git a/src/WidgetWorks.Infrastructure/Persistence/WidgetRepository.cs b/src/WidgetWorks.Infrastructure/Persistence/WidgetRepository.cs index 4df8bbb..30ebd84 100644 --- a/src/WidgetWorks.Infrastructure/Persistence/WidgetRepository.cs +++ b/src/WidgetWorks.Infrastructure/Persistence/WidgetRepository.cs @@ -14,7 +14,38 @@ public sealed class WidgetRepository(IDbConnectionFactory factory) : IWidgetRepo private const string Filter = @"where archived_at is null and (@ActiveOnly = false or is_active = true) - and (@Search is null or name ilike @Pattern or sku ilike @Pattern)"; + and (@Search is null or name ilike @Pattern or sku ilike @Pattern) + and (@Category is null or name ilike @CategoryPattern or sku ilike @CategoryPattern)"; + + /// + /// Sort clauses, chosen by key rather than built from the caller's string - the only safe way to + /// put user input near an order-by. An unknown key sorts by name, which is the catalogue default. + /// Every clause ends with name so paging is stable when prices tie; without a total ordering the + /// same row can appear on two pages. + /// + private static string OrderBy(string? sort) => sort switch + { + WidgetSort.PriceAscending => "order by price asc, name asc", + WidgetSort.PriceDescending => "order by price desc, name asc", + WidgetSort.Name => "order by name asc", + + // Featured leads with what can actually be bought and pushes sold-out items to the end. + // This used to happen in the browser over one page, which quietly meant "in stock on this + // page first"; done here it holds across the whole result set. + _ => "order by ((quantity_on_hand - quantity_reserved) > 0) desc, name asc", + }; + + /// Parameters shared by the listing and its count, so the two can never diverge. + private static object FilterParameters(WidgetQuery query) => new + { + query.ActiveOnly, + query.Search, + Pattern = query.Search is null ? null : $"%{query.Search}%", + query.Category, + CategoryPattern = query.Category is null ? null : $"%{query.Category}%", + Limit = query.PageSize, + query.Offset, + }; public async Task GetByIdAsync(Guid id, CancellationToken ct) { @@ -35,34 +66,24 @@ public sealed class WidgetRepository(IDbConnectionFactory factory) : IWidgetRepo public async Task> SearchAsync(WidgetQuery query, CancellationToken ct) { using var db = await factory.OpenAsync(ct); - var rows = await db.QueryAsync( + var rows = await db.QueryAsync(new CommandDefinition( $@"select {Columns} from widgets {Filter} - order by name + {OrderBy(query.Sort)} limit @Limit offset @Offset", - new - { - query.ActiveOnly, - query.Search, - Pattern = query.Search is null ? null : $"%{query.Search}%", - Limit = query.PageSize, - query.Offset, - }); + FilterParameters(query), + cancellationToken: ct)); return rows.ToList(); } public async Task CountAsync(WidgetQuery query, CancellationToken ct) { using var db = await factory.OpenAsync(ct); - return await db.ExecuteScalarAsync( + return await db.ExecuteScalarAsync(new CommandDefinition( $@"select count(*) from widgets {Filter}", - new - { - query.ActiveOnly, - query.Search, - Pattern = query.Search is null ? null : $"%{query.Search}%", - }); + FilterParameters(query), + cancellationToken: ct)); } public async Task AddAsync(Widget widget, CancellationToken ct) diff --git a/src/WidgetWorks.WebApi/Auth/AuthEndpoints.cs b/src/WidgetWorks.WebApi/Auth/AuthEndpoints.cs index 94b5c12..8456ded 100644 --- a/src/WidgetWorks.WebApi/Auth/AuthEndpoints.cs +++ b/src/WidgetWorks.WebApi/Auth/AuthEndpoints.cs @@ -6,6 +6,7 @@ using WidgetWorks.Application.Auth.Register; using WidgetWorks.Application.TwoFactor.Challenge; using WidgetWorks.Application.TwoFactor.Recovery; +using WidgetWorks.WebApi.RateLimiting; namespace WidgetWorks.WebApi.Auth; @@ -23,7 +24,7 @@ public static class AuthEndpoints { public static void MapAuthEndpoints(this IEndpointRouteBuilder routes) { - var group = routes.MapGroup("/auth"); + var group = routes.MapGroup("/auth").RequireRateLimiting(RateLimitPolicies.Auth); group.MapPost("/register", async (RegisterCommand command, RegisterHandler handler, CancellationToken ct) => { diff --git a/src/WidgetWorks.WebApi/Carts/CartEndpoints.cs b/src/WidgetWorks.WebApi/Carts/CartEndpoints.cs index 984b67e..c16c005 100644 --- a/src/WidgetWorks.WebApi/Carts/CartEndpoints.cs +++ b/src/WidgetWorks.WebApi/Carts/CartEndpoints.cs @@ -13,9 +13,9 @@ public static void MapCartEndpoints(this IEndpointRouteBuilder routes) { var cart = routes.MapGroup("/cart"); - cart.MapGet("/{cartId:guid}", async (Guid cartId, GetCartHandler handler, CancellationToken ct) => + cart.MapGet("/{cartId:guid}", async (Guid cartId, ClaimsPrincipal principal, GetCartHandler handler, CancellationToken ct) => { - var result = await handler.Handle(new GetCartQuery(cartId), ct); + var result = await handler.Handle(new GetCartQuery(cartId, UserId(principal)), ct); return result.IsSuccess ? Results.Ok(result.Value) : Results.NotFound(new { error = result.Error }); }); @@ -26,15 +26,15 @@ public static void MapCartEndpoints(this IEndpointRouteBuilder routes) return result.IsSuccess ? Results.Ok(result.Value) : Results.BadRequest(new { error = result.Error }); }); - cart.MapPut("/{cartId:guid}/items/{widgetId:guid}", async (Guid cartId, Guid widgetId, UpdateItemRequest body, UpdateCartItemHandler handler, CancellationToken ct) => + cart.MapPut("/{cartId:guid}/items/{widgetId:guid}", async (Guid cartId, Guid widgetId, UpdateItemRequest body, ClaimsPrincipal principal, UpdateCartItemHandler handler, CancellationToken ct) => { - var result = await handler.Handle(new UpdateCartItemCommand(cartId, widgetId, body.Quantity), ct); + var result = await handler.Handle(new UpdateCartItemCommand(cartId, widgetId, body.Quantity, UserId(principal)), ct); return result.IsSuccess ? Results.Ok(result.Value) : Results.BadRequest(new { error = result.Error }); }); - cart.MapDelete("/{cartId:guid}/items/{widgetId:guid}", async (Guid cartId, Guid widgetId, RemoveCartItemHandler handler, CancellationToken ct) => + cart.MapDelete("/{cartId:guid}/items/{widgetId:guid}", async (Guid cartId, Guid widgetId, ClaimsPrincipal principal, RemoveCartItemHandler handler, CancellationToken ct) => { - var result = await handler.Handle(new RemoveCartItemCommand(cartId, widgetId), ct); + var result = await handler.Handle(new RemoveCartItemCommand(cartId, widgetId, UserId(principal)), ct); return result.IsSuccess ? Results.Ok(result.Value) : Results.NotFound(new { error = result.Error }); }); diff --git a/src/WidgetWorks.WebApi/Catalog/CatalogEndpoints.cs b/src/WidgetWorks.WebApi/Catalog/CatalogEndpoints.cs index 28a6b7d..74f731b 100644 --- a/src/WidgetWorks.WebApi/Catalog/CatalogEndpoints.cs +++ b/src/WidgetWorks.WebApi/Catalog/CatalogEndpoints.cs @@ -16,9 +16,9 @@ public static void MapCatalogEndpoints(this IEndpointRouteBuilder routes) // Public storefront: active widgets only. var catalog = routes.MapGroup("/catalog"); - catalog.MapGet("/widgets", async (string? search, int? page, int? pageSize, BrowseWidgetsHandler handler, CancellationToken ct) => + catalog.MapGet("/widgets", async (string? search, string? category, string? sort, int? page, int? pageSize, BrowseWidgetsHandler handler, CancellationToken ct) => { - var result = await handler.Handle(new BrowseWidgetsQuery(search, IncludeInactive: false, page ?? 1, pageSize ?? 20), ct); + var result = await handler.Handle(new BrowseWidgetsQuery(search, IncludeInactive: false, page ?? 1, pageSize ?? 20, category, sort), ct); return Results.Ok(result); }); diff --git a/src/WidgetWorks.WebApi/Checkout/CheckoutEndpoints.cs b/src/WidgetWorks.WebApi/Checkout/CheckoutEndpoints.cs index 4475f91..055dd49 100644 --- a/src/WidgetWorks.WebApi/Checkout/CheckoutEndpoints.cs +++ b/src/WidgetWorks.WebApi/Checkout/CheckoutEndpoints.cs @@ -2,6 +2,7 @@ using WidgetWorks.Application.Abstractions; using WidgetWorks.Application.Checkout.PlaceOrder; using WidgetWorks.Application.Checkout.Quote; +using WidgetWorks.WebApi.RateLimiting; namespace WidgetWorks.WebApi.Checkout; @@ -21,7 +22,7 @@ public static void MapCheckoutEndpoints(this IEndpointRouteBuilder routes) body.PaymentToken); var result = await handler.Handle(command, ct); return result.IsSuccess ? Results.Ok(result.Value) : Results.BadRequest(new { error = result.Error }); - }); + }).RequireRateLimiting(RateLimitPolicies.Checkout); var group = routes.MapGroup("/checkout"); diff --git a/src/WidgetWorks.WebApi/Hosting/ReservationSweeper.cs b/src/WidgetWorks.WebApi/Hosting/ReservationSweeper.cs new file mode 100644 index 0000000..33611bf --- /dev/null +++ b/src/WidgetWorks.WebApi/Hosting/ReservationSweeper.cs @@ -0,0 +1,78 @@ +using WidgetWorks.Application.Checkout.ReleaseStale; + +namespace WidgetWorks.WebApi.Hosting; + +/// +/// Runs on a timer. +/// +/// This type is only the clock. All of the policy — how stale is stale, how many to take, what +/// releasing means — belongs to the handler, which is why that part can be tested without waiting +/// for a timer to tick. +/// +public sealed class ReservationSweeper( + IServiceScopeFactory scopes, + ReservationOptions options, + ILogger logger) : BackgroundService +{ + protected override async Task ExecuteAsync(CancellationToken stoppingToken) + { + if (!options.Enabled) + { + logger.LogInformation("Reservation sweep is disabled by configuration."); + return; + } + + var interval = TimeSpan.FromMinutes(Math.Max(1, options.SweepIntervalMinutes)); + logger.LogInformation( + "Reservation sweep running every {Interval}, releasing orders unsettled for {ExpireAfter} minutes.", + interval, + options.ExpireAfterMinutes); + + using var timer = new PeriodicTimer(interval); + + // Waits a full interval before the first pass on purpose. Startup is the worst moment to add + // database work, and nothing is so urgent that it cannot wait one interval. + while (await SafeWaitAsync(timer, stoppingToken)) + { + // A scope per tick, because the repositories are registered Scoped. A long-lived + // singleton holding a scoped dependency is the captive-dependency bug: it would pin one + // connection for the lifetime of the process. + using var scope = scopes.CreateScope(); + var handler = scope.ServiceProvider.GetRequiredService(); + + try + { + await handler.Handle(stoppingToken); + } + catch (OperationCanceledException) when (stoppingToken.IsCancellationRequested) + { + // Shutdown, not a fault. Leave the loop quietly. + break; + } + catch (Exception ex) + { + // One bad sweep must not end the loop: a transient database blip would otherwise + // silently stop reclaiming stock for the lifetime of the process, and nothing would + // report it. Logged, then the next tick tries again. + logger.LogError(ex, "Reservation sweep failed; the next pass will retry."); + } + } + } + + /// + /// Waits for the next tick, reporting false once the host is stopping. Wrapped because + /// throws on cancellation, and a cancelled wait + /// during shutdown is an ordinary ending rather than an error worth surfacing. + /// + private static async Task SafeWaitAsync(PeriodicTimer timer, CancellationToken ct) + { + try + { + return await timer.WaitForNextTickAsync(ct); + } + catch (OperationCanceledException) + { + return false; + } + } +} diff --git a/src/WidgetWorks.WebApi/Orders/OrderEndpoints.cs b/src/WidgetWorks.WebApi/Orders/OrderEndpoints.cs index 3d5c1de..e800227 100644 --- a/src/WidgetWorks.WebApi/Orders/OrderEndpoints.cs +++ b/src/WidgetWorks.WebApi/Orders/OrderEndpoints.cs @@ -1,73 +1,75 @@ -using System.Security.Claims; -using WidgetWorks.Application.Orders.Admin; -using WidgetWorks.Application.Orders.GetMine; -using WidgetWorks.Application.Orders.ListMine; -using WidgetWorks.Application.Orders.ListRecent; -using WidgetWorks.Application.Orders.Lookup; -using WidgetWorks.Application.Orders.UpdateStatus; -using WidgetWorks.WebApi.Authorization; - -namespace WidgetWorks.WebApi.Orders; - -public static class OrderEndpoints -{ - public static void MapOrderEndpoints(this IEndpointRouteBuilder routes) - { - // Guest order tracking by order number + email (anonymous). - routes.MapGet("/orders/lookup", async (string number, string email, GuestOrderLookupHandler handler, CancellationToken ct) => - { - var result = await handler.Handle(new GuestOrderLookupQuery(number, email), ct); - return result.IsSuccess ? Results.Ok(result.Value) : Results.NotFound(new { error = result.Error }); - }); - - var mine = routes.MapGroup("/orders").RequireAuthorization(); - - mine.MapGet("", async (ClaimsPrincipal principal, ListMyOrdersHandler handler, CancellationToken ct) => - { - if (UserId(principal) is not { } userId) - { - return Results.Unauthorized(); - } - - return Results.Ok(await handler.Handle(new ListMyOrdersQuery(userId), ct)); - }); - - mine.MapGet("/{id:guid}", async (Guid id, ClaimsPrincipal principal, GetMyOrderHandler handler, CancellationToken ct) => - { - if (UserId(principal) is not { } userId) - { - return Results.Unauthorized(); - } - - var result = await handler.Handle(new GetMyOrderQuery(userId, id), ct); - return result.IsSuccess ? Results.Ok(result.Value) : Results.NotFound(new { error = result.Error }); - }); - - // Admin/manager order management (ManageCatalog covers widgets, inventory, and orders). - var admin = routes.MapGroup("/admin/orders").RequireAuthorization(Policies.ManageCatalog); - - // Staff order list. Summary rows only — open one to load its items. - admin.MapGet("/", async (int? limit, ListRecentOrdersHandler handler, CancellationToken ct) => - { - var result = await handler.Handle(new ListRecentOrdersQuery(limit ?? 50), ct); - return Results.Ok(result); - }); - - admin.MapGet("/{id:guid}", async (Guid id, GetOrderByIdHandler handler, CancellationToken ct) => - { - var result = await handler.Handle(new GetOrderByIdQuery(id), ct); - return result.IsSuccess ? Results.Ok(result.Value) : Results.NotFound(new { error = result.Error }); - }); - - admin.MapPost("/{id:guid}/status", async (Guid id, UpdateStatusRequest body, UpdateOrderStatusHandler handler, CancellationToken ct) => - { - var result = await handler.Handle(new UpdateOrderStatusCommand(id, body.Status, body.TrackingNumber), ct); - return result.IsSuccess ? Results.Ok(result.Value) : Results.BadRequest(new { error = result.Error }); - }); - - static Guid? UserId(ClaimsPrincipal principal) - => Guid.TryParse(principal.FindFirst("sub")?.Value, out var id) ? id : null; - } - - public sealed record UpdateStatusRequest(string Status, string? TrackingNumber); -} +using System.Security.Claims; +using WidgetWorks.Application.Orders.Admin; +using WidgetWorks.Application.Orders.GetMine; +using WidgetWorks.Application.Orders.ListMine; +using WidgetWorks.Application.Orders.ListRecent; + +using WidgetWorks.Application.Orders.Lookup; +using WidgetWorks.Application.Orders.UpdateStatus; +using WidgetWorks.WebApi.Authorization; +using WidgetWorks.WebApi.RateLimiting; + +namespace WidgetWorks.WebApi.Orders; + +public static class OrderEndpoints +{ + public static void MapOrderEndpoints(this IEndpointRouteBuilder routes) + { + // Guest order tracking by order number + email (anonymous). + routes.MapGet("/orders/lookup", async (string number, string email, GuestOrderLookupHandler handler, CancellationToken ct) => + { + var result = await handler.Handle(new GuestOrderLookupQuery(number, email), ct); + return result.IsSuccess ? Results.Ok(result.Value) : Results.NotFound(new { error = result.Error }); + }).RequireRateLimiting(RateLimitPolicies.Lookup); + + var mine = routes.MapGroup("/orders").RequireAuthorization(); + + mine.MapGet("", async (ClaimsPrincipal principal, ListMyOrdersHandler handler, CancellationToken ct) => + { + if (UserId(principal) is not { } userId) + { + return Results.Unauthorized(); + } + + return Results.Ok(await handler.Handle(new ListMyOrdersQuery(userId), ct)); + }); + + mine.MapGet("/{id:guid}", async (Guid id, ClaimsPrincipal principal, GetMyOrderHandler handler, CancellationToken ct) => + { + if (UserId(principal) is not { } userId) + { + return Results.Unauthorized(); + } + + var result = await handler.Handle(new GetMyOrderQuery(userId, id), ct); + return result.IsSuccess ? Results.Ok(result.Value) : Results.NotFound(new { error = result.Error }); + }); + + // Admin/manager order management (ManageCatalog covers widgets, inventory, and orders). + var admin = routes.MapGroup("/admin/orders").RequireAuthorization(Policies.ManageCatalog); + + // Staff order list. Summary rows only — open one to load its items. + admin.MapGet("/", async (int? limit, ListRecentOrdersHandler handler, CancellationToken ct) => + { + var result = await handler.Handle(new ListRecentOrdersQuery(limit ?? 50), ct); + return Results.Ok(result); + }); + + admin.MapGet("/{id:guid}", async (Guid id, GetOrderByIdHandler handler, CancellationToken ct) => + { + var result = await handler.Handle(new GetOrderByIdQuery(id), ct); + return result.IsSuccess ? Results.Ok(result.Value) : Results.NotFound(new { error = result.Error }); + }); + + admin.MapPost("/{id:guid}/status", async (Guid id, UpdateStatusRequest body, UpdateOrderStatusHandler handler, CancellationToken ct) => + { + var result = await handler.Handle(new UpdateOrderStatusCommand(id, body.Status, body.TrackingNumber), ct); + return result.IsSuccess ? Results.Ok(result.Value) : Results.BadRequest(new { error = result.Error }); + }); + + static Guid? UserId(ClaimsPrincipal principal) + => Guid.TryParse(principal.FindFirst("sub")?.Value, out var id) ? id : null; + } + + public sealed record UpdateStatusRequest(string Status, string? TrackingNumber); +} diff --git a/src/WidgetWorks.WebApi/Program.cs b/src/WidgetWorks.WebApi/Program.cs index 06c13bc..0591570 100644 --- a/src/WidgetWorks.WebApi/Program.cs +++ b/src/WidgetWorks.WebApi/Program.cs @@ -17,6 +17,9 @@ using WidgetWorks.WebApi.Orders; using WidgetWorks.WebApi.Payments; using WidgetWorks.WebApi.Security; +using WidgetWorks.Application.Checkout.ReleaseStale; +using WidgetWorks.WebApi.Hosting; +using WidgetWorks.WebApi.RateLimiting; using WidgetWorks.WebApi.TwoFactor; var builder = WebApplication.CreateBuilder(args); @@ -24,6 +27,15 @@ builder.Services.AddApplication(); builder.Services.AddInfrastructure(builder.Configuration); builder.Services.AddOpenApi(); +builder.Services.AddWidgetWorksRateLimiting(builder.Configuration); + +// Stock held by an order whose payment never settles is returned to sale on a timer. Options are +// bound here so the sweep can be retuned, or turned off for a host that should not run background +// work, without a code change. +var reservationOptions = new ReservationOptions(); +builder.Configuration.GetSection("Reservations").Bind(reservationOptions); +builder.Services.AddSingleton(reservationOptions); +builder.Services.AddHostedService(); // CORS for the browser SPA (origins from config; sensible localhost defaults for dev). const string SpaCorsPolicy = "spa"; @@ -120,6 +132,10 @@ await users.GetSecurityStampAsync(userId, context.HttpContext.RequestAborted) is } app.UseCors(SpaCorsPolicy); + +// Ahead of authentication on purpose: a throttled request is rejected before the app spends +// work validating credentials, which is what keeps a guessing flood cheap to absorb. +app.UseRateLimiter(); app.UseAuthentication(); app.UseAuthorization(); diff --git a/src/WidgetWorks.WebApi/RateLimiting/ClientAddress.cs b/src/WidgetWorks.WebApi/RateLimiting/ClientAddress.cs new file mode 100644 index 0000000..33c8ac4 --- /dev/null +++ b/src/WidgetWorks.WebApi/RateLimiting/ClientAddress.cs @@ -0,0 +1,63 @@ +using Microsoft.AspNetCore.Http; + +namespace WidgetWorks.WebApi.RateLimiting; + +/// +/// Works out which caller a request belongs to, so throttling partitions by client rather than by +/// process. Kept separate from the limiter wiring because this is the part with rules worth testing: +/// everything else is framework configuration. +/// +public static class ClientAddress +{ + /// Partition used when no address can be determined, so those callers share a budget. + public const string Unknown = "unknown"; + + /// + /// Resolves the partition key for . + /// + /// X-Forwarded-For is a client-supplied header and is read only when + /// says a trusted proxy is in front. Its leftmost entry is + /// the original client; entries to the right are the proxies it passed through. + /// + public static string Resolve(HttpContext context, bool trustForwardedFor) + { + ArgumentNullException.ThrowIfNull(context); + + if (trustForwardedFor) + { + var forwarded = FirstForwardedFor(context.Request.Headers["X-Forwarded-For"]); + if (forwarded is not null) + { + return forwarded; + } + } + + return context.Connection.RemoteIpAddress?.ToString() ?? Unknown; + } + + /// + /// Takes the leftmost address from an X-Forwarded-For chain, which may arrive as one + /// comma-separated header or as several repeated headers. Returns null when nothing usable is + /// present so the caller can fall back to the connection address. + /// + private static string? FirstForwardedFor(IEnumerable headerValues) + { + foreach (var value in headerValues) + { + if (string.IsNullOrWhiteSpace(value)) + { + continue; + } + + foreach (var candidate in value.Split(',', StringSplitOptions.RemoveEmptyEntries | StringSplitOptions.TrimEntries)) + { + if (candidate.Length > 0) + { + return candidate; + } + } + } + + return null; + } +} diff --git a/src/WidgetWorks.WebApi/RateLimiting/RateLimitOptions.cs b/src/WidgetWorks.WebApi/RateLimiting/RateLimitOptions.cs new file mode 100644 index 0000000..8f53258 --- /dev/null +++ b/src/WidgetWorks.WebApi/RateLimiting/RateLimitOptions.cs @@ -0,0 +1,46 @@ +namespace WidgetWorks.WebApi.RateLimiting; + +/// +/// Throttling budgets, bound from the RateLimiting configuration section so an operator can +/// tighten a limit during an incident without a redeploy. Defaults are deliberately generous enough +/// that a real customer never meets them and tight enough that scripted abuse does. +/// +public sealed class RateLimitOptions +{ + /// + /// Whether an X-Forwarded-For header may be believed when identifying the caller. + /// + /// This is the setting that decides whether the limiter works at all behind a reverse proxy. + /// Left false while hosted behind one, every request appears to originate from the proxy, all + /// callers collapse into a single partition, and the limiter turns into a global cap that the + /// first busy minute trips for everybody — a self-inflicted outage. Set true only when a proxy + /// you control is guaranteed to be in front, because a client can otherwise forge the header + /// and mint itself unlimited partitions. + /// + public bool TrustForwardedFor { get; set; } + + /// Sign-in, registration and password-reset requests. + public RateLimitBudget Auth { get; set; } = new() { PermitLimit = 20, WindowSeconds = 60 }; + + /// Order placement. Guards card testing and the inventory-reservation abuse path. + public RateLimitBudget Checkout { get; set; } = new() { PermitLimit = 8, WindowSeconds = 60 }; + + /// Guest order lookup, which confirms an order number against an email. + public RateLimitBudget Lookup { get; set; } = new() { PermitLimit = 10, WindowSeconds = 60 }; +} + +/// A fixed-window budget: requests per . +public sealed class RateLimitBudget +{ + public int PermitLimit { get; set; } = 10; + + public int WindowSeconds { get; set; } = 60; + + /// + /// Coerces the configured pair into a usable window. A zero or negative value from configuration + /// would otherwise throw deep inside the limiter at first request rather than at startup, so it + /// falls back to the property default instead of taking the process down. + /// + public (int Permits, TimeSpan Window) Resolve() + => (PermitLimit > 0 ? PermitLimit : 10, TimeSpan.FromSeconds(WindowSeconds > 0 ? WindowSeconds : 60)); +} diff --git a/src/WidgetWorks.WebApi/RateLimiting/RateLimitingExtensions.cs b/src/WidgetWorks.WebApi/RateLimiting/RateLimitingExtensions.cs new file mode 100644 index 0000000..0313f4a --- /dev/null +++ b/src/WidgetWorks.WebApi/RateLimiting/RateLimitingExtensions.cs @@ -0,0 +1,73 @@ +using System.Globalization; +using System.Threading.RateLimiting; +using Microsoft.AspNetCore.RateLimiting; + +namespace WidgetWorks.WebApi.RateLimiting; + +/// Named throttling policies, referenced by endpoints the way authorization policies are. +public static class RateLimitPolicies +{ + /// Sign-in, registration, password reset — the credential-guessing surface. + public const string Auth = "auth"; + + /// Order placement. + public const string Checkout = "checkout"; + + /// Guest order lookup. + public const string Lookup = "lookup"; +} + +public static class RateLimitingExtensions +{ + /// + /// Registers the throttling policies. + /// + /// Only the endpoints that are both anonymous and abusable carry a policy. There is deliberately + /// no global limiter: a catalogue page issues several requests in a burst, so a global cap would + /// throttle ordinary browsing while doing nothing an endpoint policy does not already do. + /// + public static IServiceCollection AddWidgetWorksRateLimiting(this IServiceCollection services, IConfiguration configuration) + { + var options = new RateLimitOptions(); + configuration.GetSection("RateLimiting").Bind(options); + services.AddSingleton(options); + + services.AddRateLimiter(limiter => + { + limiter.AddPolicy(RateLimitPolicies.Auth, ctx => Partition(ctx, options, options.Auth)); + limiter.AddPolicy(RateLimitPolicies.Checkout, ctx => Partition(ctx, options, options.Checkout)); + limiter.AddPolicy(RateLimitPolicies.Lookup, ctx => Partition(ctx, options, options.Lookup)); + + limiter.OnRejected = async (context, ct) => + { + context.HttpContext.Response.StatusCode = StatusCodes.Status429TooManyRequests; + + // Tell the caller when to come back. A well-behaved client backs off instead of + // retrying into the wall, and an honest one that hit the limit by accident recovers + // without a support ticket. + if (context.Lease.TryGetMetadata(MetadataName.RetryAfter, out var retryAfter)) + { + context.HttpContext.Response.Headers.RetryAfter = + ((int)retryAfter.TotalSeconds).ToString(CultureInfo.InvariantCulture); + } + + await context.HttpContext.Response.WriteAsJsonAsync( + new { error = "Too many requests. Please wait a moment and try again." }, ct); + }; + }); + + return services; + } + + /// + /// One fixed window per caller. Fixed rather than sliding because the budgets here are small and + /// the extra per-partition state a sliding window keeps is not worth paying for at this size. + /// + private static RateLimitPartition Partition(HttpContext context, RateLimitOptions options, RateLimitBudget budget) + { + var (permits, window) = budget.Resolve(); + return RateLimitPartition.GetFixedWindowLimiter( + ClientAddress.Resolve(context, options.TrustForwardedFor), + _ => new FixedWindowRateLimiterOptions { PermitLimit = permits, Window = window }); + } +} diff --git a/tests/WidgetWorks.ApiTests/ApiFixture.cs b/tests/WidgetWorks.ApiTests/ApiFixture.cs index 352addc..35aa0ee 100644 --- a/tests/WidgetWorks.ApiTests/ApiFixture.cs +++ b/tests/WidgetWorks.ApiTests/ApiFixture.cs @@ -29,6 +29,7 @@ public sealed class ApiFixture : IAsyncLifetime private const string SigningKey = "test-signing-key-api-suite-0123456789abcdef"; private string _adminConnectionString = DefaultAdmin; + private string _connectionString = DefaultAdmin; public const string AdminEmail = "api-admin@widgetworks.test"; public const string ManagerEmail = "api-manager@widgetworks.test"; @@ -57,10 +58,24 @@ public async Task InitializeAsync() // UseSetting lands in host configuration, which minimal hosting folds into // builder.Configuration BEFORE Program's own code reads it; ConfigureAppConfiguration // callbacks would run too late for the Jwt options Program binds during startup. - Factory = new WebApplicationFactory().WithWebHostBuilder(host => + _connectionString = connectionString; + Factory = BuildFactory(); + + // First client boots the host: migrations run and the demo catalog is seeded. + using var client = Factory.CreateClient(); + var health = await client.GetAsync("/health"); + health.EnsureSuccessStatusCode(); + } + + /// + /// The host every test shares. Settings go through UseSetting because minimal hosting folds + /// host configuration into builder.Configuration before Program reads it. + /// + private WebApplicationFactory BuildFactory(params (string Key, string Value)[] overrides) + => new WebApplicationFactory().WithWebHostBuilder(host => { host.UseEnvironment(Environments.Production); // no OpenAPI/Scalar noise in tests - host.UseSetting("ConnectionStrings:WidgetWorks", connectionString); + host.UseSetting("ConnectionStrings:WidgetWorks", _connectionString); host.UseSetting("Jwt:SigningKey", SigningKey); host.UseSetting("Seed:DemoAdminEmail", AdminEmail); host.UseSetting("Seed:DemoAdminPassword", Password); @@ -68,13 +83,28 @@ public async Task InitializeAsync() host.UseSetting("Seed:DemoManagerPassword", Password); host.UseSetting("Seed:DemoCustomerEmail", CustomerEmail); host.UseSetting("Seed:DemoCustomerPassword", Password); + + // Every request from the test server arrives with no remote address, so all of them + // share one throttling partition and the suite would exhaust a realistic budget + // between tests. Raised here rather than weakened in the production defaults -- + // RateLimitingApiTests overrides these back down to prove the limiter really rejects. + host.UseSetting("RateLimiting:Auth:PermitLimit", "100000"); + host.UseSetting("RateLimiting:Checkout:PermitLimit", "100000"); + host.UseSetting("RateLimiting:Lookup:PermitLimit", "100000"); + + foreach (var (key, value) in overrides) + { + host.UseSetting(key, value); + } }); - // First client boots the host: migrations run and the demo catalog is seeded. - using var client = Factory.CreateClient(); - var health = await client.GetAsync("/health"); - health.EnsureSuccessStatusCode(); - } + /// + /// A second host on the same database with configuration overrides applied — for tests that + /// need to exercise behaviour the shared host deliberately turns down, such as throttling. + /// The caller owns the returned factory and should dispose it. + /// + public WebApplicationFactory FactoryWith(params (string Key, string Value)[] overrides) + => BuildFactory(overrides); public async Task DisposeAsync() { diff --git a/tests/WidgetWorks.ApiTests/ClientAddressTests.cs b/tests/WidgetWorks.ApiTests/ClientAddressTests.cs new file mode 100644 index 0000000..06f2058 --- /dev/null +++ b/tests/WidgetWorks.ApiTests/ClientAddressTests.cs @@ -0,0 +1,91 @@ +using System.Net; +using Microsoft.AspNetCore.Http; +using WidgetWorks.WebApi.RateLimiting; +using Xunit; + +namespace WidgetWorks.ApiTests; + +/// +/// Which caller a request is attributed to. This is the whole correctness of throttling: get the +/// partition key wrong and the limiter either caps everybody together or caps nobody at all. +/// +public class ClientAddressTests +{ + private static HttpContext Request(string? remoteIp, params string[] forwardedFor) + { + var context = new DefaultHttpContext(); + if (remoteIp is not null) + { + context.Connection.RemoteIpAddress = IPAddress.Parse(remoteIp); + } + + if (forwardedFor.Length > 0) + { + context.Request.Headers["X-Forwarded-For"] = forwardedFor; + } + + return context; + } + + [Fact] + public void The_connection_address_identifies_the_caller_by_default() + => Assert.Equal("203.0.113.7", ClientAddress.Resolve(Request("203.0.113.7"), trustForwardedFor: false)); + + [Fact] + public void A_forwarded_header_is_ignored_unless_a_proxy_is_trusted() + { + // Anyone can send this header. Believing it without a proxy in front would let a caller + // mint a fresh partition per request and opt out of throttling entirely. + var context = Request("203.0.113.7", "198.51.100.9"); + + Assert.Equal("203.0.113.7", ClientAddress.Resolve(context, trustForwardedFor: false)); + } + + [Fact] + public void A_trusted_proxy_reveals_the_original_client() + { + var context = Request("10.0.0.1", "198.51.100.9"); + + Assert.Equal("198.51.100.9", ClientAddress.Resolve(context, trustForwardedFor: true)); + } + + [Fact] + public void The_leftmost_entry_in_a_forwarded_chain_is_the_client() + { + // Proxies append, so the client is first and every hop after it is infrastructure. + var context = Request("10.0.0.1", "198.51.100.9, 10.0.0.8, 10.0.0.1"); + + Assert.Equal("198.51.100.9", ClientAddress.Resolve(context, trustForwardedFor: true)); + } + + [Fact] + public void A_chain_split_across_repeated_headers_is_read_the_same_way() + { + var context = Request("10.0.0.1", "198.51.100.9", "10.0.0.8"); + + Assert.Equal("198.51.100.9", ClientAddress.Resolve(context, trustForwardedFor: true)); + } + + [Theory] + [InlineData("")] + [InlineData(" ")] + [InlineData(",")] + public void An_empty_forwarded_header_falls_back_to_the_connection(string headerValue) + { + var context = Request("203.0.113.7", headerValue); + + Assert.Equal("203.0.113.7", ClientAddress.Resolve(context, trustForwardedFor: true)); + } + + [Fact] + public void Callers_with_no_determinable_address_share_one_budget() + { + // Failing closed: unattributable traffic is throttled together rather than exempted. + Assert.Equal(ClientAddress.Unknown, ClientAddress.Resolve(Request(null), trustForwardedFor: false)); + Assert.Equal(ClientAddress.Unknown, ClientAddress.Resolve(Request(null), trustForwardedFor: true)); + } + + [Fact] + public void A_null_context_is_a_programming_error_not_a_silent_pass() + => Assert.Throws(() => ClientAddress.Resolve(null!, trustForwardedFor: false)); +} diff --git a/tests/WidgetWorks.ApiTests/RateLimitingApiTests.cs b/tests/WidgetWorks.ApiTests/RateLimitingApiTests.cs new file mode 100644 index 0000000..b0f60dd --- /dev/null +++ b/tests/WidgetWorks.ApiTests/RateLimitingApiTests.cs @@ -0,0 +1,74 @@ +using System.Net; +using System.Net.Http.Json; +using Xunit; + +namespace WidgetWorks.ApiTests; + +/// +/// Proves the throttling actually engages. The shared fixture raises every budget so the suite can +/// run, which would otherwise leave this control shipped but unexercised — so these tests stand up +/// their own host with a budget of two and drive straight past it. +/// +[Collection(ApiCollection.Name)] +public sealed class RateLimitingApiTests(ApiFixture fixture) +{ + [Fact] + public async Task A_flood_of_sign_in_attempts_is_rejected_once_the_budget_is_spent() + { + using var factory = fixture.FactoryWith(("RateLimiting:Auth:PermitLimit", "2")); + using var client = factory.CreateClient(); + + var attempt = new { Email = "nobody@widgetworks.test", Password = "WrongPassword!1" }; + + // Two are allowed. Whether they succeed is beside the point: a wrong password is still a + // request, which is exactly why throttling has to sit in front of authentication. + var first = await client.PostAsJsonAsync("/auth/login", attempt); + var second = await client.PostAsJsonAsync("/auth/login", attempt); + Assert.NotEqual(HttpStatusCode.TooManyRequests, first.StatusCode); + Assert.NotEqual(HttpStatusCode.TooManyRequests, second.StatusCode); + + var third = await client.PostAsJsonAsync("/auth/login", attempt); + + Assert.Equal(HttpStatusCode.TooManyRequests, third.StatusCode); + } + + [Fact] + public async Task A_rejected_caller_is_told_how_long_to_wait() + { + using var factory = fixture.FactoryWith(("RateLimiting:Auth:PermitLimit", "1")); + using var client = factory.CreateClient(); + + var attempt = new { Email = "nobody@widgetworks.test", Password = "WrongPassword!1" }; + await client.PostAsJsonAsync("/auth/login", attempt); + var rejected = await client.PostAsJsonAsync("/auth/login", attempt); + + Assert.Equal(HttpStatusCode.TooManyRequests, rejected.StatusCode); + + // Retry-After turns a wall into a queue: a well-behaved client backs off instead of + // hammering, and an honest caller who tripped the limit recovers without asking anyone. + Assert.True(rejected.Headers.TryGetValues("Retry-After", out var retryAfter)); + Assert.True(int.TryParse(retryAfter!.First(), out var seconds) && seconds > 0); + + var body = await rejected.Content.ReadFromJsonAsync(); + Assert.False(string.IsNullOrWhiteSpace(body!.Error)); + } + + [Fact] + public async Task Throttling_is_scoped_to_the_policy_not_the_whole_api() + { + using var factory = fixture.FactoryWith(("RateLimiting:Auth:PermitLimit", "1")); + using var client = factory.CreateClient(); + + var attempt = new { Email = "nobody@widgetworks.test", Password = "WrongPassword!1" }; + await client.PostAsJsonAsync("/auth/login", attempt); + var rejected = await client.PostAsJsonAsync("/auth/login", attempt); + Assert.Equal(HttpStatusCode.TooManyRequests, rejected.StatusCode); + + // Browsing must keep working while the auth budget is spent. A global limiter would fail + // this, which is the reason there isn't one. + var browsing = await client.GetAsync("/catalog/widgets?pageSize=5"); + Assert.Equal(HttpStatusCode.OK, browsing.StatusCode); + } + + private sealed record ErrorBody(string Error); +} diff --git a/tests/WidgetWorks.IntegrationTests/CatalogAndAuthRepositoryTests.cs b/tests/WidgetWorks.IntegrationTests/CatalogAndAuthRepositoryTests.cs index 0b84ae6..f767dc7 100644 --- a/tests/WidgetWorks.IntegrationTests/CatalogAndAuthRepositoryTests.cs +++ b/tests/WidgetWorks.IntegrationTests/CatalogAndAuthRepositoryTests.cs @@ -25,7 +25,7 @@ public class CatalogAndAuthRepositoryTests(PostgresFixture db) private static string Unique(string prefix) => prefix + Guid.NewGuid().ToString("N")[..10]; - private async Task GivenWidget(int onHand = 10, bool active = true, string? name = null) + private async Task GivenWidget(int onHand = 10, bool active = true, string? name = null, decimal price = 12.5m, int reserved = 0) { var widget = new Widget { @@ -33,9 +33,9 @@ private async Task GivenWidget(int onHand = 10, bool active = true, stri Sku = Unique("SKU-").ToUpperInvariant(), Name = name ?? Unique("Widget "), Description = "Integration fixture.", - Price = 12.5m, + Price = price, QuantityOnHand = onHand, - QuantityReserved = 0, + QuantityReserved = reserved, IsActive = active, CreatedAt = Now, UpdatedAt = Now, @@ -533,4 +533,104 @@ public async Task Audit_entries_are_written_for_a_user_and_anonymously() // No read side on the port; the assertion is that neither write throws or violates the FK. Assert.NotNull(await Users.GetByIdAsync(user.Id, CancellationToken.None)); } + + // ---- category narrowing and ordering, moved here from the browser ------------------------- + // These replace the refine() unit tests: the behaviour is SQL now, so this is where it is + // proven. Every case scopes itself with a unique token so a shared database stays usable. + + [Fact] + public async Task A_category_narrows_the_listing_to_its_members() + { + var token = Unique("cat"); + await GivenWidget(name: $"Mega Widget Block {token}"); + await GivenWidget(name: $"Mega Widget Hub {token}"); + await GivenWidget(name: $"Mini Widget Block {token}"); + + var query = new WidgetQuery($"{token}", ActiveOnly: true, 1, 50, Category: "mega"); + var results = await Widgets.SearchAsync(query, CancellationToken.None); + + Assert.Equal(2, results.Count); + Assert.All(results, w => Assert.Contains("Mega", w.Name, StringComparison.Ordinal)); + // The count has to agree with the page, or the storefront reports a total it never shows. + Assert.Equal(2, await Widgets.CountAsync(query, CancellationToken.None)); + } + + [Fact] + public async Task A_search_and_a_category_narrow_together_rather_than_either_or() + { + var token = Unique("both"); + await GivenWidget(name: $"Mega Widget Turbine {token}"); + await GivenWidget(name: $"Mega Widget Block {token}"); + await GivenWidget(name: $"Mini Widget Turbine {token}"); + + var results = await Widgets.SearchAsync( + new WidgetQuery($"Turbine {token}", ActiveOnly: true, 1, 50, Category: "mega"), + CancellationToken.None); + + // "Turbine" within Mega means both conditions, not their union. + Assert.Single(results); + Assert.Contains("Mega Widget Turbine", results[0].Name, StringComparison.Ordinal); + } + + [Fact] + public async Task No_category_leaves_the_listing_alone() + { + var token = Unique("nocat"); + await GivenWidget(name: $"Mega Widget {token}"); + await GivenWidget(name: $"Mini Widget {token}"); + + var results = await Widgets.SearchAsync( + new WidgetQuery(token, ActiveOnly: true, 1, 50, Category: null), + CancellationToken.None); + + Assert.Equal(2, results.Count); + } + + [Fact] + public async Task Price_sorts_run_in_both_directions() + { + var token = Unique("price"); + await GivenWidget(name: $"B {token}", price: 30m); + await GivenWidget(name: $"A {token}", price: 10m); + await GivenWidget(name: $"C {token}", price: 20m); + + var ascending = await Widgets.SearchAsync( + new WidgetQuery(token, ActiveOnly: true, 1, 50, Sort: WidgetSort.PriceAscending), CancellationToken.None); + var descending = await Widgets.SearchAsync( + new WidgetQuery(token, ActiveOnly: true, 1, 50, Sort: WidgetSort.PriceDescending), CancellationToken.None); + + Assert.Equal([10m, 20m, 30m], ascending.Select(w => w.Price)); + Assert.Equal([30m, 20m, 10m], descending.Select(w => w.Price)); + } + + [Fact] + public async Task Featured_leads_with_what_can_actually_be_bought() + { + var token = Unique("feat"); + await GivenWidget(name: $"A sold out {token}", onHand: 5, reserved: 5); + await GivenWidget(name: $"B in stock {token}", onHand: 5); + + var results = await Widgets.SearchAsync( + new WidgetQuery(token, ActiveOnly: true, 1, 50, Sort: WidgetSort.Featured), CancellationToken.None); + + // Alphabetically the sold-out one comes first; availability outranks the name. + Assert.Contains("in stock", results[0].Name, StringComparison.Ordinal); + Assert.Contains("sold out", results[1].Name, StringComparison.Ordinal); + } + + [Fact] + public async Task An_unknown_sort_falls_back_instead_of_reaching_the_database() + { + var token = Unique("inject"); + await GivenWidget(name: $"A {token}"); + await GivenWidget(name: $"B {token}"); + + // The value is mapped through a fixed set, never interpolated, so even this is inert. + var results = await Widgets.SearchAsync( + new WidgetQuery(token, ActiveOnly: true, 1, 50, Sort: "price; drop table widgets"), + CancellationToken.None); + + Assert.Equal(2, results.Count); + Assert.Contains($"A {token}", results[0].Name, StringComparison.Ordinal); + } } diff --git a/tests/WidgetWorks.IntegrationTests/OrderRepositoryTests.cs b/tests/WidgetWorks.IntegrationTests/OrderRepositoryTests.cs index c558462..2cfb7f0 100644 --- a/tests/WidgetWorks.IntegrationTests/OrderRepositoryTests.cs +++ b/tests/WidgetWorks.IntegrationTests/OrderRepositoryTests.cs @@ -246,6 +246,60 @@ public async Task Updating_status_stores_the_tracking_number() Assert.Equal("1Z-TRACK", stored.TrackingNumber); } + [Fact] + public async Task A_repeated_payment_failure_releases_the_reservation_only_once() + { + var widget = await GivenWidget(onHand: 10); + var order = OrderFor(widget, 3); + await Orders.TryPlaceAsync(order, CancellationToken.None); + + var first = await Orders.MarkPaymentFailedAsync(order, "declined", Now, CancellationToken.None); + var second = await Orders.MarkPaymentFailedAsync(order, "declined", Now, CancellationToken.None); + + Assert.True(first); + // The row, not the caller, decides. A redelivered webhook is declined rather than + // decrementing quantity_reserved a second time and eating another order's stock. + Assert.False(second); + + var after = await Widgets.GetByIdAsync(widget.Id, CancellationToken.None); + Assert.Equal(10, after!.QuantityOnHand); + Assert.Equal(0, after.QuantityReserved); + } + + [Fact] + public async Task A_late_settlement_cannot_overwrite_an_order_that_already_failed() + { + var widget = await GivenWidget(onHand: 10); + var order = OrderFor(widget, 2); + await Orders.TryPlaceAsync(order, CancellationToken.None); + await Orders.MarkPaymentFailedAsync(order, "declined", Now, CancellationToken.None); + + var settled = await Orders.MarkPaidAsync(order.Id, "Mock", "ref-1", Now.AddMinutes(1), CancellationToken.None); + + // Marking it paid here would claim money for an order whose stock is already back on sale. + Assert.False(settled); + var stored = await Orders.GetByIdAsync(order.Id, CancellationToken.None); + Assert.Equal(OrderStatus.PaymentFailed, stored!.Status); + } + + [Fact] + public async Task A_settled_order_cannot_be_failed_by_a_stale_event() + { + var widget = await GivenWidget(onHand: 10); + var order = OrderFor(widget, 2); + await Orders.TryPlaceAsync(order, CancellationToken.None); + await Orders.MarkPaidAsync(order.Id, "Mock", "ref-2", Now, CancellationToken.None); + + var failed = await Orders.MarkPaymentFailedAsync(order, "stale", Now.AddMinutes(1), CancellationToken.None); + + Assert.False(failed); + var stored = await Orders.GetByIdAsync(order.Id, CancellationToken.None); + Assert.Equal(OrderStatus.Paid, stored!.Status); + // The reservation must survive: the goods are sold and still owed to this order. + var after = await Widgets.GetByIdAsync(widget.Id, CancellationToken.None); + Assert.Equal(2, after!.QuantityReserved); + } + [Fact] public async Task Shipping_turns_the_reservation_into_a_real_decrement() { diff --git a/tests/WidgetWorks.UnitTests/CartAccessTests.cs b/tests/WidgetWorks.UnitTests/CartAccessTests.cs new file mode 100644 index 0000000..96e1bb9 --- /dev/null +++ b/tests/WidgetWorks.UnitTests/CartAccessTests.cs @@ -0,0 +1,95 @@ +using WidgetWorks.Application.Carts; +using WidgetWorks.Application.Carts.GetCart; +using WidgetWorks.Domain.Carts; +using WidgetWorks.UnitTests.Fakes; +using Xunit; + +namespace WidgetWorks.UnitTests; + +/// +/// The cart authorization rule and its effect at the boundary. A cart id is a capability, so these +/// tests pin the line between "anyone holding the id" (guest) and "only the owner" (claimed). +/// +public class CartAccessTests +{ + private static Cart CartOwnedBy(Guid? owner) => new() { Id = Guid.NewGuid(), UserId = owner }; + + [Fact] + public void A_guest_cart_is_reachable_by_anyone_holding_its_id() + { + var cart = CartOwnedBy(null); + + // This is what lets a visitor shop before signing in. + Assert.True(CartAccess.IsPermitted(cart, null)); + Assert.True(CartAccess.IsPermitted(cart, Guid.NewGuid())); + } + + [Fact] + public void A_claimed_cart_is_reachable_only_by_its_owner() + { + var owner = Guid.NewGuid(); + var cart = CartOwnedBy(owner); + + Assert.True(CartAccess.IsPermitted(cart, owner)); + Assert.False(CartAccess.IsPermitted(cart, Guid.NewGuid())); + Assert.False(CartAccess.IsPermitted(cart, null)); + } + + [Fact] + public async Task Reading_someone_elses_cart_is_refused_and_says_only_not_found() + { + var carts = new InMemoryCartRepository(); + var widgets = new InMemoryWidgetRepository(); + var owner = Guid.NewGuid(); + var cart = await carts.CreateAsync(owner, CancellationToken.None); + + var result = await new GetCartHandler(carts, widgets) + .Handle(new GetCartQuery(cart.Id, Guid.NewGuid()), CancellationToken.None); + + Assert.False(result.IsSuccess); + // The wording matters as much as the refusal: a distinct "forbidden" would confirm that a + // cart with this id exists, which is exactly what a guesser wants to learn. + Assert.Equal("Cart not found.", result.Error); + } + + [Fact] + public async Task An_anonymous_caller_cannot_reach_a_claimed_cart() + { + var carts = new InMemoryCartRepository(); + var widgets = new InMemoryWidgetRepository(); + var cart = await carts.CreateAsync(Guid.NewGuid(), CancellationToken.None); + + var result = await new GetCartHandler(carts, widgets) + .Handle(new GetCartQuery(cart.Id, null), CancellationToken.None); + + Assert.False(result.IsSuccess); + } + + [Fact] + public async Task The_owner_can_still_read_their_own_cart() + { + var carts = new InMemoryCartRepository(); + var widgets = new InMemoryWidgetRepository(); + var owner = Guid.NewGuid(); + var cart = await carts.CreateAsync(owner, CancellationToken.None); + + var result = await new GetCartHandler(carts, widgets) + .Handle(new GetCartQuery(cart.Id, owner), CancellationToken.None); + + Assert.True(result.IsSuccess); + } + + [Fact] + public async Task A_guest_cart_still_works_for_an_anonymous_shopper() + { + var carts = new InMemoryCartRepository(); + var widgets = new InMemoryWidgetRepository(); + var cart = await carts.CreateAsync(null, CancellationToken.None); + + var result = await new GetCartHandler(carts, widgets) + .Handle(new GetCartQuery(cart.Id, null), CancellationToken.None); + + // The whole guest checkout flow depends on this staying true. + Assert.True(result.IsSuccess); + } +} diff --git a/tests/WidgetWorks.UnitTests/CartHandlerTests.cs b/tests/WidgetWorks.UnitTests/CartHandlerTests.cs index d72e9ea..6874e0e 100644 --- a/tests/WidgetWorks.UnitTests/CartHandlerTests.cs +++ b/tests/WidgetWorks.UnitTests/CartHandlerTests.cs @@ -75,7 +75,7 @@ public async Task Update_to_zero_removes_line() var created = await add.Handle(new AddCartItemCommand(null, null, widget.Id, 2), CancellationToken.None); var update = new UpdateCartItemHandler(carts, widgets, Clock()); - var result = await update.Handle(new UpdateCartItemCommand(created.Value!.Id, widget.Id, 0), CancellationToken.None); + var result = await update.Handle(new UpdateCartItemCommand(created.Value!.Id, widget.Id, 0, null), CancellationToken.None); Assert.True(result.IsSuccess); Assert.Empty(result.Value!.Items); diff --git a/tests/WidgetWorks.UnitTests/Fakes.cs b/tests/WidgetWorks.UnitTests/Fakes.cs index d9520f1..95718a3 100644 --- a/tests/WidgetWorks.UnitTests/Fakes.cs +++ b/tests/WidgetWorks.UnitTests/Fakes.cs @@ -199,29 +199,47 @@ public Task TryPlaceAsync(Order order, CancellationToken ct) return Task.FromResult(true); } - public Task MarkAwaitingPaymentAsync(Guid orderId, string provider, string reference, DateTimeOffset now, CancellationToken ct) + public Task MarkAwaitingPaymentAsync(Guid orderId, string provider, string reference, DateTimeOffset now, CancellationToken ct) { var order = Orders.First(o => o.Id == orderId); + if (order.Status != OrderStatus.Pending) + { + return Task.FromResult(false); + } + order.Status = OrderStatus.AwaitingPayment; order.PaymentProvider = provider; order.PaymentReference = reference; order.UpdatedAt = now; - return Task.CompletedTask; + return Task.FromResult(true); } - public Task MarkPaidAsync(Guid orderId, string provider, string reference, DateTimeOffset now, CancellationToken ct) + public Task MarkPaidAsync(Guid orderId, string provider, string reference, DateTimeOffset now, CancellationToken ct) { var order = Orders.First(o => o.Id == orderId); + if (!AwaitingSettlement(order.Status)) + { + return Task.FromResult(false); + } + order.Status = OrderStatus.Paid; order.PaymentProvider = provider; order.PaymentReference = reference; order.UpdatedAt = now; - return Task.CompletedTask; + return Task.FromResult(true); } - public Task MarkPaymentFailedAsync(Order order, string reason, DateTimeOffset now, CancellationToken ct) + public Task MarkPaymentFailedAsync(Order order, string reason, DateTimeOffset now, CancellationToken ct) { var stored = Orders.First(o => o.Id == order.Id); + + // Mirrors the repository's compare-and-set: a second delivery of the same failure is + // declined, so the reservation is released exactly once. + if (!AwaitingSettlement(stored.Status)) + { + return Task.FromResult(false); + } + stored.Status = OrderStatus.PaymentFailed; stored.UpdatedAt = now; foreach (var item in order.Items) @@ -232,9 +250,12 @@ public Task MarkPaymentFailedAsync(Order order, string reason, DateTimeOffset no } } - return Task.CompletedTask; + return Task.FromResult(true); } + private static bool AwaitingSettlement(string status) + => status is OrderStatus.Pending or OrderStatus.AwaitingPayment; + public Task UpdateStatusAsync(Order order, DateTimeOffset now, CancellationToken ct) { var stored = Orders.First(o => o.Id == order.Id); @@ -265,6 +286,13 @@ public Task UpdateStatusAsync(Order order, DateTimeOffset now, CancellationToken return Task.CompletedTask; } + public Task> GetStaleAwaitingPaymentAsync(DateTimeOffset cutoff, int limit, CancellationToken ct) + => Task.FromResult>(Orders + .Where(o => o.Status == OrderStatus.AwaitingPayment && o.UpdatedAt < cutoff) + .OrderBy(o => o.UpdatedAt) + .Take(limit) + .ToList()); + public Task GetByIdAsync(Guid id, CancellationToken ct) => Task.FromResult(Orders.FirstOrDefault(o => o.Id == id)); diff --git a/tests/WidgetWorks.UnitTests/OrderQueryTests.cs b/tests/WidgetWorks.UnitTests/OrderQueryTests.cs index 261b8e9..a24b98f 100644 --- a/tests/WidgetWorks.UnitTests/OrderQueryTests.cs +++ b/tests/WidgetWorks.UnitTests/OrderQueryTests.cs @@ -316,7 +316,7 @@ private static CartCtx CartSetup() public async Task Get_cart_prices_the_lines() { var c = CartSetup(); - var result = await new GetCartHandler(c.Carts, c.Widgets).Handle(new GetCartQuery(c.Cart.Id), CancellationToken.None); + var result = await new GetCartHandler(c.Carts, c.Widgets).Handle(new GetCartQuery(c.Cart.Id, null), CancellationToken.None); Assert.True(result.IsSuccess); Assert.Equal(2, result.Value!.ItemCount); @@ -327,7 +327,7 @@ public async Task Get_cart_prices_the_lines() public async Task Get_cart_fails_for_an_unknown_cart() { var c = CartSetup(); - var result = await new GetCartHandler(c.Carts, c.Widgets).Handle(new GetCartQuery(Guid.NewGuid()), CancellationToken.None); + var result = await new GetCartHandler(c.Carts, c.Widgets).Handle(new GetCartQuery(Guid.NewGuid(), null), CancellationToken.None); Assert.False(result.IsSuccess); Assert.Equal("Cart not found.", result.Error); @@ -340,7 +340,7 @@ public async Task Removing_an_item_empties_the_cart_and_touches_it() var clock = new FakeTimeProvider(Now.AddHours(1)); var result = await new RemoveCartItemHandler(c.Carts, c.Widgets, clock) - .Handle(new RemoveCartItemCommand(c.Cart.Id, c.Widget.Id), CancellationToken.None); + .Handle(new RemoveCartItemCommand(c.Cart.Id, c.Widget.Id, null), CancellationToken.None); Assert.True(result.IsSuccess); Assert.Equal(0, result.Value!.ItemCount); @@ -354,7 +354,7 @@ public async Task Removing_an_item_that_is_not_in_the_cart_is_a_no_op() var c = CartSetup(); var result = await new RemoveCartItemHandler(c.Carts, c.Widgets, new FakeTimeProvider(Now)) - .Handle(new RemoveCartItemCommand(c.Cart.Id, Guid.NewGuid()), CancellationToken.None); + .Handle(new RemoveCartItemCommand(c.Cart.Id, Guid.NewGuid(), null), CancellationToken.None); Assert.True(result.IsSuccess); Assert.Equal(2, result.Value!.ItemCount); @@ -366,7 +366,7 @@ public async Task Removing_from_an_unknown_cart_fails() var c = CartSetup(); var result = await new RemoveCartItemHandler(c.Carts, c.Widgets, new FakeTimeProvider(Now)) - .Handle(new RemoveCartItemCommand(Guid.NewGuid(), c.Widget.Id), CancellationToken.None); + .Handle(new RemoveCartItemCommand(Guid.NewGuid(), c.Widget.Id, null), CancellationToken.None); Assert.False(result.IsSuccess); Assert.Equal("Cart not found.", result.Error); diff --git a/tests/WidgetWorks.UnitTests/ReleaseStaleReservationsTests.cs b/tests/WidgetWorks.UnitTests/ReleaseStaleReservationsTests.cs new file mode 100644 index 0000000..0d119f3 --- /dev/null +++ b/tests/WidgetWorks.UnitTests/ReleaseStaleReservationsTests.cs @@ -0,0 +1,174 @@ +using Microsoft.Extensions.Logging.Abstractions; +using Microsoft.Extensions.Time.Testing; +using WidgetWorks.Application.Checkout.ReleaseStale; +using WidgetWorks.Domain.Catalog; +using WidgetWorks.Domain.Orders; +using WidgetWorks.UnitTests.Fakes; +using Xunit; + +namespace WidgetWorks.UnitTests; + +/// +/// The sweep that stops an unsettled order holding stock forever. Time is faked, so these prove the +/// policy — what counts as stale, what gets released — without a timer or a real wait. +/// +public class ReleaseStaleReservationsTests +{ + private static readonly DateTimeOffset Now = new(2026, 1, 1, 12, 0, 0, TimeSpan.Zero); + + private sealed record Harness( + InMemoryOrderRepository Orders, + InMemoryWidgetRepository Widgets, + ReleaseStaleReservationsHandler Handler, + Guid WidgetId); + + private static Harness Build(ReservationOptions? options = null) + { + var widgets = new InMemoryWidgetRepository(); + var orders = new InMemoryOrderRepository(widgets); + var widgetId = Guid.NewGuid(); + widgets.Store[widgetId] = new Widget + { + Id = widgetId, + Sku = "WW-001", + Name = "Standard Widget Block Cobalt", + Price = 9.99m, + IsActive = true, + QuantityOnHand = 20, + QuantityReserved = 0, + }; + + var handler = new ReleaseStaleReservationsHandler( + orders, + new FakeTimeProvider(Now), + options ?? new ReservationOptions(), + NullLogger.Instance); + + return new Harness(orders, widgets, handler, widgetId); + } + + /// Places an order holding stock and parks it in AwaitingPayment as of . + private static async Task GivenUnsettledOrder(Harness h, int quantity, DateTimeOffset updatedAt) + { + var order = new Order + { + Id = Guid.NewGuid(), + OrderNumber = "WW-" + Guid.NewGuid().ToString("N")[..6], + Email = "shopper@widgetworks.test", + Status = OrderStatus.Pending, + Total = 9.99m * quantity, + }; + order.Items.Add(new OrderItem + { + Id = Guid.NewGuid(), + WidgetId = h.WidgetId, + Sku = "WW-001", + Name = "Standard Widget Block Cobalt", + UnitPrice = 9.99m, + Quantity = quantity, + LineSubtotal = 9.99m * quantity, + }); + + await h.Orders.TryPlaceAsync(order, CancellationToken.None); + await h.Orders.MarkAwaitingPaymentAsync(order.Id, "Mock", "ref", updatedAt, CancellationToken.None); + return order; + } + + [Fact] + public async Task An_order_unsettled_past_the_threshold_gives_its_stock_back() + { + var h = Build(); + await GivenUnsettledOrder(h, quantity: 4, updatedAt: Now.AddMinutes(-30)); + + Assert.Equal(4, h.Widgets.Store[h.WidgetId].QuantityReserved); + + var released = await h.Handler.Handle(CancellationToken.None); + + Assert.Equal(1, released); + Assert.Equal(0, h.Widgets.Store[h.WidgetId].QuantityReserved); + // The goods never shipped, so on-hand is untouched and all 20 are sellable again. + Assert.Equal(20, h.Widgets.Store[h.WidgetId].QuantityOnHand); + } + + [Fact] + public async Task An_order_still_inside_the_window_is_left_alone() + { + var h = Build(); + await GivenUnsettledOrder(h, quantity: 4, updatedAt: Now.AddMinutes(-5)); + + var released = await h.Handler.Handle(CancellationToken.None); + + // A slow but honest bank redirect must not lose the customer's basket. + Assert.Equal(0, released); + Assert.Equal(4, h.Widgets.Store[h.WidgetId].QuantityReserved); + } + + [Fact] + public async Task A_settled_order_is_never_swept() + { + var h = Build(); + var order = await GivenUnsettledOrder(h, quantity: 4, updatedAt: Now.AddMinutes(-30)); + await h.Orders.MarkPaidAsync(order.Id, "Mock", "ref", Now.AddMinutes(-29), CancellationToken.None); + + var released = await h.Handler.Handle(CancellationToken.None); + + Assert.Equal(0, released); + // Paid stock is owed to the customer and must stay reserved until it ships. + Assert.Equal(4, h.Widgets.Store[h.WidgetId].QuantityReserved); + } + + [Fact] + public async Task A_sweep_is_safe_to_run_twice() + { + var h = Build(); + await GivenUnsettledOrder(h, quantity: 4, updatedAt: Now.AddMinutes(-30)); + + var first = await h.Handler.Handle(CancellationToken.None); + var second = await h.Handler.Handle(CancellationToken.None); + + Assert.Equal(1, first); + // Nothing left to do, and crucially no second decrement of a reservation already released. + Assert.Equal(0, second); + Assert.Equal(0, h.Widgets.Store[h.WidgetId].QuantityReserved); + } + + [Fact] + public async Task One_pass_takes_no_more_than_the_batch_size() + { + var h = Build(new ReservationOptions { BatchSize = 2 }); + for (var i = 0; i < 5; i++) + { + await GivenUnsettledOrder(h, quantity: 1, updatedAt: Now.AddMinutes(-30 - i)); + } + + var released = await h.Handler.Handle(CancellationToken.None); + + // A backlog is worked through over several sweeps rather than one long pass. + Assert.Equal(2, released); + Assert.Equal(3, h.Widgets.Store[h.WidgetId].QuantityReserved); + } + + [Fact] + public async Task The_oldest_unsettled_orders_are_released_first() + { + var h = Build(new ReservationOptions { BatchSize = 1 }); + var oldest = await GivenUnsettledOrder(h, quantity: 1, updatedAt: Now.AddHours(-3)); + await GivenUnsettledOrder(h, quantity: 1, updatedAt: Now.AddMinutes(-20)); + + await h.Handler.Handle(CancellationToken.None); + + Assert.Equal(OrderStatus.PaymentFailed, h.Orders.Orders.Single(o => o.Id == oldest.Id).Status); + } + + [Fact] + public async Task A_cancelled_sweep_stops_rather_than_finishing_the_batch() + { + var h = Build(); + await GivenUnsettledOrder(h, quantity: 1, updatedAt: Now.AddMinutes(-30)); + + using var cancelled = new CancellationTokenSource(); + await cancelled.CancelAsync(); + + await Assert.ThrowsAnyAsync(() => h.Handler.Handle(cancelled.Token)); + } +} diff --git a/tests/WidgetWorks.UnitTests/SmallServicesTests.cs b/tests/WidgetWorks.UnitTests/SmallServicesTests.cs index dfcdf3d..e26b57c 100644 --- a/tests/WidgetWorks.UnitTests/SmallServicesTests.cs +++ b/tests/WidgetWorks.UnitTests/SmallServicesTests.cs @@ -162,7 +162,7 @@ public async Task Setting_a_quantity_of_zero_removes_the_line() { var c = Setup(); - var result = await Handler(c).Handle(new UpdateCartItemCommand(c.Cart.Id, c.Widget.Id, 0), CancellationToken.None); + var result = await Handler(c).Handle(new UpdateCartItemCommand(c.Cart.Id, c.Widget.Id, 0, null), CancellationToken.None); Assert.True(result.IsSuccess); Assert.Empty(result.Value!.Items); @@ -173,7 +173,7 @@ public async Task A_negative_quantity_removes_the_line_rather_than_erroring() { var c = Setup(); - var result = await Handler(c).Handle(new UpdateCartItemCommand(c.Cart.Id, c.Widget.Id, -3), CancellationToken.None); + var result = await Handler(c).Handle(new UpdateCartItemCommand(c.Cart.Id, c.Widget.Id, -3, null), CancellationToken.None); Assert.True(result.IsSuccess); Assert.Empty(result.Value!.Items); @@ -184,7 +184,7 @@ public async Task Asking_for_more_than_exists_caps_at_what_is_available() { var c = Setup(available: 5); - var result = await Handler(c).Handle(new UpdateCartItemCommand(c.Cart.Id, c.Widget.Id, 99), CancellationToken.None); + var result = await Handler(c).Handle(new UpdateCartItemCommand(c.Cart.Id, c.Widget.Id, 99, null), CancellationToken.None); Assert.True(result.IsSuccess); Assert.Equal(5, result.Value!.Items.Single().Quantity); @@ -195,7 +195,7 @@ public async Task An_out_of_stock_widget_is_refused_with_a_reason() { var c = Setup(available: 0); - var result = await Handler(c).Handle(new UpdateCartItemCommand(c.Cart.Id, c.Widget.Id, 1), CancellationToken.None); + var result = await Handler(c).Handle(new UpdateCartItemCommand(c.Cart.Id, c.Widget.Id, 1, null), CancellationToken.None); Assert.False(result.IsSuccess); Assert.Equal("This widget is out of stock.", result.Error); @@ -206,7 +206,7 @@ public async Task A_hidden_widget_cannot_be_added_to_a_cart() { var c = Setup(active: false); - var result = await Handler(c).Handle(new UpdateCartItemCommand(c.Cart.Id, c.Widget.Id, 1), CancellationToken.None); + var result = await Handler(c).Handle(new UpdateCartItemCommand(c.Cart.Id, c.Widget.Id, 1, null), CancellationToken.None); Assert.False(result.IsSuccess); Assert.Equal("Widget not found.", result.Error); @@ -217,7 +217,7 @@ public async Task An_unknown_widget_is_refused() { var c = Setup(); - var result = await Handler(c).Handle(new UpdateCartItemCommand(c.Cart.Id, Guid.NewGuid(), 1), CancellationToken.None); + var result = await Handler(c).Handle(new UpdateCartItemCommand(c.Cart.Id, Guid.NewGuid(), 1, null), CancellationToken.None); Assert.False(result.IsSuccess); Assert.Equal("Widget not found.", result.Error); @@ -228,7 +228,7 @@ public async Task An_unknown_cart_is_refused() { var c = Setup(); - var result = await Handler(c).Handle(new UpdateCartItemCommand(Guid.NewGuid(), c.Widget.Id, 1), CancellationToken.None); + var result = await Handler(c).Handle(new UpdateCartItemCommand(Guid.NewGuid(), c.Widget.Id, 1, null), CancellationToken.None); Assert.False(result.IsSuccess); Assert.Equal("Cart not found.", result.Error); @@ -239,7 +239,7 @@ public async Task Updating_a_quantity_touches_the_cart() { var c = Setup(); - await Handler(c).Handle(new UpdateCartItemCommand(c.Cart.Id, c.Widget.Id, 3), CancellationToken.None); + await Handler(c).Handle(new UpdateCartItemCommand(c.Cart.Id, c.Widget.Id, 3, null), CancellationToken.None); Assert.Equal(Now, c.Carts.Store[c.Cart.Id].UpdatedAt); } diff --git a/web/src/lib/catalog.test.ts b/web/src/lib/catalog.test.ts index 48daa86..deb08fa 100644 --- a/web/src/lib/catalog.test.ts +++ b/web/src/lib/catalog.test.ts @@ -1,35 +1,10 @@ import { describe, expect, it } from 'vitest' -import type { WidgetView } from '../api/types' -import { CATEGORIES, categoryBySlug, refine } from './catalog' - -// The header scope select, the category rail and the sort control all funnel -// through refine(), so this covers the behaviour behind three UI controls. -const widget = (over: Partial): WidgetView => ({ - id: crypto.randomUUID(), - sku: 'WW-000', - name: 'Widget', - description: '', - imageUrl: null, - price: 10, - isActive: true, - quantityOnHand: 10, - quantityReserved: 0, - quantityAvailable: 10, - ...over, -}) - -const catalog: WidgetView[] = [ - widget({ sku: 'WW-001', name: 'Standard Widget', price: 9.99 }), - widget({ sku: 'WW-002', name: 'Deluxe Widget', price: 24.99 }), - widget({ sku: 'WW-003', name: 'Mega Widget', price: 49.99 }), - widget({ sku: 'WW-005', name: 'Widget Pro Kit', price: 79.99, quantityAvailable: 0 }), -] - -const names = (items: WidgetView[]) => items.map((w) => w.name) - -// Index rather than Array.prototype.at — the project compiles to ES2020. -const last = (items: string[]) => items[items.length - 1] +import { CATEGORIES, categoryBySlug } from './catalog' +// The browsing vocabulary the header scope select, the category rail and the +// sort control share. Filtering and ordering are the API's job now — that +// behaviour is covered by WidgetRepositoryTests against real SQL, which is +// where it lives. describe('categoryBySlug', () => { it('resolves a real category', () => { expect(categoryBySlug('mega')?.keyword).toBe('mega') @@ -45,60 +20,13 @@ describe('categoryBySlug', () => { expect(c.icon).not.toBe('') } }) -}) - -describe('refine — category filter', () => { - it('narrows to the matching category', () => { - expect(names(refine(catalog, 'deluxe', 'featured'))).toEqual(['Deluxe Widget']) - }) - it('matches the seeded kit by keyword', () => { - expect(names(refine(catalog, 'kit', 'featured'))).toEqual(['Widget Pro Kit']) - }) - - it('returns everything when no category is chosen', () => { - expect(refine(catalog, '', 'featured')).toHaveLength(catalog.length) - }) - - it('returns nothing for a category with no members', () => { - expect(refine(catalog, 'mini', 'featured')).toEqual([]) - }) - - it('does not mutate the source array', () => { - const original = [...catalog] - refine(catalog, '', 'price-desc') - expect(catalog).toEqual(original) - }) -}) - -describe('refine — sorting', () => { - it('sorts by price ascending', () => { - expect(names(refine(catalog, '', 'price-asc'))).toEqual([ - 'Standard Widget', 'Deluxe Widget', 'Mega Widget', 'Widget Pro Kit', - ]) - }) - - it('sorts by price descending', () => { - expect(names(refine(catalog, '', 'price-desc'))).toEqual([ - 'Widget Pro Kit', 'Mega Widget', 'Deluxe Widget', 'Standard Widget', - ]) - }) - - it('sorts by name', () => { - expect(names(refine(catalog, '', 'name'))).toEqual([ - 'Deluxe Widget', 'Mega Widget', 'Standard Widget', 'Widget Pro Kit', - ]) - }) - - it('featured pushes out-of-stock items to the end', () => { - expect(last(names(refine(catalog, '', 'featured')))).toBe('Widget Pro Kit') - }) - - it('falls back to featured ordering for an unknown sort', () => { - expect(last(names(refine(catalog, '', 'nonsense')))).toBe('Widget Pro Kit') - }) - - it('handles an empty catalog', () => { - expect(refine([], 'mega', 'price-asc')).toEqual([]) + it('resolves each slug to the keyword the API is asked to narrow on', () => { + // The rail stores a slug; the request sends the keyword. This is the + // assertion that catches a slug being renamed for the URL without the + // keyword following it. + for (const c of CATEGORIES.filter((c) => c.slug)) { + expect(categoryBySlug(c.slug)?.keyword).toBe(c.keyword) + } }) }) diff --git a/web/src/lib/catalog.ts b/web/src/lib/catalog.ts index 9b0f1df..fafbca0 100644 --- a/web/src/lib/catalog.ts +++ b/web/src/lib/catalog.ts @@ -1,14 +1,11 @@ // Storefront browsing vocabulary, shared by the header scope select, the // category rail and the catalog grid so all three stay in step. // -// The API searches free text (`?search=`) and pages server-side. Category and -// sort are refinements applied to the returned page in the browser: the store -// asks for a full page (PAGE_SIZE, the API's 100 cap) and narrows it here, -// which keeps the three controls composable without new endpoints. The demo -// catalog is 75 widgets, so one page still holds all of it - a catalog past -// PAGE_SIZE would drop its tail out of every category shelf. -import type { WidgetView } from '../api/types' - +// Search, category and sort are all applied by the API. They used to be +// narrowed here over a single fetched page, which meant a catalog larger than +// PAGE_SIZE lost its tail from every shelf and a sort only ordered whatever +// happened to be on that page. PAGE_SIZE is now just how many results one +// request asks for; growing past it needs a pager, not a bigger number. export const PAGE_SIZE = 100 export interface Category { @@ -43,36 +40,5 @@ export function categoryBySlug(slug: string): Category | undefined { return CATEGORIES.find((c) => c.slug === slug && c.slug !== '') } -function matchesCategory(w: WidgetView, keyword: string): boolean { - if (!keyword) return true - const needle = keyword.toLowerCase() - return ( - w.name.toLowerCase().includes(needle) || - w.sku.toLowerCase().includes(needle) || - w.description.toLowerCase().includes(needle) - ) -} - -/** Narrow to a category, then order — pure, so the grid can call it on render. */ -export function refine(items: WidgetView[], catSlug: string, sort: string): WidgetView[] { - const keyword = categoryBySlug(catSlug)?.keyword ?? '' - const filtered = keyword ? items.filter((w) => matchesCategory(w, keyword)) : items.slice() - - switch (sort) { - case 'price-asc': - return filtered.sort((a, b) => a.price - b.price) - case 'price-desc': - return filtered.sort((a, b) => b.price - a.price) - case 'name': - return filtered.sort((a, b) => a.name.localeCompare(b.name)) - default: - // "Featured" keeps the order the API returned, with anything out of - // stock pushed to the end so the grid leads with what can be bought. - return filtered.sort( - (a, b) => Number(b.quantityAvailable > 0) - Number(a.quantityAvailable > 0), - ) - } -} - /** Free-shipping threshold quoted in the header strip and cart nudge. */ export const FREE_SHIPPING_THRESHOLD = 75 diff --git a/web/src/pages/CatalogPage.tsx b/web/src/pages/CatalogPage.tsx index 08808f7..2109019 100644 --- a/web/src/pages/CatalogPage.tsx +++ b/web/src/pages/CatalogPage.tsx @@ -1,9 +1,9 @@ -import { useEffect, useMemo, useState } from 'react' +import { useEffect, useState } from 'react' import { Link, useSearchParams } from 'react-router-dom' import { api } from '../api/client' import type { Paged, WidgetView } from '../api/types' import { pseudoRating } from '../lib/img' -import { CATEGORIES, FREE_SHIPPING_THRESHOLD, PAGE_SIZE, SORTS, categoryBySlug, refine } from '../lib/catalog' +import { CATEGORIES, FREE_SHIPPING_THRESHOLD, PAGE_SIZE, SORTS, categoryBySlug } from '../lib/catalog' import { CategoryIcon } from '../components/CategoryIcon' import { AddToCartButton } from '../components/AddToCartButton' import { ProductImage } from '../components/ProductImage' @@ -58,14 +58,20 @@ export function CatalogPage() { setLoading(true) const sp = new URLSearchParams({ pageSize: String(PAGE_SIZE) }) if (q.trim()) sp.set('search', q.trim()) + // Category and sort are the server's job. Narrowing a single fetched page in the browser + // silently dropped anything past that page from a shelf, and sorted only what happened to be + // on it. Asking the API means both apply to the whole matching set. + const keyword = categoryBySlug(cat)?.keyword + if (keyword) sp.set('category', keyword) + if (sort) sp.set('sort', sort) api>(`/catalog/widgets?${sp}`) .then((d) => { if (active) { setData(d); setError(null) } }) .catch((e) => { if (active) setError(e.message) }) .finally(() => { if (active) setLoading(false) }) return () => { active = false } - }, [q]) + }, [q, cat, sort]) - const items = useMemo(() => refine(data?.items ?? [], cat, sort), [data, cat, sort]) + const items = data?.items ?? [] const category = categoryBySlug(cat) const browsing = !q && !cat diff --git a/web/src/pages/StorefrontPages.test.tsx b/web/src/pages/StorefrontPages.test.tsx index ffc8a09..10eb896 100644 --- a/web/src/pages/StorefrontPages.test.tsx +++ b/web/src/pages/StorefrontPages.test.tsx @@ -88,11 +88,12 @@ describe('CatalogPage', () => { }) it('counts what is shown, in the singular when only one matched', async () => { - stubFetch([['/catalog/widgets', paged()]]) + // The API returns the narrowed set now, so the fixture is the answer to + // ?category=mega rather than something the page filters afterwards. + stubFetch([['/catalog/widgets', paged([soldOut])]]) renderWithProviders(, { at: '/store?cat=mega', path: '/store' }) - // One of the two loaded widgets matches "mega". expect(await screen.findByText('1 product')).toBeInTheDocument() }) @@ -101,13 +102,13 @@ describe('CatalogPage', () => { renderWithProviders(, { at: '/store', path: '/store' }) - // Category and sort refine the page in the browser; the count must not pretend the - // catalog is only as big as the page. + // The grid shows one page; the count must not pretend the catalog is only + // as big as that page. expect(await screen.findByText('2 products of 40')).toBeInTheDocument() }) - it('sorting re-orders the grid in place', async () => { - stubFetch([['/catalog/widgets', paged()]]) + it('asks the API to re-order rather than sorting the page it already has', async () => { + const calls = stubFetch([['/catalog/widgets', paged()]]) const user = userEvent.setup() renderWithProviders(, { at: '/store', path: '/store' }) @@ -115,10 +116,19 @@ describe('CatalogPage', () => { await user.selectOptions(screen.getByLabelText('Sort by'), 'price-desc') - await waitFor(() => { - const titles = screen.getAllByRole('link', { name: /Widget$/ }).map((el) => el.textContent) - expect(titles).toEqual(['Mega Widget', 'Standard Widget']) // 99 before 12.50 - }) + // Sorting in the browser would only order whatever happened to be on this + // page. The request carries the choice so the ordering applies to the whole + // matching set. + await waitFor(() => expect(calls.some((c) => c.url.includes('sort=price-desc'))).toBe(true)) + }) + + it('narrows a category through the API, not in the browser', async () => { + const calls = stubFetch([['/catalog/widgets', paged()]]) + + renderWithProviders(, { at: '/store?cat=mega', path: '/store' }) + await screen.findByRole('heading', { name: 'Mega widgets' }) + + await waitFor(() => expect(calls.some((c) => c.url.includes('category=mega'))).toBe(true)) }) it('clears a category from the toolbar', async () => {