From 519e2aca5756ce7127fd85f7ce846a188a13d797 Mon Sep 17 00:00:00 2001 From: ati Date: Wed, 23 Sep 2026 23:24:40 +0100 Subject: [PATCH 01/13] stop retrying the bank POST, send expiry as MM/yyyy The standard resilience handler was registered for every HttpClient and retried the non-idempotent bank POST on 5xx/timeouts, so one payment could be charged twice. The bank client is now a typed HttpClient with its own handler, retries disabled for unsafe methods; timeouts and the circuit breaker stay. The expiry was sent as "4/2027" instead of the bank's "MM/yyyy". The mapping moves to BankHttpClient.ToBankRequest with unit tests, and the WireMock stubs now require the format so the simulator enforces the contract. Adds audit.md. Co-Authored-By: Claude Opus 5.5 --- AGENTS.md | 14 +- audit.md | 452 ++++++++++++++++++ src/Program.cs | 16 +- src/Services/BankHttpClient.cs | 24 +- .../wiremock/mappings/authorized.json | 2 +- .../wiremock/mappings/declined.json | 2 +- .../wiremock/mappings/unavailable.json | 2 +- .../PaymentEndToEndTests.cs | 13 +- .../Services/BankHttpClientTests.cs | 35 ++ 9 files changed, 529 insertions(+), 31 deletions(-) create mode 100644 audit.md create mode 100644 tests/PaymentGatewayApiUnitTests/Services/BankHttpClientTests.cs diff --git a/AGENTS.md b/AGENTS.md index 7bbc7a2..31ad83e 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -38,8 +38,9 @@ dotnet run --project tests/PaymentGatewayApiUnitTests # equivalent, xUnit v3 p Target the test project explicitly: a bare `dotnet test` (or `--solution`) fails because the web project's `Microsoft.NET.Test.Sdk` reference makes it look like a VSTest project. -`BankHttpClient` and `PaymentsRepository` are deliberately not unit-tested; they are only -mocked through their interfaces. Tests commented `// NOTE: defect` pin current, known-wrong +`BankHttpClient` and `PaymentsRepository` are deliberately not unit-tested beyond +`BankHttpClient.ToBankRequest` (the bank contract mapping); otherwise they are only mocked +through their interfaces. Tests commented `// NOTE: defect` pin current, known-wrong behaviour — update them when the defect is fixed. End-to-end tests live in `tests/PaymentGatewayApiEndToEndTests`. They are black-box: they call @@ -53,8 +54,8 @@ dotnet test --project tests/PaymentGatewayApiEndToEndTests An assembly fixture (`ApiFixture`) polls the healthcheck for 30s before failing. The base URL defaults to `http://localhost:5067`; override it with `E2E_BASE_URL` (e.g. to run against -`dotnet run` instead of the container). The suite takes ~10s because the bank's `503` goes -through the standard resilience handler's retries. String results (`"ok"`, the payment id, +`dotnet run` instead of the container). The bank client's resilience handler never retries +the `POST` (it is not idempotent), so the bank's `503` fails fast. String results (`"ok"`, the payment id, status names) come back as `text/plain`, not JSON strings. For manual checks, use the Bruno collection at `tests/bruno-collection.yml` (covers health, @@ -94,7 +95,8 @@ digits are ever stored. ### Wiring -All DI registration lives in `Program.cs`. Services are scoped; the repository is a +All DI registration lives in `Program.cs`. Services are scoped, except `BankHttpClient`, a +typed `HttpClient` (transient) whose resilience handler has retries disabled; the repository is a singleton, as is the `IAmazonDynamoDB` client registered through `AddAWSService()`. New services follow the existing pattern — interface and implementation in the same file under `Services/` — and get registered there. @@ -113,6 +115,8 @@ The WireMock mappings in `src/Simulators/wiremock/mappings/` select a response f | `0` | `503` | `InternalError` | | any missing field | `400` | `InternalError` | +The three `200`/`503` stubs also require `expiry_date` in the bank's `MM/yyyy` format, so a +malformed expiry falls through like any other unmatched request. Anything else hits the catch-all `default.json` (`400`). It carries `"priority": 10` so it never shadows the specific stubs; WireMock breaks equal-priority ties by load order. diff --git a/audit.md b/audit.md new file mode 100644 index 0000000..fc04fd2 --- /dev/null +++ b/audit.md @@ -0,0 +1,452 @@ +# Payment Gateway API: audit for a senior .NET reviewer + +**Scope:** the whole repository as published on branch `claude` at `ccaacc4`: source, tests, Docker/compose, config, docs and git history. +**Question:** what has to change so that a senior .NET reviewer concludes the author builds high-quality software? +**Method:** I read every tracked file and ran the build, the unit tests, the vulnerable-package scan and the outdated-package scan. I did not change any existing code. The only new file is this report. + +| Check | Result | +| --- | --- | +| `dotnet build PaymentGatewayApi.slnx` (SDK 10.0.112) | ✅ 0 warnings, 0 errors (but no analyzers or warnings-as-errors are configured) | +| `dotnet test --project tests/PaymentGatewayApiUnitTests` | ✅ 61/61 passed in 1.3 s | +| `dotnet list package --vulnerable --include-transitive` | ✅ no known vulnerabilities | +| `dotnet list package --outdated` | ⚠️ `AWSSDK.DynamoDBv2` 4.0.101.2 → 4.0.104, `Moq` 4.20.69 → 4.20.72 | +| E2E suite | Not run. It needs the Docker stack; I only reviewed its code. | + +--- + +## 1. Verdict + +The repo already shows good habits, and a reviewer will notice them: + +- The layers sit behind interfaces. +- `Nullable` is enabled. +- The public API models are kept apart from the bank's contract models. +- Card data is truncated to the last four digits before it is stored. +- Logging is structured. +- There is a reproducible local stack (DynamoDB Local plus a WireMock bank). +- There is a unit suite that is fast and readable. +- There is a black-box E2E suite. +- Known defects are recorded honestly in tests. + +A senior reviewer will judge a **payment** gateway on something else first: **does money move correctly when things go wrong?** On that question the code fails in several concrete ways. It can charge a card twice. It can charge a card and lose the record. It sends the bank a badly formatted expiry date. It discards the authorization code. It reports a bank outage as the client's fault. Next to those problems, a reviewer also sees: + +- ASP.NET Core features rebuilt by hand, or skipped: Options, typed `HttpClient`, `ProblemDetails`, health checks, `TimeProvider`, `CancellationToken`. +- An API contract that is inconsistent between endpoints. +- Repo hygiene that reads as unfinished: TODOs, commented-out code, "." commit messages, a placeholder README, no CI. + +**The fastest route to a convincing repo:** +1. Fix the money-correctness issues (§2). +2. Make the HTTP contract honest (§3). +3. Replace the hand-rolled plumbing with the framework's own features (§4). +4. Add CI plus in-process integration tests, so that quality is *demonstrated*, not claimed (§6, §7). + +Severity legend: 🔴 critical (a reviewer stops reading) · 🟠 high · 🟡 medium · ⚪ polish. + +--- + +## 2. Payment correctness and resilience + +### 🔴 2.1 POST to the bank is retried automatically, so one payment can be charged twice +`src/Program.cs:23-27` + +```csharp +builder.Services.ConfigureHttpClientDefaults(http => { http.AddStandardResilienceHandler(); }); +``` + +The standard resilience handler retries **every** HTTP method on 5xx, 408, 429 and timeouts. `POST /payments` to an acquiring bank is not idempotent. If the bank authorizes the payment but the response is lost or slow, the gateway sends it again. The E2E suite shows this happening: `PostPayment_CardEndingZero_InternalError` is slow *because* the POST is retried. The README even says "http retries" are omitted, which contradicts the code. + +**Change:** +- Scope the resilience handler to a single typed bank client (see 4.2), not to `ConfigureHttpClientDefaults`. +- Turn retries off for unsafe methods: `options.Retry.DisableForUnsafeHttpMethods()`. +- Alternatively, retry only if the bank supports an idempotency key, and send one derived from `PaymentId`. +- Keep the attempt and total timeouts plus the circuit breaker. + +### 🔴 2.2 The payment is persisted only *after* the bank call, so a failed write loses a charged payment +`src/Services/BankService.cs:35-41` + +The order today is: call the bank, get `Authorized`, then call `paymentRepo.Add(...)`. If DynamoDB throws at that point: +- the card has been charged, +- no record exists, +- `ErrorMiddleware` returns a 500 to the merchant, +- the merchant will probably retry and charge the card again. + +When the bank call fails (non-2xx, timeout or exception), nothing is stored at all, so there is no audit trail. + +**Change:** write the payment first with status `Pending`. Call the bank. Then update it to `Authorized`, `Declined` or `Failed`/`Unknown`. For a timeout, record the uncertain outcome explicitly; a reconciliation step would resolve it later (you can document this without building it). This one change shows more payments maturity than anything else in the repo. + +### 🔴 2.3 No idempotency for merchant requests +`src/Controllers/PaymentController.cs:57-84` + +Every POST creates a new `PaymentId`. When a merchant retries after a network blip, a second charge is created. Payment APIs (Stripe, Checkout.com, Adyen) accept an `Idempotency-Key` header. + +**Change:** accept `Idempotency-Key`. Store the key along with the result, for example as a DynamoDB item with a conditional put (`attribute_not_exists`). Replay the stored response on a duplicate. Return `409`/`422` when the same key arrives with a different body. + +### 🔴 2.4 The expiry date sent to the bank has the wrong format +`src/Services/BankHttpClient.cs:34-35` + +```csharp +// "04/2025" +string expiry = $"{payment.ExpiryMonth}/{payment.ExpiryYear}"; // produces "4/2025" +``` + +The comment on the line above states the contract (`MM/yyyy`), but the code does not follow it. Nothing catches the bug, because the WireMock stubs only check that `expiry_date` *exists*. + +**Change:** +- Use `$"{payment.ExpiryMonth:D2}/{payment.ExpiryYear}"`. +- Add a unit test for the mapping. +- Tighten the stubs so `expiry_date` must match `^(0[1-9]|1[0-2])/\d{4}$`. Then the simulator enforces the contract and does not just mirror the implementation. + +### 🟠 2.5 The bank's authorization code is thrown away +`src/Services/BankResponseProcessor.cs:50-57`, `src/Models/Responses/PostPaymentResponse.cs` + +`BankResponse.AuthorizationCode` is parsed but never mapped or stored. Capture, refund, dispute and reconciliation all need that code. + +**Change:** store it on the persisted payment record. It does not have to be exposed in the public response. + +### 🟠 2.6 An unparseable bank response is stored with a null hash key +`src/Services/BankResponseProcessor.cs:28-48`, `src/Services/BankService.cs:38-39` + +This defect is already pinned in `BankServiceTests.ExecutePayment_BankBodyUnparseable_StillPersistsResponseWithoutPaymentId`. The error path returns a `PostPaymentResponse` with only `Status` set, and `BankService` persists it anyway. `PaymentId = null` in the key makes DynamoDB reject the write. The exception reaches `ErrorMiddleware`, which returns a 500 and leaks DynamoDB's message. + +**Change:** +- The processor should return a result type such as `BankOutcome { Authorized | Declined | Failed(reason) }`, not a half-filled DTO. +- `BankService` decides what gets persisted. +- Then remove the `// NOTE: defect` test. + +### 🟠 2.7 Transport exceptions are not handled (`// todo try catch`) +`src/Services/BankHttpClient.cs:48`, `src/Services/PaymentsRepository.cs:20` + +The following all propagate as a 500 whose body contains the exception message: +- `HttpRequestException`, +- `TaskCanceledException`/`TimeoutRejectedException`, +- `BrokenCircuitException`, +- `AmazonDynamoDBException`. + +**Change:** catch them at the bank-client boundary, log them, and convert them into a domain outcome (`BankUnavailable`). Map that outcome to `502`/`503`. Do not leave TODOs about error handling in a payments codebase: either do the work or remove the comment and open an issue. + +### 🟠 2.8 An invalid expiry year crashes instead of rejecting +`src/Services/PaymentRequestValidator.cs:51` + +This defect is already pinned in unit and E2E tests. `new DateTime(0, m, 1)` throws, so the response is a 500 instead of `Rejected`. + +**Change:** check the range (for example `now.Year <= year <= now.Year + 20`) before building the date, or compare `(year, month)` tuples without creating a `DateTime`. Delete both "defect" tests and replace them with `Rejected` assertions. + +> **About the `// NOTE: defect` tests.** Recording bugs as tests is a good engineering instinct. In a portfolio repo, though, a reviewer reads *three* documented bugs that are easy to fix and asks why they were not fixed. Fix them. If one must stay, use `[Fact(Skip = "…issue #N")]` linked to a GitHub issue, so the suite does not stay green on known-wrong behaviour. + +### 🟡 2.9 Money and domain rules are underspecified +`src/Services/PaymentRequestValidator.cs:64-91`, `src/Models/Requests/PaymentRequest.cs:22` + +- `Amount` is a `decimal` in major units with a rule of `>= 1`. That rejects £0.50, accepts `1.2345`, has no upper bound, and ignores zero-decimal currencies such as JPY. Gateways normally take an **integer amount in minor units** (`long amount = 1050` → £10.50). Pick one approach and document it. +- The currency check only tests `Length == 3`: `"123"` passes, and a test even pins that behaviour. Use an ISO-4217 allow-list from configuration (`GBP`, `EUR`, `USD` is enough). Normalise the case too. +- There is no Luhn check on the card number. A senior reviewer expects one, and it is about ten lines. +- `\d` in `^\d{14,19}$` and `^\d{3,4}$` matches any Unicode digit (Arabic-Indic, full-width and so on). Use `[0-9]`, and use `[GeneratedRegex]` (source-generated, allocation-free, analyzer-friendly) instead of `Regex.IsMatch` with a string pattern. + +--- + +## 3. API contract (what the merchant sees) + +### 🔴 3.1 Status codes misreport who is at fault +`src/Controllers/PaymentController.cs:70-83` + +| Outcome | Returned today | What a reviewer expects | +| --- | --- | --- | +| Authorized | `200` + plain-text id | `201 Created` + `Location: /api/payments/{id}` + payment JSON | +| Declined | `400 "Declined"` | `201`/`200` + payment JSON with `status: "Declined"`. The request was valid; the issuer said no. | +| Rejected (validation) | `400 "Rejected"` (plain text) | `400` `ValidationProblemDetails` listing *which* fields failed | +| Bank down / unparseable | `400 "InternalError"` | `502 Bad Gateway` / `503` `ProblemDetails`. **Never** 4xx: the client did nothing wrong, and 4xx tells it not to retry. | +| Unhandled exception | `500` plain text with `exception.Message` | `500` `ProblemDetails` with `traceId`, and no internal message | + +### 🟠 3.2 The POST does not return the payment it created +`src/Controllers/PaymentController.cs:77-83` + +`PostPaymentResponse` exists and the repository builds and stores it, but POST returns only the id (when authorized) or only a status string. A declined payment is persisted, yet the merchant never learns its id, so `GET /{id}` cannot be used for it. + +**Change:** return the `PostPaymentResponse` for both Authorized and Declined. + +### 🟠 3.3 The endpoints disagree on types and formats +- `PostPayment` declares `ActionResult` (`:58`) but returns strings, so the OpenAPI document is wrong. `GetHealthcheck` declares `ActionResult`. +- The POST returns statuses as text (`"Declined"`). The GET returns `PostPaymentResponse.Status` as a **number** (`0`), because `JsonStringEnumConverter` is not registered. The persisted `PaymentRaw` JSON also stores the enum as a number, so reordering `PaymentStatus` would silently corrupt stored data. +- Model-binding failures, such as a missing `required` property, already return `ProblemDetails` JSON through `[ApiController]`, while validator failures return the plain string `"Rejected"`. The same endpoint has two different shapes for 400. +- `GET /api/payment/{paymentId}` accepts any string. It should be `{paymentId:guid}` with a `Guid` parameter, so malformed ids return 400/404 without a database call. +- `NotFoundObjectResult("payment not found")` returns a plain string. Use `NotFound()`, which returns ProblemDetails. +- The code uses `new OkObjectResult(...)`/`new BadRequestObjectResult(...)` instead of the `ControllerBase` helpers (`Ok`, `CreatedAtAction`, `ValidationProblem`, `Problem`). The helpers are idiomatic and shorter. + +**Change:** +- Add `[Produces("application/json")]` and `[ProducesResponseType]` on every action. +- Add `builder.Services.AddProblemDetails()`. +- Configure `JsonStringEnumConverter` globally, for MVC and for the repository serializer. +- Use a plural resource name: `api/payments`. + +### 🟠 3.4 The request model accepts a server-owned field +`src/Models/Requests/PaymentRequest.cs:23` + +`PaymentId` sits on the inbound DTO. Swagger therefore advertises it as an input, and the controller mutates the request object to carry it (`:60`). + +**Change:** remove it from the request. Pass the id as a parameter, or create a domain `Payment` from the request. Make DTOs immutable (`record` with `init`/`required`). + +### 🟡 3.5 Storage is coupled to the public API +`src/Services/PaymentsRepository.cs:37` + +The author (correctly) keeps the bank models apart from the API models, then persists the **public response DTO** as the storage record. Any change to the public API becomes a data migration. + +**Change:** introduce a persistence record (a `PaymentEntity` with its own attributes) and map to and from it. The stored record also needs `AuthorizationCode`, `CreatedAt`, `MerchantId`/`CallerId` and `IdempotencyKey`, which have no place in the public response. + +--- + +## 4. ASP.NET Core idioms (showing framework fluency) + +### 🟠 4.1 Configuration: use the Options pattern and validate at startup +`src/Services/BankHttpClient.cs:45`, `src/Services/PaymentsRepository.cs:39,49` + +```csharp +string uri = this.config["SimulatorBankEndpoint"] ?? string.Empty; +``` + +Injecting `IConfiguration` into services, reading magic strings, and **falling back silently to an empty URI** means a missing setting only surfaces at request time. The table name `"Payments"` is hard-coded twice. + +**Change:** +```csharp +builder.Services.AddOptions().BindConfiguration("Bank") + .ValidateDataAnnotations().ValidateOnStart(); +``` +Apply the same pattern to `DynamoOptions { TableName }`. The README lists "configuration file validation" as omitted; it takes about five lines, so add it. + +### 🟠 4.2 Use a typed `HttpClient` and keep `HttpResponseMessage` out of the abstractions +`src/Program.cs:23-27,39`, `src/Services/BankHttpClient.cs:14,46`, `src/Services/BankResponseProcessor.cs:13` + +- `IHttpClientFactory.CreateClient()` with no name, plus global defaults, means *every* HttpClient in the process gets the retry policy. +- `IBankHttpClient` returns `HttpResponseMessage`, and `IBankResponseProcessor` receives it. The transport type leaks through two abstractions, and nobody disposes the response (`BankService.cs:35`). + +**Change:** +```csharp +builder.Services.AddHttpClient((sp, c) => + c.BaseAddress = sp.GetRequiredService>().Value.BaseUrl) + .AddStandardResilienceHandler(o => o.Retry.DisableForUnsafeHttpMethods()); +``` +`IBankClient.AuthorizeAsync(BankRequest, CancellationToken)` then returns a domain `BankOutcome`. The client owns serialization, status-code mapping and disposal. `BankResponseProcessor` either becomes a private mapping inside the client or a pure static mapper. Both are easier to test than a class that needs an `HttpResponseMessage`. + +### 🟠 4.3 Pass `CancellationToken` through the whole chain +There is none anywhere: the controller, `IBankService`, `IBankHttpClient`, `IPaymentsRepository`, `PutItemAsync` and `GetItemAsync` all lack it. A disconnected client keeps consuming bank and database capacity. + +**Change:** add `CancellationToken cancellationToken` to every async interface method and flow it from the action parameter, which MVC binds from `HttpContext.RequestAborted`. Reviewers look for this first. + +### 🟠 4.4 Use `TimeProvider` instead of `DateTime.Now` +`src/Controllers/PaymentController.cs:65`, `src/Services/PaymentRequestValidator.cs:11` + +The controller reads the local server clock (`DateTime.Now`, which depends on the time zone) and passes it into the validator, only so that the validator can be tested. + +**Change:** inject `TimeProvider` into the validator (registered as `TimeProvider.System`) and use `FakeTimeProvider` from `Microsoft.Extensions.TimeProvider.Testing` in tests. `IsExpiryValid(payment)` then loses its extra parameter. + +### 🟡 4.5 Validation design +`src/Services/PaymentRequestValidator.cs`, `src/Controllers/PaymentController.cs:63-68` + +Five boolean methods, chained with `&&` in the controller: +- stop at the first failure, +- return no error detail to the caller, +- force the controller to know the list of rules, +- make the controller tests use a string `switch` to steer mocks (`PaymentControllerTests.cs:106-123`). + +**Change:** one `ValidationResult Validate(PaymentRequest)` that collects every error keyed by field and returns `ValidationProblemDetails`. Either write it by hand or use FluentValidation. An `IEndpointFilter`/action filter can run it so that the action body only handles the happy path. + +### 🟡 4.6 Error handling: use the built-in pipeline +`src/Middleware/ErrorMiddleware.cs:25-37` + +- `exception.Message` goes straight to the client. That is information disclosure: DynamoDB and HTTP messages include hosts, table names and so on. `ErrorMiddlewareTests.InvokeAsync_NextThrows_Returns500WithMessage` asserts that the leak happens. +- The code sets `StatusCode` without checking `context.Response.HasStarted`, so it throws if streaming has begun. +- A client abort (`OperationCanceledException`) is logged as an error and answered with a 500. +- The comment `// see if can replace message with correlationId` is an open design question left in the code. + +**Change:** `builder.Services.AddExceptionHandler()` + `AddProblemDetails()` + `app.UseExceptionHandler()`. The handler logs the exception and returns `ProblemDetails` with `traceId`/`correlationId` and a generic message. Fix the test so it asserts that the message is *not* leaked. + +### 🟡 4.7 Correlation and caller context +`src/Middleware/LogMiddleware.cs:21,32` + +- `CallerId` is pushed as a raw `StringValues` from an unauthenticated header. It has no length limit and is not sanitised, so it enables log injection and log bloat. It is also not a scalar; the unit test works around that with `value.ToString()` + `Contains`. +- The client-supplied `CorrelationId` is trusted without validation and never echoed back in a response header, so a merchant cannot quote it to support. +- .NET already has W3C trace context (`Activity.Current.TraceId`, the `traceparent` header). The standard header name is `X-Correlation-ID` or `traceparent`, not `CorrelationId`. + +**Change:** +- Validate the header (GUID or `^[A-Za-z0-9-]{1,64}$`) and echo it on the response. +- Push `.ToString()`. +- Enrich logs with `TraceId`. +- Take the caller identity from authentication (§5.1), not from a free-text header. +- Rename the middleware to what it does, for example `CorrelationIdMiddleware`. + +### 🟡 4.8 Health checks +`src/Controllers/PaymentController.cs:33-40` + +`GET /api/payment` returning `"ok"` is a business route used as a health check. It proves the process is up, but not that DynamoDB or the bank can be reached. + +**Change:** `builder.Services.AddHealthChecks().AddCheck("dynamodb")` and `app.MapHealthChecks("/health/live")` plus `/health/ready`. Use them in `ApiFixture`, in a compose `healthcheck:` and in the Dockerfile. + +### ⚪ 4.9 Smaller idiom issues +- **OpenAPI:** .NET 9+ ships `Microsoft.AspNetCore.OpenApi` (`AddOpenApi()`/`MapOpenApi()`), and the .NET 10 templates no longer include Swashbuckle. Moving shows the author tracks the platform. `AddEndpointsApiExplorer()` is redundant with controllers. +- **Swagger is served in every environment** (`Program.cs:47-48`). Wrap it in `if (app.Environment.IsDevelopment())`. +- Add `public partial class Program;` so that `WebApplicationFactory` works (§6.1). +- **DI:** a scoped validator, response processor and bank service hold no request state. Singleton or a static pure function is fine, and saying *why* each lifetime was chosen shows intent. +- **Naming:** `BankService` coordinates validation → bank → persistence, so it is really a `PaymentService`/`PaymentProcessor`. The log template calls it `ExecuteBankPayment` while the method is `ExecutePayment`. The controller also talks to the repository directly for GET, which bypasses the service layer that POST uses. +- `Substring(request.CardNumber.Length - 4)` → `request.CardNumber[^4..]`. +- `[HttpGet()]` → `[HttpGet]`. +- `PaymentStatus.InternalError` is commented "Should never happen" (`PaymentStatus.cs:8`), yet it is returned on every bank outage. Rename it to `Failed`/`BankUnavailable` or fix the comment. +- Consider the `[LoggerMessage]` source generator for the fixed templates. It is allocation-free, and it enforces the "one template shape" convention at compile time instead of by discipline. Today the shapes already drift: `Validator.IsExpiryMonthValid` / `IsValid=` versus the documented `MethodName, PaymentId, Status`. + +--- + +## 5. Security and compliance posture + +The README says auth and HTTPS are "intentionally omitted". A reviewer accepts omissions that are *scoped* but not ones that are *unacknowledged in the code*. Show the seams. + +### 🟠 5.1 No authentication, and GET exposes any payment to anyone (IDOR) +Any caller who knows or guesses a payment id can read another merchant's payment. + +**Minimum change:** +- An API-key or JWT authentication handler that maps a key to a `MerchantId`. +- `[Authorize]` on the controller. +- Store `MerchantId` on the payment and filter `GET` by it, returning 404 for other merchants' payments. + +This is small, and it is the difference between "demo" and "gateway". + +### 🟡 5.2 Transport and abuse protection +- `UseHttpsRedirection`/HSTS for non-development environments. +- `AddRateLimiter` (built in since .NET 7) per merchant on `POST`. +- `AllowedHosts: "*"` in the base `appsettings.json`. + +### 🟡 5.3 Card-data handling (PCI DSS mindset) +Truncating to the last four digits is good. To make it visible and enforced: +- Mark `CardNumber`/`Cvv` with a redaction attribute (`Microsoft.Extensions.Compliance.Redaction` + `[DataClassification]`), or at least override `ToString()` on `PaymentRequest`, so a future `{@payment}` log call cannot leak a PAN or CVV. +- Add a test that sends a full PAN through the pipeline and checks that no log event contains it. The E2E test already asserts this for the GET response, which is the right instinct. + +### ⚪ 5.4 Container +`src/Dockerfile` +- Add `USER $APP_UID`: the .NET 8+ images ship a non-root `app` user, and not using it is a common review flag. +- Consider the `-chiseled`/`noble-chiseled` runtime image. +- Use `dotnet publish --no-restore`. +- Add a `HEALTHCHECK`. + +--- + +## 6. Tests + +The unit suite is clean: arrange/act/assert, xUnit v3, `NullLogger`, and folders that mirror `src/`. Where it falls short is **coverage of the risky parts** and **evidence**. + +### 🟠 6.1 In-process integration tests are missing +The two layers that handle money and I/O, `BankHttpClient` and `PaymentsRepository`, are "deliberately not unit-tested". The only thing that exercises them is an E2E suite that needs `docker compose` and is not run anywhere automatically. + +**Change:** add `PaymentGatewayApiIntegrationTests` built on `WebApplicationFactory`: +- Replace the bank with a stub `HttpMessageHandler`, or `WireMock.Net` in-process (the repo already has the mapping files). +- Run DynamoDB through `Testcontainers.DynamoDb`, or replace `IPaymentsRepository` for the pure HTTP-contract tests. +- Cover: status codes and body shapes per §3.1, the `MM/yyyy` expiry mapping (§2.4), **no retry on POST** (§2.1: the handler counts calls), the bank timing out, DynamoDB throwing after authorization (§2.2), idempotent replay (§2.3), and `CancellationToken` flow. + +### 🟡 6.2 The E2E tests reuse the server's own models +`tests/PaymentGatewayApiEndToEndTests/PaymentGatewayApiEndToEndTests.csproj:16-19` + +A "black-box" test that serializes with the server's own `PaymentRequest`/`PostPaymentResponse` cannot detect contract drift. Rename a property and both sides change together, and the test still passes. + +**Change:** give the E2E project its own small request/response records, or assert on raw JSON (`JsonNode`) with explicit property names. + +### 🟡 6.3 Specific test smells +- `PaymentControllerTests.PostPayment_CardNumberInvalid_SkipsRemainingChecks` (`:133-144`) has a commented-out line (`// this.SetupValidator(isValid: false);`). It relies on Moq's loose default returning `false`. Set the card check to `false` explicitly. +- `PostPayment_ValidationFails_…` uses a `switch` on method names to configure mocks. That is a sign the validator interface is shaped wrong (§4.5). +- `ErrorMiddlewareTests` asserts the leak (`Assert.Contains("boom", body)`); see §4.6. +- `IsCurrencyCodeValid_ThreeCharacters_ReturnsTrue("123")` pins a weak rule as correct. +- The E2E `Expiry` case uses `DateTime.Now.Year - 1` and `TestData` uses `DateTime.Now.Year + 1`. These are fine, but a fixed clock (`FakeTimeProvider`, §4.4) removes the time-of-year dependence from the unit layer entirely. +- No coverage is collected. Add `Microsoft.Testing.Extensions.CodeCoverage` and publish the percentage in CI. A reviewer trusts a badge more than a claim. +- Moq 4.20.69 is outdated (4.20.72). Given the 2023 SponsorLink episode, some reviewers prefer NSubstitute. That is optional, but know why you chose Moq. + +--- + +## 7. Engineering hygiene (first impressions) + +A reviewer sees these items **before** reading any C#. + +### 🟠 7.1 No CI +There is no `.github/workflows`. + +**Change:** add one workflow that runs `dotnet restore`, `dotnet build -warnaserror`, `dotnet format --verify-no-changes`, the unit and integration tests with coverage, and the vulnerable-package scan. Optionally add a second job that runs `docker compose --profile api up` plus the E2E suite. Put a status badge in the README. + +### 🟠 7.2 README +The current README is five lines. It contains `add later how to run / debug`, typos ("Intentional kept simple so that can demo"), and a claim that retries are omitted when they are not. Meanwhile `AGENTS.md`, written for an AI agent, is thorough. A reviewer notices the imbalance: the machine got better documentation than the human. + +**Change:** a README with: +- a one-paragraph purpose and an architecture diagram (the request flow in `AGENTS.md` is ready to reuse), +- a quick start (`docker compose up`, `dotnet run`, test commands), +- an API table with example requests and responses, +- the card-number → simulator behaviour table, +- **design decisions and trade-offs**: why DynamoDB, why the separate bank models, the retry and idempotency stance, what is out of scope and what would come next. + +A short "Known limitations / next steps" section turns the omissions into deliberate choices. + +### 🟡 7.3 Leftovers that read as unfinished +- `Program.cs:20-21`: a stale comment ("using simple httpClient for dev purpose") directly above code that does something else; commented-out `AddHttpClient()`; three blank lines (`:28-30`); commented-out links and code at the end (`:56-59`). +- `// todo try catch`, `// todo add try catch for all possible scenarios`, `// or return bad request if missing correlationId`, `// see if can replace message with correlationId`, `// most probably not needed, date should be already valid`: resolve each one or move it to a GitHub issue. +- Commented-out XML in `PaymentGatewayApiUnitTests.csproj:10,13-15`. +- `deploy/aws/.gitkeep` with `deploy/aws/*` ignored: an empty promise. Remove it or add real IaC (CDK/Terraform for the table). +- `launchSettings.json` still has the IIS Express profile and an unused `sslPort` from the template, plus a profile named `PaymentGateway.Api` that does not match the project name. +- `PutItemResponse response = …` is assigned and never used (`PaymentsRepository.cs:39`). +- Unused `using` directives: `System.Collections.Generic`, `System.Net`, `System.Net.Http`, `System.Text.Json` and `Microsoft.AspNetCore.Http` in `PaymentController.cs`; `Serilog` in `BankHttpClient.cs` and `BankService.cs`; `PaymentGatewayApi.Services` inside its own namespace in `BankService.cs`; `System`/`System.Net` in `BankResponseProcessor.cs`; `System.Net` in `PaymentsRepository.cs`. `ImplicitUsings` is disabled on purpose, and that choice only pays off if the explicit lists stay exact. Enforce it with IDE0005 as an error. +- Inconsistent `using` order: `System.*`, then `PaymentGatewayApi.*`, then `Microsoft.*` in some files; alphabetical in the tests. + +### 🟡 7.4 Build-quality guard rails +There is no `Directory.Build.props`, `.editorconfig`, `Directory.Packages.props` or `.gitattributes`. + +**Change:** +- A `Directory.Build.props` with `TargetFramework`, `Nullable`, `ImplicitUsings`, `TreatWarningsAsErrors`, `AnalysisLevel=latest-recommended`, `EnforceCodeStyleInBuild=true`, and optionally `GenerateDocumentationFile` for the API project. That removes the duplication across three csproj files. +- Central Package Management (`Directory.Packages.props`), which gives one place to bump versions. +- An `.editorconfig` that encodes the house style, including `this.` qualification (`dotnet_style_qualification_for_field = true:error`), so the convention is enforced by tooling and not only described in `AGENTS.md`. +- A `.gitattributes` (`* text=auto eol=lf`). Today `PaymentGatewayApiUnitTests.csproj` mixes CRLF and LF, `PaymentsRepository.cs` alone has a UTF-8 BOM, and seven files have no final newline (`README.md`, `ErrorMiddleware.cs`, `BankResponseProcessor.cs`, `BankService.cs`, `PaymentsRepository.cs`, `logconfig.json`, the unit-test csproj). +- Bump `AWSSDK.DynamoDBv2` to 4.0.104. Consider Dependabot. + +### 🟡 7.5 Logging configuration +`src/Program.cs:14-16`, `src/logconfig.json` + +- A second `ConfigurationBuilder` reads `logconfig.json` relative to the **working directory**. It gets no environment overrides (`logconfig.Development.json`), no environment variables and no reload. +- There is no bootstrap logger and no `try/catch/finally Log.CloseAndFlush()` around `app.Run()`. Startup failures go unlogged, and the file sink may lose buffered events on shutdown. +- In the container, logs are written to files inside an ephemeral filesystem. The console sink, the one that containers and cloud platforms collect, is "deliberately terse" and **omits CorrelationId**, which is the property that matters most in production. +- There is no `UseSerilogRequestLogging()`, so there is no single summary line per request with status and elapsed time. + +**Change:** move the Serilog section into `appsettings.json` / `appsettings.Development.json` and use `builder.Services.AddSerilog((sp, lc) => lc.ReadFrom.Configuration(builder.Configuration))`. Development gets pretty console plus file output; other environments get JSON to the console (`CompactJsonFormatter`). Add request logging and the flush. + +### 🟡 7.6 Environment configuration +`appsettings.json` has `AWS:ServiceURL = http://localhost:8000`. That is a development-only value in the **base** file, so any environment that forgets to override it points at localhost. + +**Change:** move it to `appsettings.Development.json`. Also document that `dotnet run` against DynamoDB Local still needs *some* AWS credentials to resolve (compose sets dummy ones for the container only). + +### ⚪ 7.7 Git history +The history has several commits named `.` (`95e9870`, `555f4f2`, `7a4ed04`, `0ffd78e`) and many named `minor …`. Reviewers do read `git log`. + +**Change:** +- From now on, use imperative, descriptive messages (Conventional Commits is optional) and squash-merge PRs with a meaningful title. +- Rewriting already-published history is your call. If the repo is mainly a portfolio piece, a single squash onto a clean `main` is defensible. +- Fix the heading `# CLAUDE.md` in `AGENTS.md`. +- Add a `LICENSE`. + +### ⚪ 7.8 Documentation drift in `AGENTS.md` +- Line 73 names `IPostPaymentRepository`, but the code has `IPaymentsRepository`. +- Line 39 says a bare `dotnet test` fails because of "the web project's `Microsoft.NET.Test.Sdk` reference". `src/PaymentGatewayApi.csproj` has no such reference, directly or transitively. + +Inaccurate docs cost a reviewer more trust than missing ones. + +--- + +## 8. Prioritised plan + +These are ordered by how much each item moves a senior reviewer's opinion per hour of work. + +| # | Change | Refs | Effort | +| --- | --- | --- | --- | +| 1 | No automatic retry on bank POST; typed bank client scoped to its own resilience pipeline | 2.1, 4.2 | S | +| 2 | Fix `MM/yyyy` expiry formatting, and make WireMock enforce the format | 2.4 | XS | +| 3 | Fix the three pinned defects (year 0, null-key persist, unhandled transport errors) and delete the `NOTE: defect` tests | 2.6-2.8 | S | +| 4 | Honest HTTP contract: 201 + `Location` + body, Declined as a result not a 400, 502/503 for bank failures, `ProblemDetails`/`ValidationProblemDetails` everywhere, string enums, `{id:guid}` | 3.1-3.4 | M | +| 5 | `AddExceptionHandler` + `AddProblemDetails`; stop leaking exception messages | 4.6 | S | +| 6 | Persist `Pending` → call the bank → update; store `AuthorizationCode`; separate persistence entity | 2.2, 2.5, 3.5 | M | +| 7 | `CancellationToken` everywhere; `TimeProvider`; Options + `ValidateOnStart` | 4.1, 4.3, 4.4 | S | +| 8 | CI workflow (build with warnings as errors, format check, tests + coverage, vulnerable-package scan) + `Directory.Build.props` + `.editorconfig` + CPM | 7.1, 7.4 | S | +| 9 | `WebApplicationFactory` integration tests covering the scenarios in 6.1 | 6.1 | M | +| 10 | README rewrite with design decisions and trade-offs; remove TODOs and commented code; fix `AGENTS.md` drift | 7.2, 7.3, 7.8 | S | +| 11 | `Idempotency-Key` support | 2.3 | M | +| 12 | API-key authentication + merchant scoping on GET; rate limiting; HTTPS/HSTS outside development | 5.1, 5.2 | M | +| 13 | Health checks (live and ready), non-root container, Serilog config consolidation, console output in JSON | 4.8, 5.4, 7.5 | S | +| 14 | Validator redesign (collect all errors, Luhn, ISO-4217 allow-list, minor units, `[GeneratedRegex]` with `[0-9]`) | 2.9, 4.5 | S | + +Items 1-10 are roughly two to three focused days. Once they are done, the repo shows a reviewer that the author knows where payment systems fail and knows the framework's own tools. That is the signal "builds high-quality software" depends on. diff --git a/src/Program.cs b/src/Program.cs index 25ef009..4f25dbe 100644 --- a/src/Program.cs +++ b/src/Program.cs @@ -7,6 +7,7 @@ using Microsoft.Extensions.Configuration; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Hosting; +using Microsoft.Extensions.Http.Resilience; using Serilog; var builder = WebApplication.CreateBuilder(args); @@ -17,16 +18,10 @@ builder.Services.AddControllers(); -// using simple httpClient for dev purpose, ideally should be a separate, fine tuned library for retries, circuit breaker -// builder.Services.AddHttpClient(); - -builder.Services.ConfigureHttpClientDefaults(http => -{ - http.AddStandardResilienceHandler(); -}); -builder.Services.AddHttpClient(); - - +// a payment POST is not idempotent, a retry after a lost response could charge the card twice; +// keep the timeouts and circuit breaker, but never retry it +builder.Services.AddHttpClient() + .AddStandardResilienceHandler(options => options.Retry.DisableForUnsafeHttpMethods()); var awsOptions = builder.Configuration.GetAWSOptions(); awsOptions.DefaultClientConfig.Timeout = TimeSpan.FromSeconds(10); @@ -36,7 +31,6 @@ builder.Services.AddSingleton(); builder.Services.AddScoped(); -builder.Services.AddScoped(); builder.Services.AddScoped(); builder.Services.AddScoped(); diff --git a/src/Services/BankHttpClient.cs b/src/Services/BankHttpClient.cs index ec64bb8..1b1c098 100644 --- a/src/Services/BankHttpClient.cs +++ b/src/Services/BankHttpClient.cs @@ -21,39 +21,41 @@ public class BankHttpClient : IBankHttpClient { private readonly ILogger logger; private readonly IConfiguration config; - private readonly IHttpClientFactory httpFactory; + private readonly HttpClient client; - public BankHttpClient(ILogger logger, IConfiguration config, IHttpClientFactory httpFactory) + public BankHttpClient(ILogger logger, IConfiguration config, HttpClient client) { this.logger = logger; this.config = config; - this.httpFactory = httpFactory; + this.client = client; } - public async Task PostBankPayment(PaymentRequest payment) + public static BankRequest ToBankRequest(PaymentRequest payment) { - // "04/2025" - string expiry = $"{payment.ExpiryMonth}/{payment.ExpiryYear}"; - BankRequest request = new() + return new BankRequest { + // the bank expects "MM/yyyy", e.g. "04/2025" CardNumber = payment.CardNumber, - ExpiryDate = expiry, + ExpiryDate = $"{payment.ExpiryMonth:D2}/{payment.ExpiryYear}", Currency = payment.Currency, Amount = payment.Amount, Cvv = payment.Cvv }; + } + public async Task PostBankPayment(PaymentRequest payment) + { + BankRequest request = ToBankRequest(payment); string uri = this.config["SimulatorBankEndpoint"] ?? string.Empty; - HttpClient client = this.httpFactory.CreateClient(); HttpResponseMessage response; try { - response = await client.PostAsJsonAsync(uri, request); + response = await this.client.PostAsJsonAsync(uri, request); } catch (Exception exception) { - // bank unreachable, timed out, or circuit open once the resilience handler gave up; + // bank unreachable, timed out, or circuit open; // answer as the bank would when unavailable so BankService maps it to InternalError this.logger.LogError(exception, "PostBankPayment, PaymentId={PaymentId}, Status={Status}", payment.PaymentId, "Failed"); diff --git a/src/Simulators/wiremock/mappings/authorized.json b/src/Simulators/wiremock/mappings/authorized.json index a11f16b..ddc83f9 100644 --- a/src/Simulators/wiremock/mappings/authorized.json +++ b/src/Simulators/wiremock/mappings/authorized.json @@ -4,7 +4,7 @@ "url": "/payments", "bodyPatterns": [ { "matchesJsonPath": { "expression": "$.card_number", "matches": "^.*[13579]$" } }, - { "matchesJsonPath": "$.expiry_date" }, + { "matchesJsonPath": { "expression": "$.expiry_date", "matches": "^(0[1-9]|1[0-2])/[0-9]{4}$" } }, { "matchesJsonPath": "$.currency" }, { "matchesJsonPath": "$.amount" }, { "matchesJsonPath": "$.cvv" } diff --git a/src/Simulators/wiremock/mappings/declined.json b/src/Simulators/wiremock/mappings/declined.json index 9b7ccb1..a7aa61f 100644 --- a/src/Simulators/wiremock/mappings/declined.json +++ b/src/Simulators/wiremock/mappings/declined.json @@ -4,7 +4,7 @@ "url": "/payments", "bodyPatterns": [ { "matchesJsonPath": { "expression": "$.card_number", "matches": "^.*[2468]$" } }, - { "matchesJsonPath": "$.expiry_date" }, + { "matchesJsonPath": { "expression": "$.expiry_date", "matches": "^(0[1-9]|1[0-2])/[0-9]{4}$" } }, { "matchesJsonPath": "$.currency" }, { "matchesJsonPath": "$.amount" }, { "matchesJsonPath": "$.cvv" } diff --git a/src/Simulators/wiremock/mappings/unavailable.json b/src/Simulators/wiremock/mappings/unavailable.json index 7fb2f3b..15bf743 100644 --- a/src/Simulators/wiremock/mappings/unavailable.json +++ b/src/Simulators/wiremock/mappings/unavailable.json @@ -4,7 +4,7 @@ "url": "/payments", "bodyPatterns": [ { "matchesJsonPath": { "expression": "$.card_number", "matches": "^.*0$" } }, - { "matchesJsonPath": "$.expiry_date" }, + { "matchesJsonPath": { "expression": "$.expiry_date", "matches": "^(0[1-9]|1[0-2])/[0-9]{4}$" } }, { "matchesJsonPath": "$.currency" }, { "matchesJsonPath": "$.amount" }, { "matchesJsonPath": "$.cvv" } diff --git a/tests/PaymentGatewayApiEndToEndTests/PaymentEndToEndTests.cs b/tests/PaymentGatewayApiEndToEndTests/PaymentEndToEndTests.cs index 66c3c91..73ff989 100644 --- a/tests/PaymentGatewayApiEndToEndTests/PaymentEndToEndTests.cs +++ b/tests/PaymentGatewayApiEndToEndTests/PaymentEndToEndTests.cs @@ -54,6 +54,18 @@ public async Task PostPayment_OddCard_AuthorizedAndRetrievable() Assert.Equal(payment.Amount, stored.Amount); } + [Fact] + public async Task PostPayment_SingleDigitExpiryMonth_Authorized() + { + // the simulator only answers a "MM/yyyy" expiry, so a month sent as "4/..." would fall through to its 400 + PaymentRequest payment = TestData.ValidPaymentRequest(TestData.AuthorizedCard); + payment.ExpiryMonth = 4; + + HttpResponseMessage response = await this.client.PostAsJsonAsync("api/payment", payment, TestContext.Current.CancellationToken); + + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + } + [Fact] public async Task PostPayment_EvenCard_Declined() { @@ -67,7 +79,6 @@ public async Task PostPayment_EvenCard_Declined() [Fact] public async Task PostPayment_CardEndingZero_InternalError() { - // slow, the resilience handler retries the simulator's 503 before giving up PaymentRequest payment = TestData.ValidPaymentRequest(TestData.BankUnavailableCard); HttpResponseMessage response = await this.client.PostAsJsonAsync("api/payment", payment, TestContext.Current.CancellationToken); diff --git a/tests/PaymentGatewayApiUnitTests/Services/BankHttpClientTests.cs b/tests/PaymentGatewayApiUnitTests/Services/BankHttpClientTests.cs new file mode 100644 index 0000000..8cadfbd --- /dev/null +++ b/tests/PaymentGatewayApiUnitTests/Services/BankHttpClientTests.cs @@ -0,0 +1,35 @@ +using PaymentGatewayApi.Models; +using PaymentGatewayApi.Services; +using Xunit; + +namespace PaymentGatewayApiUnitTests.Services; + +public class BankHttpClientTests +{ + [Theory] + [InlineData(4, 2027, "04/2027")] + [InlineData(11, 2027, "11/2027")] + public void ToBankRequest_FormatsExpiryAsTwoDigitMonthAndFourDigitYear(int month, int year, string expected) + { + PaymentRequest payment = TestData.ValidPaymentRequest(); + payment.ExpiryMonth = month; + payment.ExpiryYear = year; + + BankRequest request = BankHttpClient.ToBankRequest(payment); + + Assert.Equal(expected, request.ExpiryDate); + } + + [Fact] + public void ToBankRequest_MapsPaymentFields() + { + PaymentRequest payment = TestData.ValidPaymentRequest(); + + BankRequest request = BankHttpClient.ToBankRequest(payment); + + Assert.Equal("2222405343248873", request.CardNumber); + Assert.Equal("GBP", request.Currency); + Assert.Equal(100, request.Amount); + Assert.Equal("123", request.Cvv); + } +} From 0ce5137b33d3a1371836cf4a5df0e99c67f8b256 Mon Sep 17 00:00:00 2001 From: ati Date: Wed, 23 Sep 2026 23:29:45 +0100 Subject: [PATCH 02/13] reject out of range expiry years, never persist an unparseable bank answer An expiry year outside DateTime's range made the validator throw, so the merchant got a 500 instead of a rejection; year and month are now compared directly, and an expiry more than 20 years ahead is rejected too. When the bank's body could not be parsed, the processor returned a response with only Status set and BankService persisted it with a null hash key. The processor now returns null and BankService answers InternalError without persisting. Both pinned "NOTE: defect" tests become assertions of the fix. Co-Authored-By: Claude Opus 5.5 --- src/Services/BankResponseProcessor.cs | 13 ++++++------- src/Services/BankService.cs | 9 ++++++++- src/Services/PaymentRequestValidator.cs | 10 +++++++--- .../PaymentEndToEndTests.cs | 15 ++------------- .../Services/BankResponseProcessorTests.cs | 14 +++++++------- .../Services/BankServiceTests.cs | 11 +++++------ .../Services/PaymentRequestValidatorTests.cs | 18 ++++++------------ 7 files changed, 41 insertions(+), 49 deletions(-) diff --git a/src/Services/BankResponseProcessor.cs b/src/Services/BankResponseProcessor.cs index 6b24e7b..6544c3f 100644 --- a/src/Services/BankResponseProcessor.cs +++ b/src/Services/BankResponseProcessor.cs @@ -10,7 +10,8 @@ namespace PaymentGatewayApi.Services; public interface IBankResponseProcessor { - Task ParseBankResponse(PaymentRequest payment, HttpResponseMessage response); + // null when the bank's body cannot be used, so no half-filled response is ever persisted + Task ParseBankResponse(PaymentRequest payment, HttpResponseMessage response); } public class BankResponseProcessor : IBankResponseProcessor @@ -22,10 +23,9 @@ public BankResponseProcessor(ILogger logger) this.logger = logger; } - public async Task ParseBankResponse(PaymentRequest request, HttpResponseMessage response) + public async Task ParseBankResponse(PaymentRequest request, HttpResponseMessage response) { BankResponse? bankResponse; - PostPaymentResponse payResponse = new(); try { @@ -35,18 +35,17 @@ public async Task ParseBankResponse(PaymentRequest request, { this.logger.LogError(exception, "ParseBankResponse exception, PaymentId={PaymentId}, Status={Status}", request.PaymentId, "Failed"); - payResponse.Status = PaymentStatus.InternalError; - return payResponse; + return null; } if (bankResponse is null) { this.logger.LogError("ParseBankResponse null response, PaymentId={PaymentId}, Status={Status}", request.PaymentId, "Failed"); - payResponse.Status = PaymentStatus.InternalError; - return payResponse; + return null; } + PostPaymentResponse payResponse = new(); payResponse.Status = bankResponse.Authorized ? PaymentStatus.Authorized : PaymentStatus.Declined; payResponse.CardNumberLastFour = request.CardNumber.Substring(request.CardNumber.Length - 4); diff --git a/src/Services/BankService.cs b/src/Services/BankService.cs index ddb83bb..b4a1166 100644 --- a/src/Services/BankService.cs +++ b/src/Services/BankService.cs @@ -35,7 +35,14 @@ public async Task ExecutePayment(PaymentRequest payment) HttpResponseMessage bankResponse = await this.bankHttpClient.PostBankPayment(payment); if (bankResponse.IsSuccessStatusCode) { - PostPaymentResponse paymentResponse = await this.responseProcessor.ParseBankResponse(payment, bankResponse); + PostPaymentResponse? paymentResponse = await this.responseProcessor.ParseBankResponse(payment, bankResponse); + if (paymentResponse is null) + { + // the bank answered 2xx but its body is unusable, so the outcome is unknown and there is nothing to persist + this.logger.LogError("ExecuteBankPayment, PaymentId={PaymentId}, Status={Status}", payment.PaymentId, "Failed"); + return PaymentStatus.InternalError; + } + await this.paymentRepo.Add(paymentResponse); return paymentResponse.Status; diff --git a/src/Services/PaymentRequestValidator.cs b/src/Services/PaymentRequestValidator.cs index 86c7c94..74b4919 100644 --- a/src/Services/PaymentRequestValidator.cs +++ b/src/Services/PaymentRequestValidator.cs @@ -16,6 +16,9 @@ public interface IPaymentRequestValidator public class PaymentRequestValidator : IPaymentRequestValidator { + // cards are issued with an expiry a few years out, anything further is a typo or garbage + private const int MaxExpiryYearsAhead = 20; + private readonly ILogger logger; public PaymentRequestValidator(ILogger logger) @@ -38,7 +41,6 @@ public bool IsCardNumberValid(PaymentRequest payment) public bool IsExpiryValid(PaymentRequest payment, DateTime now) { - // most probably not needed, date should be already valid if (payment.ExpiryMonth < 1 || payment.ExpiryMonth > 12) { this.logger.LogInformation( @@ -48,8 +50,10 @@ public bool IsExpiryValid(PaymentRequest payment, DateTime now) return false; } - var expiry = new DateTime(payment.ExpiryYear, payment.ExpiryMonth, 1); - if (expiry < new DateTime(now.Year, now.Month, 1)) + // compare year and month directly, building a DateTime would throw for an out of range year + bool isExpired = payment.ExpiryYear < now.Year || (payment.ExpiryYear == now.Year && payment.ExpiryMonth < now.Month); + bool isTooFarAhead = payment.ExpiryYear > now.Year + MaxExpiryYearsAhead; + if (isExpired || isTooFarAhead) { this.logger.LogInformation( "Validator.IsExpiryValid, PaymentId={PaymentId}, IsValid={IsValid}", diff --git a/tests/PaymentGatewayApiEndToEndTests/PaymentEndToEndTests.cs b/tests/PaymentGatewayApiEndToEndTests/PaymentEndToEndTests.cs index 73ff989..b476c1a 100644 --- a/tests/PaymentGatewayApiEndToEndTests/PaymentEndToEndTests.cs +++ b/tests/PaymentGatewayApiEndToEndTests/PaymentEndToEndTests.cs @@ -89,6 +89,7 @@ public async Task PostPayment_CardEndingZero_InternalError() [Theory] [InlineData("CardNumber")] [InlineData("Expiry")] + [InlineData("ExpiryYearZero")] [InlineData("Amount")] [InlineData("Currency")] [InlineData("Cvv")] @@ -99,6 +100,7 @@ public async Task PostPayment_InvalidField_Rejected(string invalidField) { case "CardNumber": payment.CardNumber = "1234"; break; case "Expiry": payment.ExpiryYear = DateTime.Now.Year - 1; break; + case "ExpiryYearZero": payment.ExpiryYear = 0; break; case "Amount": payment.Amount = 0; break; case "Currency": payment.Currency = "GBPX"; break; case "Cvv": payment.Cvv = "12"; break; @@ -109,19 +111,6 @@ public async Task PostPayment_InvalidField_Rejected(string invalidField) await AssertBadRequest(response, PaymentStatus.Rejected); } - [Fact] - public async Task PostPayment_ExpiryYearZero_ReturnsInternalServerError() - { - // NOTE: defect - an out of range year is not validated, the validator throws and - // ErrorMiddleware answers 500 instead of the payment being rejected - PaymentRequest payment = TestData.ValidPaymentRequest(TestData.AuthorizedCard); - payment.ExpiryYear = 0; - - HttpResponseMessage response = await this.client.PostAsJsonAsync("api/payment", payment, TestContext.Current.CancellationToken); - - Assert.Equal(HttpStatusCode.InternalServerError, response.StatusCode); - } - [Fact] public async Task GetPayment_UnknownId_ReturnsNotFound() { diff --git a/tests/PaymentGatewayApiUnitTests/Services/BankResponseProcessorTests.cs b/tests/PaymentGatewayApiUnitTests/Services/BankResponseProcessorTests.cs index 827f372..486c37c 100644 --- a/tests/PaymentGatewayApiUnitTests/Services/BankResponseProcessorTests.cs +++ b/tests/PaymentGatewayApiUnitTests/Services/BankResponseProcessorTests.cs @@ -28,8 +28,9 @@ public async Task ParseBankResponse_Authorized_MapsRequestOntoResponse() using HttpResponseMessage response = JsonResponse( """{"authorized":true,"authorization_code":"0bb07405-6d44-4b50-a14f-7ae0beff13ad"}"""); - PostPaymentResponse result = await this.processor.ParseBankResponse(request, response); + PostPaymentResponse? result = await this.processor.ParseBankResponse(request, response); + Assert.NotNull(result); Assert.Equal(PaymentStatus.Authorized, result.Status); Assert.Equal(TestData.PaymentId, result.PaymentId); Assert.Equal("8873", result.CardNumberLastFour); @@ -45,8 +46,9 @@ public async Task ParseBankResponse_NotAuthorized_ReturnsDeclined() PaymentRequest request = TestData.ValidPaymentRequest(); using HttpResponseMessage response = JsonResponse("""{"authorized":false,"authorization_code":""}"""); - PostPaymentResponse result = await this.processor.ParseBankResponse(request, response); + PostPaymentResponse? result = await this.processor.ParseBankResponse(request, response); + Assert.NotNull(result); Assert.Equal(PaymentStatus.Declined, result.Status); Assert.Equal(TestData.PaymentId, result.PaymentId); Assert.Equal("8873", result.CardNumberLastFour); @@ -56,15 +58,13 @@ public async Task ParseBankResponse_NotAuthorized_ReturnsDeclined() [InlineData("{not json")] [InlineData("null")] [InlineData("")] - public async Task ParseBankResponse_UnusableBody_ReturnsInternalError(string body) + public async Task ParseBankResponse_UnusableBody_ReturnsNull(string body) { PaymentRequest request = TestData.ValidPaymentRequest(); using HttpResponseMessage response = JsonResponse(body); - PostPaymentResponse result = await this.processor.ParseBankResponse(request, response); + PostPaymentResponse? result = await this.processor.ParseBankResponse(request, response); - Assert.Equal(PaymentStatus.InternalError, result.Status); - Assert.Null(result.PaymentId); - Assert.Null(result.CardNumberLastFour); + Assert.Null(result); } } diff --git a/tests/PaymentGatewayApiUnitTests/Services/BankServiceTests.cs b/tests/PaymentGatewayApiUnitTests/Services/BankServiceTests.cs index 2dc7063..e3cebeb 100644 --- a/tests/PaymentGatewayApiUnitTests/Services/BankServiceTests.cs +++ b/tests/PaymentGatewayApiUnitTests/Services/BankServiceTests.cs @@ -68,17 +68,16 @@ public async Task ExecutePayment_BankReturnsNonSuccess_ReturnsInternalErrorWitho } [Fact] - public async Task ExecutePayment_BankBodyUnparseable_StillPersistsResponseWithoutPaymentId() + public async Task ExecutePayment_BankBodyUnparseable_ReturnsInternalErrorWithoutPersisting() { - // NOTE: defect - when the bank body cannot be parsed the processor returns a response with only - // Status set, and BankService persists it anyway, with a null PaymentId as the DynamoDB hash key PaymentRequest payment = TestData.ValidPaymentRequest(); - PostPaymentResponse parsed = new() { Status = PaymentStatus.InternalError }; - this.SetupBank(payment, HttpStatusCode.OK, parsed); + HttpResponseMessage bankResponse = new(HttpStatusCode.OK); + this.bankHttpClient.Setup(c => c.PostBankPayment(payment)).ReturnsAsync(bankResponse); + this.responseProcessor.Setup(p => p.ParseBankResponse(payment, bankResponse)).ReturnsAsync((PostPaymentResponse?)null); PaymentStatus result = await this.service.ExecutePayment(payment); Assert.Equal(PaymentStatus.InternalError, result); - this.paymentRepo.Verify(r => r.Add(It.Is(p => p.PaymentId == null)), Times.Once); + this.paymentRepo.Verify(r => r.Add(It.IsAny()), Times.Never); } } diff --git a/tests/PaymentGatewayApiUnitTests/Services/PaymentRequestValidatorTests.cs b/tests/PaymentGatewayApiUnitTests/Services/PaymentRequestValidatorTests.cs index 809d5c0..4401838 100644 --- a/tests/PaymentGatewayApiUnitTests/Services/PaymentRequestValidatorTests.cs +++ b/tests/PaymentGatewayApiUnitTests/Services/PaymentRequestValidatorTests.cs @@ -42,6 +42,7 @@ public void IsCardNumberValid_InvalidCardNumber_ReturnsFalse(string cardNumber) [InlineData(9, 2026)] // current month [InlineData(10, 2026)] [InlineData(1, 2027)] + [InlineData(12, 2046)] // 20 years ahead public void IsExpiryValid_CurrentOrFutureExpiry_ReturnsTrue(int month, int year) { PaymentRequest payment = TestData.ValidPaymentRequest(); @@ -56,7 +57,11 @@ public void IsExpiryValid_CurrentOrFutureExpiry_ReturnsTrue(int month, int year) [InlineData(12, 2025)] [InlineData(0, 2027)] [InlineData(13, 2027)] - public void IsExpiryValid_PastExpiryOrInvalidMonth_ReturnsFalse(int month, int year) + [InlineData(1, 2047)] // more than 20 years ahead + [InlineData(1, 0)] // out of DateTime's range + [InlineData(1, -1)] + [InlineData(1, int.MaxValue)] + public void IsExpiryValid_PastOrOutOfRangeExpiryOrInvalidMonth_ReturnsFalse(int month, int year) { PaymentRequest payment = TestData.ValidPaymentRequest(); payment.ExpiryMonth = month; @@ -65,17 +70,6 @@ public void IsExpiryValid_PastExpiryOrInvalidMonth_ReturnsFalse(int month, int y Assert.False(this.validator.IsExpiryValid(payment, Now)); } - [Fact] - public void IsExpiryValid_YearZero_Throws() - { - // NOTE: defect - an out of range year is not validated, so new DateTime(0, ...) throws - // instead of the payment being rejected - PaymentRequest payment = TestData.ValidPaymentRequest(); - payment.ExpiryYear = 0; - - Assert.Throws(() => this.validator.IsExpiryValid(payment, Now)); - } - [Theory] [InlineData(1)] [InlineData(100)] From a764754cfaf2f288ea4dab6dfef53ea4594dcd15 Mon Sep 17 00:00:00 2001 From: ati Date: Wed, 23 Sep 2026 23:34:09 +0100 Subject: [PATCH 03/13] enforce the house style with analyzers, editorconfig and central packages Directory.Build.props sets the shared target framework, nullable and usings settings, latest-recommended analyzers, code-style enforcement in build and warnings-as-errors. .editorconfig encodes the conventions AGENTS.md only described ("this." on fields, System usings first, no unused usings), and Directory.Packages.props gives one place to bump versions (AWSSDK.DynamoDBv2 4.0.104, Moq 4.20.72). .gitattributes pins LF line endings. Fixes what the analyzers found: logging moves to [LoggerMessage] source- generated methods (CA1848), unused usings are removed, IPaymentsRepository.Get becomes Find (CA1716), and the BOM and missing final newlines are gone. The Docker build context moves to the repository root so the image sees the shared props files. Co-Authored-By: Claude Opus 5.5 --- .dockerignore | 11 ++++ .editorconfig | 60 +++++++++++++++++++ .gitattributes | 2 + AGENTS.md | 13 ++-- Directory.Build.props | 17 ++++++ Directory.Packages.props | 20 +++++++ README.md | 2 +- global.json | 4 ++ src/.dockerignore | 7 --- src/Controllers/PaymentController.cs | 28 +++++---- src/Dockerfile | 14 +++-- src/Middleware/ErrorMiddleware.cs | 14 ++--- src/Middleware/LogMiddleware.cs | 4 +- src/Models/Responses/PostPaymentResponse.cs | 2 - src/PaymentGatewayApi.csproj | 16 ++--- src/Program.cs | 6 +- src/Services/BankHttpClient.cs | 17 +++--- src/Services/BankResponseProcessor.cs | 35 ++++++----- src/Services/BankService.cs | 20 ++++--- src/Services/PaymentRequestValidator.cs | 31 ++++------ src/Services/PaymentsRepository.cs | 34 ++++++----- src/docker-compose.yml | 4 +- src/logconfig.json | 2 +- .../ApiFixture.cs | 2 +- .../PaymentGatewayApiEndToEndTests.csproj | 27 ++++----- .../Controllers/PaymentControllerTests.cs | 4 +- .../Middleware/LogMiddlewareTests.cs | 2 +- .../PaymentGatewayApiUnitTests.csproj | 44 ++++++-------- 28 files changed, 274 insertions(+), 168 deletions(-) create mode 100644 .dockerignore create mode 100644 .editorconfig create mode 100644 .gitattributes create mode 100644 Directory.Build.props create mode 100644 Directory.Packages.props delete mode 100644 src/.dockerignore diff --git a/.dockerignore b/.dockerignore new file mode 100644 index 0000000..89e6c3a --- /dev/null +++ b/.dockerignore @@ -0,0 +1,11 @@ +**/bin/ +**/obj/ +.git/ +.github/ +tests/ +deploy/ +*.md +src/z-logs/ +src/Simulators/ +src/docker-compose.yml +src/Dockerfile diff --git a/.editorconfig b/.editorconfig new file mode 100644 index 0000000..ba5496e --- /dev/null +++ b/.editorconfig @@ -0,0 +1,60 @@ +root = true + +[*] +charset = utf-8 +end_of_line = lf +insert_final_newline = true +trim_trailing_whitespace = true +indent_style = space +indent_size = 4 + +[*.{json,yml,yaml,props,csproj,slnx,xml}] +indent_size = 2 + +[*.md] +trim_trailing_whitespace = false + +[*.cs] +# usings: System first, then alphabetical, outside the file-scoped namespace +dotnet_sort_system_directives_first = true +dotnet_separate_import_directive_groups = false +csharp_using_directive_placement = outside_namespace:error +dotnet_diagnostic.IDE0005.severity = error +dotnet_diagnostic.IDE0065.severity = error + +csharp_style_namespace_declarations = file_scoped:error +dotnet_diagnostic.IDE0161.severity = error + +# house style: "this." on every field access, and only on fields +dotnet_style_qualification_for_field = true:error +dotnet_style_qualification_for_property = false +dotnet_style_qualification_for_method = false +dotnet_style_qualification_for_event = false +dotnet_diagnostic.IDE0009.severity = error + +dotnet_style_readonly_field = true:error +dotnet_diagnostic.IDE0044.severity = error + +csharp_prefer_braces = true:error +dotnet_diagnostic.IDE0011.severity = error + +# private static readonly fields are PascalCase, like constants +dotnet_naming_rule.static_readonly_fields_pascal_case.severity = error +dotnet_naming_rule.static_readonly_fields_pascal_case.symbols = static_readonly_fields +dotnet_naming_rule.static_readonly_fields_pascal_case.style = pascal_case +dotnet_naming_symbols.static_readonly_fields.applicable_kinds = field +dotnet_naming_symbols.static_readonly_fields.required_modifiers = static, readonly +dotnet_naming_style.pascal_case.capitalization = pascal_case + +# private instance fields are camelCase without an underscore prefix +dotnet_naming_rule.private_fields_camel_case.severity = error +dotnet_naming_rule.private_fields_camel_case.symbols = private_fields +dotnet_naming_rule.private_fields_camel_case.style = camel_case +dotnet_naming_symbols.private_fields.applicable_kinds = field +dotnet_naming_symbols.private_fields.applicable_accessibilities = private +dotnet_naming_symbols.private_fields.required_modifiers = readonly +dotnet_naming_style.camel_case.capitalization = camel_case + +[tests/**.cs] +# test names read as Method_Scenario_Expectation +dotnet_diagnostic.CA1707.severity = none diff --git a/.gitattributes b/.gitattributes new file mode 100644 index 0000000..d0d6371 --- /dev/null +++ b/.gitattributes @@ -0,0 +1,2 @@ +* text=auto eol=lf +*.png binary diff --git a/AGENTS.md b/AGENTS.md index 31ad83e..f40b810 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -18,7 +18,7 @@ API container on `:5067` (don't combine it with `dotnet run`, they share the por ```bash dotnet build PaymentGatewayApi.slnx # from repo root dotnet run --project src/PaymentGatewayApi.csproj # http://localhost:5067, Swagger at /swagger -docker build -t payment-gateway-api src/ # container image +docker build -f src/Dockerfile -t payment-gateway-api . # container image, from repo root ``` `dotnet run` sets the working directory to `src/`, which is what makes `logconfig.json` and @@ -122,10 +122,15 @@ shadows the specific stubs; WireMock breaks equal-priority ties by load order. ## Conventions +- **The build enforces the style.** `Directory.Build.props` turns on `latest-recommended` + analyzers, code-style enforcement and warnings-as-errors; `.editorconfig` holds the rules. + Run `dotnet format PaymentGatewayApi.slnx` before committing. Package versions live only in + `Directory.Packages.props` (central package management). - **Explicit `using` directives** — `ImplicitUsings` is disabled, so every file declares - its own. `Nullable` is enabled. -- **`this.` prefix** on all field access, consistently throughout. -- **Structured logging** follows one template shape: + its own, System first; unused ones fail the build (IDE0005). `Nullable` is enabled. +- **`this.` prefix** on all field access and only there (IDE0009). +- **Logging goes through `[LoggerMessage]`** partial methods at the bottom of each class + (`CA1848` rejects `logger.LogX(...)` calls). Templates follow one shape: `"MethodName, PaymentId={PaymentId}, Status={Status}"`, with `Status` values like `"Started"` / `"Completed"` / `"Failed"`. `CallerId` and `CorrelationId` are enriched automatically by `LogMiddleware` from the request headers (a missing `CorrelationId` diff --git a/Directory.Build.props b/Directory.Build.props new file mode 100644 index 0000000..34c49af --- /dev/null +++ b/Directory.Build.props @@ -0,0 +1,17 @@ + + + + net10.0 + enable + disable + + true + latest-recommended + true + + + true + $(NoWarn);CS1591 + + + diff --git a/Directory.Packages.props b/Directory.Packages.props new file mode 100644 index 0000000..bf8e3a8 --- /dev/null +++ b/Directory.Packages.props @@ -0,0 +1,20 @@ + + + + true + + + + + + + + + + + + + + + + diff --git a/README.md b/README.md index dba2873..aa4e717 100644 --- a/README.md +++ b/README.md @@ -8,4 +8,4 @@ Certain things are omitted (verbosity in logs, authorization, https, http retrie More verbose than console ones logs are in directory `z-logs`. -add later how to run / debug \ No newline at end of file +add later how to run / debug diff --git a/global.json b/global.json index 3140116..1d364c6 100644 --- a/global.json +++ b/global.json @@ -1,4 +1,8 @@ { + "sdk": { + "version": "10.0.100", + "rollForward": "latestFeature" + }, "test": { "runner": "Microsoft.Testing.Platform" } diff --git a/src/.dockerignore b/src/.dockerignore deleted file mode 100644 index 1249f94..0000000 --- a/src/.dockerignore +++ /dev/null @@ -1,7 +0,0 @@ -bin/ -obj/ -z-logs/ -Simulators/ -docker-compose.yml -Dockerfile -.dockerignore diff --git a/src/Controllers/PaymentController.cs b/src/Controllers/PaymentController.cs index 8e05c74..bee22ff 100644 --- a/src/Controllers/PaymentController.cs +++ b/src/Controllers/PaymentController.cs @@ -1,20 +1,15 @@ using System; -using System.Collections.Generic; -using System.Net; -using System.Net.Http; -using System.Text.Json; using System.Threading.Tasks; -using PaymentGatewayApi.Models; -using PaymentGatewayApi.Services; -using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Mvc; using Microsoft.Extensions.Logging; +using PaymentGatewayApi.Models; +using PaymentGatewayApi.Services; namespace PaymentGatewayApi.Controllers; [Route("api/[controller]")] [ApiController] -public class PaymentController : ControllerBase +public partial class PaymentController : ControllerBase { private readonly ILogger logger; private readonly IPaymentsRepository paymentRepo; @@ -34,7 +29,7 @@ public PaymentController( [HttpGet()] public ActionResult GetHealthcheck() { - this.logger.LogInformation("GetHealthcheck, Status={Status}", "Started"); + this.LogGetHealthcheck("Started"); return new OkObjectResult("ok"); } @@ -43,9 +38,9 @@ public ActionResult GetHealthcheck() [HttpGet("{paymentId}")] public async Task> GetPayment(string paymentId) { - this.logger.LogInformation("GetPayment, PaymentId={PaymentId}, Status={Status}", paymentId, "Started"); + this.LogGetPayment(paymentId, "Started"); - var payment = await this.paymentRepo.Get(paymentId); + var payment = await this.paymentRepo.Find(paymentId); if (payment is null) { return new NotFoundObjectResult("payment not found"); @@ -58,7 +53,7 @@ public async Task> GetPayment(string paymentId public async Task> PostPayment([FromBody] PaymentRequest payment) { payment.PaymentId = Guid.NewGuid().ToString(); - this.logger.LogInformation("PostPayment, PaymentId={PaymentId}, Status={Status}", payment.PaymentId, "Started"); + this.LogPostPayment(payment.PaymentId, "Started"); bool isValid = this.validator.IsCardNumberValid(payment) && @@ -82,4 +77,13 @@ public async Task> PostPayment([FromBody] PaymentReq return new BadRequestObjectResult(status.ToString()); } + + [LoggerMessage(Level = LogLevel.Information, Message = "GetHealthcheck, Status={Status}")] + private partial void LogGetHealthcheck(string status); + + [LoggerMessage(Level = LogLevel.Information, Message = "GetPayment, PaymentId={PaymentId}, Status={Status}")] + private partial void LogGetPayment(string paymentId, string status); + + [LoggerMessage(Level = LogLevel.Information, Message = "PostPayment, PaymentId={PaymentId}, Status={Status}")] + private partial void LogPostPayment(string? paymentId, string status); } diff --git a/src/Dockerfile b/src/Dockerfile index d6fd2a8..74d51a2 100644 --- a/src/Dockerfile +++ b/src/Dockerfile @@ -1,13 +1,17 @@ +# built from the repository root, it needs the shared Directory.Build.props / Directory.Packages.props: +# docker build -f src/Dockerfile -t payment-gateway-api . FROM mcr.microsoft.com/dotnet/sdk:10.0 AS build -WORKDIR /src +WORKDIR /repo -COPY PaymentGatewayApi.csproj . -RUN dotnet restore PaymentGatewayApi.csproj +COPY global.json Directory.Build.props Directory.Packages.props .editorconfig ./ +COPY src/PaymentGatewayApi.csproj src/ +RUN dotnet restore src/PaymentGatewayApi.csproj -COPY . . -RUN dotnet publish PaymentGatewayApi.csproj \ +COPY src/ src/ +RUN dotnet publish src/PaymentGatewayApi.csproj \ -c Release \ -o /app/publish \ + --no-restore \ /p:UseAppHost=false FROM mcr.microsoft.com/dotnet/aspnet:10.0 AS final diff --git a/src/Middleware/ErrorMiddleware.cs b/src/Middleware/ErrorMiddleware.cs index 13922cd..acf973d 100644 --- a/src/Middleware/ErrorMiddleware.cs +++ b/src/Middleware/ErrorMiddleware.cs @@ -5,7 +5,7 @@ namespace PaymentGatewayApi.Middleware; -public class ErrorMiddleware +public partial class ErrorMiddleware { private readonly RequestDelegate next; private readonly ILogger logger; @@ -20,14 +20,11 @@ public async Task InvokeAsync(HttpContext context) { try { - await next(context); + await this.next(context); } catch (Exception exception) { - this.logger.LogError( - exception, - "Unhandled exception, Status={Status}", - "Failed"); + this.LogUnhandled(exception, "Failed"); context.Response.StatusCode = StatusCodes.Status500InternalServerError; @@ -37,4 +34,7 @@ public async Task InvokeAsync(HttpContext context) return; } } -} \ No newline at end of file + + [LoggerMessage(Level = LogLevel.Error, Message = "Unhandled exception, Status={Status}")] + private partial void LogUnhandled(Exception exception, string status); +} diff --git a/src/Middleware/LogMiddleware.cs b/src/Middleware/LogMiddleware.cs index f98d992..ed0ef86 100644 --- a/src/Middleware/LogMiddleware.cs +++ b/src/Middleware/LogMiddleware.cs @@ -23,7 +23,7 @@ public async Task InvokeAsync(HttpContext context) { correlationId = correlationHeader; } - else + else { // or return bad request if missing correlationId correlationId = Guid.NewGuid().ToString(); @@ -32,7 +32,7 @@ public async Task InvokeAsync(HttpContext context) using (LogContext.PushProperty("CallerId", context.Request.Headers["CallerId"])) using (LogContext.PushProperty("CorrelationId", correlationId)) { - await next(context); + await this.next(context); } } } diff --git a/src/Models/Responses/PostPaymentResponse.cs b/src/Models/Responses/PostPaymentResponse.cs index d84de15..8842659 100644 --- a/src/Models/Responses/PostPaymentResponse.cs +++ b/src/Models/Responses/PostPaymentResponse.cs @@ -1,5 +1,3 @@ -using System; - namespace PaymentGatewayApi.Models; public class PostPaymentResponse diff --git a/src/PaymentGatewayApi.csproj b/src/PaymentGatewayApi.csproj index 9a3d37a..bbf30c0 100644 --- a/src/PaymentGatewayApi.csproj +++ b/src/PaymentGatewayApi.csproj @@ -1,17 +1,11 @@ - - net10.0 - enable - disable - - - - - - - + + + + + diff --git a/src/Program.cs b/src/Program.cs index 4f25dbe..d1e7a3b 100644 --- a/src/Program.cs +++ b/src/Program.cs @@ -1,13 +1,11 @@ using System; using Amazon.DynamoDBv2; -using Amazon.Extensions.NETCore.Setup; -using PaymentGatewayApi.Middleware; -using PaymentGatewayApi.Services; using Microsoft.AspNetCore.Builder; using Microsoft.Extensions.Configuration; using Microsoft.Extensions.DependencyInjection; -using Microsoft.Extensions.Hosting; using Microsoft.Extensions.Http.Resilience; +using PaymentGatewayApi.Middleware; +using PaymentGatewayApi.Services; using Serilog; var builder = WebApplication.CreateBuilder(args); diff --git a/src/Services/BankHttpClient.cs b/src/Services/BankHttpClient.cs index 1b1c098..c813495 100644 --- a/src/Services/BankHttpClient.cs +++ b/src/Services/BankHttpClient.cs @@ -3,10 +3,9 @@ using System.Net.Http; using System.Net.Http.Json; using System.Threading.Tasks; -using PaymentGatewayApi.Models; using Microsoft.Extensions.Configuration; using Microsoft.Extensions.Logging; -using Serilog; +using PaymentGatewayApi.Models; namespace PaymentGatewayApi.Services; @@ -17,7 +16,7 @@ public interface IBankHttpClient -public class BankHttpClient : IBankHttpClient +public partial class BankHttpClient : IBankHttpClient { private readonly ILogger logger; private readonly IConfiguration config; @@ -57,15 +56,19 @@ public async Task PostBankPayment(PaymentRequest payment) { // bank unreachable, timed out, or circuit open; // answer as the bank would when unavailable so BankService maps it to InternalError - this.logger.LogError(exception, "PostBankPayment, PaymentId={PaymentId}, Status={Status}", payment.PaymentId, "Failed"); + this.LogPostFailed(exception, payment.PaymentId, "Failed"); return new HttpResponseMessage(HttpStatusCode.ServiceUnavailable); } - this.logger.LogInformation( - "PostBankPayment, PaymentId={PaymentId}, StatusCode={StatusCode}", - payment.PaymentId, (int)response.StatusCode); + this.LogPosted(payment.PaymentId, "Completed", (int)response.StatusCode); return response; } + + [LoggerMessage(Level = LogLevel.Error, Message = "PostBankPayment, PaymentId={PaymentId}, Status={Status}")] + private partial void LogPostFailed(Exception exception, string? paymentId, string status); + + [LoggerMessage(Level = LogLevel.Information, Message = "PostBankPayment, PaymentId={PaymentId}, Status={Status}, StatusCode={StatusCode}")] + private partial void LogPosted(string? paymentId, string status, int statusCode); } diff --git a/src/Services/BankResponseProcessor.cs b/src/Services/BankResponseProcessor.cs index 6544c3f..000345c 100644 --- a/src/Services/BankResponseProcessor.cs +++ b/src/Services/BankResponseProcessor.cs @@ -2,9 +2,8 @@ using System.Net.Http; using System.Net.Http.Json; using System.Threading.Tasks; -using PaymentGatewayApi.Models; using Microsoft.Extensions.Logging; -using System.Net; +using PaymentGatewayApi.Models; namespace PaymentGatewayApi.Services; @@ -14,7 +13,7 @@ public interface IBankResponseProcessor Task ParseBankResponse(PaymentRequest payment, HttpResponseMessage response); } -public class BankResponseProcessor : IBankResponseProcessor +public partial class BankResponseProcessor : IBankResponseProcessor { private readonly ILogger logger; @@ -23,7 +22,7 @@ public BankResponseProcessor(ILogger logger) this.logger = logger; } - public async Task ParseBankResponse(PaymentRequest request, HttpResponseMessage response) + public async Task ParseBankResponse(PaymentRequest payment, HttpResponseMessage response) { BankResponse? bankResponse; @@ -33,32 +32,36 @@ public BankResponseProcessor(ILogger logger) } catch (Exception exception) { - this.logger.LogError(exception, "ParseBankResponse exception, PaymentId={PaymentId}, Status={Status}", request.PaymentId, "Failed"); + this.LogUnparseable(exception, payment.PaymentId, "Failed"); return null; } if (bankResponse is null) { - this.logger.LogError("ParseBankResponse null response, PaymentId={PaymentId}, Status={Status}", request.PaymentId, "Failed"); + this.LogUnparseable(null, payment.PaymentId, "Failed"); return null; } PostPaymentResponse payResponse = new(); payResponse.Status = bankResponse.Authorized ? PaymentStatus.Authorized : PaymentStatus.Declined; - payResponse.CardNumberLastFour = request.CardNumber.Substring(request.CardNumber.Length - 4); + payResponse.CardNumberLastFour = payment.CardNumber.Substring(payment.CardNumber.Length - 4); - payResponse.PaymentId = request.PaymentId; - payResponse.ExpiryMonth = request.ExpiryMonth; - payResponse.ExpiryYear = request.ExpiryYear; - payResponse.Currency = request.Currency; - payResponse.Amount = request.Amount; + payResponse.PaymentId = payment.PaymentId; + payResponse.ExpiryMonth = payment.ExpiryMonth; + payResponse.ExpiryYear = payment.ExpiryYear; + payResponse.Currency = payment.Currency; + payResponse.Amount = payment.Amount; - this.logger.LogInformation( - "ParseBankResponse, PaymentId={PaymentId}, Status={Status}, Authorized={Authorized}", - request.PaymentId, "Completed", bankResponse.Authorized); + this.LogParsed(payment.PaymentId, "Completed", bankResponse.Authorized); return payResponse; } -} \ No newline at end of file + + [LoggerMessage(Level = LogLevel.Error, Message = "ParseBankResponse, PaymentId={PaymentId}, Status={Status}")] + private partial void LogUnparseable(Exception? exception, string? paymentId, string status); + + [LoggerMessage(Level = LogLevel.Information, Message = "ParseBankResponse, PaymentId={PaymentId}, Status={Status}, Authorized={Authorized}")] + private partial void LogParsed(string? paymentId, string status, bool authorized); +} diff --git a/src/Services/BankService.cs b/src/Services/BankService.cs index b4a1166..a447cf9 100644 --- a/src/Services/BankService.cs +++ b/src/Services/BankService.cs @@ -1,9 +1,7 @@ using System.Net.Http; using System.Threading.Tasks; -using PaymentGatewayApi.Models; -using PaymentGatewayApi.Services; using Microsoft.Extensions.Logging; -using Serilog; +using PaymentGatewayApi.Models; namespace PaymentGatewayApi.Services; @@ -12,7 +10,7 @@ public interface IBankService Task ExecutePayment(PaymentRequest payment); } -public class BankService : IBankService +public partial class BankService : IBankService { private readonly ILogger logger; private readonly IBankHttpClient bankHttpClient; @@ -30,7 +28,7 @@ public BankService( public async Task ExecutePayment(PaymentRequest payment) { - this.logger.LogInformation("ExecuteBankPayment, PaymentId={PaymentId}, Status={Status}", payment.PaymentId, "Started"); + this.LogExecutePayment(payment.PaymentId, "Started"); HttpResponseMessage bankResponse = await this.bankHttpClient.PostBankPayment(payment); if (bankResponse.IsSuccessStatusCode) @@ -39,7 +37,7 @@ public async Task ExecutePayment(PaymentRequest payment) if (paymentResponse is null) { // the bank answered 2xx but its body is unusable, so the outcome is unknown and there is nothing to persist - this.logger.LogError("ExecuteBankPayment, PaymentId={PaymentId}, Status={Status}", payment.PaymentId, "Failed"); + this.LogExecutePaymentFailed(payment.PaymentId, "Failed"); return PaymentStatus.InternalError; } @@ -49,8 +47,14 @@ public async Task ExecutePayment(PaymentRequest payment) } else { - this.logger.LogError("ExecuteBankPayment, PaymentId={PaymentId}, Status={Status}", payment.PaymentId, "Failed"); + this.LogExecutePaymentFailed(payment.PaymentId, "Failed"); return PaymentStatus.InternalError; } } -} \ No newline at end of file + + [LoggerMessage(Level = LogLevel.Information, Message = "ExecutePayment, PaymentId={PaymentId}, Status={Status}")] + private partial void LogExecutePayment(string? paymentId, string status); + + [LoggerMessage(Level = LogLevel.Error, Message = "ExecutePayment, PaymentId={PaymentId}, Status={Status}")] + private partial void LogExecutePaymentFailed(string? paymentId, string status); +} diff --git a/src/Services/PaymentRequestValidator.cs b/src/Services/PaymentRequestValidator.cs index 74b4919..294286d 100644 --- a/src/Services/PaymentRequestValidator.cs +++ b/src/Services/PaymentRequestValidator.cs @@ -1,7 +1,7 @@ using System; using System.Text.RegularExpressions; -using PaymentGatewayApi.Models; using Microsoft.Extensions.Logging; +using PaymentGatewayApi.Models; namespace PaymentGatewayApi.Services; @@ -14,7 +14,7 @@ public interface IPaymentRequestValidator bool IsCvvValid(PaymentRequest payment); } -public class PaymentRequestValidator : IPaymentRequestValidator +public partial class PaymentRequestValidator : IPaymentRequestValidator { // cards are issued with an expiry a few years out, anything further is a typo or garbage private const int MaxExpiryYearsAhead = 20; @@ -30,9 +30,7 @@ public bool IsCardNumberValid(PaymentRequest payment) { if (!Regex.IsMatch(payment.CardNumber, @"^\d{14,19}$")) { - this.logger.LogInformation( - "Validator.IsCardNumberValid, PaymentId={PaymentId}, IsValid={IsValid}", - payment.PaymentId, false); + this.LogRejected(payment.PaymentId, "Rejected", "IsCardNumberValid"); return false; } @@ -43,9 +41,7 @@ public bool IsExpiryValid(PaymentRequest payment, DateTime now) { if (payment.ExpiryMonth < 1 || payment.ExpiryMonth > 12) { - this.logger.LogInformation( - "Validator.IsExpiryMonthValid, PaymentId={PaymentId}, IsValid={IsValid}", - payment.PaymentId, false); + this.LogRejected(payment.PaymentId, "Rejected", "IsExpiryMonthValid"); return false; } @@ -55,9 +51,7 @@ public bool IsExpiryValid(PaymentRequest payment, DateTime now) bool isTooFarAhead = payment.ExpiryYear > now.Year + MaxExpiryYearsAhead; if (isExpired || isTooFarAhead) { - this.logger.LogInformation( - "Validator.IsExpiryValid, PaymentId={PaymentId}, IsValid={IsValid}", - payment.PaymentId, false); + this.LogRejected(payment.PaymentId, "Rejected", "IsExpiryValid"); return false; } @@ -69,9 +63,7 @@ public bool IsAmountValid(PaymentRequest payment) { if (payment.Amount < 1) { - this.logger.LogInformation( - "Validator.IsAmountValid, PaymentId={PaymentId}, IsValid={IsValid}", - payment.PaymentId, false); + this.LogRejected(payment.PaymentId, "Rejected", "IsAmountValid"); return false; } @@ -84,9 +76,7 @@ public bool IsCurrencyCodeValid(PaymentRequest payment) // should be a list of accepted currencies, via database, config if (payment.Currency.Length != 3) { - this.logger.LogInformation( - "Validator.IsCurrencyCodeValid, PaymentId={PaymentId}, IsValid={IsValid}", - payment.PaymentId, false); + this.LogRejected(payment.PaymentId, "Rejected", "IsCurrencyCodeValid"); return false; } @@ -99,13 +89,14 @@ public bool IsCvvValid(PaymentRequest payment) // 3 or 4 digits if (!Regex.IsMatch(payment.Cvv, @"^\d{3,4}$")) { - this.logger.LogInformation( - "Validator.IsCvvValid, PaymentId={PaymentId}, IsValid={IsValid}", - payment.PaymentId, false); + this.LogRejected(payment.PaymentId, "Rejected", "IsCvvValid"); return false; } return true; } + + [LoggerMessage(Level = LogLevel.Information, Message = "Validate, PaymentId={PaymentId}, Status={Status}, Rule={Rule}")] + private partial void LogRejected(string? paymentId, string status, string rule); } diff --git a/src/Services/PaymentsRepository.cs b/src/Services/PaymentsRepository.cs index 32423d8..dffdcd0 100644 --- a/src/Services/PaymentsRepository.cs +++ b/src/Services/PaymentsRepository.cs @@ -1,23 +1,21 @@ -using System; +using System; using System.Collections.Generic; -using System.Linq; -using System.Net; using System.Text.Json; using System.Threading.Tasks; using Amazon.DynamoDBv2; using Amazon.DynamoDBv2.Model; -using PaymentGatewayApi.Models; using Microsoft.Extensions.Logging; +using PaymentGatewayApi.Models; namespace PaymentGatewayApi.Services; public interface IPaymentsRepository { Task Add(PostPaymentResponse payment); - Task Get(string paymentId); + Task Find(string paymentId); } -public class PaymentsRepository : IPaymentsRepository +public partial class PaymentsRepository : IPaymentsRepository { private readonly ILogger logger; private readonly IAmazonDynamoDB dynamo; @@ -44,14 +42,11 @@ public async Task Add(PostPaymentResponse payment) { // the bank has already answered, so failing the request here would hide an authorized payment // from the merchant; log the raw item instead so it can be reconciled - this.logger.LogError( - exception, - "AddPayment, PaymentId={PaymentId}, Status={Status}, PaymentRaw={PaymentRaw}", - payment.PaymentId, "Failed", item["PaymentRaw"].S); + this.LogAddFailed(exception, payment.PaymentId, "Failed", item["PaymentRaw"].S); } } - public async Task Get(string paymentId) + public async Task Find(string paymentId) { var key = new Dictionary { @@ -65,7 +60,7 @@ public async Task Add(PostPaymentResponse payment) { GetItemResponse response = await this.dynamo.GetItemAsync(request); - if (response.Item != null && response.Item.Any()) + if (response.Item is { Count: > 0 }) { string raw = response.Item["PaymentRaw"].S; payment = JsonSerializer.Deserialize(raw); @@ -78,7 +73,7 @@ public async Task Add(PostPaymentResponse payment) catch (Exception exception) { // rethrow so ErrorMiddleware answers 500: returning null would report an unreadable payment as not found - this.logger.LogError(exception, "GetPayment, PaymentId={PaymentId}, Status={Status}", paymentId, "Failed"); + this.LogGetFailed(exception, paymentId, "Failed"); throw; } @@ -88,9 +83,18 @@ public async Task Add(PostPaymentResponse payment) } else { - this.logger.LogInformation("GetPayment, PaymentId={PaymentId}, Status={Status}", paymentId, "NotFound"); + this.LogGetNotFound(paymentId, "NotFound"); return null; } } -} \ No newline at end of file + + [LoggerMessage(Level = LogLevel.Error, Message = "AddPayment, PaymentId={PaymentId}, Status={Status}, PaymentRaw={PaymentRaw}")] + private partial void LogAddFailed(Exception exception, string? paymentId, string status, string paymentRaw); + + [LoggerMessage(Level = LogLevel.Error, Message = "GetPayment, PaymentId={PaymentId}, Status={Status}")] + private partial void LogGetFailed(Exception exception, string paymentId, string status); + + [LoggerMessage(Level = LogLevel.Information, Message = "GetPayment, PaymentId={PaymentId}, Status={Status}")] + private partial void LogGetNotFound(string paymentId, string status); +} diff --git a/src/docker-compose.yml b/src/docker-compose.yml index 6cf5efb..b761ceb 100644 --- a/src/docker-compose.yml +++ b/src/docker-compose.yml @@ -53,7 +53,9 @@ services: payment-gateway-api: container_name: payment-gateway-api profiles: ["api"] - build: . + build: + context: .. + dockerfile: src/Dockerfile ports: - "5067:8080" environment: diff --git a/src/logconfig.json b/src/logconfig.json index 9b4268c..1f48554 100644 --- a/src/logconfig.json +++ b/src/logconfig.json @@ -41,4 +41,4 @@ "FromLogContext" ] } -} \ No newline at end of file +} diff --git a/tests/PaymentGatewayApiEndToEndTests/ApiFixture.cs b/tests/PaymentGatewayApiEndToEndTests/ApiFixture.cs index 4c811da..a511f3f 100644 --- a/tests/PaymentGatewayApiEndToEndTests/ApiFixture.cs +++ b/tests/PaymentGatewayApiEndToEndTests/ApiFixture.cs @@ -9,7 +9,7 @@ namespace PaymentGatewayApiEndToEndTests; // shared by every test, waits for the api started by docker compose to answer its healthcheck -public class ApiFixture : IAsyncLifetime +public sealed class ApiFixture : IAsyncLifetime { private static readonly TimeSpan StartupTimeout = TimeSpan.FromSeconds(30); diff --git a/tests/PaymentGatewayApiEndToEndTests/PaymentGatewayApiEndToEndTests.csproj b/tests/PaymentGatewayApiEndToEndTests/PaymentGatewayApiEndToEndTests.csproj index 067b8c6..88e9963 100644 --- a/tests/PaymentGatewayApiEndToEndTests/PaymentGatewayApiEndToEndTests.csproj +++ b/tests/PaymentGatewayApiEndToEndTests/PaymentGatewayApiEndToEndTests.csproj @@ -1,21 +1,18 @@ - - disable - enable - Exe - net10.0 - true - false - + + Exe + true + false + - - - + + + - - - - + + + + diff --git a/tests/PaymentGatewayApiUnitTests/Controllers/PaymentControllerTests.cs b/tests/PaymentGatewayApiUnitTests/Controllers/PaymentControllerTests.cs index 4556e30..84a6482 100644 --- a/tests/PaymentGatewayApiUnitTests/Controllers/PaymentControllerTests.cs +++ b/tests/PaymentGatewayApiUnitTests/Controllers/PaymentControllerTests.cs @@ -48,7 +48,7 @@ public void GetHealthcheck_ReturnsOk() public async Task GetPayment_Found_ReturnsOkWithPayment() { PostPaymentResponse payment = new() { PaymentId = TestData.PaymentId, Status = PaymentStatus.Authorized }; - this.paymentRepo.Setup(r => r.Get(TestData.PaymentId)).ReturnsAsync(payment); + this.paymentRepo.Setup(r => r.Find(TestData.PaymentId)).ReturnsAsync(payment); ActionResult result = await this.controller.GetPayment(TestData.PaymentId); @@ -59,7 +59,7 @@ public async Task GetPayment_Found_ReturnsOkWithPayment() [Fact] public async Task GetPayment_Missing_ReturnsNotFound() { - this.paymentRepo.Setup(r => r.Get(It.IsAny())).ReturnsAsync((PostPaymentResponse?)null); + this.paymentRepo.Setup(r => r.Find(It.IsAny())).ReturnsAsync((PostPaymentResponse?)null); ActionResult result = await this.controller.GetPayment(TestData.PaymentId); diff --git a/tests/PaymentGatewayApiUnitTests/Middleware/LogMiddlewareTests.cs b/tests/PaymentGatewayApiUnitTests/Middleware/LogMiddlewareTests.cs index a5c8089..4583ff5 100644 --- a/tests/PaymentGatewayApiUnitTests/Middleware/LogMiddlewareTests.cs +++ b/tests/PaymentGatewayApiUnitTests/Middleware/LogMiddlewareTests.cs @@ -89,7 +89,7 @@ public async Task InvokeAsync_CallerIdHeader_IsPushedToLogContext() Assert.Contains("merchant-1", value.ToString()); } - private class CollectingSink : ILogEventSink + private sealed class CollectingSink : ILogEventSink { public List Events { get; } = new(); diff --git a/tests/PaymentGatewayApiUnitTests/PaymentGatewayApiUnitTests.csproj b/tests/PaymentGatewayApiUnitTests/PaymentGatewayApiUnitTests.csproj index 369f6b7..01b34b1 100644 --- a/tests/PaymentGatewayApiUnitTests/PaymentGatewayApiUnitTests.csproj +++ b/tests/PaymentGatewayApiUnitTests/PaymentGatewayApiUnitTests.csproj @@ -1,26 +1,18 @@ - - - - disable - enable - Exe - net10.0 - true - false - - - - - - - - - - - - - - - \ No newline at end of file + + + + Exe + true + false + + + + + + + + + + + + From 4a78ccae605147b9aec7ba74eadca4c759f7edb9 Mon Sep 17 00:00:00 2001 From: ati Date: Wed, 23 Sep 2026 23:38:55 +0100 Subject: [PATCH 04/13] bind settings to validated options, inject TimeProvider, flow cancellation Bank and DynamoDB settings bind to BankOptions / DynamoOptions, validated at startup, instead of IConfiguration lookups that fell back to an empty URI at request time; the table name is no longer hard-coded twice. The typed bank client gets its base address from the options, and its timeouts are explicit under Bank:Resilience. Local-only values move to appsettings.Development.json. The validator takes a TimeProvider and compares in UTC instead of receiving the server's local DateTime.Now from the controller; tests pin a FakeTimeProvider. Every async interface takes a CancellationToken, flowed from the action. The payment POST honours it only until the bank is called: cancelling after that would leave the bank's outcome unknown, so the bank call and the write that records its answer run to completion. Co-Authored-By: Claude Opus 5.5 --- AGENTS.md | 6 ++- Directory.Packages.props | 1 + src/Configuration/BankOptions.cs | 13 +++++++ src/Configuration/DynamoOptions.cs | 11 ++++++ src/Controllers/PaymentController.cs | 11 +++--- src/Program.cs | 22 +++++++++-- src/Services/BankHttpClient.cs | 16 +++----- src/Services/BankResponseProcessor.cs | 7 ++-- src/Services/BankService.cs | 16 +++++--- src/Services/PaymentRequestValidator.cs | 11 ++++-- src/Services/PaymentsRepository.cs | 21 +++++++---- src/appsettings.Development.json | 8 ++++ src/appsettings.json | 11 +++++- src/docker-compose.yml | 2 +- .../Controllers/PaymentControllerTests.cs | 35 +++++++++--------- .../PaymentGatewayApiUnitTests.csproj | 1 + .../Services/BankResponseProcessorTests.cs | 6 +-- .../Services/BankServiceTests.cs | 37 +++++++++++++------ .../Services/PaymentRequestValidatorTests.cs | 10 ++--- 19 files changed, 167 insertions(+), 78 deletions(-) create mode 100644 src/Configuration/BankOptions.cs create mode 100644 src/Configuration/DynamoOptions.cs create mode 100644 src/appsettings.Development.json diff --git a/AGENTS.md b/AGENTS.md index f40b810..1ebd051 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -136,6 +136,8 @@ shadows the specific stubs; WireMock breaks equal-priority ties by load order. automatically by `LogMiddleware` from the request headers (a missing `CorrelationId` header gets a generated GUID), so never pass them by hand. - **Serilog is configured from `src/logconfig.json`, not `appsettings.json`.** - `appsettings.json` holds `SimulatorBankEndpoint` and the `AWS` block (pointed at - `http://localhost:8000` for DynamoDB Local). Verbose file logs go to `src/z-logs/` + Settings bind to validated options classes in `Configuration/` (`Bank`, `Dynamo`), + checked at startup. Local-only values (the simulator URL, `AWS:ServiceURL` pointed at + DynamoDB Local) live in `appsettings.Development.json`; the compose file sets them for + the container through environment variables. Verbose file logs go to `src/z-logs/` (gitignored); the console sink is deliberately terse. diff --git a/Directory.Packages.props b/Directory.Packages.props index bf8e3a8..d752f9d 100644 --- a/Directory.Packages.props +++ b/Directory.Packages.props @@ -13,6 +13,7 @@ + diff --git a/src/Configuration/BankOptions.cs b/src/Configuration/BankOptions.cs new file mode 100644 index 0000000..a372645 --- /dev/null +++ b/src/Configuration/BankOptions.cs @@ -0,0 +1,13 @@ +using System; +using System.ComponentModel.DataAnnotations; + +namespace PaymentGatewayApi.Configuration; + +public sealed class BankOptions +{ + public const string SectionName = "Bank"; + + // the acquiring bank's base address, payments are POSTed to "payments" under it + [Required] + public Uri BaseUrl { get; set; } = null!; +} diff --git a/src/Configuration/DynamoOptions.cs b/src/Configuration/DynamoOptions.cs new file mode 100644 index 0000000..8427591 --- /dev/null +++ b/src/Configuration/DynamoOptions.cs @@ -0,0 +1,11 @@ +using System.ComponentModel.DataAnnotations; + +namespace PaymentGatewayApi.Configuration; + +public sealed class DynamoOptions +{ + public const string SectionName = "Dynamo"; + + [Required] + public string PaymentsTable { get; set; } = null!; +} diff --git a/src/Controllers/PaymentController.cs b/src/Controllers/PaymentController.cs index bee22ff..dabd901 100644 --- a/src/Controllers/PaymentController.cs +++ b/src/Controllers/PaymentController.cs @@ -1,4 +1,5 @@ using System; +using System.Threading; using System.Threading.Tasks; using Microsoft.AspNetCore.Mvc; using Microsoft.Extensions.Logging; @@ -36,11 +37,11 @@ public ActionResult GetHealthcheck() // curl -k http://localhost:5067/api/payment/16bd4749-d150-48d1-a3ce-257457d74d1f [HttpGet("{paymentId}")] - public async Task> GetPayment(string paymentId) + public async Task> GetPayment(string paymentId, CancellationToken cancellationToken) { this.LogGetPayment(paymentId, "Started"); - var payment = await this.paymentRepo.Find(paymentId); + var payment = await this.paymentRepo.Find(paymentId, cancellationToken); if (payment is null) { return new NotFoundObjectResult("payment not found"); @@ -50,14 +51,14 @@ public async Task> GetPayment(string paymentId } [HttpPost()] - public async Task> PostPayment([FromBody] PaymentRequest payment) + public async Task> PostPayment([FromBody] PaymentRequest payment, CancellationToken cancellationToken) { payment.PaymentId = Guid.NewGuid().ToString(); this.LogPostPayment(payment.PaymentId, "Started"); bool isValid = this.validator.IsCardNumberValid(payment) && - this.validator.IsExpiryValid(payment, DateTime.Now) && + this.validator.IsExpiryValid(payment) && this.validator.IsAmountValid(payment) && this.validator.IsCurrencyCodeValid(payment) && this.validator.IsCvvValid(payment); @@ -69,7 +70,7 @@ public async Task> PostPayment([FromBody] PaymentReq // good place to split into a separate service if validation would include fraud detection, sanction screening, etc - var status = await this.bankService.ExecutePayment(payment); + var status = await this.bankService.ExecutePayment(payment, cancellationToken); if (status == PaymentStatus.Authorized) { return new OkObjectResult(payment.PaymentId); diff --git a/src/Program.cs b/src/Program.cs index d1e7a3b..6181091 100644 --- a/src/Program.cs +++ b/src/Program.cs @@ -4,6 +4,8 @@ using Microsoft.Extensions.Configuration; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Http.Resilience; +using Microsoft.Extensions.Options; +using PaymentGatewayApi.Configuration; using PaymentGatewayApi.Middleware; using PaymentGatewayApi.Services; using Serilog; @@ -16,10 +18,24 @@ builder.Services.AddControllers(); +// a missing or malformed setting fails the startup, not the first request that needs it +builder.Services.AddOptions() + .BindConfiguration(BankOptions.SectionName) + .ValidateDataAnnotations() + .ValidateOnStart(); +builder.Services.AddOptions() + .BindConfiguration(DynamoOptions.SectionName) + .ValidateDataAnnotations() + .ValidateOnStart(); + +builder.Services.AddSingleton(TimeProvider.System); + // a payment POST is not idempotent, a retry after a lost response could charge the card twice; -// keep the timeouts and circuit breaker, but never retry it -builder.Services.AddHttpClient() - .AddStandardResilienceHandler(options => options.Retry.DisableForUnsafeHttpMethods()); +// keep the timeouts and circuit breaker (tunable under Bank:Resilience), but never retry it +builder.Services.AddHttpClient((services, client) => + client.BaseAddress = services.GetRequiredService>().Value.BaseUrl) + .AddStandardResilienceHandler(builder.Configuration.GetSection("Bank:Resilience")) + .Configure(options => options.Retry.DisableForUnsafeHttpMethods()); var awsOptions = builder.Configuration.GetAWSOptions(); awsOptions.DefaultClientConfig.Timeout = TimeSpan.FromSeconds(10); diff --git a/src/Services/BankHttpClient.cs b/src/Services/BankHttpClient.cs index c813495..3faa53f 100644 --- a/src/Services/BankHttpClient.cs +++ b/src/Services/BankHttpClient.cs @@ -2,8 +2,8 @@ using System.Net; using System.Net.Http; using System.Net.Http.Json; +using System.Threading; using System.Threading.Tasks; -using Microsoft.Extensions.Configuration; using Microsoft.Extensions.Logging; using PaymentGatewayApi.Models; @@ -11,21 +11,18 @@ namespace PaymentGatewayApi.Services; public interface IBankHttpClient { - Task PostBankPayment(PaymentRequest payment); + Task PostBankPayment(PaymentRequest payment, CancellationToken cancellationToken); } - - public partial class BankHttpClient : IBankHttpClient { private readonly ILogger logger; - private readonly IConfiguration config; private readonly HttpClient client; - public BankHttpClient(ILogger logger, IConfiguration config, HttpClient client) + // the base address comes from BankOptions, set where the typed client is registered + public BankHttpClient(ILogger logger, HttpClient client) { this.logger = logger; - this.config = config; this.client = client; } @@ -42,15 +39,14 @@ public static BankRequest ToBankRequest(PaymentRequest payment) }; } - public async Task PostBankPayment(PaymentRequest payment) + public async Task PostBankPayment(PaymentRequest payment, CancellationToken cancellationToken) { BankRequest request = ToBankRequest(payment); - string uri = this.config["SimulatorBankEndpoint"] ?? string.Empty; HttpResponseMessage response; try { - response = await this.client.PostAsJsonAsync(uri, request); + response = await this.client.PostAsJsonAsync(new Uri("payments", UriKind.Relative), request, cancellationToken); } catch (Exception exception) { diff --git a/src/Services/BankResponseProcessor.cs b/src/Services/BankResponseProcessor.cs index 000345c..92b31fa 100644 --- a/src/Services/BankResponseProcessor.cs +++ b/src/Services/BankResponseProcessor.cs @@ -1,6 +1,7 @@ using System; using System.Net.Http; using System.Net.Http.Json; +using System.Threading; using System.Threading.Tasks; using Microsoft.Extensions.Logging; using PaymentGatewayApi.Models; @@ -10,7 +11,7 @@ namespace PaymentGatewayApi.Services; public interface IBankResponseProcessor { // null when the bank's body cannot be used, so no half-filled response is ever persisted - Task ParseBankResponse(PaymentRequest payment, HttpResponseMessage response); + Task ParseBankResponse(PaymentRequest payment, HttpResponseMessage response, CancellationToken cancellationToken); } public partial class BankResponseProcessor : IBankResponseProcessor @@ -22,13 +23,13 @@ public BankResponseProcessor(ILogger logger) this.logger = logger; } - public async Task ParseBankResponse(PaymentRequest payment, HttpResponseMessage response) + public async Task ParseBankResponse(PaymentRequest payment, HttpResponseMessage response, CancellationToken cancellationToken) { BankResponse? bankResponse; try { - bankResponse = await response.Content.ReadFromJsonAsync(); + bankResponse = await response.Content.ReadFromJsonAsync(cancellationToken); } catch (Exception exception) { diff --git a/src/Services/BankService.cs b/src/Services/BankService.cs index a447cf9..d8c1437 100644 --- a/src/Services/BankService.cs +++ b/src/Services/BankService.cs @@ -1,4 +1,5 @@ using System.Net.Http; +using System.Threading; using System.Threading.Tasks; using Microsoft.Extensions.Logging; using PaymentGatewayApi.Models; @@ -7,7 +8,7 @@ namespace PaymentGatewayApi.Services; public interface IBankService { - Task ExecutePayment(PaymentRequest payment); + Task ExecutePayment(PaymentRequest payment, CancellationToken cancellationToken); } public partial class BankService : IBankService @@ -26,14 +27,19 @@ public BankService( this.paymentRepo = paymentRepo; } - public async Task ExecutePayment(PaymentRequest payment) + public async Task ExecutePayment(PaymentRequest payment, CancellationToken cancellationToken) { this.LogExecutePayment(payment.PaymentId, "Started"); - HttpResponseMessage bankResponse = await this.bankHttpClient.PostBankPayment(payment); + // a client that disconnects can still stop the payment before the bank is called; once it is, + // cancelling would leave the bank's outcome unknown, so the bank call and the write that + // records its answer run to completion (bounded by the resilience handler's timeouts) + cancellationToken.ThrowIfCancellationRequested(); + + HttpResponseMessage bankResponse = await this.bankHttpClient.PostBankPayment(payment, CancellationToken.None); if (bankResponse.IsSuccessStatusCode) { - PostPaymentResponse? paymentResponse = await this.responseProcessor.ParseBankResponse(payment, bankResponse); + PostPaymentResponse? paymentResponse = await this.responseProcessor.ParseBankResponse(payment, bankResponse, CancellationToken.None); if (paymentResponse is null) { // the bank answered 2xx but its body is unusable, so the outcome is unknown and there is nothing to persist @@ -41,7 +47,7 @@ public async Task ExecutePayment(PaymentRequest payment) return PaymentStatus.InternalError; } - await this.paymentRepo.Add(paymentResponse); + await this.paymentRepo.Add(paymentResponse, CancellationToken.None); return paymentResponse.Status; } diff --git a/src/Services/PaymentRequestValidator.cs b/src/Services/PaymentRequestValidator.cs index 294286d..9014ab4 100644 --- a/src/Services/PaymentRequestValidator.cs +++ b/src/Services/PaymentRequestValidator.cs @@ -8,7 +8,7 @@ namespace PaymentGatewayApi.Services; public interface IPaymentRequestValidator { bool IsCardNumberValid(PaymentRequest payment); - bool IsExpiryValid(PaymentRequest payment, DateTime now); + bool IsExpiryValid(PaymentRequest payment); bool IsAmountValid(PaymentRequest payment); bool IsCurrencyCodeValid(PaymentRequest payment); bool IsCvvValid(PaymentRequest payment); @@ -20,10 +20,12 @@ public partial class PaymentRequestValidator : IPaymentRequestValidator private const int MaxExpiryYearsAhead = 20; private readonly ILogger logger; + private readonly TimeProvider timeProvider; - public PaymentRequestValidator(ILogger logger) + public PaymentRequestValidator(ILogger logger, TimeProvider timeProvider) { this.logger = logger; + this.timeProvider = timeProvider; } public bool IsCardNumberValid(PaymentRequest payment) @@ -37,8 +39,11 @@ public bool IsCardNumberValid(PaymentRequest payment) return true; } - public bool IsExpiryValid(PaymentRequest payment, DateTime now) + public bool IsExpiryValid(PaymentRequest payment) { + // UTC, not the server's local time zone; a card is valid through the last day of its expiry month + DateTimeOffset now = this.timeProvider.GetUtcNow(); + if (payment.ExpiryMonth < 1 || payment.ExpiryMonth > 12) { this.LogRejected(payment.PaymentId, "Rejected", "IsExpiryMonthValid"); diff --git a/src/Services/PaymentsRepository.cs b/src/Services/PaymentsRepository.cs index dffdcd0..38f8dd4 100644 --- a/src/Services/PaymentsRepository.cs +++ b/src/Services/PaymentsRepository.cs @@ -1,32 +1,37 @@ using System; using System.Collections.Generic; using System.Text.Json; +using System.Threading; using System.Threading.Tasks; using Amazon.DynamoDBv2; using Amazon.DynamoDBv2.Model; using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Options; +using PaymentGatewayApi.Configuration; using PaymentGatewayApi.Models; namespace PaymentGatewayApi.Services; public interface IPaymentsRepository { - Task Add(PostPaymentResponse payment); - Task Find(string paymentId); + Task Add(PostPaymentResponse payment, CancellationToken cancellationToken); + Task Find(string paymentId, CancellationToken cancellationToken); } public partial class PaymentsRepository : IPaymentsRepository { private readonly ILogger logger; private readonly IAmazonDynamoDB dynamo; + private readonly string tableName; - public PaymentsRepository(ILogger logger, IAmazonDynamoDB dynamo) + public PaymentsRepository(ILogger logger, IAmazonDynamoDB dynamo, IOptions options) { this.logger = logger; this.dynamo = dynamo; + this.tableName = options.Value.PaymentsTable; } - public async Task Add(PostPaymentResponse payment) + public async Task Add(PostPaymentResponse payment, CancellationToken cancellationToken) { var item = new Dictionary { @@ -36,7 +41,7 @@ public async Task Add(PostPaymentResponse payment) try { - await this.dynamo.PutItemAsync("Payments", item); + await this.dynamo.PutItemAsync(this.tableName, item, cancellationToken); } catch (Exception exception) { @@ -46,19 +51,19 @@ public async Task Add(PostPaymentResponse payment) } } - public async Task Find(string paymentId) + public async Task Find(string paymentId, CancellationToken cancellationToken) { var key = new Dictionary { ["PaymentId"] = new AttributeValue { S = paymentId } }; - GetItemRequest request = new() { Key = key, TableName = "Payments" }; + GetItemRequest request = new() { Key = key, TableName = this.tableName }; PostPaymentResponse? payment; try { - GetItemResponse response = await this.dynamo.GetItemAsync(request); + GetItemResponse response = await this.dynamo.GetItemAsync(request, cancellationToken); if (response.Item is { Count: > 0 }) { diff --git a/src/appsettings.Development.json b/src/appsettings.Development.json new file mode 100644 index 0000000..78414c6 --- /dev/null +++ b/src/appsettings.Development.json @@ -0,0 +1,8 @@ +{ + "Bank": { + "BaseUrl": "http://localhost:8080/" + }, + "AWS": { + "ServiceURL": "http://localhost:8000" + } +} diff --git a/src/appsettings.json b/src/appsettings.json index 7bfeaa6..3fbb303 100644 --- a/src/appsettings.json +++ b/src/appsettings.json @@ -1,9 +1,16 @@ { "AllowedHosts": "*", - "SimulatorBankEndpoint": "http://localhost:8080/payments", + "Bank": { + "Resilience": { + "AttemptTimeout": { "Timeout": "00:00:10" }, + "TotalRequestTimeout": { "Timeout": "00:00:30" } + } + }, + "Dynamo": { + "PaymentsTable": "Payments" + }, "AWS": { "Region": "eu-west-2", - "ServiceURL": "http://localhost:8000", "MaxErrorRetry": 0 } } diff --git a/src/docker-compose.yml b/src/docker-compose.yml index b761ceb..41c521b 100644 --- a/src/docker-compose.yml +++ b/src/docker-compose.yml @@ -59,7 +59,7 @@ services: ports: - "5067:8080" environment: - SimulatorBankEndpoint: "http://bank-simulator-wiremock:8080/payments" + Bank__BaseUrl: "http://bank-simulator-wiremock:8080/" AWS__ServiceURL: "http://dynamodb:8000" AWS_ACCESS_KEY_ID: "dummy" AWS_SECRET_ACCESS_KEY: "dummy" diff --git a/tests/PaymentGatewayApiUnitTests/Controllers/PaymentControllerTests.cs b/tests/PaymentGatewayApiUnitTests/Controllers/PaymentControllerTests.cs index 84a6482..89ad740 100644 --- a/tests/PaymentGatewayApiUnitTests/Controllers/PaymentControllerTests.cs +++ b/tests/PaymentGatewayApiUnitTests/Controllers/PaymentControllerTests.cs @@ -1,4 +1,5 @@ using System; +using System.Threading; using System.Threading.Tasks; using Microsoft.AspNetCore.Mvc; using Microsoft.Extensions.Logging.Abstractions; @@ -29,7 +30,7 @@ public PaymentControllerTests() private void SetupValidator() { this.validator.Setup(v => v.IsCardNumberValid(It.IsAny())).Returns(true); - this.validator.Setup(v => v.IsExpiryValid(It.IsAny(), It.IsAny())).Returns(true); + this.validator.Setup(v => v.IsExpiryValid(It.IsAny())).Returns(true); this.validator.Setup(v => v.IsAmountValid(It.IsAny())).Returns(true); this.validator.Setup(v => v.IsCurrencyCodeValid(It.IsAny())).Returns(true); this.validator.Setup(v => v.IsCvvValid(It.IsAny())).Returns(true); @@ -48,9 +49,9 @@ public void GetHealthcheck_ReturnsOk() public async Task GetPayment_Found_ReturnsOkWithPayment() { PostPaymentResponse payment = new() { PaymentId = TestData.PaymentId, Status = PaymentStatus.Authorized }; - this.paymentRepo.Setup(r => r.Find(TestData.PaymentId)).ReturnsAsync(payment); + this.paymentRepo.Setup(r => r.Find(TestData.PaymentId, It.IsAny())).ReturnsAsync(payment); - ActionResult result = await this.controller.GetPayment(TestData.PaymentId); + ActionResult result = await this.controller.GetPayment(TestData.PaymentId, TestContext.Current.CancellationToken); OkObjectResult ok = Assert.IsType(result.Result); Assert.Same(payment, ok.Value); @@ -59,9 +60,9 @@ public async Task GetPayment_Found_ReturnsOkWithPayment() [Fact] public async Task GetPayment_Missing_ReturnsNotFound() { - this.paymentRepo.Setup(r => r.Find(It.IsAny())).ReturnsAsync((PostPaymentResponse?)null); + this.paymentRepo.Setup(r => r.Find(It.IsAny(), It.IsAny())).ReturnsAsync((PostPaymentResponse?)null); - ActionResult result = await this.controller.GetPayment(TestData.PaymentId); + ActionResult result = await this.controller.GetPayment(TestData.PaymentId, TestContext.Current.CancellationToken); Assert.IsType(result.Result); } @@ -71,9 +72,9 @@ public async Task PostPayment_Authorized_ReturnsOkWithNewPaymentId() { PaymentRequest payment = TestData.ValidPaymentRequest(); this.SetupValidator(); - this.bankService.Setup(b => b.ExecutePayment(payment)).ReturnsAsync(PaymentStatus.Authorized); + this.bankService.Setup(b => b.ExecutePayment(payment, It.IsAny())).ReturnsAsync(PaymentStatus.Authorized); - ActionResult result = await this.controller.PostPayment(payment); + ActionResult result = await this.controller.PostPayment(payment, TestContext.Current.CancellationToken); OkObjectResult ok = Assert.IsType(result.Result); string paymentId = Assert.IsType(ok.Value); @@ -86,12 +87,12 @@ public async Task PostPayment_ClientSuppliedPaymentId_IsReplaced() { PaymentRequest payment = TestData.ValidPaymentRequest(); this.SetupValidator(); - this.bankService.Setup(b => b.ExecutePayment(payment)).ReturnsAsync(PaymentStatus.Authorized); + this.bankService.Setup(b => b.ExecutePayment(payment, It.IsAny())).ReturnsAsync(PaymentStatus.Authorized); - await this.controller.PostPayment(payment); + await this.controller.PostPayment(payment, TestContext.Current.CancellationToken); Assert.NotEqual(TestData.PaymentId, payment.PaymentId); - this.bankService.Verify(b => b.ExecutePayment(It.Is(p => p.PaymentId != TestData.PaymentId)), Times.Once); + this.bankService.Verify(b => b.ExecutePayment(It.Is(p => p.PaymentId != TestData.PaymentId), It.IsAny()), Times.Once); } [Theory] @@ -109,7 +110,7 @@ public async Task PostPayment_ValidationFails_ReturnsRejectedWithoutCallingBank( this.validator.Setup(v => v.IsCardNumberValid(It.IsAny())).Returns(false); break; case nameof(IPaymentRequestValidator.IsExpiryValid): - this.validator.Setup(v => v.IsExpiryValid(It.IsAny(), It.IsAny())).Returns(false); + this.validator.Setup(v => v.IsExpiryValid(It.IsAny())).Returns(false); break; case nameof(IPaymentRequestValidator.IsAmountValid): this.validator.Setup(v => v.IsAmountValid(It.IsAny())).Returns(false); @@ -122,11 +123,11 @@ public async Task PostPayment_ValidationFails_ReturnsRejectedWithoutCallingBank( break; } - ActionResult result = await this.controller.PostPayment(TestData.ValidPaymentRequest()); + ActionResult result = await this.controller.PostPayment(TestData.ValidPaymentRequest(), TestContext.Current.CancellationToken); BadRequestObjectResult badRequest = Assert.IsType(result.Result); Assert.Equal(nameof(PaymentStatus.Rejected), badRequest.Value); - this.bankService.Verify(b => b.ExecutePayment(It.IsAny()), Times.Never); + this.bankService.Verify(b => b.ExecutePayment(It.IsAny(), It.IsAny()), Times.Never); } [Fact] @@ -134,10 +135,10 @@ public async Task PostPayment_CardNumberInvalid_SkipsRemainingChecks() { // this.SetupValidator(isValid: false); - await this.controller.PostPayment(TestData.ValidPaymentRequest()); + await this.controller.PostPayment(TestData.ValidPaymentRequest(), TestContext.Current.CancellationToken); this.validator.Verify(v => v.IsCardNumberValid(It.IsAny()), Times.Once); - this.validator.Verify(v => v.IsExpiryValid(It.IsAny(), It.IsAny()), Times.Never); + this.validator.Verify(v => v.IsExpiryValid(It.IsAny()), Times.Never); this.validator.Verify(v => v.IsAmountValid(It.IsAny()), Times.Never); this.validator.Verify(v => v.IsCurrencyCodeValid(It.IsAny()), Times.Never); this.validator.Verify(v => v.IsCvvValid(It.IsAny()), Times.Never); @@ -149,9 +150,9 @@ public async Task PostPayment_CardNumberInvalid_SkipsRemainingChecks() public async Task PostPayment_NotAuthorized_ReturnsBadRequestWithStatus(PaymentStatus status) { this.SetupValidator(); - this.bankService.Setup(b => b.ExecutePayment(It.IsAny())).ReturnsAsync(status); + this.bankService.Setup(b => b.ExecutePayment(It.IsAny(), It.IsAny())).ReturnsAsync(status); - ActionResult result = await this.controller.PostPayment(TestData.ValidPaymentRequest()); + ActionResult result = await this.controller.PostPayment(TestData.ValidPaymentRequest(), TestContext.Current.CancellationToken); BadRequestObjectResult badRequest = Assert.IsType(result.Result); Assert.Equal(status.ToString(), badRequest.Value); diff --git a/tests/PaymentGatewayApiUnitTests/PaymentGatewayApiUnitTests.csproj b/tests/PaymentGatewayApiUnitTests/PaymentGatewayApiUnitTests.csproj index 01b34b1..23d2dac 100644 --- a/tests/PaymentGatewayApiUnitTests/PaymentGatewayApiUnitTests.csproj +++ b/tests/PaymentGatewayApiUnitTests/PaymentGatewayApiUnitTests.csproj @@ -7,6 +7,7 @@ + diff --git a/tests/PaymentGatewayApiUnitTests/Services/BankResponseProcessorTests.cs b/tests/PaymentGatewayApiUnitTests/Services/BankResponseProcessorTests.cs index 486c37c..ed27b42 100644 --- a/tests/PaymentGatewayApiUnitTests/Services/BankResponseProcessorTests.cs +++ b/tests/PaymentGatewayApiUnitTests/Services/BankResponseProcessorTests.cs @@ -28,7 +28,7 @@ public async Task ParseBankResponse_Authorized_MapsRequestOntoResponse() using HttpResponseMessage response = JsonResponse( """{"authorized":true,"authorization_code":"0bb07405-6d44-4b50-a14f-7ae0beff13ad"}"""); - PostPaymentResponse? result = await this.processor.ParseBankResponse(request, response); + PostPaymentResponse? result = await this.processor.ParseBankResponse(request, response, TestContext.Current.CancellationToken); Assert.NotNull(result); Assert.Equal(PaymentStatus.Authorized, result.Status); @@ -46,7 +46,7 @@ public async Task ParseBankResponse_NotAuthorized_ReturnsDeclined() PaymentRequest request = TestData.ValidPaymentRequest(); using HttpResponseMessage response = JsonResponse("""{"authorized":false,"authorization_code":""}"""); - PostPaymentResponse? result = await this.processor.ParseBankResponse(request, response); + PostPaymentResponse? result = await this.processor.ParseBankResponse(request, response, TestContext.Current.CancellationToken); Assert.NotNull(result); Assert.Equal(PaymentStatus.Declined, result.Status); @@ -63,7 +63,7 @@ public async Task ParseBankResponse_UnusableBody_ReturnsNull(string body) PaymentRequest request = TestData.ValidPaymentRequest(); using HttpResponseMessage response = JsonResponse(body); - PostPaymentResponse? result = await this.processor.ParseBankResponse(request, response); + PostPaymentResponse? result = await this.processor.ParseBankResponse(request, response, TestContext.Current.CancellationToken); Assert.Null(result); } diff --git a/tests/PaymentGatewayApiUnitTests/Services/BankServiceTests.cs b/tests/PaymentGatewayApiUnitTests/Services/BankServiceTests.cs index e3cebeb..6dde0a9 100644 --- a/tests/PaymentGatewayApiUnitTests/Services/BankServiceTests.cs +++ b/tests/PaymentGatewayApiUnitTests/Services/BankServiceTests.cs @@ -1,5 +1,7 @@ +using System; using System.Net; using System.Net.Http; +using System.Threading; using System.Threading.Tasks; using Microsoft.Extensions.Logging.Abstractions; using Moq; @@ -28,11 +30,11 @@ public BankServiceTests() private void SetupBank(PaymentRequest payment, HttpStatusCode statusCode, PostPaymentResponse? parsed = null) { HttpResponseMessage bankResponse = new(statusCode); - this.bankHttpClient.Setup(c => c.PostBankPayment(payment)).ReturnsAsync(bankResponse); + this.bankHttpClient.Setup(c => c.PostBankPayment(payment, CancellationToken.None)).ReturnsAsync(bankResponse); if (parsed is not null) { - this.responseProcessor.Setup(p => p.ParseBankResponse(payment, bankResponse)).ReturnsAsync(parsed); + this.responseProcessor.Setup(p => p.ParseBankResponse(payment, bankResponse, CancellationToken.None)).ReturnsAsync(parsed); } } @@ -45,10 +47,10 @@ public async Task ExecutePayment_BankAnswers_PersistsAndReturnsParsedStatus(Paym PostPaymentResponse parsed = new() { PaymentId = payment.PaymentId, Status = status }; this.SetupBank(payment, HttpStatusCode.OK, parsed); - PaymentStatus result = await this.service.ExecutePayment(payment); + PaymentStatus result = await this.service.ExecutePayment(payment, TestContext.Current.CancellationToken); Assert.Equal(status, result); - this.paymentRepo.Verify(r => r.Add(parsed), Times.Once); + this.paymentRepo.Verify(r => r.Add(parsed, It.IsAny()), Times.Once); } [Theory] @@ -59,12 +61,25 @@ public async Task ExecutePayment_BankReturnsNonSuccess_ReturnsInternalErrorWitho PaymentRequest payment = TestData.ValidPaymentRequest(); this.SetupBank(payment, statusCode); - PaymentStatus result = await this.service.ExecutePayment(payment); + PaymentStatus result = await this.service.ExecutePayment(payment, TestContext.Current.CancellationToken); Assert.Equal(PaymentStatus.InternalError, result); this.responseProcessor.Verify( - p => p.ParseBankResponse(It.IsAny(), It.IsAny()), Times.Never); - this.paymentRepo.Verify(r => r.Add(It.IsAny()), Times.Never); + p => p.ParseBankResponse(It.IsAny(), It.IsAny(), It.IsAny()), Times.Never); + this.paymentRepo.Verify(r => r.Add(It.IsAny(), It.IsAny()), Times.Never); + } + + [Fact] + public async Task ExecutePayment_CancelledBeforeBankCall_ThrowsWithoutCallingBank() + { + using CancellationTokenSource cancelled = new(); + await cancelled.CancelAsync(); + + await Assert.ThrowsAnyAsync( + () => this.service.ExecutePayment(TestData.ValidPaymentRequest(), cancelled.Token)); + + this.bankHttpClient.Verify( + c => c.PostBankPayment(It.IsAny(), It.IsAny()), Times.Never); } [Fact] @@ -72,12 +87,12 @@ public async Task ExecutePayment_BankBodyUnparseable_ReturnsInternalErrorWithout { PaymentRequest payment = TestData.ValidPaymentRequest(); HttpResponseMessage bankResponse = new(HttpStatusCode.OK); - this.bankHttpClient.Setup(c => c.PostBankPayment(payment)).ReturnsAsync(bankResponse); - this.responseProcessor.Setup(p => p.ParseBankResponse(payment, bankResponse)).ReturnsAsync((PostPaymentResponse?)null); + this.bankHttpClient.Setup(c => c.PostBankPayment(payment, CancellationToken.None)).ReturnsAsync(bankResponse); + this.responseProcessor.Setup(p => p.ParseBankResponse(payment, bankResponse, CancellationToken.None)).ReturnsAsync((PostPaymentResponse?)null); - PaymentStatus result = await this.service.ExecutePayment(payment); + PaymentStatus result = await this.service.ExecutePayment(payment, TestContext.Current.CancellationToken); Assert.Equal(PaymentStatus.InternalError, result); - this.paymentRepo.Verify(r => r.Add(It.IsAny()), Times.Never); + this.paymentRepo.Verify(r => r.Add(It.IsAny(), It.IsAny()), Times.Never); } } diff --git a/tests/PaymentGatewayApiUnitTests/Services/PaymentRequestValidatorTests.cs b/tests/PaymentGatewayApiUnitTests/Services/PaymentRequestValidatorTests.cs index 4401838..a47cf65 100644 --- a/tests/PaymentGatewayApiUnitTests/Services/PaymentRequestValidatorTests.cs +++ b/tests/PaymentGatewayApiUnitTests/Services/PaymentRequestValidatorTests.cs @@ -1,5 +1,6 @@ using System; using Microsoft.Extensions.Logging.Abstractions; +using Microsoft.Extensions.Time.Testing; using PaymentGatewayApi.Models; using PaymentGatewayApi.Services; using Xunit; @@ -8,9 +9,8 @@ namespace PaymentGatewayApiUnitTests.Services; public class PaymentRequestValidatorTests { - private static readonly DateTime Now = new(2026, 9, 15); - - private readonly PaymentRequestValidator validator = new(NullLogger.Instance); + private readonly PaymentRequestValidator validator = new( + NullLogger.Instance, new FakeTimeProvider(new DateTimeOffset(2026, 9, 15, 12, 0, 0, TimeSpan.Zero))); [Theory] [InlineData("22224053432488")] // 14 digits @@ -49,7 +49,7 @@ public void IsExpiryValid_CurrentOrFutureExpiry_ReturnsTrue(int month, int year) payment.ExpiryMonth = month; payment.ExpiryYear = year; - Assert.True(this.validator.IsExpiryValid(payment, Now)); + Assert.True(this.validator.IsExpiryValid(payment)); } [Theory] @@ -67,7 +67,7 @@ public void IsExpiryValid_PastOrOutOfRangeExpiryOrInvalidMonth_ReturnsFalse(int payment.ExpiryMonth = month; payment.ExpiryYear = year; - Assert.False(this.validator.IsExpiryValid(payment, Now)); + Assert.False(this.validator.IsExpiryValid(payment)); } [Theory] From f1d232fca0a5609f668fb4737111eed7bb4562c0 Mon Sep 17 00:00:00 2001 From: ati Date: Wed, 23 Sep 2026 23:43:49 +0100 Subject: [PATCH 05/13] record the payment before calling the bank, keep the authorization code The payment was persisted only after the bank answered, so a failed write lost a charged payment and a bank failure left no record at all. PaymentService (was BankService) now stores the payment as Pending, calls the bank, then records the outcome with a conditional write that only completes a Pending payment. If that last write fails the merchant still gets the real outcome and a Critical log line carries what reconciliation needs. BankClient (was BankHttpClient + BankResponseProcessor) owns the bank contract and returns a BankOutcome instead of leaking HttpResponseMessage through two abstractions; it disposes the response and maps every failure mode: 4xx, 503 and an open circuit are Failed (nothing charged), timeouts, dropped connections, other 5xx and unreadable bodies are Unknown (may have been charged). It is now unit-tested through a stub HttpMessageHandler. The authorization code is stored. Storage gets its own shape, one attribute per field with the status by name, instead of the public response serialised as JSON; PaymentStatus becomes the payment lifecycle (Pending, Authorized, Declined, Failed, Unknown). GET goes through the service and takes a Guid. Co-Authored-By: Claude Opus 5.5 --- AGENTS.md | 66 ++++---- src/Controllers/PaymentController.cs | 54 +++---- src/Models/BankOutcome.cs | 25 +++ src/Models/Payment.cs | 18 +++ src/Models/PaymentStatus.cs | 10 +- src/Models/Requests/PaymentRequest.cs | 1 - src/Models/Responses/PostPaymentResponse.cs | 15 ++ src/Program.cs | 10 +- src/Services/BankClient.cs | 119 ++++++++++++++ src/Services/BankHttpClient.cs | 70 -------- src/Services/BankResponseProcessor.cs | 68 -------- src/Services/BankService.cs | 66 -------- src/Services/PaymentRequestValidator.cs | 16 +- src/Services/PaymentService.cs | 98 ++++++++++++ src/Services/PaymentsRepository.cs | 133 ++++++++------- .../PaymentEndToEndTests.cs | 12 +- .../Controllers/PaymentControllerTests.cs | 79 ++++----- .../Services/BankClientTests.cs | 147 +++++++++++++++++ .../Services/BankHttpClientTests.cs | 35 ---- .../Services/BankResponseProcessorTests.cs | 70 -------- .../Services/BankServiceTests.cs | 98 ------------ .../Services/PaymentServiceTests.cs | 151 ++++++++++++++++++ .../StubHttpMessageHandler.cs | 29 ++++ tests/PaymentGatewayApiUnitTests/TestData.cs | 21 ++- 24 files changed, 812 insertions(+), 599 deletions(-) create mode 100644 src/Models/BankOutcome.cs create mode 100644 src/Models/Payment.cs create mode 100644 src/Services/BankClient.cs delete mode 100644 src/Services/BankHttpClient.cs delete mode 100644 src/Services/BankResponseProcessor.cs delete mode 100644 src/Services/BankService.cs create mode 100644 src/Services/PaymentService.cs create mode 100644 tests/PaymentGatewayApiUnitTests/Services/BankClientTests.cs delete mode 100644 tests/PaymentGatewayApiUnitTests/Services/BankHttpClientTests.cs delete mode 100644 tests/PaymentGatewayApiUnitTests/Services/BankResponseProcessorTests.cs delete mode 100644 tests/PaymentGatewayApiUnitTests/Services/BankServiceTests.cs create mode 100644 tests/PaymentGatewayApiUnitTests/Services/PaymentServiceTests.cs create mode 100644 tests/PaymentGatewayApiUnitTests/StubHttpMessageHandler.cs diff --git a/AGENTS.md b/AGENTS.md index 1ebd051..c2f36bc 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -38,9 +38,8 @@ dotnet run --project tests/PaymentGatewayApiUnitTests # equivalent, xUnit v3 p Target the test project explicitly: a bare `dotnet test` (or `--solution`) fails because the web project's `Microsoft.NET.Test.Sdk` reference makes it look like a VSTest project. -`BankHttpClient` and `PaymentsRepository` are deliberately not unit-tested beyond -`BankHttpClient.ToBankRequest` (the bank contract mapping); otherwise they are only mocked -through their interfaces. Tests commented `// NOTE: defect` pin current, known-wrong +`BankClient` is unit-tested through a `StubHttpMessageHandler`. `PaymentsRepository` is +deliberately not unit-tested; it is only mocked through its interface. Tests commented `// NOTE: defect` pin current, known-wrong behaviour — update them when the defect is fixed. End-to-end tests live in `tests/PaymentGatewayApiEndToEndTests`. They are black-box: they call @@ -66,39 +65,50 @@ written above each action in `Controllers/PaymentController.cs`. A `POST /api/payment` flows through one chain, each link behind an interface: -`PaymentController` assigns a new `PaymentId` GUID, runs all five -`IPaymentRequestValidator` checks (`Services/PaymentRequestValidator.cs`), then hands off to -`IBankService` (`Services/BankService.cs`). `BankService` calls `IBankHttpClient` -(`Services/BankHttpClient.cs`, POSTs to the `SimulatorBankEndpoint`), passes a successful -response to `IBankResponseProcessor` (`Services/BankResponseProcessor.cs`) to map it onto a -`PostPaymentResponse`, and persists that via `IPostPaymentRepository` -(`Services/PaymentsRepository.cs`) before returning the status. +`PaymentController` runs the five `IPaymentRequestValidator` checks +(`Services/PaymentRequestValidator.cs`), then hands the request to `IPaymentService` +(`Services/PaymentService.cs`), which: -`PaymentStatus` (`Models/PaymentStatus.cs`) is the contract between layers: +1. stores a `Payment` with status `Pending` through `IPaymentsRepository` + (`Services/PaymentsRepository.cs`), so a card is never charged without a record; if this + write fails, the request fails and the bank is never called; +2. calls `IBankClient.Authorize` (`Services/BankClient.cs`), a typed `HttpClient` that owns + the bank contract and turns every answer, or its absence, into a `BankOutcome`; +3. records the outcome and the bank's authorization code with `Complete`, which only + overwrites a payment that is still `Pending`. If that write fails, the merchant still gets + the real outcome, and a `Critical` log line carries what reconciliation needs. -- `Rejected` — local validation failed; the bank was never called. -- `Declined` — the bank answered `authorized: false`. -- `InternalError` — the bank returned non-2xx, or its body could not be parsed. +`PaymentStatus` (`Models/PaymentStatus.cs`) is the payment's lifecycle, stored by name: -### Models come in two pairs, deliberately +- `Pending` — recorded, the bank has not answered. +- `Authorized` / `Declined` — the bank's answer. +- `Failed` — the bank answered 4xx or 503, or the circuit breaker is open: nothing was charged. +- `Unknown` — a timeout, a dropped connection, another 5xx or an unreadable body: the card + may have been charged, so the payment needs reconciliation. -`PaymentRequest` / `PostPaymentResponse` are this gateway's own API shape. -`BankRequest` / `BankResponse` model the acquiring bank's contract and use `snake_case` -through `[JsonPropertyName]` (`card_number`, `expiry_date`, `authorization_code`). Keep -them separate — collapsing them couples our public API to the bank's. +A request that fails validation never becomes a payment. + +### Models come in pairs, deliberately + +`PaymentRequest` / `PostPaymentResponse` are this gateway's own API shape, `Payment` is its +domain record, and `BankRequest` / `BankResponse` model the acquiring bank's contract with +`snake_case` names through `[JsonPropertyName]` (`card_number`, `expiry_date`, +`authorization_code`). Keep them separate: collapsing them couples our public API to the +bank's, or our storage to either. ### Persistence -DynamoDB table `Payments`, hash key `PaymentId`. Each item is just `PaymentId` (S) plus -`PaymentRaw` (S) holding the JSON-serialized `PostPaymentResponse`. Only the last four card -digits are ever stored. +DynamoDB table `Payments`, hash key `PaymentId`. Each item has one attribute per `Payment` +field (`Status` as its name, timestamps as ISO 8601, `AuthorizationCode` only once +authorized); only the last four card digits are ever stored. The mapping lives in +`PaymentsRepository.ToItem` / `FromItem`. ### Wiring -All DI registration lives in `Program.cs`. Services are scoped, except `BankHttpClient`, a -typed `HttpClient` (transient) whose resilience handler has retries disabled; the repository is a -singleton, as is the `IAmazonDynamoDB` client registered through -`AddAWSService()`. New services follow the existing pattern — interface +All DI registration lives in `Program.cs`. The repository and the validator are stateless +singletons, as is the `IAmazonDynamoDB` client registered through +`AddAWSService()`. `BankClient` is a typed `HttpClient` (transient) whose +resilience handler has retries disabled, and `PaymentService` is scoped so it never captures it. New services follow the existing pattern — interface and implementation in the same file under `Services/` — and get registered there. Middleware order is `LogMiddleware` then `ErrorMiddleware`. @@ -112,8 +122,8 @@ The WireMock mappings in `src/Simulators/wiremock/mappings/` select a response f | --- | --- | --- | | odd digit | `200` `authorized: true` | `Authorized` | | even digit (not 0) | `200` `authorized: false` | `Declined` | -| `0` | `503` | `InternalError` | -| any missing field | `400` | `InternalError` | +| `0` | `503` | `Failed` | +| any missing field | `400` | `Failed` | The three `200`/`503` stubs also require `expiry_date` in the bank's `MM/yyyy` format, so a malformed expiry falls through like any other unmatched request. diff --git a/src/Controllers/PaymentController.cs b/src/Controllers/PaymentController.cs index dabd901..d5ae047 100644 --- a/src/Controllers/PaymentController.cs +++ b/src/Controllers/PaymentController.cs @@ -13,21 +13,18 @@ namespace PaymentGatewayApi.Controllers; public partial class PaymentController : ControllerBase { private readonly ILogger logger; - private readonly IPaymentsRepository paymentRepo; private readonly IPaymentRequestValidator validator; - private readonly IBankService bankService; + private readonly IPaymentService paymentService; - public PaymentController( - ILogger logger, IPaymentsRepository paymentRepo, IPaymentRequestValidator validator, IBankService bankService) + public PaymentController(ILogger logger, IPaymentRequestValidator validator, IPaymentService paymentService) { this.logger = logger; - this.paymentRepo = paymentRepo; this.validator = validator; - this.bankService = bankService; + this.paymentService = paymentService; } // curl -k http://localhost:5067/api/payment - [HttpGet()] + [HttpGet] public ActionResult GetHealthcheck() { this.LogGetHealthcheck("Started"); @@ -36,55 +33,52 @@ public ActionResult GetHealthcheck() } // curl -k http://localhost:5067/api/payment/16bd4749-d150-48d1-a3ce-257457d74d1f - [HttpGet("{paymentId}")] - public async Task> GetPayment(string paymentId, CancellationToken cancellationToken) + [HttpGet("{paymentId:guid}")] + public async Task> GetPayment(Guid paymentId, CancellationToken cancellationToken) { this.LogGetPayment(paymentId, "Started"); - var payment = await this.paymentRepo.Find(paymentId, cancellationToken); + Payment? payment = await this.paymentService.FindPayment(paymentId, cancellationToken); if (payment is null) { return new NotFoundObjectResult("payment not found"); } - return new OkObjectResult(payment); + return new OkObjectResult(PostPaymentResponse.From(payment)); } - [HttpPost()] - public async Task> PostPayment([FromBody] PaymentRequest payment, CancellationToken cancellationToken) + [HttpPost] + public async Task> PostPayment([FromBody] PaymentRequest request, CancellationToken cancellationToken) { - payment.PaymentId = Guid.NewGuid().ToString(); - this.LogPostPayment(payment.PaymentId, "Started"); + this.LogPostPayment("Started"); bool isValid = - this.validator.IsCardNumberValid(payment) && - this.validator.IsExpiryValid(payment) && - this.validator.IsAmountValid(payment) && - this.validator.IsCurrencyCodeValid(payment) && - this.validator.IsCvvValid(payment); + this.validator.IsCardNumberValid(request) && + this.validator.IsExpiryValid(request) && + this.validator.IsAmountValid(request) && + this.validator.IsCurrencyCodeValid(request) && + this.validator.IsCvvValid(request); if (!isValid) { - return new BadRequestObjectResult(PaymentStatus.Rejected.ToString()); + return new BadRequestObjectResult("Rejected"); } - // good place to split into a separate service if validation would include fraud detection, sanction screening, etc - - var status = await this.bankService.ExecutePayment(payment, cancellationToken); - if (status == PaymentStatus.Authorized) + Payment payment = await this.paymentService.ProcessPayment(request, cancellationToken); + if (payment.Status == PaymentStatus.Authorized) { - return new OkObjectResult(payment.PaymentId); + return new OkObjectResult(payment.Id.ToString()); } - return new BadRequestObjectResult(status.ToString()); + return new BadRequestObjectResult(payment.Status.ToString()); } [LoggerMessage(Level = LogLevel.Information, Message = "GetHealthcheck, Status={Status}")] private partial void LogGetHealthcheck(string status); [LoggerMessage(Level = LogLevel.Information, Message = "GetPayment, PaymentId={PaymentId}, Status={Status}")] - private partial void LogGetPayment(string paymentId, string status); + private partial void LogGetPayment(Guid paymentId, string status); - [LoggerMessage(Level = LogLevel.Information, Message = "PostPayment, PaymentId={PaymentId}, Status={Status}")] - private partial void LogPostPayment(string? paymentId, string status); + [LoggerMessage(Level = LogLevel.Information, Message = "PostPayment, Status={Status}")] + private partial void LogPostPayment(string status); } diff --git a/src/Models/BankOutcome.cs b/src/Models/BankOutcome.cs new file mode 100644 index 0000000..46ecdae --- /dev/null +++ b/src/Models/BankOutcome.cs @@ -0,0 +1,25 @@ +namespace PaymentGatewayApi.Models; + +// what the acquiring bank said about one payment, in the gateway's own terms +public sealed record BankOutcome +{ + public static readonly BankOutcome Declined = new(PaymentStatus.Declined, null); + public static readonly BankOutcome Failed = new(PaymentStatus.Failed, null); + public static readonly BankOutcome Unknown = new(PaymentStatus.Unknown, null); + + private BankOutcome(PaymentStatus status, string? authorizationCode) + { + this.Status = status; + this.AuthorizationCode = authorizationCode; + } + + public PaymentStatus Status { get; } + + // needed later to capture, refund or dispute the payment + public string? AuthorizationCode { get; } + + public static BankOutcome Authorized(string? authorizationCode) + { + return new BankOutcome(PaymentStatus.Authorized, authorizationCode); + } +} diff --git a/src/Models/Payment.cs b/src/Models/Payment.cs new file mode 100644 index 0000000..57057f6 --- /dev/null +++ b/src/Models/Payment.cs @@ -0,0 +1,18 @@ +using System; + +namespace PaymentGatewayApi.Models; + +// the gateway's record of a payment; only the last four card digits are ever kept +public sealed record Payment +{ + public required Guid Id { get; init; } + public required PaymentStatus Status { get; init; } + public required string CardNumberLastFour { get; init; } + public required int ExpiryMonth { get; init; } + public required int ExpiryYear { get; init; } + public required string Currency { get; init; } + public required decimal Amount { get; init; } + public string? AuthorizationCode { get; init; } + public required DateTimeOffset CreatedAt { get; init; } + public required DateTimeOffset UpdatedAt { get; init; } +} diff --git a/src/Models/PaymentStatus.cs b/src/Models/PaymentStatus.cs index 77cdc8a..c3336d9 100644 --- a/src/Models/PaymentStatus.cs +++ b/src/Models/PaymentStatus.cs @@ -1,9 +1,11 @@ namespace PaymentGatewayApi.Models; +// persisted by name, so members can be reordered without corrupting stored payments public enum PaymentStatus { - Authorized, // the payment was authorized by the call to the acquiring bank - Declined, // the payment was declined by the call to the acquiring bank - Rejected, // No payment could be created as invalid information was supplied to the payment gateway - InternalError // Payment could not be completed due to an internal error. Should never happen + Pending, // recorded, the bank has not answered yet + Authorized, // the acquiring bank authorized the payment + Declined, // the acquiring bank declined the payment + Failed, // the bank refused the request or was unavailable, the card was not charged + Unknown // no usable answer from the bank (timeout, unreadable body), the card may have been charged and needs reconciliation } diff --git a/src/Models/Requests/PaymentRequest.cs b/src/Models/Requests/PaymentRequest.cs index fce54e7..24e9b30 100644 --- a/src/Models/Requests/PaymentRequest.cs +++ b/src/Models/Requests/PaymentRequest.cs @@ -20,5 +20,4 @@ public class PaymentRequest public int ExpiryYear { get; set; } public decimal Amount { get; set; } - public string? PaymentId { get; set; } } diff --git a/src/Models/Responses/PostPaymentResponse.cs b/src/Models/Responses/PostPaymentResponse.cs index 8842659..b95dbb0 100644 --- a/src/Models/Responses/PostPaymentResponse.cs +++ b/src/Models/Responses/PostPaymentResponse.cs @@ -9,4 +9,19 @@ public class PostPaymentResponse public int ExpiryYear { get; set; } public string? Currency { get; set; } public decimal Amount { get; set; } + + // the authorization code and timestamps stay internal to the gateway + public static PostPaymentResponse From(Payment payment) + { + return new PostPaymentResponse + { + PaymentId = payment.Id.ToString(), + Status = payment.Status, + CardNumberLastFour = payment.CardNumberLastFour, + ExpiryMonth = payment.ExpiryMonth, + ExpiryYear = payment.ExpiryYear, + Currency = payment.Currency, + Amount = payment.Amount + }; + } } diff --git a/src/Program.cs b/src/Program.cs index 6181091..e179b97 100644 --- a/src/Program.cs +++ b/src/Program.cs @@ -32,7 +32,7 @@ // a payment POST is not idempotent, a retry after a lost response could charge the card twice; // keep the timeouts and circuit breaker (tunable under Bank:Resilience), but never retry it -builder.Services.AddHttpClient((services, client) => +builder.Services.AddHttpClient((services, client) => client.BaseAddress = services.GetRequiredService>().Value.BaseUrl) .AddStandardResilienceHandler(builder.Configuration.GetSection("Bank:Resilience")) .Configure(options => options.Retry.DisableForUnsafeHttpMethods()); @@ -42,11 +42,11 @@ builder.Services.AddDefaultAWSOptions(awsOptions); builder.Services.AddAWSService(); +// stateless, so shared; the payment service is scoped because it holds the typed bank client, +// which must not be captured by a singleton (its handler is rotated by IHttpClientFactory) builder.Services.AddSingleton(); - -builder.Services.AddScoped(); -builder.Services.AddScoped(); -builder.Services.AddScoped(); +builder.Services.AddSingleton(); +builder.Services.AddScoped(); builder.Services.AddEndpointsApiExplorer(); builder.Services.AddSwaggerGen(); diff --git a/src/Services/BankClient.cs b/src/Services/BankClient.cs new file mode 100644 index 0000000..088c1c2 --- /dev/null +++ b/src/Services/BankClient.cs @@ -0,0 +1,119 @@ +using System; +using System.Net; +using System.Net.Http; +using System.Net.Http.Json; +using System.Text.Json; +using System.Threading; +using System.Threading.Tasks; +using Microsoft.Extensions.Logging; +using PaymentGatewayApi.Models; +using Polly.CircuitBreaker; + +namespace PaymentGatewayApi.Services; + +public interface IBankClient +{ + Task Authorize(Guid paymentId, PaymentRequest payment, CancellationToken cancellationToken); +} + +// owns the acquiring bank's contract: the request shape, and turning every answer (or its absence) into a BankOutcome +public partial class BankClient : IBankClient +{ + private static readonly Uri PaymentsUri = new("payments", UriKind.Relative); + + private readonly ILogger logger; + private readonly HttpClient client; + + // the base address comes from BankOptions, set where the typed client is registered + public BankClient(ILogger logger, HttpClient client) + { + this.logger = logger; + this.client = client; + } + + public static BankRequest ToBankRequest(PaymentRequest payment) + { + return new BankRequest + { + // the bank expects "MM/yyyy", e.g. "04/2025" + CardNumber = payment.CardNumber, + ExpiryDate = $"{payment.ExpiryMonth:D2}/{payment.ExpiryYear}", + Currency = payment.Currency, + Amount = payment.Amount, + Cvv = payment.Cvv + }; + } + + public async Task Authorize(Guid paymentId, PaymentRequest payment, CancellationToken cancellationToken) + { + HttpResponseMessage response; + try + { + response = await this.client.PostAsJsonAsync(PaymentsUri, ToBankRequest(payment), cancellationToken); + } + catch (BrokenCircuitException exception) + { + // the circuit breaker short-circuited the call, the request never left the gateway + this.LogBankUnavailable(exception, paymentId, "Failed"); + return BankOutcome.Failed; + } + catch (Exception exception) when (exception is not OperationCanceledException || !cancellationToken.IsCancellationRequested) + { + // timed out or the connection dropped: the bank may have received and authorized the payment + this.LogBankUnavailable(exception, paymentId, "Unknown"); + return BankOutcome.Unknown; + } + + using (response) + { + if (!response.IsSuccessStatusCode) + { + return this.FromErrorStatus(paymentId, response.StatusCode); + } + + BankResponse? bankResponse; + try + { + bankResponse = await response.Content.ReadFromJsonAsync(cancellationToken); + } + catch (Exception exception) when (exception is JsonException or NotSupportedException) + { + this.LogBankAnswerUnreadable(exception, paymentId, "Unknown"); + return BankOutcome.Unknown; + } + + if (bankResponse is null) + { + this.LogBankAnswerUnreadable(null, paymentId, "Unknown"); + return BankOutcome.Unknown; + } + + this.LogBankAnswered(paymentId, "Completed", bankResponse.Authorized); + + return bankResponse.Authorized ? BankOutcome.Authorized(bankResponse.AuthorizationCode) : BankOutcome.Declined; + } + } + + private BankOutcome FromErrorStatus(Guid paymentId, HttpStatusCode statusCode) + { + // a 4xx means the bank rejected the request and a 503 that it did not process it, so nothing was charged; + // any other 5xx says nothing reliable about whether the payment went through + bool notCharged = (int)statusCode < 500 || statusCode == HttpStatusCode.ServiceUnavailable; + BankOutcome outcome = notCharged ? BankOutcome.Failed : BankOutcome.Unknown; + + this.LogBankError(paymentId, outcome.Status.ToString(), (int)statusCode); + return outcome; + } + + [LoggerMessage(Level = LogLevel.Error, Message = "Authorize, PaymentId={PaymentId}, Status={Status}")] + private partial void LogBankUnavailable(Exception exception, Guid paymentId, string status); + + [LoggerMessage(Level = LogLevel.Error, Message = "Authorize, PaymentId={PaymentId}, Status={Status}, StatusCode={StatusCode}")] + private partial void LogBankError(Guid paymentId, string status, int statusCode); + + [LoggerMessage(Level = LogLevel.Error, Message = "Authorize, PaymentId={PaymentId}, Status={Status}, the bank's answer could not be read")] + private partial void LogBankAnswerUnreadable(Exception? exception, Guid paymentId, string status); + + [LoggerMessage(Level = LogLevel.Information, Message = "Authorize, PaymentId={PaymentId}, Status={Status}, Authorized={Authorized}")] + private partial void LogBankAnswered(Guid paymentId, string status, bool authorized); +} diff --git a/src/Services/BankHttpClient.cs b/src/Services/BankHttpClient.cs deleted file mode 100644 index 3faa53f..0000000 --- a/src/Services/BankHttpClient.cs +++ /dev/null @@ -1,70 +0,0 @@ -using System; -using System.Net; -using System.Net.Http; -using System.Net.Http.Json; -using System.Threading; -using System.Threading.Tasks; -using Microsoft.Extensions.Logging; -using PaymentGatewayApi.Models; - -namespace PaymentGatewayApi.Services; - -public interface IBankHttpClient -{ - Task PostBankPayment(PaymentRequest payment, CancellationToken cancellationToken); -} - -public partial class BankHttpClient : IBankHttpClient -{ - private readonly ILogger logger; - private readonly HttpClient client; - - // the base address comes from BankOptions, set where the typed client is registered - public BankHttpClient(ILogger logger, HttpClient client) - { - this.logger = logger; - this.client = client; - } - - public static BankRequest ToBankRequest(PaymentRequest payment) - { - return new BankRequest - { - // the bank expects "MM/yyyy", e.g. "04/2025" - CardNumber = payment.CardNumber, - ExpiryDate = $"{payment.ExpiryMonth:D2}/{payment.ExpiryYear}", - Currency = payment.Currency, - Amount = payment.Amount, - Cvv = payment.Cvv - }; - } - - public async Task PostBankPayment(PaymentRequest payment, CancellationToken cancellationToken) - { - BankRequest request = ToBankRequest(payment); - - HttpResponseMessage response; - try - { - response = await this.client.PostAsJsonAsync(new Uri("payments", UriKind.Relative), request, cancellationToken); - } - catch (Exception exception) - { - // bank unreachable, timed out, or circuit open; - // answer as the bank would when unavailable so BankService maps it to InternalError - this.LogPostFailed(exception, payment.PaymentId, "Failed"); - - return new HttpResponseMessage(HttpStatusCode.ServiceUnavailable); - } - - this.LogPosted(payment.PaymentId, "Completed", (int)response.StatusCode); - - return response; - } - - [LoggerMessage(Level = LogLevel.Error, Message = "PostBankPayment, PaymentId={PaymentId}, Status={Status}")] - private partial void LogPostFailed(Exception exception, string? paymentId, string status); - - [LoggerMessage(Level = LogLevel.Information, Message = "PostBankPayment, PaymentId={PaymentId}, Status={Status}, StatusCode={StatusCode}")] - private partial void LogPosted(string? paymentId, string status, int statusCode); -} diff --git a/src/Services/BankResponseProcessor.cs b/src/Services/BankResponseProcessor.cs deleted file mode 100644 index 92b31fa..0000000 --- a/src/Services/BankResponseProcessor.cs +++ /dev/null @@ -1,68 +0,0 @@ -using System; -using System.Net.Http; -using System.Net.Http.Json; -using System.Threading; -using System.Threading.Tasks; -using Microsoft.Extensions.Logging; -using PaymentGatewayApi.Models; - -namespace PaymentGatewayApi.Services; - -public interface IBankResponseProcessor -{ - // null when the bank's body cannot be used, so no half-filled response is ever persisted - Task ParseBankResponse(PaymentRequest payment, HttpResponseMessage response, CancellationToken cancellationToken); -} - -public partial class BankResponseProcessor : IBankResponseProcessor -{ - private readonly ILogger logger; - - public BankResponseProcessor(ILogger logger) - { - this.logger = logger; - } - - public async Task ParseBankResponse(PaymentRequest payment, HttpResponseMessage response, CancellationToken cancellationToken) - { - BankResponse? bankResponse; - - try - { - bankResponse = await response.Content.ReadFromJsonAsync(cancellationToken); - } - catch (Exception exception) - { - this.LogUnparseable(exception, payment.PaymentId, "Failed"); - - return null; - } - - if (bankResponse is null) - { - this.LogUnparseable(null, payment.PaymentId, "Failed"); - - return null; - } - - PostPaymentResponse payResponse = new(); - payResponse.Status = bankResponse.Authorized ? PaymentStatus.Authorized : PaymentStatus.Declined; - payResponse.CardNumberLastFour = payment.CardNumber.Substring(payment.CardNumber.Length - 4); - - payResponse.PaymentId = payment.PaymentId; - payResponse.ExpiryMonth = payment.ExpiryMonth; - payResponse.ExpiryYear = payment.ExpiryYear; - payResponse.Currency = payment.Currency; - payResponse.Amount = payment.Amount; - - this.LogParsed(payment.PaymentId, "Completed", bankResponse.Authorized); - - return payResponse; - } - - [LoggerMessage(Level = LogLevel.Error, Message = "ParseBankResponse, PaymentId={PaymentId}, Status={Status}")] - private partial void LogUnparseable(Exception? exception, string? paymentId, string status); - - [LoggerMessage(Level = LogLevel.Information, Message = "ParseBankResponse, PaymentId={PaymentId}, Status={Status}, Authorized={Authorized}")] - private partial void LogParsed(string? paymentId, string status, bool authorized); -} diff --git a/src/Services/BankService.cs b/src/Services/BankService.cs deleted file mode 100644 index d8c1437..0000000 --- a/src/Services/BankService.cs +++ /dev/null @@ -1,66 +0,0 @@ -using System.Net.Http; -using System.Threading; -using System.Threading.Tasks; -using Microsoft.Extensions.Logging; -using PaymentGatewayApi.Models; - -namespace PaymentGatewayApi.Services; - -public interface IBankService -{ - Task ExecutePayment(PaymentRequest payment, CancellationToken cancellationToken); -} - -public partial class BankService : IBankService -{ - private readonly ILogger logger; - private readonly IBankHttpClient bankHttpClient; - private readonly IBankResponseProcessor responseProcessor; - private readonly IPaymentsRepository paymentRepo; - - public BankService( - ILogger logger, IBankHttpClient bankHttpClient, IBankResponseProcessor responseProcessor, IPaymentsRepository paymentRepo) - { - this.logger = logger; - this.bankHttpClient = bankHttpClient; - this.responseProcessor = responseProcessor; - this.paymentRepo = paymentRepo; - } - - public async Task ExecutePayment(PaymentRequest payment, CancellationToken cancellationToken) - { - this.LogExecutePayment(payment.PaymentId, "Started"); - - // a client that disconnects can still stop the payment before the bank is called; once it is, - // cancelling would leave the bank's outcome unknown, so the bank call and the write that - // records its answer run to completion (bounded by the resilience handler's timeouts) - cancellationToken.ThrowIfCancellationRequested(); - - HttpResponseMessage bankResponse = await this.bankHttpClient.PostBankPayment(payment, CancellationToken.None); - if (bankResponse.IsSuccessStatusCode) - { - PostPaymentResponse? paymentResponse = await this.responseProcessor.ParseBankResponse(payment, bankResponse, CancellationToken.None); - if (paymentResponse is null) - { - // the bank answered 2xx but its body is unusable, so the outcome is unknown and there is nothing to persist - this.LogExecutePaymentFailed(payment.PaymentId, "Failed"); - return PaymentStatus.InternalError; - } - - await this.paymentRepo.Add(paymentResponse, CancellationToken.None); - - return paymentResponse.Status; - } - else - { - this.LogExecutePaymentFailed(payment.PaymentId, "Failed"); - return PaymentStatus.InternalError; - } - } - - [LoggerMessage(Level = LogLevel.Information, Message = "ExecutePayment, PaymentId={PaymentId}, Status={Status}")] - private partial void LogExecutePayment(string? paymentId, string status); - - [LoggerMessage(Level = LogLevel.Error, Message = "ExecutePayment, PaymentId={PaymentId}, Status={Status}")] - private partial void LogExecutePaymentFailed(string? paymentId, string status); -} diff --git a/src/Services/PaymentRequestValidator.cs b/src/Services/PaymentRequestValidator.cs index 9014ab4..f51ec95 100644 --- a/src/Services/PaymentRequestValidator.cs +++ b/src/Services/PaymentRequestValidator.cs @@ -32,7 +32,7 @@ public bool IsCardNumberValid(PaymentRequest payment) { if (!Regex.IsMatch(payment.CardNumber, @"^\d{14,19}$")) { - this.LogRejected(payment.PaymentId, "Rejected", "IsCardNumberValid"); + this.LogRejected("Rejected", "IsCardNumberValid"); return false; } @@ -46,7 +46,7 @@ public bool IsExpiryValid(PaymentRequest payment) if (payment.ExpiryMonth < 1 || payment.ExpiryMonth > 12) { - this.LogRejected(payment.PaymentId, "Rejected", "IsExpiryMonthValid"); + this.LogRejected("Rejected", "IsExpiryMonthValid"); return false; } @@ -56,7 +56,7 @@ public bool IsExpiryValid(PaymentRequest payment) bool isTooFarAhead = payment.ExpiryYear > now.Year + MaxExpiryYearsAhead; if (isExpired || isTooFarAhead) { - this.LogRejected(payment.PaymentId, "Rejected", "IsExpiryValid"); + this.LogRejected("Rejected", "IsExpiryValid"); return false; } @@ -68,7 +68,7 @@ public bool IsAmountValid(PaymentRequest payment) { if (payment.Amount < 1) { - this.LogRejected(payment.PaymentId, "Rejected", "IsAmountValid"); + this.LogRejected("Rejected", "IsAmountValid"); return false; } @@ -81,7 +81,7 @@ public bool IsCurrencyCodeValid(PaymentRequest payment) // should be a list of accepted currencies, via database, config if (payment.Currency.Length != 3) { - this.LogRejected(payment.PaymentId, "Rejected", "IsCurrencyCodeValid"); + this.LogRejected("Rejected", "IsCurrencyCodeValid"); return false; } @@ -94,7 +94,7 @@ public bool IsCvvValid(PaymentRequest payment) // 3 or 4 digits if (!Regex.IsMatch(payment.Cvv, @"^\d{3,4}$")) { - this.LogRejected(payment.PaymentId, "Rejected", "IsCvvValid"); + this.LogRejected("Rejected", "IsCvvValid"); return false; } @@ -102,6 +102,6 @@ public bool IsCvvValid(PaymentRequest payment) return true; } - [LoggerMessage(Level = LogLevel.Information, Message = "Validate, PaymentId={PaymentId}, Status={Status}, Rule={Rule}")] - private partial void LogRejected(string? paymentId, string status, string rule); + [LoggerMessage(Level = LogLevel.Information, Message = "Validate, Status={Status}, Rule={Rule}")] + private partial void LogRejected(string status, string rule); } diff --git a/src/Services/PaymentService.cs b/src/Services/PaymentService.cs new file mode 100644 index 0000000..53456f2 --- /dev/null +++ b/src/Services/PaymentService.cs @@ -0,0 +1,98 @@ +using System; +using System.Threading; +using System.Threading.Tasks; +using Microsoft.Extensions.Logging; +using PaymentGatewayApi.Models; + +namespace PaymentGatewayApi.Services; + +public interface IPaymentService +{ + // takes a validated request through the bank and returns the payment with the bank's outcome + Task ProcessPayment(PaymentRequest request, CancellationToken cancellationToken); + + Task FindPayment(Guid paymentId, CancellationToken cancellationToken); +} + +public partial class PaymentService : IPaymentService +{ + private readonly ILogger logger; + private readonly IBankClient bankClient; + private readonly IPaymentsRepository paymentRepo; + private readonly TimeProvider timeProvider; + + public PaymentService( + ILogger logger, IBankClient bankClient, IPaymentsRepository paymentRepo, TimeProvider timeProvider) + { + this.logger = logger; + this.bankClient = bankClient; + this.paymentRepo = paymentRepo; + this.timeProvider = timeProvider; + } + + public async Task ProcessPayment(PaymentRequest request, CancellationToken cancellationToken) + { + DateTimeOffset now = this.timeProvider.GetUtcNow(); + Payment pending = new() + { + Id = Guid.NewGuid(), + Status = PaymentStatus.Pending, + CardNumberLastFour = request.CardNumber[^4..], + ExpiryMonth = request.ExpiryMonth, + ExpiryYear = request.ExpiryYear, + Currency = request.Currency, + Amount = request.Amount, + CreatedAt = now, + UpdatedAt = now + }; + + this.LogProcessPayment(pending.Id, "Started"); + + // recorded before the bank is called, so a card can never be charged without a record of it; + // if this write fails the request fails and the bank is never called + await this.paymentRepo.Add(pending, cancellationToken); + + // a disconnected client can still stop the payment here; once the bank is called, cancelling + // would leave its outcome unknown, so the call and the write that records it run to completion + // (bounded by the bank client's resilience timeouts) + cancellationToken.ThrowIfCancellationRequested(); + + BankOutcome outcome = await this.bankClient.Authorize(pending.Id, request, CancellationToken.None); + + Payment completed = pending with + { + Status = outcome.Status, + AuthorizationCode = outcome.AuthorizationCode, + UpdatedAt = this.timeProvider.GetUtcNow() + }; + + try + { + await this.paymentRepo.Complete(completed, CancellationToken.None); + } + catch (Exception exception) + { + // the bank has answered, so the merchant still gets the real outcome; the stored payment + // stays Pending, and this log line carries what reconciliation needs to complete it + this.LogCompleteFailed(exception, completed.Id, "Failed", completed.Status, completed.AuthorizationCode); + return completed; + } + + this.LogPaymentCompleted(completed.Id, "Completed", completed.Status); + return completed; + } + + public Task FindPayment(Guid paymentId, CancellationToken cancellationToken) + { + return this.paymentRepo.Find(paymentId, cancellationToken); + } + + [LoggerMessage(Level = LogLevel.Information, Message = "ProcessPayment, PaymentId={PaymentId}, Status={Status}")] + private partial void LogProcessPayment(Guid paymentId, string status); + + [LoggerMessage(Level = LogLevel.Information, Message = "ProcessPayment, PaymentId={PaymentId}, Status={Status}, PaymentStatus={PaymentStatus}")] + private partial void LogPaymentCompleted(Guid paymentId, string status, PaymentStatus paymentStatus); + + [LoggerMessage(Level = LogLevel.Critical, Message = "ProcessPayment, PaymentId={PaymentId}, Status={Status}, the bank's outcome was not stored, PaymentStatus={PaymentStatus}, AuthorizationCode={AuthorizationCode}")] + private partial void LogCompleteFailed(Exception exception, Guid paymentId, string status, PaymentStatus paymentStatus, string? authorizationCode); +} diff --git a/src/Services/PaymentsRepository.cs b/src/Services/PaymentsRepository.cs index 38f8dd4..1e87d85 100644 --- a/src/Services/PaymentsRepository.cs +++ b/src/Services/PaymentsRepository.cs @@ -1,11 +1,10 @@ using System; using System.Collections.Generic; -using System.Text.Json; +using System.Globalization; using System.Threading; using System.Threading.Tasks; using Amazon.DynamoDBv2; using Amazon.DynamoDBv2.Model; -using Microsoft.Extensions.Logging; using Microsoft.Extensions.Options; using PaymentGatewayApi.Configuration; using PaymentGatewayApi.Models; @@ -14,92 +13,108 @@ namespace PaymentGatewayApi.Services; public interface IPaymentsRepository { - Task Add(PostPaymentResponse payment, CancellationToken cancellationToken); - Task Find(string paymentId, CancellationToken cancellationToken); + // stores a new payment, failing if its id already exists + Task Add(Payment payment, CancellationToken cancellationToken); + + // records the bank's outcome on a payment that is still pending + Task Complete(Payment payment, CancellationToken cancellationToken); + + Task Find(Guid paymentId, CancellationToken cancellationToken); } -public partial class PaymentsRepository : IPaymentsRepository +// one item per payment, keyed by PaymentId, with one attribute per field; the item is the storage +// contract, kept apart from the public response so either can change without migrating the other +public class PaymentsRepository : IPaymentsRepository { - private readonly ILogger logger; private readonly IAmazonDynamoDB dynamo; private readonly string tableName; - public PaymentsRepository(ILogger logger, IAmazonDynamoDB dynamo, IOptions options) + public PaymentsRepository(IAmazonDynamoDB dynamo, IOptions options) { - this.logger = logger; this.dynamo = dynamo; this.tableName = options.Value.PaymentsTable; } - public async Task Add(PostPaymentResponse payment, CancellationToken cancellationToken) + public async Task Add(Payment payment, CancellationToken cancellationToken) { - var item = new Dictionary + PutItemRequest request = new() { - ["PaymentId"] = new AttributeValue { S = payment.PaymentId }, - ["PaymentRaw"] = new AttributeValue { S = JsonSerializer.Serialize(payment) } + TableName = this.tableName, + Item = ToItem(payment), + ConditionExpression = "attribute_not_exists(PaymentId)" }; - try - { - await this.dynamo.PutItemAsync(this.tableName, item, cancellationToken); - } - catch (Exception exception) - { - // the bank has already answered, so failing the request here would hide an authorized payment - // from the merchant; log the raw item instead so it can be reconciled - this.LogAddFailed(exception, payment.PaymentId, "Failed", item["PaymentRaw"].S); - } + await this.dynamo.PutItemAsync(request, cancellationToken); } - public async Task Find(string paymentId, CancellationToken cancellationToken) + public async Task Complete(Payment payment, CancellationToken cancellationToken) { - var key = new Dictionary + // an outcome is recorded once: a payment that already left Pending is never overwritten + PutItemRequest request = new() { - ["PaymentId"] = new AttributeValue { S = paymentId } + TableName = this.tableName, + Item = ToItem(payment), + ConditionExpression = "#status = :pending", + ExpressionAttributeNames = new Dictionary { ["#status"] = "Status" }, + ExpressionAttributeValues = new Dictionary + { + [":pending"] = new AttributeValue { S = nameof(PaymentStatus.Pending) } + } }; - GetItemRequest request = new() { Key = key, TableName = this.tableName }; + await this.dynamo.PutItemAsync(request, cancellationToken); + } - PostPaymentResponse? payment; - try + public async Task Find(Guid paymentId, CancellationToken cancellationToken) + { + GetItemRequest request = new() { - GetItemResponse response = await this.dynamo.GetItemAsync(request, cancellationToken); + TableName = this.tableName, + Key = new Dictionary { ["PaymentId"] = new AttributeValue { S = paymentId.ToString() } } + }; - if (response.Item is { Count: > 0 }) - { - string raw = response.Item["PaymentRaw"].S; - payment = JsonSerializer.Deserialize(raw); - } - else - { - payment = null; - } - } - catch (Exception exception) + GetItemResponse response = await this.dynamo.GetItemAsync(request, cancellationToken); + + return response.Item is { Count: > 0 } item ? FromItem(item) : null; + } + + private static Dictionary ToItem(Payment payment) + { + Dictionary item = new() { - // rethrow so ErrorMiddleware answers 500: returning null would report an unreadable payment as not found - this.LogGetFailed(exception, paymentId, "Failed"); - throw; - } + ["PaymentId"] = new AttributeValue { S = payment.Id.ToString() }, + ["Status"] = new AttributeValue { S = payment.Status.ToString() }, + ["CardNumberLastFour"] = new AttributeValue { S = payment.CardNumberLastFour }, + ["ExpiryMonth"] = new AttributeValue { N = payment.ExpiryMonth.ToString(CultureInfo.InvariantCulture) }, + ["ExpiryYear"] = new AttributeValue { N = payment.ExpiryYear.ToString(CultureInfo.InvariantCulture) }, + ["Currency"] = new AttributeValue { S = payment.Currency }, + ["Amount"] = new AttributeValue { N = payment.Amount.ToString(CultureInfo.InvariantCulture) }, + ["CreatedAt"] = new AttributeValue { S = payment.CreatedAt.ToString("O", CultureInfo.InvariantCulture) }, + ["UpdatedAt"] = new AttributeValue { S = payment.UpdatedAt.ToString("O", CultureInfo.InvariantCulture) } + }; - if (payment is not null) + if (payment.AuthorizationCode is not null) { - return payment; + item["AuthorizationCode"] = new AttributeValue { S = payment.AuthorizationCode }; } - else - { - this.LogGetNotFound(paymentId, "NotFound"); - return null; - } + return item; } - [LoggerMessage(Level = LogLevel.Error, Message = "AddPayment, PaymentId={PaymentId}, Status={Status}, PaymentRaw={PaymentRaw}")] - private partial void LogAddFailed(Exception exception, string? paymentId, string status, string paymentRaw); - - [LoggerMessage(Level = LogLevel.Error, Message = "GetPayment, PaymentId={PaymentId}, Status={Status}")] - private partial void LogGetFailed(Exception exception, string paymentId, string status); - - [LoggerMessage(Level = LogLevel.Information, Message = "GetPayment, PaymentId={PaymentId}, Status={Status}")] - private partial void LogGetNotFound(string paymentId, string status); + private static Payment FromItem(Dictionary item) + { + return new Payment + { + Id = Guid.Parse(item["PaymentId"].S), + Status = Enum.Parse(item["Status"].S), + CardNumberLastFour = item["CardNumberLastFour"].S, + ExpiryMonth = int.Parse(item["ExpiryMonth"].N, CultureInfo.InvariantCulture), + ExpiryYear = int.Parse(item["ExpiryYear"].N, CultureInfo.InvariantCulture), + Currency = item["Currency"].S, + Amount = decimal.Parse(item["Amount"].N, CultureInfo.InvariantCulture), + AuthorizationCode = item.TryGetValue("AuthorizationCode", out AttributeValue? code) ? code.S : null, + CreatedAt = DateTimeOffset.Parse(item["CreatedAt"].S, CultureInfo.InvariantCulture), + UpdatedAt = DateTimeOffset.Parse(item["UpdatedAt"].S, CultureInfo.InvariantCulture) + }; + } } diff --git a/tests/PaymentGatewayApiEndToEndTests/PaymentEndToEndTests.cs b/tests/PaymentGatewayApiEndToEndTests/PaymentEndToEndTests.cs index b476c1a..46ff87b 100644 --- a/tests/PaymentGatewayApiEndToEndTests/PaymentEndToEndTests.cs +++ b/tests/PaymentGatewayApiEndToEndTests/PaymentEndToEndTests.cs @@ -73,17 +73,17 @@ public async Task PostPayment_EvenCard_Declined() HttpResponseMessage response = await this.client.PostAsJsonAsync("api/payment", payment, TestContext.Current.CancellationToken); - await AssertBadRequest(response, PaymentStatus.Declined); + await AssertBadRequest(response, nameof(PaymentStatus.Declined)); } [Fact] - public async Task PostPayment_CardEndingZero_InternalError() + public async Task PostPayment_CardEndingZero_Failed() { PaymentRequest payment = TestData.ValidPaymentRequest(TestData.BankUnavailableCard); HttpResponseMessage response = await this.client.PostAsJsonAsync("api/payment", payment, TestContext.Current.CancellationToken); - await AssertBadRequest(response, PaymentStatus.InternalError); + await AssertBadRequest(response, nameof(PaymentStatus.Failed)); } [Theory] @@ -108,7 +108,7 @@ public async Task PostPayment_InvalidField_Rejected(string invalidField) HttpResponseMessage response = await this.client.PostAsJsonAsync("api/payment", payment, TestContext.Current.CancellationToken); - await AssertBadRequest(response, PaymentStatus.Rejected); + await AssertBadRequest(response, "Rejected"); } [Fact] @@ -119,9 +119,9 @@ public async Task GetPayment_UnknownId_ReturnsNotFound() Assert.Equal(HttpStatusCode.NotFound, response.StatusCode); } - private static async Task AssertBadRequest(HttpResponseMessage response, PaymentStatus expected) + private static async Task AssertBadRequest(HttpResponseMessage response, string expected) { Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode); - Assert.Equal(expected.ToString(), await response.Content.ReadAsStringAsync(TestContext.Current.CancellationToken)); + Assert.Equal(expected, await response.Content.ReadAsStringAsync(TestContext.Current.CancellationToken)); } } diff --git a/tests/PaymentGatewayApiUnitTests/Controllers/PaymentControllerTests.cs b/tests/PaymentGatewayApiUnitTests/Controllers/PaymentControllerTests.cs index 89ad740..7c113bb 100644 --- a/tests/PaymentGatewayApiUnitTests/Controllers/PaymentControllerTests.cs +++ b/tests/PaymentGatewayApiUnitTests/Controllers/PaymentControllerTests.cs @@ -13,18 +13,14 @@ namespace PaymentGatewayApiUnitTests.Controllers; public class PaymentControllerTests { - private readonly Mock paymentRepo = new(); private readonly Mock validator = new(); - private readonly Mock bankService = new(); + private readonly Mock paymentService = new(); private readonly PaymentController controller; public PaymentControllerTests() { this.controller = new PaymentController( - NullLogger.Instance, - this.paymentRepo.Object, - this.validator.Object, - this.bankService.Object); + NullLogger.Instance, this.validator.Object, this.paymentService.Object); } private void SetupValidator() @@ -36,6 +32,13 @@ private void SetupValidator() this.validator.Setup(v => v.IsCvvValid(It.IsAny())).Returns(true); } + private void SetupPayment(PaymentStatus status) + { + this.paymentService + .Setup(s => s.ProcessPayment(It.IsAny(), It.IsAny())) + .ReturnsAsync(TestData.Payment(status)); + } + [Fact] public void GetHealthcheck_ReturnsOk() { @@ -48,19 +51,25 @@ public void GetHealthcheck_ReturnsOk() [Fact] public async Task GetPayment_Found_ReturnsOkWithPayment() { - PostPaymentResponse payment = new() { PaymentId = TestData.PaymentId, Status = PaymentStatus.Authorized }; - this.paymentRepo.Setup(r => r.Find(TestData.PaymentId, It.IsAny())).ReturnsAsync(payment); + this.paymentService + .Setup(s => s.FindPayment(TestData.PaymentId, It.IsAny())) + .ReturnsAsync(TestData.Payment(PaymentStatus.Authorized)); ActionResult result = await this.controller.GetPayment(TestData.PaymentId, TestContext.Current.CancellationToken); OkObjectResult ok = Assert.IsType(result.Result); - Assert.Same(payment, ok.Value); + PostPaymentResponse response = Assert.IsType(ok.Value); + Assert.Equal(TestData.PaymentId.ToString(), response.PaymentId); + Assert.Equal(PaymentStatus.Authorized, response.Status); + Assert.Equal("8873", response.CardNumberLastFour); } [Fact] public async Task GetPayment_Missing_ReturnsNotFound() { - this.paymentRepo.Setup(r => r.Find(It.IsAny(), It.IsAny())).ReturnsAsync((PostPaymentResponse?)null); + this.paymentService + .Setup(s => s.FindPayment(It.IsAny(), It.IsAny())) + .ReturnsAsync((Payment?)null); ActionResult result = await this.controller.GetPayment(TestData.PaymentId, TestContext.Current.CancellationToken); @@ -68,31 +77,15 @@ public async Task GetPayment_Missing_ReturnsNotFound() } [Fact] - public async Task PostPayment_Authorized_ReturnsOkWithNewPaymentId() + public async Task PostPayment_Authorized_ReturnsOkWithPaymentId() { - PaymentRequest payment = TestData.ValidPaymentRequest(); this.SetupValidator(); - this.bankService.Setup(b => b.ExecutePayment(payment, It.IsAny())).ReturnsAsync(PaymentStatus.Authorized); + this.SetupPayment(PaymentStatus.Authorized); - ActionResult result = await this.controller.PostPayment(payment, TestContext.Current.CancellationToken); + ActionResult result = await this.controller.PostPayment(TestData.ValidPaymentRequest(), TestContext.Current.CancellationToken); OkObjectResult ok = Assert.IsType(result.Result); - string paymentId = Assert.IsType(ok.Value); - Assert.True(Guid.TryParse(paymentId, out _)); - Assert.Equal(payment.PaymentId, paymentId); - } - - [Fact] - public async Task PostPayment_ClientSuppliedPaymentId_IsReplaced() - { - PaymentRequest payment = TestData.ValidPaymentRequest(); - this.SetupValidator(); - this.bankService.Setup(b => b.ExecutePayment(payment, It.IsAny())).ReturnsAsync(PaymentStatus.Authorized); - - await this.controller.PostPayment(payment, TestContext.Current.CancellationToken); - - Assert.NotEqual(TestData.PaymentId, payment.PaymentId); - this.bankService.Verify(b => b.ExecutePayment(It.Is(p => p.PaymentId != TestData.PaymentId), It.IsAny()), Times.Once); + Assert.Equal(TestData.PaymentId.ToString(), ok.Value); } [Theory] @@ -101,7 +94,7 @@ public async Task PostPayment_ClientSuppliedPaymentId_IsReplaced() [InlineData(nameof(IPaymentRequestValidator.IsAmountValid))] [InlineData(nameof(IPaymentRequestValidator.IsCurrencyCodeValid))] [InlineData(nameof(IPaymentRequestValidator.IsCvvValid))] - public async Task PostPayment_ValidationFails_ReturnsRejectedWithoutCallingBank(string failingCheck) + public async Task PostPayment_ValidationFails_ReturnsRejectedWithoutProcessing(string failingCheck) { this.SetupValidator(); switch (failingCheck) @@ -126,31 +119,19 @@ public async Task PostPayment_ValidationFails_ReturnsRejectedWithoutCallingBank( ActionResult result = await this.controller.PostPayment(TestData.ValidPaymentRequest(), TestContext.Current.CancellationToken); BadRequestObjectResult badRequest = Assert.IsType(result.Result); - Assert.Equal(nameof(PaymentStatus.Rejected), badRequest.Value); - this.bankService.Verify(b => b.ExecutePayment(It.IsAny(), It.IsAny()), Times.Never); - } - - [Fact] - public async Task PostPayment_CardNumberInvalid_SkipsRemainingChecks() - { - // this.SetupValidator(isValid: false); - - await this.controller.PostPayment(TestData.ValidPaymentRequest(), TestContext.Current.CancellationToken); - - this.validator.Verify(v => v.IsCardNumberValid(It.IsAny()), Times.Once); - this.validator.Verify(v => v.IsExpiryValid(It.IsAny()), Times.Never); - this.validator.Verify(v => v.IsAmountValid(It.IsAny()), Times.Never); - this.validator.Verify(v => v.IsCurrencyCodeValid(It.IsAny()), Times.Never); - this.validator.Verify(v => v.IsCvvValid(It.IsAny()), Times.Never); + Assert.Equal("Rejected", badRequest.Value); + this.paymentService.Verify( + s => s.ProcessPayment(It.IsAny(), It.IsAny()), Times.Never); } [Theory] [InlineData(PaymentStatus.Declined)] - [InlineData(PaymentStatus.InternalError)] + [InlineData(PaymentStatus.Failed)] + [InlineData(PaymentStatus.Unknown)] public async Task PostPayment_NotAuthorized_ReturnsBadRequestWithStatus(PaymentStatus status) { this.SetupValidator(); - this.bankService.Setup(b => b.ExecutePayment(It.IsAny(), It.IsAny())).ReturnsAsync(status); + this.SetupPayment(status); ActionResult result = await this.controller.PostPayment(TestData.ValidPaymentRequest(), TestContext.Current.CancellationToken); diff --git a/tests/PaymentGatewayApiUnitTests/Services/BankClientTests.cs b/tests/PaymentGatewayApiUnitTests/Services/BankClientTests.cs new file mode 100644 index 0000000..388e21d --- /dev/null +++ b/tests/PaymentGatewayApiUnitTests/Services/BankClientTests.cs @@ -0,0 +1,147 @@ +using System; +using System.Net; +using System.Net.Http; +using System.Text; +using System.Text.Json; +using System.Threading.Tasks; +using Microsoft.Extensions.Logging.Abstractions; +using PaymentGatewayApi.Models; +using PaymentGatewayApi.Services; +using Polly.CircuitBreaker; +using Polly.Timeout; +using Xunit; + +namespace PaymentGatewayApiUnitTests.Services; + +public class BankClientTests +{ + private static (BankClient Client, StubHttpMessageHandler Handler) NewClient(Func respond) + { + StubHttpMessageHandler handler = new(respond); + HttpClient http = new(handler) { BaseAddress = new Uri("http://bank.test/") }; + + return (new BankClient(NullLogger.Instance, http), handler); + } + + private static HttpResponseMessage Json(HttpStatusCode statusCode, string body) + { + return new HttpResponseMessage(statusCode) { Content = new StringContent(body, Encoding.UTF8, "application/json") }; + } + + private static Task Authorize(BankClient client) + { + return client.Authorize(TestData.PaymentId, TestData.ValidPaymentRequest(), TestContext.Current.CancellationToken); + } + + [Theory] + [InlineData(4, 2027, "04/2027")] + [InlineData(11, 2027, "11/2027")] + public void ToBankRequest_FormatsExpiryAsTwoDigitMonthAndFourDigitYear(int month, int year, string expected) + { + PaymentRequest payment = TestData.ValidPaymentRequest(); + payment.ExpiryMonth = month; + payment.ExpiryYear = year; + + BankRequest request = BankClient.ToBankRequest(payment); + + Assert.Equal(expected, request.ExpiryDate); + } + + [Fact] + public async Task Authorize_PostsTheBankContractToPayments() + { + (BankClient client, StubHttpMessageHandler handler) = NewClient(_ => Json(HttpStatusCode.OK, """{"authorized":false}""")); + + await Authorize(client); + + HttpRequestMessage request = Assert.Single(handler.Requests); + Assert.Equal(HttpMethod.Post, request.Method); + Assert.Equal(new Uri("http://bank.test/payments"), request.RequestUri); + + using JsonDocument body = JsonDocument.Parse(handler.Bodies[0]); + Assert.Equal("2222405343248873", body.RootElement.GetProperty("card_number").GetString()); + Assert.Equal("11/2027", body.RootElement.GetProperty("expiry_date").GetString()); + Assert.Equal("GBP", body.RootElement.GetProperty("currency").GetString()); + Assert.Equal(100, body.RootElement.GetProperty("amount").GetDecimal()); + Assert.Equal("123", body.RootElement.GetProperty("cvv").GetString()); + } + + [Fact] + public async Task Authorize_BankAuthorizes_ReturnsAuthorizedWithCode() + { + (BankClient client, _) = NewClient(_ => Json( + HttpStatusCode.OK, """{"authorized":true,"authorization_code":"0bb07405-6d44-4b50-a14f-7ae0beff13ad"}""")); + + BankOutcome outcome = await Authorize(client); + + Assert.Equal(PaymentStatus.Authorized, outcome.Status); + Assert.Equal("0bb07405-6d44-4b50-a14f-7ae0beff13ad", outcome.AuthorizationCode); + } + + [Fact] + public async Task Authorize_BankDeclines_ReturnsDeclined() + { + (BankClient client, _) = NewClient(_ => Json(HttpStatusCode.OK, """{"authorized":false,"authorization_code":""}""")); + + BankOutcome outcome = await Authorize(client); + + Assert.Equal(PaymentStatus.Declined, outcome.Status); + } + + [Theory] + [InlineData(HttpStatusCode.BadRequest, PaymentStatus.Failed)] + [InlineData(HttpStatusCode.ServiceUnavailable, PaymentStatus.Failed)] + [InlineData(HttpStatusCode.InternalServerError, PaymentStatus.Unknown)] + [InlineData(HttpStatusCode.GatewayTimeout, PaymentStatus.Unknown)] + public async Task Authorize_BankReturnsError_MapsWhetherTheCardCouldHaveBeenCharged(HttpStatusCode statusCode, PaymentStatus expected) + { + (BankClient client, _) = NewClient(_ => Json(statusCode, "{}")); + + BankOutcome outcome = await Authorize(client); + + Assert.Equal(expected, outcome.Status); + } + + [Theory] + [InlineData("{not json")] + [InlineData("null")] + [InlineData("")] + public async Task Authorize_UnreadableBody_ReturnsUnknown(string body) + { + (BankClient client, _) = NewClient(_ => Json(HttpStatusCode.OK, body)); + + BankOutcome outcome = await Authorize(client); + + Assert.Equal(PaymentStatus.Unknown, outcome.Status); + } + + [Fact] + public async Task Authorize_CircuitOpen_ReturnsFailed() + { + (BankClient client, _) = NewClient(_ => throw new BrokenCircuitException()); + + BankOutcome outcome = await Authorize(client); + + Assert.Equal(PaymentStatus.Failed, outcome.Status); + } + + [Fact] + public async Task Authorize_TimedOut_ReturnsUnknown() + { + (BankClient client, _) = NewClient(_ => throw new TimeoutRejectedException()); + + BankOutcome outcome = await Authorize(client); + + Assert.Equal(PaymentStatus.Unknown, outcome.Status); + } + + [Fact] + public async Task Authorize_ConnectionDropped_ReturnsUnknown() + { + (BankClient client, _) = NewClient(_ => throw new HttpRequestException("connection reset")); + + BankOutcome outcome = await Authorize(client); + + Assert.Equal(PaymentStatus.Unknown, outcome.Status); + } +} diff --git a/tests/PaymentGatewayApiUnitTests/Services/BankHttpClientTests.cs b/tests/PaymentGatewayApiUnitTests/Services/BankHttpClientTests.cs deleted file mode 100644 index 8cadfbd..0000000 --- a/tests/PaymentGatewayApiUnitTests/Services/BankHttpClientTests.cs +++ /dev/null @@ -1,35 +0,0 @@ -using PaymentGatewayApi.Models; -using PaymentGatewayApi.Services; -using Xunit; - -namespace PaymentGatewayApiUnitTests.Services; - -public class BankHttpClientTests -{ - [Theory] - [InlineData(4, 2027, "04/2027")] - [InlineData(11, 2027, "11/2027")] - public void ToBankRequest_FormatsExpiryAsTwoDigitMonthAndFourDigitYear(int month, int year, string expected) - { - PaymentRequest payment = TestData.ValidPaymentRequest(); - payment.ExpiryMonth = month; - payment.ExpiryYear = year; - - BankRequest request = BankHttpClient.ToBankRequest(payment); - - Assert.Equal(expected, request.ExpiryDate); - } - - [Fact] - public void ToBankRequest_MapsPaymentFields() - { - PaymentRequest payment = TestData.ValidPaymentRequest(); - - BankRequest request = BankHttpClient.ToBankRequest(payment); - - Assert.Equal("2222405343248873", request.CardNumber); - Assert.Equal("GBP", request.Currency); - Assert.Equal(100, request.Amount); - Assert.Equal("123", request.Cvv); - } -} diff --git a/tests/PaymentGatewayApiUnitTests/Services/BankResponseProcessorTests.cs b/tests/PaymentGatewayApiUnitTests/Services/BankResponseProcessorTests.cs deleted file mode 100644 index ed27b42..0000000 --- a/tests/PaymentGatewayApiUnitTests/Services/BankResponseProcessorTests.cs +++ /dev/null @@ -1,70 +0,0 @@ -using System.Net; -using System.Net.Http; -using System.Text; -using System.Threading.Tasks; -using Microsoft.Extensions.Logging.Abstractions; -using PaymentGatewayApi.Models; -using PaymentGatewayApi.Services; -using Xunit; - -namespace PaymentGatewayApiUnitTests.Services; - -public class BankResponseProcessorTests -{ - private readonly BankResponseProcessor processor = new(NullLogger.Instance); - - private static HttpResponseMessage JsonResponse(string body) - { - return new HttpResponseMessage(HttpStatusCode.OK) - { - Content = new StringContent(body, Encoding.UTF8, "application/json") - }; - } - - [Fact] - public async Task ParseBankResponse_Authorized_MapsRequestOntoResponse() - { - PaymentRequest request = TestData.ValidPaymentRequest(); - using HttpResponseMessage response = JsonResponse( - """{"authorized":true,"authorization_code":"0bb07405-6d44-4b50-a14f-7ae0beff13ad"}"""); - - PostPaymentResponse? result = await this.processor.ParseBankResponse(request, response, TestContext.Current.CancellationToken); - - Assert.NotNull(result); - Assert.Equal(PaymentStatus.Authorized, result.Status); - Assert.Equal(TestData.PaymentId, result.PaymentId); - Assert.Equal("8873", result.CardNumberLastFour); - Assert.Equal(11, result.ExpiryMonth); - Assert.Equal(2027, result.ExpiryYear); - Assert.Equal("GBP", result.Currency); - Assert.Equal(100, result.Amount); - } - - [Fact] - public async Task ParseBankResponse_NotAuthorized_ReturnsDeclined() - { - PaymentRequest request = TestData.ValidPaymentRequest(); - using HttpResponseMessage response = JsonResponse("""{"authorized":false,"authorization_code":""}"""); - - PostPaymentResponse? result = await this.processor.ParseBankResponse(request, response, TestContext.Current.CancellationToken); - - Assert.NotNull(result); - Assert.Equal(PaymentStatus.Declined, result.Status); - Assert.Equal(TestData.PaymentId, result.PaymentId); - Assert.Equal("8873", result.CardNumberLastFour); - } - - [Theory] - [InlineData("{not json")] - [InlineData("null")] - [InlineData("")] - public async Task ParseBankResponse_UnusableBody_ReturnsNull(string body) - { - PaymentRequest request = TestData.ValidPaymentRequest(); - using HttpResponseMessage response = JsonResponse(body); - - PostPaymentResponse? result = await this.processor.ParseBankResponse(request, response, TestContext.Current.CancellationToken); - - Assert.Null(result); - } -} diff --git a/tests/PaymentGatewayApiUnitTests/Services/BankServiceTests.cs b/tests/PaymentGatewayApiUnitTests/Services/BankServiceTests.cs deleted file mode 100644 index 6dde0a9..0000000 --- a/tests/PaymentGatewayApiUnitTests/Services/BankServiceTests.cs +++ /dev/null @@ -1,98 +0,0 @@ -using System; -using System.Net; -using System.Net.Http; -using System.Threading; -using System.Threading.Tasks; -using Microsoft.Extensions.Logging.Abstractions; -using Moq; -using PaymentGatewayApi.Models; -using PaymentGatewayApi.Services; -using Xunit; - -namespace PaymentGatewayApiUnitTests.Services; - -public class BankServiceTests -{ - private readonly Mock bankHttpClient = new(); - private readonly Mock responseProcessor = new(); - private readonly Mock paymentRepo = new(); - private readonly BankService service; - - public BankServiceTests() - { - this.service = new BankService( - NullLogger.Instance, - this.bankHttpClient.Object, - this.responseProcessor.Object, - this.paymentRepo.Object); - } - - private void SetupBank(PaymentRequest payment, HttpStatusCode statusCode, PostPaymentResponse? parsed = null) - { - HttpResponseMessage bankResponse = new(statusCode); - this.bankHttpClient.Setup(c => c.PostBankPayment(payment, CancellationToken.None)).ReturnsAsync(bankResponse); - - if (parsed is not null) - { - this.responseProcessor.Setup(p => p.ParseBankResponse(payment, bankResponse, CancellationToken.None)).ReturnsAsync(parsed); - } - } - - [Theory] - [InlineData(PaymentStatus.Authorized)] - [InlineData(PaymentStatus.Declined)] - public async Task ExecutePayment_BankAnswers_PersistsAndReturnsParsedStatus(PaymentStatus status) - { - PaymentRequest payment = TestData.ValidPaymentRequest(); - PostPaymentResponse parsed = new() { PaymentId = payment.PaymentId, Status = status }; - this.SetupBank(payment, HttpStatusCode.OK, parsed); - - PaymentStatus result = await this.service.ExecutePayment(payment, TestContext.Current.CancellationToken); - - Assert.Equal(status, result); - this.paymentRepo.Verify(r => r.Add(parsed, It.IsAny()), Times.Once); - } - - [Theory] - [InlineData(HttpStatusCode.ServiceUnavailable)] - [InlineData(HttpStatusCode.BadRequest)] - public async Task ExecutePayment_BankReturnsNonSuccess_ReturnsInternalErrorWithoutPersisting(HttpStatusCode statusCode) - { - PaymentRequest payment = TestData.ValidPaymentRequest(); - this.SetupBank(payment, statusCode); - - PaymentStatus result = await this.service.ExecutePayment(payment, TestContext.Current.CancellationToken); - - Assert.Equal(PaymentStatus.InternalError, result); - this.responseProcessor.Verify( - p => p.ParseBankResponse(It.IsAny(), It.IsAny(), It.IsAny()), Times.Never); - this.paymentRepo.Verify(r => r.Add(It.IsAny(), It.IsAny()), Times.Never); - } - - [Fact] - public async Task ExecutePayment_CancelledBeforeBankCall_ThrowsWithoutCallingBank() - { - using CancellationTokenSource cancelled = new(); - await cancelled.CancelAsync(); - - await Assert.ThrowsAnyAsync( - () => this.service.ExecutePayment(TestData.ValidPaymentRequest(), cancelled.Token)); - - this.bankHttpClient.Verify( - c => c.PostBankPayment(It.IsAny(), It.IsAny()), Times.Never); - } - - [Fact] - public async Task ExecutePayment_BankBodyUnparseable_ReturnsInternalErrorWithoutPersisting() - { - PaymentRequest payment = TestData.ValidPaymentRequest(); - HttpResponseMessage bankResponse = new(HttpStatusCode.OK); - this.bankHttpClient.Setup(c => c.PostBankPayment(payment, CancellationToken.None)).ReturnsAsync(bankResponse); - this.responseProcessor.Setup(p => p.ParseBankResponse(payment, bankResponse, CancellationToken.None)).ReturnsAsync((PostPaymentResponse?)null); - - PaymentStatus result = await this.service.ExecutePayment(payment, TestContext.Current.CancellationToken); - - Assert.Equal(PaymentStatus.InternalError, result); - this.paymentRepo.Verify(r => r.Add(It.IsAny(), It.IsAny()), Times.Never); - } -} diff --git a/tests/PaymentGatewayApiUnitTests/Services/PaymentServiceTests.cs b/tests/PaymentGatewayApiUnitTests/Services/PaymentServiceTests.cs new file mode 100644 index 0000000..ced2690 --- /dev/null +++ b/tests/PaymentGatewayApiUnitTests/Services/PaymentServiceTests.cs @@ -0,0 +1,151 @@ +using System; +using System.Collections.Generic; +using System.Threading; +using System.Threading.Tasks; +using Amazon.DynamoDBv2; +using Microsoft.Extensions.Logging.Abstractions; +using Microsoft.Extensions.Time.Testing; +using Moq; +using PaymentGatewayApi.Models; +using PaymentGatewayApi.Services; +using Xunit; + +namespace PaymentGatewayApiUnitTests.Services; + +public class PaymentServiceTests +{ + private readonly Mock bankClient = new(); + private readonly Mock paymentRepo = new(); + private readonly FakeTimeProvider timeProvider = new(TestData.Now); + private readonly PaymentService service; + + public PaymentServiceTests() + { + this.service = new PaymentService( + NullLogger.Instance, this.bankClient.Object, this.paymentRepo.Object, this.timeProvider); + } + + private void SetupBank(BankOutcome outcome) + { + this.bankClient + .Setup(c => c.Authorize(It.IsAny(), It.IsAny(), It.IsAny())) + .ReturnsAsync(outcome); + } + + [Fact] + public async Task ProcessPayment_StoresPendingPaymentBeforeCallingBank() + { + List calls = new(); + Payment? added = null; + this.paymentRepo + .Setup(r => r.Add(It.IsAny(), It.IsAny())) + .Callback((payment, _) => + { + calls.Add("add"); + added = payment; + }); + this.bankClient + .Setup(c => c.Authorize(It.IsAny(), It.IsAny(), It.IsAny())) + .Callback(() => calls.Add("bank")) + .ReturnsAsync(BankOutcome.Declined); + + await this.service.ProcessPayment(TestData.ValidPaymentRequest(), TestContext.Current.CancellationToken); + + Assert.Equal(["add", "bank"], calls); + Assert.NotNull(added); + Assert.Equal(PaymentStatus.Pending, added.Status); + Assert.Equal("8873", added.CardNumberLastFour); + Assert.Equal(TestData.Now, added.CreatedAt); + } + + [Theory] + [InlineData(PaymentStatus.Authorized)] + [InlineData(PaymentStatus.Declined)] + [InlineData(PaymentStatus.Failed)] + [InlineData(PaymentStatus.Unknown)] + public async Task ProcessPayment_RecordsAndReturnsTheBankOutcome(PaymentStatus status) + { + BankOutcome outcome = status switch + { + PaymentStatus.Authorized => BankOutcome.Authorized("auth-code"), + PaymentStatus.Declined => BankOutcome.Declined, + PaymentStatus.Failed => BankOutcome.Failed, + _ => BankOutcome.Unknown + }; + this.SetupBank(outcome); + this.bankClient + .Setup(c => c.Authorize(It.IsAny(), It.IsAny(), It.IsAny())) + .Callback(() => this.timeProvider.Advance(TimeSpan.FromSeconds(2))) + .ReturnsAsync(outcome); + + Payment result = await this.service.ProcessPayment(TestData.ValidPaymentRequest(), TestContext.Current.CancellationToken); + + Assert.Equal(status, result.Status); + Assert.Equal(outcome.AuthorizationCode, result.AuthorizationCode); + Assert.Equal(TestData.Now, result.CreatedAt); + Assert.Equal(TestData.Now.AddSeconds(2), result.UpdatedAt); + this.paymentRepo.Verify(r => r.Complete(result, CancellationToken.None), Times.Once); + } + + [Fact] + public async Task ProcessPayment_PendingWriteFails_ThrowsWithoutCallingBank() + { + this.paymentRepo + .Setup(r => r.Add(It.IsAny(), It.IsAny())) + .ThrowsAsync(new AmazonDynamoDBException("unavailable")); + + await Assert.ThrowsAsync( + () => this.service.ProcessPayment(TestData.ValidPaymentRequest(), TestContext.Current.CancellationToken)); + + this.bankClient.Verify( + c => c.Authorize(It.IsAny(), It.IsAny(), It.IsAny()), Times.Never); + } + + [Fact] + public async Task ProcessPayment_OutcomeWriteFails_StillReturnsTheBankOutcome() + { + this.SetupBank(BankOutcome.Authorized("auth-code")); + this.paymentRepo + .Setup(r => r.Complete(It.IsAny(), It.IsAny())) + .ThrowsAsync(new AmazonDynamoDBException("unavailable")); + + Payment result = await this.service.ProcessPayment(TestData.ValidPaymentRequest(), TestContext.Current.CancellationToken); + + Assert.Equal(PaymentStatus.Authorized, result.Status); + } + + [Fact] + public async Task ProcessPayment_CancelledBeforeBankCall_ThrowsWithoutCallingBank() + { + using CancellationTokenSource cancelled = new(); + await cancelled.CancelAsync(); + + await Assert.ThrowsAnyAsync( + () => this.service.ProcessPayment(TestData.ValidPaymentRequest(), cancelled.Token)); + + this.bankClient.Verify( + c => c.Authorize(It.IsAny(), It.IsAny(), It.IsAny()), Times.Never); + } + + [Fact] + public async Task ProcessPayment_BankCallIgnoresTheRequestToken() + { + this.SetupBank(BankOutcome.Declined); + + await this.service.ProcessPayment(TestData.ValidPaymentRequest(), TestContext.Current.CancellationToken); + + this.bankClient.Verify( + c => c.Authorize(It.IsAny(), It.IsAny(), CancellationToken.None), Times.Once); + } + + [Fact] + public async Task FindPayment_ReturnsStoredPayment() + { + Payment stored = TestData.Payment(PaymentStatus.Authorized); + this.paymentRepo.Setup(r => r.Find(TestData.PaymentId, It.IsAny())).ReturnsAsync(stored); + + Payment? result = await this.service.FindPayment(TestData.PaymentId, TestContext.Current.CancellationToken); + + Assert.Same(stored, result); + } +} diff --git a/tests/PaymentGatewayApiUnitTests/StubHttpMessageHandler.cs b/tests/PaymentGatewayApiUnitTests/StubHttpMessageHandler.cs new file mode 100644 index 0000000..a3cc4c2 --- /dev/null +++ b/tests/PaymentGatewayApiUnitTests/StubHttpMessageHandler.cs @@ -0,0 +1,29 @@ +using System; +using System.Collections.Generic; +using System.Net.Http; +using System.Threading; +using System.Threading.Tasks; + +namespace PaymentGatewayApiUnitTests; + +// answers every request with the given function and records what was sent +public sealed class StubHttpMessageHandler : HttpMessageHandler +{ + private readonly Func respond; + + public StubHttpMessageHandler(Func respond) + { + this.respond = respond; + } + + public List Requests { get; } = new(); + public List Bodies { get; } = new(); + + protected override async Task SendAsync(HttpRequestMessage request, CancellationToken cancellationToken) + { + this.Requests.Add(request); + this.Bodies.Add(request.Content is null ? string.Empty : await request.Content.ReadAsStringAsync(cancellationToken)); + + return this.respond(request); + } +} diff --git a/tests/PaymentGatewayApiUnitTests/TestData.cs b/tests/PaymentGatewayApiUnitTests/TestData.cs index a887814..a61f345 100644 --- a/tests/PaymentGatewayApiUnitTests/TestData.cs +++ b/tests/PaymentGatewayApiUnitTests/TestData.cs @@ -1,10 +1,12 @@ +using System; using PaymentGatewayApi.Models; namespace PaymentGatewayApiUnitTests; public static class TestData { - public const string PaymentId = "16bd4749-d150-48d1-a3ce-257457d74d1f"; + public static readonly Guid PaymentId = Guid.Parse("16bd4749-d150-48d1-a3ce-257457d74d1f"); + public static readonly DateTimeOffset Now = new(2026, 9, 15, 12, 0, 0, TimeSpan.Zero); public static PaymentRequest ValidPaymentRequest() { @@ -15,8 +17,23 @@ public static PaymentRequest ValidPaymentRequest() Currency = "GBP", ExpiryMonth = 11, ExpiryYear = 2027, + Amount = 100 + }; + } + + public static Payment Payment(PaymentStatus status) + { + return new Payment + { + Id = PaymentId, + Status = status, + CardNumberLastFour = "8873", + ExpiryMonth = 11, + ExpiryYear = 2027, + Currency = "GBP", Amount = 100, - PaymentId = PaymentId + CreatedAt = Now, + UpdatedAt = Now }; } } From 6b9bc85943114d0a4795b9ce3fa56cc0c5639150 Mon Sep 17 00:00:00 2001 From: ati Date: Wed, 23 Sep 2026 23:50:52 +0100 Subject: [PATCH 06/13] make the HTTP contract honest: 201, ProblemDetails, 502 for bank failures The POST answered 200 with a bare id, 400 "Declined" for a valid request the issuer refused, and 400 "InternalError" when the bank was down, telling the client it was at fault and should not retry. Now, under api/payments: - Authorized and Declined are 201 Created with Location and the payment body; - a bank failure is 502 ProblemDetails carrying paymentId and paymentStatus; - validation failures are 400 ValidationProblemDetails listing every broken field, the same shape model binding already used; - GET takes {paymentId:guid} and answers 404 ProblemDetails; - unhandled exceptions go through AddExceptionHandler + AddProblemDetails and no longer leak the exception message; a client abort is not logged as an error. The validator returns all errors keyed by JSON field instead of five booleans that stopped at the first failure. Amounts are integers in minor units, currencies come from a configured ISO 4217 allow-list (case-insensitive, normalised to upper case), patterns are [GeneratedRegex] with [0-9] instead of \d. PaymentId leaves the request model, which becomes an immutable record whose ToString masks the card number and CVV. Enums serialize as names and JSON respects nullable annotations. The E2E suite no longer references the api project: it owns its request record and asserts on raw JSON, so contract drift fails it. Co-Authored-By: Claude Opus 5.5 --- AGENTS.md | 49 ++++-- src/Configuration/PaymentOptions.cs | 13 ++ src/Controllers/PaymentController.cs | 84 --------- src/Controllers/PaymentsController.cs | 105 +++++++++++ src/Middleware/ErrorMiddleware.cs | 40 ----- src/Middleware/GlobalExceptionHandler.cs | 57 ++++++ src/Models/Payment.cs | 2 +- src/Models/Requests/BankRequest.cs | 7 +- src/Models/Requests/PaymentRequest.cs | 38 ++-- src/Models/Responses/PaymentResponse.cs | 29 +++ src/Models/Responses/PostPaymentResponse.cs | 27 --- src/Program.cs | 22 ++- src/Services/BankClient.cs | 2 +- src/Services/PaymentRequestValidator.cs | 109 ++++++------ src/Services/PaymentService.cs | 2 +- src/Services/PaymentsRepository.cs | 2 +- src/appsettings.json | 3 + .../ApiFixture.cs | 7 +- .../PaymentEndToEndTests.cs | 165 +++++++++++------- .../PaymentGatewayApiEndToEndTests.csproj | 7 +- .../TestData.cs | 16 +- .../Controllers/PaymentControllerTests.cs | 141 --------------- .../Controllers/PaymentsControllerTests.cs | 140 +++++++++++++++ .../Middleware/ErrorMiddlewareTests.cs | 53 ------ .../Middleware/GlobalExceptionHandlerTests.cs | 70 ++++++++ .../Models/PaymentRequestTests.cs | 19 ++ .../Services/BankClientTests.cs | 18 +- .../Services/PaymentRequestValidatorTests.cs | 156 +++++++++-------- tests/PaymentGatewayApiUnitTests/TestData.cs | 4 +- tests/bruno-collection.yml | 46 ++--- 30 files changed, 812 insertions(+), 621 deletions(-) create mode 100644 src/Configuration/PaymentOptions.cs delete mode 100644 src/Controllers/PaymentController.cs create mode 100644 src/Controllers/PaymentsController.cs delete mode 100644 src/Middleware/ErrorMiddleware.cs create mode 100644 src/Middleware/GlobalExceptionHandler.cs create mode 100644 src/Models/Responses/PaymentResponse.cs delete mode 100644 src/Models/Responses/PostPaymentResponse.cs delete mode 100644 tests/PaymentGatewayApiUnitTests/Controllers/PaymentControllerTests.cs create mode 100644 tests/PaymentGatewayApiUnitTests/Controllers/PaymentsControllerTests.cs delete mode 100644 tests/PaymentGatewayApiUnitTests/Middleware/ErrorMiddlewareTests.cs create mode 100644 tests/PaymentGatewayApiUnitTests/Middleware/GlobalExceptionHandlerTests.cs create mode 100644 tests/PaymentGatewayApiUnitTests/Models/PaymentRequestTests.cs diff --git a/AGENTS.md b/AGENTS.md index c2f36bc..083b303 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -43,7 +43,7 @@ deliberately not unit-tested; it is only mocked through its interface. Tests com behaviour — update them when the defect is fixed. End-to-end tests live in `tests/PaymentGatewayApiEndToEndTests`. They are black-box: they call -the running API over HTTP (the `src` project is referenced only for its models), so they need +the running API over HTTP and do not reference the `src` project, so they need the whole stack up, including the API container, which sits behind the compose `api` profile: ```bash @@ -51,23 +51,23 @@ cd src && docker compose --profile api up -d --build # api on :5067, same por dotnet test --project tests/PaymentGatewayApiEndToEndTests ``` -An assembly fixture (`ApiFixture`) polls the healthcheck for 30s before failing. The base URL +An assembly fixture (`ApiFixture`) polls `/health/live` for 30s before failing. The base URL defaults to `http://localhost:5067`; override it with `E2E_BASE_URL` (e.g. to run against `dotnet run` instead of the container). The bank client's resilience handler never retries -the `POST` (it is not idempotent), so the bank's `503` fails fast. String results (`"ok"`, the payment id, -status names) come back as `text/plain`, not JSON strings. +the `POST` (it is not idempotent), so the bank's `503` fails fast. The suite owns its request +record and reads responses as raw JSON (`JsonNode`), so it catches contract drift. For manual checks, use the Bruno collection at `tests/bruno-collection.yml` (covers health, -get-by-id, authorized/declined/500 posts, and a direct simulator post), or the `curl` lines -written above each action in `Controllers/PaymentController.cs`. +get-by-id, authorized/declined/502 posts, and a direct simulator post), or the `curl` lines +written above each action in `Controllers/PaymentsController.cs`. ## Architecture -A `POST /api/payment` flows through one chain, each link behind an interface: +A `POST /api/payments` flows through one chain, each link behind an interface: -`PaymentController` runs the five `IPaymentRequestValidator` checks -(`Services/PaymentRequestValidator.cs`), then hands the request to `IPaymentService` -(`Services/PaymentService.cs`), which: +`PaymentsController` asks `IPaymentRequestValidator.Validate` (`Services/PaymentRequestValidator.cs`) +for every broken rule and answers `400` `ValidationProblemDetails` if there are any; otherwise +it hands the request to `IPaymentService` (`Services/PaymentService.cs`), which: 1. stores a `Payment` with status `Pending` through `IPaymentsRepository` (`Services/PaymentsRepository.cs`), so a card is never charged without a record; if this @@ -88,9 +88,23 @@ A `POST /api/payment` flows through one chain, each link behind an interface: A request that fails validation never becomes a payment. +### HTTP contract + +| Outcome | Response | +| --- | --- | +| `Authorized` / `Declined` | `201` + `Location: /api/payments/{id}` + the payment | +| invalid request (validator or model binding) | `400` `ValidationProblemDetails`, errors keyed by JSON field | +| `Failed` / `Unknown` | `502` `ProblemDetails` with `paymentId` and `paymentStatus` extensions | +| `GET` of an unknown id | `404` `ProblemDetails` | +| unhandled exception | `500` `ProblemDetails` (`GlobalExceptionHandler`), never the exception message | + +Amounts are integers in the currency's minor unit (`1050` is £10.50), currencies come from +`Payments:SupportedCurrencies`, and statuses serialize as names. `PaymentRequest` is a +record whose `ToString` masks the card number and CVV. + ### Models come in pairs, deliberately -`PaymentRequest` / `PostPaymentResponse` are this gateway's own API shape, `Payment` is its +`PaymentRequest` / `PaymentResponse` are this gateway's own API shape, `Payment` is its domain record, and `BankRequest` / `BankResponse` model the acquiring bank's contract with `snake_case` names through `[JsonPropertyName]` (`card_number`, `expiry_date`, `authorization_code`). Keep them separate: collapsing them couples our public API to the @@ -111,7 +125,8 @@ singletons, as is the `IAmazonDynamoDB` client registered through resilience handler has retries disabled, and `PaymentService` is scoped so it never captures it. New services follow the existing pattern — interface and implementation in the same file under `Services/` — and get registered there. -Middleware order is `LogMiddleware` then `ErrorMiddleware`. +Middleware order is `LogMiddleware`, then `UseExceptionHandler` (with `GlobalExceptionHandler` +and `AddProblemDetails`), then `UseStatusCodePages` so bodiless errors get a `ProblemDetails` too. ## Bank simulator behaviour @@ -120,10 +135,10 @@ The WireMock mappings in `src/Simulators/wiremock/mappings/` select a response f | Card ends in | Simulator response | Gateway result | | --- | --- | --- | -| odd digit | `200` `authorized: true` | `Authorized` | -| even digit (not 0) | `200` `authorized: false` | `Declined` | -| `0` | `503` | `Failed` | -| any missing field | `400` | `Failed` | +| odd digit | `200` `authorized: true` | `201` `Authorized` | +| even digit (not 0) | `200` `authorized: false` | `201` `Declined` | +| `0` | `503` | `502` `Failed` | +| any missing field | `400` | `502` `Failed` | The three `200`/`503` stubs also require `expiry_date` in the bank's `MM/yyyy` format, so a malformed expiry falls through like any other unmatched request. @@ -146,7 +161,7 @@ shadows the specific stubs; WireMock breaks equal-priority ties by load order. automatically by `LogMiddleware` from the request headers (a missing `CorrelationId` header gets a generated GUID), so never pass them by hand. - **Serilog is configured from `src/logconfig.json`, not `appsettings.json`.** - Settings bind to validated options classes in `Configuration/` (`Bank`, `Dynamo`), + Settings bind to validated options classes in `Configuration/` (`Bank`, `Payments`, `Dynamo`), checked at startup. Local-only values (the simulator URL, `AWS:ServiceURL` pointed at DynamoDB Local) live in `appsettings.Development.json`; the compose file sets them for the container through environment variables. Verbose file logs go to `src/z-logs/` diff --git a/src/Configuration/PaymentOptions.cs b/src/Configuration/PaymentOptions.cs new file mode 100644 index 0000000..e8c1aae --- /dev/null +++ b/src/Configuration/PaymentOptions.cs @@ -0,0 +1,13 @@ +using System.ComponentModel.DataAnnotations; + +namespace PaymentGatewayApi.Configuration; + +public sealed class PaymentOptions +{ + public const string SectionName = "Payments"; + + // ISO 4217 codes the gateway accepts + [Required] + [MinLength(1)] + public string[] SupportedCurrencies { get; set; } = []; +} diff --git a/src/Controllers/PaymentController.cs b/src/Controllers/PaymentController.cs deleted file mode 100644 index d5ae047..0000000 --- a/src/Controllers/PaymentController.cs +++ /dev/null @@ -1,84 +0,0 @@ -using System; -using System.Threading; -using System.Threading.Tasks; -using Microsoft.AspNetCore.Mvc; -using Microsoft.Extensions.Logging; -using PaymentGatewayApi.Models; -using PaymentGatewayApi.Services; - -namespace PaymentGatewayApi.Controllers; - -[Route("api/[controller]")] -[ApiController] -public partial class PaymentController : ControllerBase -{ - private readonly ILogger logger; - private readonly IPaymentRequestValidator validator; - private readonly IPaymentService paymentService; - - public PaymentController(ILogger logger, IPaymentRequestValidator validator, IPaymentService paymentService) - { - this.logger = logger; - this.validator = validator; - this.paymentService = paymentService; - } - - // curl -k http://localhost:5067/api/payment - [HttpGet] - public ActionResult GetHealthcheck() - { - this.LogGetHealthcheck("Started"); - - return new OkObjectResult("ok"); - } - - // curl -k http://localhost:5067/api/payment/16bd4749-d150-48d1-a3ce-257457d74d1f - [HttpGet("{paymentId:guid}")] - public async Task> GetPayment(Guid paymentId, CancellationToken cancellationToken) - { - this.LogGetPayment(paymentId, "Started"); - - Payment? payment = await this.paymentService.FindPayment(paymentId, cancellationToken); - if (payment is null) - { - return new NotFoundObjectResult("payment not found"); - } - - return new OkObjectResult(PostPaymentResponse.From(payment)); - } - - [HttpPost] - public async Task> PostPayment([FromBody] PaymentRequest request, CancellationToken cancellationToken) - { - this.LogPostPayment("Started"); - - bool isValid = - this.validator.IsCardNumberValid(request) && - this.validator.IsExpiryValid(request) && - this.validator.IsAmountValid(request) && - this.validator.IsCurrencyCodeValid(request) && - this.validator.IsCvvValid(request); - - if (!isValid) - { - return new BadRequestObjectResult("Rejected"); - } - - Payment payment = await this.paymentService.ProcessPayment(request, cancellationToken); - if (payment.Status == PaymentStatus.Authorized) - { - return new OkObjectResult(payment.Id.ToString()); - } - - return new BadRequestObjectResult(payment.Status.ToString()); - } - - [LoggerMessage(Level = LogLevel.Information, Message = "GetHealthcheck, Status={Status}")] - private partial void LogGetHealthcheck(string status); - - [LoggerMessage(Level = LogLevel.Information, Message = "GetPayment, PaymentId={PaymentId}, Status={Status}")] - private partial void LogGetPayment(Guid paymentId, string status); - - [LoggerMessage(Level = LogLevel.Information, Message = "PostPayment, Status={Status}")] - private partial void LogPostPayment(string status); -} diff --git a/src/Controllers/PaymentsController.cs b/src/Controllers/PaymentsController.cs new file mode 100644 index 0000000..2090ee0 --- /dev/null +++ b/src/Controllers/PaymentsController.cs @@ -0,0 +1,105 @@ +using System; +using System.Collections.Generic; +using System.Threading; +using System.Threading.Tasks; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Mvc; +using Microsoft.AspNetCore.Mvc.ModelBinding; +using Microsoft.Extensions.Logging; +using PaymentGatewayApi.Models; +using PaymentGatewayApi.Services; + +namespace PaymentGatewayApi.Controllers; + +[ApiController] +[Route("api/payments")] +public partial class PaymentsController : ControllerBase +{ + private readonly ILogger logger; + private readonly IPaymentRequestValidator validator; + private readonly IPaymentService paymentService; + + public PaymentsController(ILogger logger, IPaymentRequestValidator validator, IPaymentService paymentService) + { + this.logger = logger; + this.validator = validator; + this.paymentService = paymentService; + } + + // curl http://localhost:5067/api/payments/16bd4749-d150-48d1-a3ce-257457d74d1f + [HttpGet("{paymentId:guid}")] + [ProducesResponseType(StatusCodes.Status200OK, "application/json")] + [ProducesResponseType(StatusCodes.Status404NotFound, "application/problem+json")] + public async Task> GetPayment(Guid paymentId, CancellationToken cancellationToken) + { + this.LogGetPayment(paymentId, "Started"); + + Payment? payment = await this.paymentService.FindPayment(paymentId, cancellationToken); + if (payment is null) + { + this.LogGetPayment(paymentId, "NotFound"); + return this.NotFound(); + } + + return PaymentResponse.From(payment); + } + + // curl -X POST http://localhost:5067/api/payments -H "Content-Type: application/json" \ + // -d '{"cardNumber":"2222405343248873","expiryMonth":4,"expiryYear":2030,"currency":"GBP","amount":1050,"cvv":"123"}' + [HttpPost] + [Consumes("application/json")] + [ProducesResponseType(StatusCodes.Status201Created, "application/json")] + [ProducesResponseType(StatusCodes.Status400BadRequest, "application/problem+json")] + [ProducesResponseType(StatusCodes.Status502BadGateway, "application/problem+json")] + public async Task> PostPayment(PaymentRequest request, CancellationToken cancellationToken) + { + IReadOnlyDictionary errors = this.validator.Validate(request); + if (errors.Count > 0) + { + // no payment is created for an invalid request, so the bank is never called + return this.ValidationProblem(ToModelState(errors)); + } + + Payment payment = await this.paymentService.ProcessPayment(request, cancellationToken); + + // a declined payment is a valid outcome, the issuer said no: it is created like an authorized one + if (payment.Status is PaymentStatus.Authorized or PaymentStatus.Declined) + { + return this.CreatedAtAction(nameof(this.GetPayment), new { paymentId = payment.Id }, PaymentResponse.From(payment)); + } + + return this.BankProblem(payment); + } + + // the bank, not the merchant, is at fault, so never a 4xx (which tells a client not to retry) + private ObjectResult BankProblem(Payment payment) + { + string detail = payment.Status == PaymentStatus.Failed + ? "The acquiring bank did not process the payment and the card was not charged. It is safe to retry." + : "The acquiring bank's answer was not received, so the card may have been charged. Check the payment before retrying."; + + ProblemDetails problem = this.ProblemDetailsFactory.CreateProblemDetails( + this.HttpContext, StatusCodes.Status502BadGateway, "The acquiring bank could not complete the payment.", detail: detail); + problem.Extensions["paymentId"] = payment.Id; + problem.Extensions["paymentStatus"] = payment.Status.ToString(); + + return new ObjectResult(problem) { StatusCode = StatusCodes.Status502BadGateway }; + } + + private static ModelStateDictionary ToModelState(IReadOnlyDictionary errors) + { + ModelStateDictionary modelState = new(); + foreach ((string field, string[] messages) in errors) + { + foreach (string message in messages) + { + modelState.AddModelError(field, message); + } + } + + return modelState; + } + + [LoggerMessage(Level = LogLevel.Information, Message = "GetPayment, PaymentId={PaymentId}, Status={Status}")] + private partial void LogGetPayment(Guid paymentId, string status); +} diff --git a/src/Middleware/ErrorMiddleware.cs b/src/Middleware/ErrorMiddleware.cs deleted file mode 100644 index acf973d..0000000 --- a/src/Middleware/ErrorMiddleware.cs +++ /dev/null @@ -1,40 +0,0 @@ -using System; -using System.Threading.Tasks; -using Microsoft.AspNetCore.Http; -using Microsoft.Extensions.Logging; - -namespace PaymentGatewayApi.Middleware; - -public partial class ErrorMiddleware -{ - private readonly RequestDelegate next; - private readonly ILogger logger; - - public ErrorMiddleware(RequestDelegate next, ILogger logger) - { - this.next = next; - this.logger = logger; - } - - public async Task InvokeAsync(HttpContext context) - { - try - { - await this.next(context); - } - catch (Exception exception) - { - this.LogUnhandled(exception, "Failed"); - - context.Response.StatusCode = StatusCodes.Status500InternalServerError; - - // see if can replace message with correlationId - await context.Response.WriteAsync($"Unhandled exception\n{exception.Message}\n"); - - return; - } - } - - [LoggerMessage(Level = LogLevel.Error, Message = "Unhandled exception, Status={Status}")] - private partial void LogUnhandled(Exception exception, string status); -} diff --git a/src/Middleware/GlobalExceptionHandler.cs b/src/Middleware/GlobalExceptionHandler.cs new file mode 100644 index 0000000..1e238f1 --- /dev/null +++ b/src/Middleware/GlobalExceptionHandler.cs @@ -0,0 +1,57 @@ +using System; +using System.Threading; +using System.Threading.Tasks; +using Microsoft.AspNetCore.Diagnostics; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Mvc; +using Microsoft.Extensions.Logging; + +namespace PaymentGatewayApi.Middleware; + +// last resort for anything the pipeline did not handle: log it, answer a generic ProblemDetails +// with the trace id, and never echo the exception message (it can name hosts, tables, keys) +public partial class GlobalExceptionHandler : IExceptionHandler +{ + // nginx's convention for "client closed request", the client is gone and never reads it + public const int ClientClosedRequest = 499; + + private readonly ILogger logger; + private readonly IProblemDetailsService problemDetailsService; + + public GlobalExceptionHandler(ILogger logger, IProblemDetailsService problemDetailsService) + { + this.logger = logger; + this.problemDetailsService = problemDetailsService; + } + + public async ValueTask TryHandleAsync(HttpContext httpContext, Exception exception, CancellationToken cancellationToken) + { + if (exception is OperationCanceledException && httpContext.RequestAborted.IsCancellationRequested) + { + this.LogClientAborted(httpContext.Request.Path, "Cancelled"); + httpContext.Response.StatusCode = ClientClosedRequest; + return true; + } + + this.LogUnhandled(exception, httpContext.Request.Path, "Failed"); + + httpContext.Response.StatusCode = StatusCodes.Status500InternalServerError; + return await this.problemDetailsService.TryWriteAsync(new ProblemDetailsContext + { + HttpContext = httpContext, + Exception = exception, + ProblemDetails = new ProblemDetails + { + Status = StatusCodes.Status500InternalServerError, + Title = "An unexpected error occurred.", + Detail = "Quote the traceId to support." + } + }); + } + + [LoggerMessage(Level = LogLevel.Error, Message = "Unhandled exception, Path={Path}, Status={Status}")] + private partial void LogUnhandled(Exception exception, PathString path, string status); + + [LoggerMessage(Level = LogLevel.Information, Message = "Request aborted by the client, Path={Path}, Status={Status}")] + private partial void LogClientAborted(PathString path, string status); +} diff --git a/src/Models/Payment.cs b/src/Models/Payment.cs index 57057f6..488f3c6 100644 --- a/src/Models/Payment.cs +++ b/src/Models/Payment.cs @@ -11,7 +11,7 @@ public sealed record Payment public required int ExpiryMonth { get; init; } public required int ExpiryYear { get; init; } public required string Currency { get; init; } - public required decimal Amount { get; init; } + public required int Amount { get; init; } public string? AuthorizationCode { get; init; } public required DateTimeOffset CreatedAt { get; init; } public required DateTimeOffset UpdatedAt { get; init; } diff --git a/src/Models/Requests/BankRequest.cs b/src/Models/Requests/BankRequest.cs index c670f42..4f50e47 100644 --- a/src/Models/Requests/BankRequest.cs +++ b/src/Models/Requests/BankRequest.cs @@ -7,7 +7,7 @@ namespace PaymentGatewayApi.Models; // "card_number": "2222405343248873", // "expiry_date": "04/2025", // "currency": "GBP", -// "amount": 100, +// "amount": 1050, // "cvv": "123" // } @@ -20,6 +20,9 @@ public class BankRequest public required string ExpiryDate { get; set; } public required string Currency { get; set; } - public decimal Amount { get; set; } + + // minor currency unit + public int Amount { get; set; } + public required string Cvv { get; set; } } diff --git a/src/Models/Requests/PaymentRequest.cs b/src/Models/Requests/PaymentRequest.cs index 24e9b30..ffccbd3 100644 --- a/src/Models/Requests/PaymentRequest.cs +++ b/src/Models/Requests/PaymentRequest.cs @@ -1,23 +1,39 @@ +using System.Globalization; +using System.Text; + namespace PaymentGatewayApi.Models; // example json payload // { -// "cardnumber": "2222405343248873", -// "ExpiryMonth": "11", -// "ExpiryYear": "2027", +// "cardNumber": "2222405343248873", +// "expiryMonth": 4, +// "expiryYear": 2027, // "currency": "GBP", -// "amount": 100, +// "amount": 1050, // "cvv": "123" // } -public class PaymentRequest +public sealed record PaymentRequest { - public required string CardNumber { get; set; } - public required string Cvv { get; set; } - public required string Currency { get; set; } + public required string CardNumber { get; init; } + public required int ExpiryMonth { get; init; } + public required int ExpiryYear { get; init; } + + // ISO 4217 code, one of Payments:SupportedCurrencies + public required string Currency { get; init; } + + // in the currency's minor unit, 1050 is £10.50 + public required int Amount { get; init; } - public int ExpiryMonth { get; set; } - public int ExpiryYear { get; set; } + public required string Cvv { get; init; } - public decimal Amount { get; set; } + // a record prints every property by default; keep the card number and CVV out of any + // log line or exception message that formats the request + private bool PrintMembers(StringBuilder builder) + { + builder.Append("CardNumber = ****, "); + builder.Append(CultureInfo.InvariantCulture, $"ExpiryMonth = {this.ExpiryMonth}, ExpiryYear = {this.ExpiryYear}, "); + builder.Append(CultureInfo.InvariantCulture, $"Currency = {this.Currency}, Amount = {this.Amount}, Cvv = ***"); + return true; + } } diff --git a/src/Models/Responses/PaymentResponse.cs b/src/Models/Responses/PaymentResponse.cs new file mode 100644 index 0000000..9e7b42b --- /dev/null +++ b/src/Models/Responses/PaymentResponse.cs @@ -0,0 +1,29 @@ +using System; + +namespace PaymentGatewayApi.Models; + +public sealed record PaymentResponse +{ + public required Guid Id { get; init; } + public required PaymentStatus Status { get; init; } + public required string CardNumberLastFour { get; init; } + public required int ExpiryMonth { get; init; } + public required int ExpiryYear { get; init; } + public required string Currency { get; init; } + public required int Amount { get; init; } + + // the authorization code and timestamps stay internal to the gateway + public static PaymentResponse From(Payment payment) + { + return new PaymentResponse + { + Id = payment.Id, + Status = payment.Status, + CardNumberLastFour = payment.CardNumberLastFour, + ExpiryMonth = payment.ExpiryMonth, + ExpiryYear = payment.ExpiryYear, + Currency = payment.Currency, + Amount = payment.Amount + }; + } +} diff --git a/src/Models/Responses/PostPaymentResponse.cs b/src/Models/Responses/PostPaymentResponse.cs deleted file mode 100644 index b95dbb0..0000000 --- a/src/Models/Responses/PostPaymentResponse.cs +++ /dev/null @@ -1,27 +0,0 @@ -namespace PaymentGatewayApi.Models; - -public class PostPaymentResponse -{ - public string? PaymentId { get; set; } - public PaymentStatus Status { get; set; } - public string? CardNumberLastFour { get; set; } - public int ExpiryMonth { get; set; } - public int ExpiryYear { get; set; } - public string? Currency { get; set; } - public decimal Amount { get; set; } - - // the authorization code and timestamps stay internal to the gateway - public static PostPaymentResponse From(Payment payment) - { - return new PostPaymentResponse - { - PaymentId = payment.Id.ToString(), - Status = payment.Status, - CardNumberLastFour = payment.CardNumberLastFour, - ExpiryMonth = payment.ExpiryMonth, - ExpiryYear = payment.ExpiryYear, - Currency = payment.Currency, - Amount = payment.Amount - }; - } -} diff --git a/src/Program.cs b/src/Program.cs index e179b97..5ef90d5 100644 --- a/src/Program.cs +++ b/src/Program.cs @@ -1,4 +1,5 @@ using System; +using System.Text.Json.Serialization; using Amazon.DynamoDBv2; using Microsoft.AspNetCore.Builder; using Microsoft.Extensions.Configuration; @@ -16,13 +17,28 @@ var log = new LoggerConfiguration().ReadFrom.Configuration(config).CreateLogger(); builder.Host.UseSerilog(log); -builder.Services.AddControllers(); +builder.Services.AddControllers() + .AddJsonOptions(options => + { + // statuses read as names, and a null where the model says non-nullable is a 400, not a crash later + options.JsonSerializerOptions.Converters.Add(new JsonStringEnumConverter()); + options.JsonSerializerOptions.RespectNullableAnnotations = true; + }); + +// every error body is a ProblemDetails with the request's traceId, never an exception message +builder.Services.AddProblemDetails(); +builder.Services.AddExceptionHandler(); +builder.Services.AddHealthChecks(); // a missing or malformed setting fails the startup, not the first request that needs it builder.Services.AddOptions() .BindConfiguration(BankOptions.SectionName) .ValidateDataAnnotations() .ValidateOnStart(); +builder.Services.AddOptions() + .BindConfiguration(PaymentOptions.SectionName) + .ValidateDataAnnotations() + .ValidateOnStart(); builder.Services.AddOptions() .BindConfiguration(DynamoOptions.SectionName) .ValidateDataAnnotations() @@ -56,8 +72,10 @@ app.UseSwaggerUI(); app.UseMiddleware(); -app.UseMiddleware(); +app.UseExceptionHandler(); +app.UseStatusCodePages(); +app.MapHealthChecks("/health/live"); app.MapControllers(); app.Run(); diff --git a/src/Services/BankClient.cs b/src/Services/BankClient.cs index 088c1c2..fee2d45 100644 --- a/src/Services/BankClient.cs +++ b/src/Services/BankClient.cs @@ -38,7 +38,7 @@ public static BankRequest ToBankRequest(PaymentRequest payment) // the bank expects "MM/yyyy", e.g. "04/2025" CardNumber = payment.CardNumber, ExpiryDate = $"{payment.ExpiryMonth:D2}/{payment.ExpiryYear}", - Currency = payment.Currency, + Currency = payment.Currency.ToUpperInvariant(), Amount = payment.Amount, Cvv = payment.Cvv }; diff --git a/src/Services/PaymentRequestValidator.cs b/src/Services/PaymentRequestValidator.cs index f51ec95..f21afaf 100644 --- a/src/Services/PaymentRequestValidator.cs +++ b/src/Services/PaymentRequestValidator.cs @@ -1,17 +1,17 @@ using System; +using System.Collections.Generic; using System.Text.RegularExpressions; using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Options; +using PaymentGatewayApi.Configuration; using PaymentGatewayApi.Models; namespace PaymentGatewayApi.Services; public interface IPaymentRequestValidator { - bool IsCardNumberValid(PaymentRequest payment); - bool IsExpiryValid(PaymentRequest payment); - bool IsAmountValid(PaymentRequest payment); - bool IsCurrencyCodeValid(PaymentRequest payment); - bool IsCvvValid(PaymentRequest payment); + // every rule the request breaks, keyed by its JSON field name; empty when the request is valid + IReadOnlyDictionary Validate(PaymentRequest request); } public partial class PaymentRequestValidator : IPaymentRequestValidator @@ -21,87 +21,78 @@ public partial class PaymentRequestValidator : IPaymentRequestValidator private readonly ILogger logger; private readonly TimeProvider timeProvider; + private readonly HashSet supportedCurrencies; - public PaymentRequestValidator(ILogger logger, TimeProvider timeProvider) + public PaymentRequestValidator( + ILogger logger, TimeProvider timeProvider, IOptions options) { this.logger = logger; this.timeProvider = timeProvider; + this.supportedCurrencies = new HashSet(options.Value.SupportedCurrencies, StringComparer.OrdinalIgnoreCase); } - public bool IsCardNumberValid(PaymentRequest payment) + public IReadOnlyDictionary Validate(PaymentRequest request) { - if (!Regex.IsMatch(payment.CardNumber, @"^\d{14,19}$")) + Dictionary errors = new(); + + // no Luhn check: the bank simulator picks its answer from the last digit, which a Luhn + // check digit would fix, and the issuer validates the number anyway + if (!CardNumberPattern().IsMatch(request.CardNumber)) { - this.LogRejected("Rejected", "IsCardNumberValid"); - return false; + errors["cardNumber"] = ["Must be 14 to 19 digits."]; } - return true; - } - - public bool IsExpiryValid(PaymentRequest payment) - { - // UTC, not the server's local time zone; a card is valid through the last day of its expiry month - DateTimeOffset now = this.timeProvider.GetUtcNow(); - - if (payment.ExpiryMonth < 1 || payment.ExpiryMonth > 12) + if (request.ExpiryMonth is < 1 or > 12) { - this.LogRejected("Rejected", "IsExpiryMonthValid"); - - return false; + errors["expiryMonth"] = ["Must be between 1 and 12."]; } - - // compare year and month directly, building a DateTime would throw for an out of range year - bool isExpired = payment.ExpiryYear < now.Year || (payment.ExpiryYear == now.Year && payment.ExpiryMonth < now.Month); - bool isTooFarAhead = payment.ExpiryYear > now.Year + MaxExpiryYearsAhead; - if (isExpired || isTooFarAhead) + else if (this.IsExpired(request)) { - this.LogRejected("Rejected", "IsExpiryValid"); - - return false; + errors["expiryYear"] = ["The card has expired."]; } - - return true; - } - - public bool IsAmountValid(PaymentRequest payment) - { - if (payment.Amount < 1) + else if (request.ExpiryYear > this.timeProvider.GetUtcNow().Year + MaxExpiryYearsAhead) { - this.LogRejected("Rejected", "IsAmountValid"); + errors["expiryYear"] = [$"Must be at most {MaxExpiryYearsAhead} years ahead."]; + } - return false; + if (!this.supportedCurrencies.Contains(request.Currency)) + { + errors["currency"] = [$"Must be one of {string.Join(", ", this.supportedCurrencies)}."]; } - return true; - } + if (request.Amount <= 0) + { + errors["amount"] = ["Must be a positive amount in the currency's minor unit."]; + } - public bool IsCurrencyCodeValid(PaymentRequest payment) - { - // should be a list of accepted currencies, via database, config - if (payment.Currency.Length != 3) + if (!CvvPattern().IsMatch(request.Cvv)) { - this.LogRejected("Rejected", "IsCurrencyCodeValid"); + errors["cvv"] = ["Must be 3 or 4 digits."]; + } - return false; + if (errors.Count > 0) + { + this.LogRejected("Rejected", errors.Keys); } - return true; + return errors; } - public bool IsCvvValid(PaymentRequest payment) + // compares year and month directly, building a DateTime would throw for an out of range year; + // UTC, not the server's time zone, and a card is valid through the last day of its expiry month + private bool IsExpired(PaymentRequest request) { - // 3 or 4 digits - if (!Regex.IsMatch(payment.Cvv, @"^\d{3,4}$")) - { - this.LogRejected("Rejected", "IsCvvValid"); + DateTimeOffset now = this.timeProvider.GetUtcNow(); + return request.ExpiryYear < now.Year || (request.ExpiryYear == now.Year && request.ExpiryMonth < now.Month); + } - return false; - } + // [0-9], not \d, which also matches other scripts' digits + [GeneratedRegex("^[0-9]{14,19}$")] + private static partial Regex CardNumberPattern(); - return true; - } + [GeneratedRegex("^[0-9]{3,4}$")] + private static partial Regex CvvPattern(); - [LoggerMessage(Level = LogLevel.Information, Message = "Validate, Status={Status}, Rule={Rule}")] - private partial void LogRejected(string status, string rule); + [LoggerMessage(Level = LogLevel.Information, Message = "Validate, Status={Status}, Fields={Fields}")] + private partial void LogRejected(string status, IEnumerable fields); } diff --git a/src/Services/PaymentService.cs b/src/Services/PaymentService.cs index 53456f2..342702b 100644 --- a/src/Services/PaymentService.cs +++ b/src/Services/PaymentService.cs @@ -40,7 +40,7 @@ public async Task ProcessPayment(PaymentRequest request, CancellationTo CardNumberLastFour = request.CardNumber[^4..], ExpiryMonth = request.ExpiryMonth, ExpiryYear = request.ExpiryYear, - Currency = request.Currency, + Currency = request.Currency.ToUpperInvariant(), Amount = request.Amount, CreatedAt = now, UpdatedAt = now diff --git a/src/Services/PaymentsRepository.cs b/src/Services/PaymentsRepository.cs index 1e87d85..13f6ded 100644 --- a/src/Services/PaymentsRepository.cs +++ b/src/Services/PaymentsRepository.cs @@ -111,7 +111,7 @@ private static Payment FromItem(Dictionary item) ExpiryMonth = int.Parse(item["ExpiryMonth"].N, CultureInfo.InvariantCulture), ExpiryYear = int.Parse(item["ExpiryYear"].N, CultureInfo.InvariantCulture), Currency = item["Currency"].S, - Amount = decimal.Parse(item["Amount"].N, CultureInfo.InvariantCulture), + Amount = int.Parse(item["Amount"].N, CultureInfo.InvariantCulture), AuthorizationCode = item.TryGetValue("AuthorizationCode", out AttributeValue? code) ? code.S : null, CreatedAt = DateTimeOffset.Parse(item["CreatedAt"].S, CultureInfo.InvariantCulture), UpdatedAt = DateTimeOffset.Parse(item["UpdatedAt"].S, CultureInfo.InvariantCulture) diff --git a/src/appsettings.json b/src/appsettings.json index 3fbb303..e2c796f 100644 --- a/src/appsettings.json +++ b/src/appsettings.json @@ -6,6 +6,9 @@ "TotalRequestTimeout": { "Timeout": "00:00:30" } } }, + "Payments": { + "SupportedCurrencies": [ "GBP", "EUR", "USD" ] + }, "Dynamo": { "PaymentsTable": "Payments" }, diff --git a/tests/PaymentGatewayApiEndToEndTests/ApiFixture.cs b/tests/PaymentGatewayApiEndToEndTests/ApiFixture.cs index a511f3f..d44d366 100644 --- a/tests/PaymentGatewayApiEndToEndTests/ApiFixture.cs +++ b/tests/PaymentGatewayApiEndToEndTests/ApiFixture.cs @@ -8,7 +8,7 @@ namespace PaymentGatewayApiEndToEndTests; -// shared by every test, waits for the api started by docker compose to answer its healthcheck +// shared by every test, waits for the api started by docker compose to answer its liveness check public sealed class ApiFixture : IAsyncLifetime { private static readonly TimeSpan StartupTimeout = TimeSpan.FromSeconds(30); @@ -19,11 +19,10 @@ public ApiFixture() { string baseUrl = Environment.GetEnvironmentVariable("E2E_BASE_URL") ?? "http://localhost:5067"; - // long timeout, the api retries the bank's 503 before answering this.Client = new HttpClient { BaseAddress = new Uri(baseUrl), - Timeout = TimeSpan.FromSeconds(60) + Timeout = TimeSpan.FromSeconds(30) }; this.Client.DefaultRequestHeaders.Add("CallerId", "e2e-tests"); } @@ -35,7 +34,7 @@ public async ValueTask InitializeAsync() { try { - HttpResponseMessage response = await this.Client.GetAsync("api/payment"); + HttpResponseMessage response = await this.Client.GetAsync(new Uri("health/live", UriKind.Relative)); if (response.IsSuccessStatusCode) { return; diff --git a/tests/PaymentGatewayApiEndToEndTests/PaymentEndToEndTests.cs b/tests/PaymentGatewayApiEndToEndTests/PaymentEndToEndTests.cs index 46ff87b..23cbfc9 100644 --- a/tests/PaymentGatewayApiEndToEndTests/PaymentEndToEndTests.cs +++ b/tests/PaymentGatewayApiEndToEndTests/PaymentEndToEndTests.cs @@ -2,8 +2,8 @@ using System.Net; using System.Net.Http; using System.Net.Http.Json; +using System.Text.Json.Nodes; using System.Threading.Tasks; -using PaymentGatewayApi.Models; using Xunit; namespace PaymentGatewayApiEndToEndTests; @@ -17,111 +17,148 @@ public PaymentEndToEndTests(ApiFixture fixture) this.client = fixture.Client; } + private Task Post(PaymentBody payment) + { + return this.client.PostAsJsonAsync("api/payments", payment, TestContext.Current.CancellationToken); + } + + private static async Task ReadJson(HttpResponseMessage response) + { + JsonNode? json = JsonNode.Parse(await response.Content.ReadAsStringAsync(TestContext.Current.CancellationToken)); + Assert.NotNull(json); + return json; + } + + private static async Task AssertProblem(HttpResponseMessage response, HttpStatusCode expected) + { + Assert.Equal(expected, response.StatusCode); + Assert.Equal("application/problem+json", response.Content.Headers.ContentType?.MediaType); + + JsonNode problem = await ReadJson(response); + Assert.Equal((int)expected, problem["status"]?.GetValue()); + Assert.NotNull(problem["traceId"]); + return problem; + } + [Fact] - public async Task Healthcheck_ReturnsOk() + public async Task HealthLive_ReturnsOk() { - HttpResponseMessage response = await this.client.GetAsync("api/payment", TestContext.Current.CancellationToken); + HttpResponseMessage response = await this.client.GetAsync("health/live", TestContext.Current.CancellationToken); Assert.Equal(HttpStatusCode.OK, response.StatusCode); - Assert.Equal("ok", await response.Content.ReadAsStringAsync(TestContext.Current.CancellationToken)); } - [Fact] - public async Task PostPayment_OddCard_AuthorizedAndRetrievable() + [Theory] + [InlineData(TestData.AuthorizedCard, "Authorized")] + [InlineData(TestData.DeclinedCard, "Declined")] + public async Task PostPayment_BankAnswers_CreatedAndRetrievable(string cardNumber, string expectedStatus) { - PaymentRequest payment = TestData.ValidPaymentRequest(TestData.AuthorizedCard); + PaymentBody payment = TestData.ValidPayment(cardNumber); - HttpResponseMessage postResponse = await this.client.PostAsJsonAsync("api/payment", payment, TestContext.Current.CancellationToken); + HttpResponseMessage postResponse = await this.Post(payment); - Assert.Equal(HttpStatusCode.OK, postResponse.StatusCode); - string paymentId = await postResponse.Content.ReadAsStringAsync(TestContext.Current.CancellationToken); - Assert.True(Guid.TryParse(paymentId, out _)); + Assert.Equal(HttpStatusCode.Created, postResponse.StatusCode); + JsonNode created = await ReadJson(postResponse); + Guid paymentId = Guid.Parse(created["id"]!.GetValue()); + Assert.Equal(expectedStatus, created["status"]?.GetValue()); + Assert.NotNull(postResponse.Headers.Location); + Assert.EndsWith($"/api/payments/{paymentId}", postResponse.Headers.Location.ToString(), StringComparison.OrdinalIgnoreCase); - HttpResponseMessage getResponse = await this.client.GetAsync($"api/payment/{paymentId}", TestContext.Current.CancellationToken); + HttpResponseMessage getResponse = await this.client.GetAsync(postResponse.Headers.Location, TestContext.Current.CancellationToken); Assert.Equal(HttpStatusCode.OK, getResponse.StatusCode); string raw = await getResponse.Content.ReadAsStringAsync(TestContext.Current.CancellationToken); - Assert.DoesNotContain(TestData.AuthorizedCard, raw); - - PostPaymentResponse? stored = await getResponse.Content.ReadFromJsonAsync(TestContext.Current.CancellationToken); - Assert.NotNull(stored); - Assert.Equal(paymentId, stored.PaymentId); - Assert.Equal(PaymentStatus.Authorized, stored.Status); - Assert.Equal("8873", stored.CardNumberLastFour); - Assert.Equal(payment.ExpiryMonth, stored.ExpiryMonth); - Assert.Equal(payment.ExpiryYear, stored.ExpiryYear); - Assert.Equal(payment.Currency, stored.Currency); - Assert.Equal(payment.Amount, stored.Amount); + Assert.DoesNotContain(cardNumber, raw); + Assert.DoesNotContain("cvv", raw, StringComparison.OrdinalIgnoreCase); + + JsonNode stored = JsonNode.Parse(raw)!; + Assert.Equal(paymentId.ToString(), stored["id"]?.GetValue()); + Assert.Equal(expectedStatus, stored["status"]?.GetValue()); + Assert.Equal(cardNumber[^4..], stored["cardNumberLastFour"]?.GetValue()); + Assert.Equal(payment.ExpiryMonth, stored["expiryMonth"]?.GetValue()); + Assert.Equal(payment.ExpiryYear, stored["expiryYear"]?.GetValue()); + Assert.Equal(payment.Currency, stored["currency"]?.GetValue()); + Assert.Equal(payment.Amount, stored["amount"]?.GetValue()); } [Fact] public async Task PostPayment_SingleDigitExpiryMonth_Authorized() { // the simulator only answers a "MM/yyyy" expiry, so a month sent as "4/..." would fall through to its 400 - PaymentRequest payment = TestData.ValidPaymentRequest(TestData.AuthorizedCard); - payment.ExpiryMonth = 4; - - HttpResponseMessage response = await this.client.PostAsJsonAsync("api/payment", payment, TestContext.Current.CancellationToken); + HttpResponseMessage response = await this.Post(TestData.ValidPayment(TestData.AuthorizedCard) with { ExpiryMonth = 4 }); - Assert.Equal(HttpStatusCode.OK, response.StatusCode); + Assert.Equal(HttpStatusCode.Created, response.StatusCode); + Assert.Equal("Authorized", (await ReadJson(response))["status"]?.GetValue()); } [Fact] - public async Task PostPayment_EvenCard_Declined() + public async Task PostPayment_BankUnavailable_BadGatewayWithFailedPaymentOnRecord() { - PaymentRequest payment = TestData.ValidPaymentRequest(TestData.DeclinedCard); + HttpResponseMessage response = await this.Post(TestData.ValidPayment(TestData.BankUnavailableCard)); - HttpResponseMessage response = await this.client.PostAsJsonAsync("api/payment", payment, TestContext.Current.CancellationToken); + JsonNode problem = await AssertProblem(response, HttpStatusCode.BadGateway); + Assert.Equal("Failed", problem["paymentStatus"]?.GetValue()); - await AssertBadRequest(response, nameof(PaymentStatus.Declined)); + string paymentId = problem["paymentId"]!.GetValue(); + HttpResponseMessage getResponse = await this.client.GetAsync($"api/payments/{paymentId}", TestContext.Current.CancellationToken); + Assert.Equal(HttpStatusCode.OK, getResponse.StatusCode); + Assert.Equal("Failed", (await ReadJson(getResponse))["status"]?.GetValue()); } - [Fact] - public async Task PostPayment_CardEndingZero_Failed() + [Theory] + [InlineData("cardNumber")] + [InlineData("expiryMonth")] + [InlineData("expiryYear")] + [InlineData("amount")] + [InlineData("currency")] + [InlineData("cvv")] + public async Task PostPayment_InvalidField_ValidationProblemNamingTheField(string field) { - PaymentRequest payment = TestData.ValidPaymentRequest(TestData.BankUnavailableCard); - - HttpResponseMessage response = await this.client.PostAsJsonAsync("api/payment", payment, TestContext.Current.CancellationToken); - - await AssertBadRequest(response, nameof(PaymentStatus.Failed)); + PaymentBody payment = TestData.ValidPayment(TestData.AuthorizedCard); + payment = field switch + { + "cardNumber" => payment with { CardNumber = "1234" }, + "expiryMonth" => payment with { ExpiryMonth = 13 }, + "expiryYear" => payment with { ExpiryYear = 0 }, + "amount" => payment with { Amount = 0 }, + "currency" => payment with { Currency = "JPY" }, + _ => payment with { Cvv = "12" } + }; + + HttpResponseMessage response = await this.Post(payment); + + JsonNode problem = await AssertProblem(response, HttpStatusCode.BadRequest); + JsonObject errors = problem["errors"]!.AsObject(); + Assert.Single(errors); + Assert.True(errors.ContainsKey(field)); } [Theory] - [InlineData("CardNumber")] - [InlineData("Expiry")] - [InlineData("ExpiryYearZero")] - [InlineData("Amount")] - [InlineData("Currency")] - [InlineData("Cvv")] - public async Task PostPayment_InvalidField_Rejected(string invalidField) + [InlineData("""{"expiryMonth":11,"expiryYear":2030,"currency":"GBP","amount":1050,"cvv":"123"}""")] // no card number + [InlineData("""{"cardNumber":null,"expiryMonth":11,"expiryYear":2030,"currency":"GBP","amount":1050,"cvv":"123"}""")] + [InlineData("""{"cardNumber":"2222405343248873","expiryMonth":11,"expiryYear":2030,"currency":"GBP","amount":10.5,"cvv":"123"}""")] + public async Task PostPayment_MalformedBody_ValidationProblem(string body) { - PaymentRequest payment = TestData.ValidPaymentRequest(TestData.AuthorizedCard); - switch (invalidField) - { - case "CardNumber": payment.CardNumber = "1234"; break; - case "Expiry": payment.ExpiryYear = DateTime.Now.Year - 1; break; - case "ExpiryYearZero": payment.ExpiryYear = 0; break; - case "Amount": payment.Amount = 0; break; - case "Currency": payment.Currency = "GBPX"; break; - case "Cvv": payment.Cvv = "12"; break; - } + using StringContent content = new(body, System.Text.Encoding.UTF8, "application/json"); - HttpResponseMessage response = await this.client.PostAsJsonAsync("api/payment", payment, TestContext.Current.CancellationToken); + HttpResponseMessage response = await this.client.PostAsync("api/payments", content, TestContext.Current.CancellationToken); - await AssertBadRequest(response, "Rejected"); + await AssertProblem(response, HttpStatusCode.BadRequest); } [Fact] - public async Task GetPayment_UnknownId_ReturnsNotFound() + public async Task GetPayment_UnknownId_NotFoundProblem() { - HttpResponseMessage response = await this.client.GetAsync($"api/payment/{Guid.NewGuid()}", TestContext.Current.CancellationToken); + HttpResponseMessage response = await this.client.GetAsync($"api/payments/{Guid.NewGuid()}", TestContext.Current.CancellationToken); - Assert.Equal(HttpStatusCode.NotFound, response.StatusCode); + await AssertProblem(response, HttpStatusCode.NotFound); } - private static async Task AssertBadRequest(HttpResponseMessage response, string expected) + [Fact] + public async Task GetPayment_MalformedId_NotFound() { - Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode); - Assert.Equal(expected, await response.Content.ReadAsStringAsync(TestContext.Current.CancellationToken)); + HttpResponseMessage response = await this.client.GetAsync("api/payments/not-a-guid", TestContext.Current.CancellationToken); + + Assert.Equal(HttpStatusCode.NotFound, response.StatusCode); } } diff --git a/tests/PaymentGatewayApiEndToEndTests/PaymentGatewayApiEndToEndTests.csproj b/tests/PaymentGatewayApiEndToEndTests/PaymentGatewayApiEndToEndTests.csproj index 88e9963..3a15a3f 100644 --- a/tests/PaymentGatewayApiEndToEndTests/PaymentGatewayApiEndToEndTests.csproj +++ b/tests/PaymentGatewayApiEndToEndTests/PaymentGatewayApiEndToEndTests.csproj @@ -6,13 +6,10 @@ false + - - - - - diff --git a/tests/PaymentGatewayApiEndToEndTests/TestData.cs b/tests/PaymentGatewayApiEndToEndTests/TestData.cs index a3a859d..0408ab0 100644 --- a/tests/PaymentGatewayApiEndToEndTests/TestData.cs +++ b/tests/PaymentGatewayApiEndToEndTests/TestData.cs @@ -1,8 +1,10 @@ using System; -using PaymentGatewayApi.Models; namespace PaymentGatewayApiEndToEndTests; +// the request as a merchant would send it, independent of the api's own model +public sealed record PaymentBody(string CardNumber, int ExpiryMonth, int ExpiryYear, string Currency, int Amount, string Cvv); + public static class TestData { // the bank simulator picks its response from the last card digit @@ -10,16 +12,8 @@ public static class TestData public const string DeclinedCard = "2222405343248872"; public const string BankUnavailableCard = "2222405343248870"; - public static PaymentRequest ValidPaymentRequest(string cardNumber) + public static PaymentBody ValidPayment(string cardNumber) { - return new PaymentRequest - { - CardNumber = cardNumber, - Cvv = "123", - Currency = "GBP", - ExpiryMonth = 11, - ExpiryYear = DateTime.Now.Year + 1, - Amount = 100 - }; + return new PaymentBody(cardNumber, ExpiryMonth: 11, ExpiryYear: DateTime.UtcNow.Year + 1, Currency: "GBP", Amount: 1050, Cvv: "123"); } } diff --git a/tests/PaymentGatewayApiUnitTests/Controllers/PaymentControllerTests.cs b/tests/PaymentGatewayApiUnitTests/Controllers/PaymentControllerTests.cs deleted file mode 100644 index 7c113bb..0000000 --- a/tests/PaymentGatewayApiUnitTests/Controllers/PaymentControllerTests.cs +++ /dev/null @@ -1,141 +0,0 @@ -using System; -using System.Threading; -using System.Threading.Tasks; -using Microsoft.AspNetCore.Mvc; -using Microsoft.Extensions.Logging.Abstractions; -using Moq; -using PaymentGatewayApi.Controllers; -using PaymentGatewayApi.Models; -using PaymentGatewayApi.Services; -using Xunit; - -namespace PaymentGatewayApiUnitTests.Controllers; - -public class PaymentControllerTests -{ - private readonly Mock validator = new(); - private readonly Mock paymentService = new(); - private readonly PaymentController controller; - - public PaymentControllerTests() - { - this.controller = new PaymentController( - NullLogger.Instance, this.validator.Object, this.paymentService.Object); - } - - private void SetupValidator() - { - this.validator.Setup(v => v.IsCardNumberValid(It.IsAny())).Returns(true); - this.validator.Setup(v => v.IsExpiryValid(It.IsAny())).Returns(true); - this.validator.Setup(v => v.IsAmountValid(It.IsAny())).Returns(true); - this.validator.Setup(v => v.IsCurrencyCodeValid(It.IsAny())).Returns(true); - this.validator.Setup(v => v.IsCvvValid(It.IsAny())).Returns(true); - } - - private void SetupPayment(PaymentStatus status) - { - this.paymentService - .Setup(s => s.ProcessPayment(It.IsAny(), It.IsAny())) - .ReturnsAsync(TestData.Payment(status)); - } - - [Fact] - public void GetHealthcheck_ReturnsOk() - { - ActionResult result = this.controller.GetHealthcheck(); - - OkObjectResult ok = Assert.IsType(result.Result); - Assert.Equal("ok", ok.Value); - } - - [Fact] - public async Task GetPayment_Found_ReturnsOkWithPayment() - { - this.paymentService - .Setup(s => s.FindPayment(TestData.PaymentId, It.IsAny())) - .ReturnsAsync(TestData.Payment(PaymentStatus.Authorized)); - - ActionResult result = await this.controller.GetPayment(TestData.PaymentId, TestContext.Current.CancellationToken); - - OkObjectResult ok = Assert.IsType(result.Result); - PostPaymentResponse response = Assert.IsType(ok.Value); - Assert.Equal(TestData.PaymentId.ToString(), response.PaymentId); - Assert.Equal(PaymentStatus.Authorized, response.Status); - Assert.Equal("8873", response.CardNumberLastFour); - } - - [Fact] - public async Task GetPayment_Missing_ReturnsNotFound() - { - this.paymentService - .Setup(s => s.FindPayment(It.IsAny(), It.IsAny())) - .ReturnsAsync((Payment?)null); - - ActionResult result = await this.controller.GetPayment(TestData.PaymentId, TestContext.Current.CancellationToken); - - Assert.IsType(result.Result); - } - - [Fact] - public async Task PostPayment_Authorized_ReturnsOkWithPaymentId() - { - this.SetupValidator(); - this.SetupPayment(PaymentStatus.Authorized); - - ActionResult result = await this.controller.PostPayment(TestData.ValidPaymentRequest(), TestContext.Current.CancellationToken); - - OkObjectResult ok = Assert.IsType(result.Result); - Assert.Equal(TestData.PaymentId.ToString(), ok.Value); - } - - [Theory] - [InlineData(nameof(IPaymentRequestValidator.IsCardNumberValid))] - [InlineData(nameof(IPaymentRequestValidator.IsExpiryValid))] - [InlineData(nameof(IPaymentRequestValidator.IsAmountValid))] - [InlineData(nameof(IPaymentRequestValidator.IsCurrencyCodeValid))] - [InlineData(nameof(IPaymentRequestValidator.IsCvvValid))] - public async Task PostPayment_ValidationFails_ReturnsRejectedWithoutProcessing(string failingCheck) - { - this.SetupValidator(); - switch (failingCheck) - { - case nameof(IPaymentRequestValidator.IsCardNumberValid): - this.validator.Setup(v => v.IsCardNumberValid(It.IsAny())).Returns(false); - break; - case nameof(IPaymentRequestValidator.IsExpiryValid): - this.validator.Setup(v => v.IsExpiryValid(It.IsAny())).Returns(false); - break; - case nameof(IPaymentRequestValidator.IsAmountValid): - this.validator.Setup(v => v.IsAmountValid(It.IsAny())).Returns(false); - break; - case nameof(IPaymentRequestValidator.IsCurrencyCodeValid): - this.validator.Setup(v => v.IsCurrencyCodeValid(It.IsAny())).Returns(false); - break; - case nameof(IPaymentRequestValidator.IsCvvValid): - this.validator.Setup(v => v.IsCvvValid(It.IsAny())).Returns(false); - break; - } - - ActionResult result = await this.controller.PostPayment(TestData.ValidPaymentRequest(), TestContext.Current.CancellationToken); - - BadRequestObjectResult badRequest = Assert.IsType(result.Result); - Assert.Equal("Rejected", badRequest.Value); - this.paymentService.Verify( - s => s.ProcessPayment(It.IsAny(), It.IsAny()), Times.Never); - } - - [Theory] - [InlineData(PaymentStatus.Declined)] - [InlineData(PaymentStatus.Failed)] - [InlineData(PaymentStatus.Unknown)] - public async Task PostPayment_NotAuthorized_ReturnsBadRequestWithStatus(PaymentStatus status) - { - this.SetupValidator(); - this.SetupPayment(status); - - ActionResult result = await this.controller.PostPayment(TestData.ValidPaymentRequest(), TestContext.Current.CancellationToken); - - BadRequestObjectResult badRequest = Assert.IsType(result.Result); - Assert.Equal(status.ToString(), badRequest.Value); - } -} diff --git a/tests/PaymentGatewayApiUnitTests/Controllers/PaymentsControllerTests.cs b/tests/PaymentGatewayApiUnitTests/Controllers/PaymentsControllerTests.cs new file mode 100644 index 0000000..201b8dd --- /dev/null +++ b/tests/PaymentGatewayApiUnitTests/Controllers/PaymentsControllerTests.cs @@ -0,0 +1,140 @@ +using System; +using System.Collections.Generic; +using System.Threading; +using System.Threading.Tasks; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Mvc; +using Microsoft.AspNetCore.Mvc.Infrastructure; +using Microsoft.AspNetCore.Mvc.ModelBinding; +using Microsoft.Extensions.Logging.Abstractions; +using Moq; +using PaymentGatewayApi.Controllers; +using PaymentGatewayApi.Models; +using PaymentGatewayApi.Services; +using Xunit; + +namespace PaymentGatewayApiUnitTests.Controllers; + +public class PaymentsControllerTests +{ + private readonly Mock validator = new(); + private readonly Mock paymentService = new(); + private readonly PaymentsController controller; + + public PaymentsControllerTests() + { + this.validator.Setup(v => v.Validate(It.IsAny())).Returns(new Dictionary()); + + this.controller = new PaymentsController( + NullLogger.Instance, this.validator.Object, this.paymentService.Object) + { + ControllerContext = new ControllerContext { HttpContext = new DefaultHttpContext() }, + ProblemDetailsFactory = new SimpleProblemDetailsFactory() + }; + } + + private void SetupPayment(PaymentStatus status) + { + this.paymentService + .Setup(s => s.ProcessPayment(It.IsAny(), It.IsAny())) + .ReturnsAsync(TestData.Payment(status)); + } + + private Task> Post() + { + return this.controller.PostPayment(TestData.ValidPaymentRequest(), TestContext.Current.CancellationToken); + } + + [Fact] + public async Task GetPayment_Found_ReturnsPayment() + { + this.paymentService + .Setup(s => s.FindPayment(TestData.PaymentId, It.IsAny())) + .ReturnsAsync(TestData.Payment(PaymentStatus.Authorized)); + + ActionResult result = await this.controller.GetPayment(TestData.PaymentId, TestContext.Current.CancellationToken); + + PaymentResponse response = Assert.IsType(result.Value); + Assert.Equal(TestData.PaymentId, response.Id); + Assert.Equal(PaymentStatus.Authorized, response.Status); + Assert.Equal("8873", response.CardNumberLastFour); + } + + [Fact] + public async Task GetPayment_Missing_ReturnsNotFound() + { + this.paymentService + .Setup(s => s.FindPayment(It.IsAny(), It.IsAny())) + .ReturnsAsync((Payment?)null); + + ActionResult result = await this.controller.GetPayment(TestData.PaymentId, TestContext.Current.CancellationToken); + + Assert.IsType(result.Result); + } + + [Theory] + [InlineData(PaymentStatus.Authorized)] + [InlineData(PaymentStatus.Declined)] // the issuer said no, the request itself was fine + public async Task PostPayment_BankAnswered_ReturnsCreatedPaymentWithLocation(PaymentStatus status) + { + this.SetupPayment(status); + + ActionResult result = await this.Post(); + + CreatedAtActionResult created = Assert.IsType(result.Result); + Assert.Equal(nameof(PaymentsController.GetPayment), created.ActionName); + Assert.Equal(TestData.PaymentId, created.RouteValues?["paymentId"]); + PaymentResponse response = Assert.IsType(created.Value); + Assert.Equal(TestData.PaymentId, response.Id); + Assert.Equal(status, response.Status); + } + + [Fact] + public async Task PostPayment_Invalid_ReturnsValidationProblemWithoutProcessing() + { + this.validator + .Setup(v => v.Validate(It.IsAny())) + .Returns(new Dictionary { ["cvv"] = ["Must be 3 or 4 digits."] }); + + ActionResult result = await this.Post(); + + BadRequestObjectResult badRequest = Assert.IsType(result.Result); + ValidationProblemDetails problem = Assert.IsType(badRequest.Value); + Assert.Equal(["Must be 3 or 4 digits."], problem.Errors["cvv"]); + this.paymentService.Verify( + s => s.ProcessPayment(It.IsAny(), It.IsAny()), Times.Never); + } + + [Theory] + [InlineData(PaymentStatus.Failed)] + [InlineData(PaymentStatus.Unknown)] + public async Task PostPayment_BankCouldNotComplete_ReturnsBadGatewayWithPaymentId(PaymentStatus status) + { + this.SetupPayment(status); + + ActionResult result = await this.Post(); + + ObjectResult objectResult = Assert.IsType(result.Result); + Assert.Equal(StatusCodes.Status502BadGateway, objectResult.StatusCode); + ProblemDetails problem = Assert.IsType(objectResult.Value); + Assert.Equal(TestData.PaymentId, problem.Extensions["paymentId"]); + Assert.Equal(status.ToString(), problem.Extensions["paymentStatus"]); + } + + // stands in for MVC's factory, which needs the full service provider + private sealed class SimpleProblemDetailsFactory : ProblemDetailsFactory + { + public override ProblemDetails CreateProblemDetails( + HttpContext httpContext, int? statusCode = null, string? title = null, string? type = null, string? detail = null, string? instance = null) + { + return new ProblemDetails { Status = statusCode, Title = title, Type = type, Detail = detail, Instance = instance }; + } + + public override ValidationProblemDetails CreateValidationProblemDetails( + HttpContext httpContext, ModelStateDictionary modelStateDictionary, int? statusCode = null, string? title = null, + string? type = null, string? detail = null, string? instance = null) + { + return new ValidationProblemDetails(modelStateDictionary) { Status = statusCode ?? StatusCodes.Status400BadRequest }; + } + } +} diff --git a/tests/PaymentGatewayApiUnitTests/Middleware/ErrorMiddlewareTests.cs b/tests/PaymentGatewayApiUnitTests/Middleware/ErrorMiddlewareTests.cs deleted file mode 100644 index 1e48c70..0000000 --- a/tests/PaymentGatewayApiUnitTests/Middleware/ErrorMiddlewareTests.cs +++ /dev/null @@ -1,53 +0,0 @@ -using System; -using System.IO; -using System.Threading.Tasks; -using Microsoft.AspNetCore.Http; -using Microsoft.Extensions.Logging.Abstractions; -using PaymentGatewayApi.Middleware; -using Xunit; - -namespace PaymentGatewayApiUnitTests.Middleware; - -public class ErrorMiddlewareTests -{ - private static DefaultHttpContext NewContext() - { - DefaultHttpContext context = new(); - context.Response.Body = new MemoryStream(); - return context; - } - - private static string ReadBody(HttpContext context) - { - context.Response.Body.Position = 0; - using StreamReader reader = new(context.Response.Body); - return reader.ReadToEnd(); - } - - [Fact] - public async Task InvokeAsync_NextSucceeds_LeavesResponseUntouched() - { - DefaultHttpContext context = NewContext(); - ErrorMiddleware middleware = new(_ => Task.CompletedTask, NullLogger.Instance); - - await middleware.InvokeAsync(context); - - Assert.Equal(StatusCodes.Status200OK, context.Response.StatusCode); - Assert.Equal(string.Empty, ReadBody(context)); - } - - [Fact] - public async Task InvokeAsync_NextThrows_Returns500WithMessage() - { - DefaultHttpContext context = NewContext(); - ErrorMiddleware middleware = new( - _ => throw new InvalidOperationException("boom"), NullLogger.Instance); - - await middleware.InvokeAsync(context); - - Assert.Equal(StatusCodes.Status500InternalServerError, context.Response.StatusCode); - string body = ReadBody(context); - Assert.Contains("Unhandled exception", body); - Assert.Contains("boom", body); - } -} diff --git a/tests/PaymentGatewayApiUnitTests/Middleware/GlobalExceptionHandlerTests.cs b/tests/PaymentGatewayApiUnitTests/Middleware/GlobalExceptionHandlerTests.cs new file mode 100644 index 0000000..7645e76 --- /dev/null +++ b/tests/PaymentGatewayApiUnitTests/Middleware/GlobalExceptionHandlerTests.cs @@ -0,0 +1,70 @@ +using System; +using System.IO; +using System.Text.Json; +using System.Threading; +using System.Threading.Tasks; +using Microsoft.AspNetCore.Http; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Logging.Abstractions; +using PaymentGatewayApi.Middleware; +using Xunit; + +namespace PaymentGatewayApiUnitTests.Middleware; + +public class GlobalExceptionHandlerTests +{ + private readonly ServiceProvider services = new ServiceCollection().AddLogging().AddProblemDetails().BuildServiceProvider(); + + private GlobalExceptionHandler NewHandler() + { + return new GlobalExceptionHandler( + NullLogger.Instance, this.services.GetRequiredService()); + } + + private DefaultHttpContext NewContext() + { + return new DefaultHttpContext { RequestServices = this.services, Response = { Body = new MemoryStream() } }; + } + + private static string ReadBody(HttpContext context) + { + context.Response.Body.Position = 0; + using StreamReader reader = new(context.Response.Body); + return reader.ReadToEnd(); + } + + [Fact] + public async Task TryHandleAsync_Exception_Returns500ProblemWithoutTheMessage() + { + DefaultHttpContext context = this.NewContext(); + + bool handled = await this.NewHandler().TryHandleAsync( + context, new InvalidOperationException("table Payments on dynamodb.internal:8000"), TestContext.Current.CancellationToken); + + Assert.True(handled); + Assert.Equal(StatusCodes.Status500InternalServerError, context.Response.StatusCode); + Assert.StartsWith("application/problem+json", context.Response.ContentType); + + string body = ReadBody(context); + Assert.DoesNotContain("dynamodb", body, StringComparison.OrdinalIgnoreCase); + using JsonDocument problem = JsonDocument.Parse(body); + Assert.Equal(500, problem.RootElement.GetProperty("status").GetInt32()); + Assert.True(problem.RootElement.TryGetProperty("traceId", out _)); + } + + [Fact] + public async Task TryHandleAsync_ClientAborted_AnswersWithoutABody() + { + using CancellationTokenSource aborted = new(); + await aborted.CancelAsync(); + DefaultHttpContext context = this.NewContext(); + context.RequestAborted = aborted.Token; + + bool handled = await this.NewHandler().TryHandleAsync( + context, new OperationCanceledException(aborted.Token), TestContext.Current.CancellationToken); + + Assert.True(handled); + Assert.Equal(GlobalExceptionHandler.ClientClosedRequest, context.Response.StatusCode); + Assert.Equal(string.Empty, ReadBody(context)); + } +} diff --git a/tests/PaymentGatewayApiUnitTests/Models/PaymentRequestTests.cs b/tests/PaymentGatewayApiUnitTests/Models/PaymentRequestTests.cs new file mode 100644 index 0000000..0f22624 --- /dev/null +++ b/tests/PaymentGatewayApiUnitTests/Models/PaymentRequestTests.cs @@ -0,0 +1,19 @@ +using PaymentGatewayApi.Models; +using Xunit; + +namespace PaymentGatewayApiUnitTests.Models; + +public class PaymentRequestTests +{ + [Fact] + public void ToString_LeavesOutCardNumberAndCvv() + { + PaymentRequest request = TestData.ValidPaymentRequest() with { Cvv = "987" }; + + string printed = request.ToString(); + + Assert.DoesNotContain("2222405343248873", printed); + Assert.DoesNotContain("987", printed); + Assert.Contains("GBP", printed); + } +} diff --git a/tests/PaymentGatewayApiUnitTests/Services/BankClientTests.cs b/tests/PaymentGatewayApiUnitTests/Services/BankClientTests.cs index 388e21d..5eb0fb1 100644 --- a/tests/PaymentGatewayApiUnitTests/Services/BankClientTests.cs +++ b/tests/PaymentGatewayApiUnitTests/Services/BankClientTests.cs @@ -38,9 +38,7 @@ private static Task Authorize(BankClient client) [InlineData(11, 2027, "11/2027")] public void ToBankRequest_FormatsExpiryAsTwoDigitMonthAndFourDigitYear(int month, int year, string expected) { - PaymentRequest payment = TestData.ValidPaymentRequest(); - payment.ExpiryMonth = month; - payment.ExpiryYear = year; + PaymentRequest payment = TestData.ValidPaymentRequest() with { ExpiryMonth = month, ExpiryYear = year }; BankRequest request = BankClient.ToBankRequest(payment); @@ -62,10 +60,22 @@ public async Task Authorize_PostsTheBankContractToPayments() Assert.Equal("2222405343248873", body.RootElement.GetProperty("card_number").GetString()); Assert.Equal("11/2027", body.RootElement.GetProperty("expiry_date").GetString()); Assert.Equal("GBP", body.RootElement.GetProperty("currency").GetString()); - Assert.Equal(100, body.RootElement.GetProperty("amount").GetDecimal()); + Assert.Equal(1050, body.RootElement.GetProperty("amount").GetInt32()); Assert.Equal("123", body.RootElement.GetProperty("cvv").GetString()); } + [Fact] + public async Task Authorize_LowerCaseCurrency_IsSentUpperCase() + { + (BankClient client, StubHttpMessageHandler handler) = NewClient(_ => Json(HttpStatusCode.OK, """{"authorized":false}""")); + + await client.Authorize( + TestData.PaymentId, TestData.ValidPaymentRequest() with { Currency = "gbp" }, TestContext.Current.CancellationToken); + + using JsonDocument body = JsonDocument.Parse(handler.Bodies[0]); + Assert.Equal("GBP", body.RootElement.GetProperty("currency").GetString()); + } + [Fact] public async Task Authorize_BankAuthorizes_ReturnsAuthorizedWithCode() { diff --git a/tests/PaymentGatewayApiUnitTests/Services/PaymentRequestValidatorTests.cs b/tests/PaymentGatewayApiUnitTests/Services/PaymentRequestValidatorTests.cs index a47cf65..78141a7 100644 --- a/tests/PaymentGatewayApiUnitTests/Services/PaymentRequestValidatorTests.cs +++ b/tests/PaymentGatewayApiUnitTests/Services/PaymentRequestValidatorTests.cs @@ -1,6 +1,8 @@ -using System; +using System.Collections.Generic; using Microsoft.Extensions.Logging.Abstractions; +using Microsoft.Extensions.Options; using Microsoft.Extensions.Time.Testing; +using PaymentGatewayApi.Configuration; using PaymentGatewayApi.Models; using PaymentGatewayApi.Services; using Xunit; @@ -9,19 +11,57 @@ namespace PaymentGatewayApiUnitTests.Services; public class PaymentRequestValidatorTests { + // the clock is pinned to 15 September 2026 private readonly PaymentRequestValidator validator = new( - NullLogger.Instance, new FakeTimeProvider(new DateTimeOffset(2026, 9, 15, 12, 0, 0, TimeSpan.Zero))); + NullLogger.Instance, + new FakeTimeProvider(TestData.Now), + Options.Create(new PaymentOptions { SupportedCurrencies = ["GBP", "EUR", "USD"] })); + + private void AssertValid(PaymentRequest request) + { + Assert.Empty(this.validator.Validate(request)); + } + + private void AssertOnlyInvalid(PaymentRequest request, string field) + { + IReadOnlyDictionary errors = this.validator.Validate(request); + + KeyValuePair error = Assert.Single(errors); + Assert.Equal(field, error.Key); + Assert.NotEmpty(error.Value); + } + + [Fact] + public void Validate_ValidRequest_ReturnsNoErrors() + { + this.AssertValid(TestData.ValidPaymentRequest()); + } + + [Fact] + public void Validate_EveryFieldInvalid_ReturnsEveryError() + { + PaymentRequest request = new() + { + CardNumber = "1234", + ExpiryMonth = 13, + ExpiryYear = 2027, + Currency = "XXX", + Amount = 0, + Cvv = "1" + }; + + IReadOnlyDictionary errors = this.validator.Validate(request); + + Assert.Equal(["amount", "cardNumber", "currency", "cvv", "expiryMonth"], [.. new SortedSet(errors.Keys)]); + } [Theory] [InlineData("22224053432488")] // 14 digits [InlineData("2222405343248873")] // 16 digits [InlineData("2222405343248873123")] // 19 digits - public void IsCardNumberValid_ValidCardNumber_ReturnsTrue(string cardNumber) + public void Validate_CardNumberOf14To19Digits_IsValid(string cardNumber) { - PaymentRequest payment = TestData.ValidPaymentRequest(); - payment.CardNumber = cardNumber; - - Assert.True(this.validator.IsCardNumberValid(payment)); + this.AssertValid(TestData.ValidPaymentRequest() with { CardNumber = cardNumber }); } [Theory] @@ -29,13 +69,11 @@ public void IsCardNumberValid_ValidCardNumber_ReturnsTrue(string cardNumber) [InlineData("22224053432488731234")] // 20 digits [InlineData("22224053432488ab")] [InlineData("2222 4053 4324 8873")] + [InlineData("٢٢٢٢٤٠٥٣٤٣٢٤٨٨٧٣")] // Arabic-Indic digits match \d, not [0-9] [InlineData("")] - public void IsCardNumberValid_InvalidCardNumber_ReturnsFalse(string cardNumber) + public void Validate_CardNumberNotDigits_IsInvalid(string cardNumber) { - PaymentRequest payment = TestData.ValidPaymentRequest(); - payment.CardNumber = cardNumber; - - Assert.False(this.validator.IsCardNumberValid(payment)); + this.AssertOnlyInvalid(TestData.ValidPaymentRequest() with { CardNumber = cardNumber }, "cardNumber"); } [Theory] @@ -43,88 +81,73 @@ public void IsCardNumberValid_InvalidCardNumber_ReturnsFalse(string cardNumber) [InlineData(10, 2026)] [InlineData(1, 2027)] [InlineData(12, 2046)] // 20 years ahead - public void IsExpiryValid_CurrentOrFutureExpiry_ReturnsTrue(int month, int year) + public void Validate_CurrentOrFutureExpiry_IsValid(int month, int year) { - PaymentRequest payment = TestData.ValidPaymentRequest(); - payment.ExpiryMonth = month; - payment.ExpiryYear = year; + this.AssertValid(TestData.ValidPaymentRequest() with { ExpiryMonth = month, ExpiryYear = year }); + } - Assert.True(this.validator.IsExpiryValid(payment)); + [Theory] + [InlineData(0)] + [InlineData(13)] + public void Validate_ExpiryMonthOutOfRange_IsInvalid(int month) + { + this.AssertOnlyInvalid(TestData.ValidPaymentRequest() with { ExpiryMonth = month }, "expiryMonth"); } [Theory] - [InlineData(8, 2026)] // previous month + [InlineData(8, 2026)] // previous month [InlineData(12, 2025)] - [InlineData(0, 2027)] - [InlineData(13, 2027)] - [InlineData(1, 2047)] // more than 20 years ahead - [InlineData(1, 0)] // out of DateTime's range + [InlineData(1, 2047)] // more than 20 years ahead + [InlineData(1, 0)] // out of DateTime's range [InlineData(1, -1)] [InlineData(1, int.MaxValue)] - public void IsExpiryValid_PastOrOutOfRangeExpiryOrInvalidMonth_ReturnsFalse(int month, int year) + public void Validate_ExpiredOrTooFarAhead_IsInvalid(int month, int year) { - PaymentRequest payment = TestData.ValidPaymentRequest(); - payment.ExpiryMonth = month; - payment.ExpiryYear = year; - - Assert.False(this.validator.IsExpiryValid(payment)); + this.AssertOnlyInvalid(TestData.ValidPaymentRequest() with { ExpiryMonth = month, ExpiryYear = year }, "expiryYear"); } [Theory] - [InlineData(1)] - [InlineData(100)] - public void IsAmountValid_PositiveAmount_ReturnsTrue(decimal amount) + [InlineData("GBP")] + [InlineData("EUR")] + [InlineData("USD")] + [InlineData("gbp")] + public void Validate_SupportedCurrency_IsValid(string currency) { - PaymentRequest payment = TestData.ValidPaymentRequest(); - payment.Amount = amount; - - Assert.True(this.validator.IsAmountValid(payment)); + this.AssertValid(TestData.ValidPaymentRequest() with { Currency = currency }); } [Theory] - [InlineData(0)] - [InlineData(-5)] - [InlineData(0.99)] - public void IsAmountValid_AmountBelowOne_ReturnsFalse(decimal amount) + [InlineData("JPY")] // a real code, but not a supported one + [InlineData("123")] + [InlineData("GBPX")] + [InlineData("")] + public void Validate_UnsupportedCurrency_IsInvalid(string currency) { - PaymentRequest payment = TestData.ValidPaymentRequest(); - payment.Amount = amount; - - Assert.False(this.validator.IsAmountValid(payment)); + this.AssertOnlyInvalid(TestData.ValidPaymentRequest() with { Currency = currency }, "currency"); } [Theory] - [InlineData("GBP")] - [InlineData("123")] // NOTE: only the length is checked, not that the code is a real currency - public void IsCurrencyCodeValid_ThreeCharacters_ReturnsTrue(string currency) + [InlineData(1)] // one penny + [InlineData(1050)] + public void Validate_PositiveMinorUnitAmount_IsValid(int amount) { - PaymentRequest payment = TestData.ValidPaymentRequest(); - payment.Currency = currency; - - Assert.True(this.validator.IsCurrencyCodeValid(payment)); + this.AssertValid(TestData.ValidPaymentRequest() with { Amount = amount }); } [Theory] - [InlineData("")] - [InlineData("GB")] - [InlineData("GBPX")] - public void IsCurrencyCodeValid_NotThreeCharacters_ReturnsFalse(string currency) + [InlineData(0)] + [InlineData(-5)] + public void Validate_ZeroOrNegativeAmount_IsInvalid(int amount) { - PaymentRequest payment = TestData.ValidPaymentRequest(); - payment.Currency = currency; - - Assert.False(this.validator.IsCurrencyCodeValid(payment)); + this.AssertOnlyInvalid(TestData.ValidPaymentRequest() with { Amount = amount }, "amount"); } [Theory] [InlineData("123")] [InlineData("1234")] - public void IsCvvValid_ThreeOrFourDigits_ReturnsTrue(string cvv) + public void Validate_CvvOfThreeOrFourDigits_IsValid(string cvv) { - PaymentRequest payment = TestData.ValidPaymentRequest(); - payment.Cvv = cvv; - - Assert.True(this.validator.IsCvvValid(payment)); + this.AssertValid(TestData.ValidPaymentRequest() with { Cvv = cvv }); } [Theory] @@ -132,11 +155,8 @@ public void IsCvvValid_ThreeOrFourDigits_ReturnsTrue(string cvv) [InlineData("12345")] [InlineData("12a")] [InlineData("")] - public void IsCvvValid_InvalidCvv_ReturnsFalse(string cvv) + public void Validate_CvvNotThreeOrFourDigits_IsInvalid(string cvv) { - PaymentRequest payment = TestData.ValidPaymentRequest(); - payment.Cvv = cvv; - - Assert.False(this.validator.IsCvvValid(payment)); + this.AssertOnlyInvalid(TestData.ValidPaymentRequest() with { Cvv = cvv }, "cvv"); } } diff --git a/tests/PaymentGatewayApiUnitTests/TestData.cs b/tests/PaymentGatewayApiUnitTests/TestData.cs index a61f345..68b673b 100644 --- a/tests/PaymentGatewayApiUnitTests/TestData.cs +++ b/tests/PaymentGatewayApiUnitTests/TestData.cs @@ -17,7 +17,7 @@ public static PaymentRequest ValidPaymentRequest() Currency = "GBP", ExpiryMonth = 11, ExpiryYear = 2027, - Amount = 100 + Amount = 1050 }; } @@ -31,7 +31,7 @@ public static Payment Payment(PaymentStatus status) ExpiryMonth = 11, ExpiryYear = 2027, Currency = "GBP", - Amount = 100, + Amount = 1050, CreatedAt = Now, UpdatedAt = Now }; diff --git a/tests/bruno-collection.yml b/tests/bruno-collection.yml index 993bc4a..65249f2 100644 --- a/tests/bruno-collection.yml +++ b/tests/bruno-collection.yml @@ -19,7 +19,7 @@ items: seq: 2 http: method: GET - url: http://localhost:5067/api/payment/abcbc011-c8d9-484b-a67a-28fbda7461f3 + url: http://localhost:5067/api/payments/abcbc011-c8d9-484b-a67a-28fbda7461f3 headers: - name: CallerId value: bruno-client @@ -31,14 +31,14 @@ items: maxRedirects: 5 forwardAuthorizationHeader: true - info: - name: get payment health + name: get health live type: http seq: 1 http: method: GET - url: http://localhost:5067/api/payment + url: http://localhost:5067/health/live headers: - - name: Callerid + - name: CallerId value: bruno-client auth: inherit settings: @@ -48,12 +48,12 @@ items: maxRedirects: 5 forwardAuthorizationHeader: true - info: - name: post payment accepted + name: post payment authorized type: http seq: 3 http: method: POST - url: http://localhost:5067/api/payment + url: http://localhost:5067/api/payments headers: - name: Content-Type value: application/json @@ -68,11 +68,11 @@ items: type: json data: |- { - "cardnumber": "2222405343248873", - "ExpiryMonth": "11", - "ExpiryYear": "2027", + "cardNumber": "2222405343248873", + "expiryMonth": 4, + "expiryYear": 2030, "currency": "GBP", - "amount": 100, + "amount": 1050, "cvv": "123" } auth: inherit @@ -88,7 +88,7 @@ items: seq: 4 http: method: POST - url: http://localhost:5067/api/payment + url: http://localhost:5067/api/payments headers: - name: Content-Type value: application/json @@ -103,11 +103,11 @@ items: type: json data: |- { - "cardnumber": "2222405343248872", - "ExpiryMonth": "11", - "ExpiryYear": "2027", + "cardNumber": "2222405343248872", + "expiryMonth": 4, + "expiryYear": 2030, "currency": "GBP", - "amount": 100, + "amount": 1050, "cvv": "123" } auth: inherit @@ -118,12 +118,12 @@ items: maxRedirects: 5 forwardAuthorizationHeader: true - info: - name: post payment error 500 + name: post payment bank unavailable 502 type: http seq: 5 http: method: POST - url: http://localhost:5067/api/payment + url: http://localhost:5067/api/payments headers: - name: Content-Type value: application/json @@ -138,11 +138,11 @@ items: type: json data: |- { - "cardnumber": "2222405343248870", - "ExpiryMonth": "11", - "ExpiryYear": "2027", + "cardNumber": "2222405343248870", + "expiryMonth": 4, + "expiryYear": 2030, "currency": "GBP", - "amount": 100, + "amount": 1050, "cvv": "123" } auth: inherit @@ -169,9 +169,9 @@ items: data: |- { "card_number": "2222405343248873", - "expiry_date": "04/2025", + "expiry_date": "04/2030", "currency": "GBP", - "amount": 100, + "amount": 1050, "cvv": "123" } auth: inherit From b2c321260fb20880c6b14a4a3a7cbcabfd9784ed Mon Sep 17 00:00:00 2001 From: ati Date: Wed, 23 Sep 2026 23:56:30 +0100 Subject: [PATCH 07/13] correlate every log line, configure Serilog from appsettings, log as JSON LogMiddleware becomes CorrelationIdMiddleware. It reads X-Correlation-ID (the standard name, was CorrelationId), accepts it only if it matches ^[A-Za-z0-9-]{1,64}$ so a header cannot inject or bloat log lines, echoes it on the response and adds it to every ProblemDetails. The free-text CallerId header is no longer logged; caller identity belongs to authentication. Serilog moves from a second ConfigurationBuilder over logconfig.json into the Serilog section of appsettings, so it gets environment overrides and environment variables. The console writes compact JSON with the correlation, trace and span ids by default (the old terse console dropped CorrelationId); Development swaps in a readable format and keeps the z-logs files. A startup logger and try/catch/finally with CloseAndFlush make startup failures visible and flush the file sink; UseSerilogRequestLogging adds one summary line per request. Co-Authored-By: Claude Opus 5.5 --- AGENTS.md | 27 ++-- src/Middleware/CorrelationIdMiddleware.cs | 54 +++++++ src/Middleware/LogMiddleware.cs | 38 ----- src/Program.cs | 143 +++++++++++------- src/Services/PaymentRequestValidator.cs | 5 +- src/appsettings.Development.json | 30 ++++ src/appsettings.json | 20 +++ src/logconfig.json | 44 ------ .../ApiFixture.cs | 1 - .../CorrelationIdMiddlewareTests.cs | 121 +++++++++++++++ .../Middleware/LogMiddlewareTests.cs | 101 ------------- tests/bruno-collection.yml | 22 +-- 12 files changed, 334 insertions(+), 272 deletions(-) create mode 100644 src/Middleware/CorrelationIdMiddleware.cs delete mode 100644 src/Middleware/LogMiddleware.cs delete mode 100644 src/logconfig.json create mode 100644 tests/PaymentGatewayApiUnitTests/Middleware/CorrelationIdMiddlewareTests.cs delete mode 100644 tests/PaymentGatewayApiUnitTests/Middleware/LogMiddlewareTests.cs diff --git a/AGENTS.md b/AGENTS.md index 083b303..def5164 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -21,8 +21,8 @@ dotnet run --project src/PaymentGatewayApi.csproj # http://localhost:5067, Sw docker build -f src/Dockerfile -t payment-gateway-api . # container image, from repo root ``` -`dotnet run` sets the working directory to `src/`, which is what makes `logconfig.json` and -the `z-logs/` output path resolve. +`dotnet run` sets the working directory to `src/`, which is what makes the Development +`z-logs/` output path resolve. ### Tests @@ -125,7 +125,7 @@ singletons, as is the `IAmazonDynamoDB` client registered through resilience handler has retries disabled, and `PaymentService` is scoped so it never captures it. New services follow the existing pattern — interface and implementation in the same file under `Services/` — and get registered there. -Middleware order is `LogMiddleware`, then `UseExceptionHandler` (with `GlobalExceptionHandler` +Middleware order is `CorrelationIdMiddleware`, `UseSerilogRequestLogging`, then `UseExceptionHandler` (with `GlobalExceptionHandler` and `AddProblemDetails`), then `UseStatusCodePages` so bodiless errors get a `ProblemDetails` too. ## Bank simulator behaviour @@ -157,12 +157,15 @@ shadows the specific stubs; WireMock breaks equal-priority ties by load order. - **Logging goes through `[LoggerMessage]`** partial methods at the bottom of each class (`CA1848` rejects `logger.LogX(...)` calls). Templates follow one shape: `"MethodName, PaymentId={PaymentId}, Status={Status}"`, with `Status` values like - `"Started"` / `"Completed"` / `"Failed"`. `CallerId` and `CorrelationId` are enriched - automatically by `LogMiddleware` from the request headers (a missing `CorrelationId` - header gets a generated GUID), so never pass them by hand. -- **Serilog is configured from `src/logconfig.json`, not `appsettings.json`.** - Settings bind to validated options classes in `Configuration/` (`Bank`, `Payments`, `Dynamo`), - checked at startup. Local-only values (the simulator URL, `AWS:ServiceURL` pointed at - DynamoDB Local) live in `appsettings.Development.json`; the compose file sets them for - the container through environment variables. Verbose file logs go to `src/z-logs/` - (gitignored); the console sink is deliberately terse. + `"Started"` / `"Completed"` / `"Failed"`. `CorrelationIdMiddleware` pushes `CorrelationId` + (the caller's `X-Correlation-ID` if it matches `^[A-Za-z0-9-]{1,64}$`, a new GUID otherwise, + echoed on the response and in every `ProblemDetails`), and Serilog adds `TraceId`/`SpanId`, + so never pass them by hand. Never log a `PaymentRequest`'s card number or CVV. +- **Serilog is configured from the `Serilog` section of `appsettings*.json`.** The base file + writes compact JSON to the console (what containers collect); `appsettings.Development.json` + swaps in a readable console format and adds files under `src/z-logs/` (gitignored). + `UseSerilogRequestLogging` writes one summary line per request. +- **Settings bind to validated options classes** in `Configuration/` (`Bank`, `Payments`, + `Dynamo`), checked at startup. Local-only values (the simulator URL, `AWS:ServiceURL` + pointed at DynamoDB Local) live in `appsettings.Development.json`; the compose file sets + them for the container through environment variables. diff --git a/src/Middleware/CorrelationIdMiddleware.cs b/src/Middleware/CorrelationIdMiddleware.cs new file mode 100644 index 0000000..08c8df5 --- /dev/null +++ b/src/Middleware/CorrelationIdMiddleware.cs @@ -0,0 +1,54 @@ +using System; +using System.Text.RegularExpressions; +using System.Threading.Tasks; +using Microsoft.AspNetCore.Http; +using Serilog.Context; + +namespace PaymentGatewayApi.Middleware; + +// gives every request a correlation id: the caller's X-Correlation-ID when it is well formed, a new +// GUID otherwise. It is pushed onto every log line of the request and echoed on the response, so a +// merchant can quote it to support. The W3C trace id (traceparent) flows alongside it on its own. +public partial class CorrelationIdMiddleware +{ + public const string HeaderName = "X-Correlation-ID"; + + private static readonly object ItemKey = new(); + + private readonly RequestDelegate next; + + public CorrelationIdMiddleware(RequestDelegate next) + { + this.next = next; + } + + public static string? Get(HttpContext context) + { + return context.Items.TryGetValue(ItemKey, out object? value) ? value as string : null; + } + + public async Task InvokeAsync(HttpContext context) + { + // an unauthenticated header lands in every log line: anything that is not a short token + // (newlines, markup, kilobytes of padding) is replaced rather than trusted + string header = context.Request.Headers[HeaderName].ToString(); + string correlationId = ValidCorrelationId().IsMatch(header) ? header : Guid.NewGuid().ToString(); + + context.Items[ItemKey] = correlationId; + + // set when the response starts, so it survives the exception handler clearing the headers + context.Response.OnStarting(() => + { + context.Response.Headers[HeaderName] = correlationId; + return Task.CompletedTask; + }); + + using (LogContext.PushProperty("CorrelationId", correlationId)) + { + await this.next(context); + } + } + + [GeneratedRegex("^[A-Za-z0-9-]{1,64}$")] + private static partial Regex ValidCorrelationId(); +} diff --git a/src/Middleware/LogMiddleware.cs b/src/Middleware/LogMiddleware.cs deleted file mode 100644 index ed0ef86..0000000 --- a/src/Middleware/LogMiddleware.cs +++ /dev/null @@ -1,38 +0,0 @@ -using System; -using System.Linq; -using System.Threading.Tasks; -using Microsoft.AspNetCore.Http; -using Serilog.Context; - -namespace PaymentGatewayApi.Middleware; - -public class LogMiddleware -{ - private readonly RequestDelegate next; - - public LogMiddleware(RequestDelegate next) - { - this.next = next; - } - - public async Task InvokeAsync(HttpContext context) - { - string? correlationId; - string? correlationHeader = context.Request.Headers["CorrelationId"].FirstOrDefault(); - if (!string.IsNullOrWhiteSpace(correlationHeader)) - { - correlationId = correlationHeader; - } - else - { - // or return bad request if missing correlationId - correlationId = Guid.NewGuid().ToString(); - } - - using (LogContext.PushProperty("CallerId", context.Request.Headers["CallerId"])) - using (LogContext.PushProperty("CorrelationId", correlationId)) - { - await this.next(context); - } - } -} diff --git a/src/Program.cs b/src/Program.cs index 5ef90d5..79ba38a 100644 --- a/src/Program.cs +++ b/src/Program.cs @@ -4,82 +4,109 @@ using Microsoft.AspNetCore.Builder; using Microsoft.Extensions.Configuration; using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Hosting; using Microsoft.Extensions.Http.Resilience; using Microsoft.Extensions.Options; using PaymentGatewayApi.Configuration; using PaymentGatewayApi.Middleware; using PaymentGatewayApi.Services; using Serilog; +using Serilog.Formatting.Compact; -var builder = WebApplication.CreateBuilder(args); +// catches what goes wrong before the configured logger exists, e.g. an invalid setting +Log.Logger = new LoggerConfiguration().WriteTo.Console(new RenderedCompactJsonFormatter()).CreateLogger(); -var config = new ConfigurationBuilder().AddJsonFile("logconfig.json").Build(); -var log = new LoggerConfiguration().ReadFrom.Configuration(config).CreateLogger(); -builder.Host.UseSerilog(log); +try +{ + var builder = WebApplication.CreateBuilder(args); -builder.Services.AddControllers() - .AddJsonOptions(options => + // sinks, levels and formats come from the "Serilog" section: JSON to the console by default, + // readable console plus files under z-logs/ in Development; ReadFrom.Services lets tests add a sink + builder.Services.AddSerilog((services, logger) => logger + .ReadFrom.Configuration(builder.Configuration) + .ReadFrom.Services(services)); + + builder.Services.AddControllers() + .AddJsonOptions(options => + { + // statuses read as names, and a null where the model says non-nullable is a 400, not a crash later + options.JsonSerializerOptions.Converters.Add(new JsonStringEnumConverter()); + options.JsonSerializerOptions.RespectNullableAnnotations = true; + }); + + // every error body is a ProblemDetails with the request's traceId and correlationId, never an exception message + builder.Services.AddProblemDetails(options => options.CustomizeProblemDetails = context => { - // statuses read as names, and a null where the model says non-nullable is a 400, not a crash later - options.JsonSerializerOptions.Converters.Add(new JsonStringEnumConverter()); - options.JsonSerializerOptions.RespectNullableAnnotations = true; + if (CorrelationIdMiddleware.Get(context.HttpContext) is { } correlationId) + { + context.ProblemDetails.Extensions["correlationId"] = correlationId; + } }); + builder.Services.AddExceptionHandler(); + builder.Services.AddHealthChecks(); -// every error body is a ProblemDetails with the request's traceId, never an exception message -builder.Services.AddProblemDetails(); -builder.Services.AddExceptionHandler(); -builder.Services.AddHealthChecks(); - -// a missing or malformed setting fails the startup, not the first request that needs it -builder.Services.AddOptions() - .BindConfiguration(BankOptions.SectionName) - .ValidateDataAnnotations() - .ValidateOnStart(); -builder.Services.AddOptions() - .BindConfiguration(PaymentOptions.SectionName) - .ValidateDataAnnotations() - .ValidateOnStart(); -builder.Services.AddOptions() - .BindConfiguration(DynamoOptions.SectionName) - .ValidateDataAnnotations() - .ValidateOnStart(); + // a missing or malformed setting fails the startup, not the first request that needs it + builder.Services.AddOptions() + .BindConfiguration(BankOptions.SectionName) + .ValidateDataAnnotations() + .ValidateOnStart(); + builder.Services.AddOptions() + .BindConfiguration(PaymentOptions.SectionName) + .ValidateDataAnnotations() + .ValidateOnStart(); + builder.Services.AddOptions() + .BindConfiguration(DynamoOptions.SectionName) + .ValidateDataAnnotations() + .ValidateOnStart(); -builder.Services.AddSingleton(TimeProvider.System); + builder.Services.AddSingleton(TimeProvider.System); -// a payment POST is not idempotent, a retry after a lost response could charge the card twice; -// keep the timeouts and circuit breaker (tunable under Bank:Resilience), but never retry it -builder.Services.AddHttpClient((services, client) => - client.BaseAddress = services.GetRequiredService>().Value.BaseUrl) - .AddStandardResilienceHandler(builder.Configuration.GetSection("Bank:Resilience")) - .Configure(options => options.Retry.DisableForUnsafeHttpMethods()); + // a payment POST is not idempotent, a retry after a lost response could charge the card twice; + // keep the timeouts and circuit breaker (tunable under Bank:Resilience), but never retry it + builder.Services.AddHttpClient((services, client) => + client.BaseAddress = services.GetRequiredService>().Value.BaseUrl) + .AddStandardResilienceHandler(builder.Configuration.GetSection("Bank:Resilience")) + .Configure(options => options.Retry.DisableForUnsafeHttpMethods()); -var awsOptions = builder.Configuration.GetAWSOptions(); -awsOptions.DefaultClientConfig.Timeout = TimeSpan.FromSeconds(10); -builder.Services.AddDefaultAWSOptions(awsOptions); -builder.Services.AddAWSService(); + var awsOptions = builder.Configuration.GetAWSOptions(); + awsOptions.DefaultClientConfig.Timeout = TimeSpan.FromSeconds(10); + builder.Services.AddDefaultAWSOptions(awsOptions); + builder.Services.AddAWSService(); -// stateless, so shared; the payment service is scoped because it holds the typed bank client, -// which must not be captured by a singleton (its handler is rotated by IHttpClientFactory) -builder.Services.AddSingleton(); -builder.Services.AddSingleton(); -builder.Services.AddScoped(); + // stateless, so shared; the payment service is scoped because it holds the typed bank client, + // which must not be captured by a singleton (its handler is rotated by IHttpClientFactory) + builder.Services.AddSingleton(); + builder.Services.AddSingleton(); + builder.Services.AddScoped(); -builder.Services.AddEndpointsApiExplorer(); -builder.Services.AddSwaggerGen(); + builder.Services.AddEndpointsApiExplorer(); + builder.Services.AddSwaggerGen(); -var app = builder.Build(); -app.UseSwagger(); -app.UseSwaggerUI(); + var app = builder.Build(); + app.UseSwagger(); + app.UseSwaggerUI(); -app.UseMiddleware(); -app.UseExceptionHandler(); -app.UseStatusCodePages(); + // the correlation id wraps everything else, so every log line of the request carries it, + // including the request summary and the exception handler's + app.UseMiddleware(); + app.UseSerilogRequestLogging(); + app.UseExceptionHandler(); + app.UseStatusCodePages(); -app.MapHealthChecks("/health/live"); -app.MapControllers(); -app.Run(); + app.MapHealthChecks("/health/live"); + app.MapControllers(); -// https://learn.microsoft.com/en-us/aspnet/core/fundamentals/http-requests?view=aspnetcore-10.0#header-propagation-middleware -// https://github.com/dotnet/aspnetcore/issues/48581 -// .AddPollyRetries(log); -// app.UseHeaderPropagation(); + app.Run(); + return 0; +} +catch (Exception exception) when (exception is not HostAbortedException) +{ + // HostAbortedException is how tooling such as WebApplicationFactory stops the host on purpose + Log.Fatal(exception, "Startup, Status={Status}", "Failed"); + return 1; +} +finally +{ + // the file sink buffers, flush it before the process exits + await Log.CloseAndFlushAsync(); +} diff --git a/src/Services/PaymentRequestValidator.cs b/src/Services/PaymentRequestValidator.cs index f21afaf..600bc4b 100644 --- a/src/Services/PaymentRequestValidator.cs +++ b/src/Services/PaymentRequestValidator.cs @@ -72,7 +72,8 @@ public IReadOnlyDictionary Validate(PaymentRequest request) if (errors.Count > 0) { - this.LogRejected("Rejected", errors.Keys); + string fields = string.Join(",", errors.Keys); + this.LogRejected("Rejected", fields); } return errors; @@ -94,5 +95,5 @@ private bool IsExpired(PaymentRequest request) private static partial Regex CvvPattern(); [LoggerMessage(Level = LogLevel.Information, Message = "Validate, Status={Status}, Fields={Fields}")] - private partial void LogRejected(string status, IEnumerable fields); + private partial void LogRejected(string status, string fields); } diff --git a/src/appsettings.Development.json b/src/appsettings.Development.json index 78414c6..d5069aa 100644 --- a/src/appsettings.Development.json +++ b/src/appsettings.Development.json @@ -1,4 +1,34 @@ { + "Serilog": { + "Using": [ "Serilog.Sinks.Console", "Serilog.Sinks.File" ], + "WriteTo": { + "Console": { + "Args": { + "formatter": { + "type": "Serilog.Formatting.Display.MessageTemplateTextFormatter, Serilog", + "outputTemplate": "[{Timestamp:HH:mm:ss.fff} {Level:u3}] {CorrelationId} {Message:l}{NewLine}{Exception}" + } + } + }, + "InfoFile": { + "Name": "File", + "Args": { + "path": "z-logs/info-.log", + "outputTemplate": "[{Timestamp:yyyy-MM-dd HH:mm:ss.fff} {Level:u3}] CorrelationId={CorrelationId}, TraceId={TraceId}, Message={Message:l}, Source={SourceContext}{NewLine}{Exception}", + "rollingInterval": "Day" + } + }, + "ErrorFile": { + "Name": "File", + "Args": { + "path": "z-logs/error-.log", + "outputTemplate": "[{Timestamp:yyyy-MM-dd HH:mm:ss.fff} {Level:u3}] CorrelationId={CorrelationId}, TraceId={TraceId}, Message={Message:l}, Source={SourceContext}{NewLine}{Exception}", + "rollingInterval": "Day", + "restrictedToMinimumLevel": "Warning" + } + } + } + }, "Bank": { "BaseUrl": "http://localhost:8080/" }, diff --git a/src/appsettings.json b/src/appsettings.json index e2c796f..a0d5fc9 100644 --- a/src/appsettings.json +++ b/src/appsettings.json @@ -1,5 +1,25 @@ { "AllowedHosts": "*", + "Serilog": { + "Using": [ "Serilog.Sinks.Console" ], + "MinimumLevel": { + "Default": "Information", + "Override": { + "Microsoft.AspNetCore": "Warning", + "System.Net.Http": "Warning", + "Polly": "Warning" + } + }, + "WriteTo": { + "Console": { + "Name": "Console", + "Args": { + "formatter": { "type": "Serilog.Formatting.Compact.RenderedCompactJsonFormatter, Serilog.Formatting.Compact" } + } + } + }, + "Enrich": [ "FromLogContext" ] + }, "Bank": { "Resilience": { "AttemptTimeout": { "Timeout": "00:00:10" }, diff --git a/src/logconfig.json b/src/logconfig.json deleted file mode 100644 index 1f48554..0000000 --- a/src/logconfig.json +++ /dev/null @@ -1,44 +0,0 @@ -{ - "Serilog": { - "Using": [ - "Serilog.Sinks.Console", - "Serilog.Sinks.File" - ], - "MinimumLevel": { - "Default": "Information", - "Override": { - "Microsoft.AspNetCore": "Warning", - "System.Net.Http": "Warning" - } - }, - "WriteTo": [ - { - "Name": "Console", - "Args": { - "theme": "Serilog.Sinks.SystemConsole.Themes.AnsiConsoleTheme::Sixteen, Serilog.Sinks.Console", - "outputTemplate": "[{Timestamp:HH:mm:ss.fff}] [{Level:u3}], Message={Message}{NewLine}" - } - }, - { - "Name": "File", - "Args": { - "path": "z-logs/info-.log", - "outputTemplate": "[{Timestamp:yyyy-MM-dd HH:mm:ss.fff}] [{Level:u3}] CallerId={CallerId:l}, CorrelationId={CorrelationId:l}, Message={Message}, Source={SourceContext}{NewLine}", - "rollingInterval": "Day" - } - }, - { - "Name": "File", - "Args": { - "path": "z-logs/error-.log", - "outputTemplate": "[{Timestamp:yyyy-MM-dd HH:mm:ss.fff}] [{Level:u3}] CallerId={CallerId:l}, CorrelationId={CorrelationId:l}, Message={Message},Source={SourceContext}{NewLine}Exception={Exception}{NewLine}", - "rollingInterval": "Day", - "restrictedToMinimumLevel": "Warning" - } - } - ], - "Enrich": [ - "FromLogContext" - ] - } -} diff --git a/tests/PaymentGatewayApiEndToEndTests/ApiFixture.cs b/tests/PaymentGatewayApiEndToEndTests/ApiFixture.cs index d44d366..0fa5e22 100644 --- a/tests/PaymentGatewayApiEndToEndTests/ApiFixture.cs +++ b/tests/PaymentGatewayApiEndToEndTests/ApiFixture.cs @@ -24,7 +24,6 @@ public ApiFixture() BaseAddress = new Uri(baseUrl), Timeout = TimeSpan.FromSeconds(30) }; - this.Client.DefaultRequestHeaders.Add("CallerId", "e2e-tests"); } public async ValueTask InitializeAsync() diff --git a/tests/PaymentGatewayApiUnitTests/Middleware/CorrelationIdMiddlewareTests.cs b/tests/PaymentGatewayApiUnitTests/Middleware/CorrelationIdMiddlewareTests.cs new file mode 100644 index 0000000..1c9940c --- /dev/null +++ b/tests/PaymentGatewayApiUnitTests/Middleware/CorrelationIdMiddlewareTests.cs @@ -0,0 +1,121 @@ +using System; +using System.Collections.Generic; +using System.Threading.Tasks; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Http.Features; +using PaymentGatewayApi.Middleware; +using Serilog; +using Serilog.Core; +using Serilog.Events; +using Xunit; + +namespace PaymentGatewayApiUnitTests.Middleware; + +public class CorrelationIdMiddlewareTests +{ + private readonly CollectingSink sink = new(); + private readonly Logger logger; + + public CorrelationIdMiddlewareTests() + { + this.logger = new LoggerConfiguration() + .Enrich.FromLogContext() + .WriteTo.Sink(this.sink) + .CreateLogger(); + } + + private static DefaultHttpContext NewContext(string? header = null) + { + DefaultHttpContext context = new(); + context.Features.Set(new StartableResponseFeature()); + if (header is not null) + { + context.Request.Headers[CorrelationIdMiddleware.HeaderName] = header; + } + + return context; + } + + // runs the middleware with a next delegate that logs one event and starts the response + private async Task InvokeAndCapture(DefaultHttpContext context) + { + CorrelationIdMiddleware middleware = new(_ => + { + this.logger.Information("next"); + return Task.CompletedTask; + }); + + await middleware.InvokeAsync(context); + await ((StartableResponseFeature)context.Features.Get()!).StartAsync(); + + LogEvent logEvent = Assert.Single(this.sink.Events); + Assert.True(logEvent.Properties.TryGetValue("CorrelationId", out LogEventPropertyValue? value)); + return Assert.IsType(value).Value?.ToString(); + } + + [Theory] + [InlineData("abc")] + [InlineData("16bd4749-d150-48d1-a3ce-257457d74d1f")] + public async Task InvokeAsync_WellFormedHeader_IsLoggedStoredAndEchoed(string header) + { + DefaultHttpContext context = NewContext(header); + + string? logged = await this.InvokeAndCapture(context); + + Assert.Equal(header, logged); + Assert.Equal(header, CorrelationIdMiddleware.Get(context)); + Assert.Equal(header, context.Response.Headers[CorrelationIdMiddleware.HeaderName].ToString()); + } + + [Theory] + [InlineData(null)] + [InlineData("")] + [InlineData("abc\ninjected log line")] + [InlineData("