diff --git a/docs/guide/handlers/persistence.md b/docs/guide/handlers/persistence.md index 263093a135..8cee9af0d0 100644 --- a/docs/guide/handlers/persistence.md +++ b/docs/guide/handlers/persistence.md @@ -80,9 +80,48 @@ By default, Wolverine is assuming that any parameter value marked with `[Entity] * As a message handler, it will just log that the entity could not be found and otherwise exit cleanly without doing any further processing * As an HTTP endpoint, the handler would write out a status code of 404 (not found) and exit otherwise +You can choose a different answer with the `OnMissing` property: + +| `OnMissing` | Message handler | HTTP endpoint | +|---|---|---| +| `Simple404` (default) | Log it and stop | Empty **404** | +| `ProblemDetailsWith400` | Log it and stop | **400** with a `ProblemDetails` body | +| `ProblemDetailsWith404` | Log it and stop | **404** with a `ProblemDetails` body | +| `EmptyContentWith204` | Log it and stop | Empty **204** | +| `ThrowException` | Throws `RequiredDataMissingException` | Throws `RequiredDataMissingException` | + If you need or want any other kind of failure handling on the entity not being found, you'll need to use explicit code instead, maybe with a `LoadAsync()` "before" method to still keep your main -handler or endpoint method a *pure function*. +handler or endpoint method a *pure function*. + +### Answering 204 instead of 404 + +A bare 404 is indistinguishable from "you called a Url that does not exist." If you would rather say +"the Url is correct, but there is no body," use `OnMissing.EmptyContentWith204`: + +```cs +[WolverineGet("/api/alerts/config/services/{serviceName}")] +public static ServiceAlertOverrides Get( + [Entity(OnMissing = OnMissing.EmptyContentWith204)] ServiceAlertOverrides overrides) + => overrides; +``` + +A request for a `serviceName` that has no overrides answers `204` with an empty body, and the generated +OpenAPI advertises `200` and `204` rather than `200` and `404`. + +::: warning +Think about your clients before you reach for this. A 404 puts a miss on the failure branch of every +HTTP client; a 204 puts it on the success branch. Code that does `response.EnsureSuccessStatusCode()` +will start passing, and generated typed clients (NSwag, Kiota, Refit) will map the 204 onto their +success path — so a miss can surface later as a null dereference instead of at the call site. If what +you actually want is a *distinguishable* 404, `OnMissing.ProblemDetailsWith404` names the type and the +identity in a `application/problem+json` body and keeps the failure on the failure branch. +::: + +On a `GET` or `QUERY` endpoint, `EmptyContentWith204` also forces the entity to be treated as required +even if you wrote `Required = false`. Running the endpoint body with a null entity so it can return an +empty body anyway buys nothing, and it is the one combination where "not required" and "answer 204" +contradict each other. This does not apply to message handlers or to other HTTP methods. If you genuinely don't need the `[Entity]` value to be required, you can do this instead: @@ -401,6 +440,14 @@ public static class MyHandler The resolution order is: **Explicit attribute value > Global default > Built-in default** (`Simple404` / `true`). +::: tip +`EntityDefaults.OnMissing` reaches every attribute that loads data this way — `[Entity]`, `[Document]`, +`[Aggregate]`, `[ReadAggregate]`, `[WriteAggregate]`, `[ReadModel]`, `[WriteModel]`, and the DCB +attributes. Setting it to `OnMissing.EmptyContentWith204` changes the answer for *all* of them, including +aggregate endpoints you may not have been thinking about. Set it on the individual attributes instead if +you only meant a subset. +::: + Some other facts to know about `[Entity]` usage: * Supported by the Marten, EF Core, and RavenDb integration diff --git a/docs/guide/http/endpoints.md b/docs/guide/http/endpoints.md index 1c16c3295e..bca5c4699d 100644 --- a/docs/guide/http/endpoints.md +++ b/docs/guide/http/endpoints.md @@ -241,6 +241,67 @@ the underlying OpenAPI stack emits 3.2. See [JSON serialization for more information](/guide/http/json) +## When the Response Body is Null + +When an endpoint's resource comes back `null`, Wolverine writes an empty **404**: + +```cs +[WolverineGet("/api/alerts/config/services/{serviceName}")] +public static async Task Get(string serviceName, IDocumentSession session) + // A miss here answers 404 with an empty body + => await session.LoadAsync(serviceName); +``` + +A bare 404 is indistinguishable from "you called a Url that does not exist." If you would rather say +"the Url is correct, but there is no body," decorate the endpoint method with `[NoContentIfMissing]` to +get an empty **204** instead: + +```cs +[WolverineGet("/api/alerts/config/services/{serviceName}"), NoContentIfMissing] +public static async Task Get(string serviceName, IDocumentSession session) + // A miss now answers 204 with an empty body + => await session.LoadAsync(serviceName); +``` + +The generated OpenAPI follows: the endpoint advertises `200` and `204` rather than `200` and `404`. + +`[NoContentIfMissing]` can also go on the endpoint *class*, where it applies to every endpoint method in +that class. A method level declaration always wins over a class level one, and `[NotFoundIfMissing]` is +how a single method opts back out. + +### An Application Wide Default + +Set `WolverineHttpOptions.OnMissingResponseBody` to change the default for the whole application: + +```cs +app.MapWolverineEndpoints(opts => +{ + opts.OnMissingResponseBody = OnMissingResponseBody.NoContent204; +}); +``` + +Use `[NotFoundIfMissing]` on any endpoint or endpoint class that should keep answering 404. + +::: warning +`[NoContentIfMissing]` is only legal on `GET` and `QUERY` endpoints, and the application wide setting +only reaches those methods. Those are the safe, side effect free reads where an empty answer is a benign +outcome. On a `POST` or `PUT`, a 204 in place of a resource would turn a failed command into an apparent +success for the caller, so Wolverine fails fast at bootstrapping time rather than let you ship it. + +Be deliberate about this even on reads. A 404 puts a miss on the failure branch of every HTTP client; a +204 puts it on the success branch. Code that calls `response.EnsureSuccessStatusCode()` will start +passing, and generated typed clients (NSwag, Kiota, Refit) will map the 204 onto their success path. +::: + +::: tip +This setting is strictly about the response *body*. What happens when a required entity cannot be loaded +in the first place is a separate question, answered by +[`OnMissing` on `[Entity]`](/guide/handlers/persistence#answering-204-instead-of-404) — including its own +`OnMissing.EmptyContentWith204`. In an endpoint like +`Get([Entity] Thing thing) => thing`, the entity guard runs *before* the endpoint body, so it is +`OnMissing` — not `[NoContentIfMissing]` — that decides the answer. +::: + ## Returning Strings To create an endpoint that writes a string with `content-type` = "text/plain", just return a string as your resource type, so `string`, `Task`, or `ValueTask` diff --git a/docs/guide/http/marten.md b/docs/guide/http/marten.md index 1db2141a40..94e781ee17 100644 --- a/docs/guide/http/marten.md +++ b/docs/guide/http/marten.md @@ -82,6 +82,10 @@ In the code above, if the `Invoice` document does not exist, the route will stop If you, for whatever reason, want your handler executed even if the document does not exist, then you can set the `DocumentAttribute.Required` property to `false`. +Use the `OnMissing` property for any other answer — a `ProblemDetails` body, a thrown exception, or an +empty `204` with `OnMissing.EmptyContentWith204` . See +[the full `OnMissing` table](/guide/handlers/persistence#using-entity-for-message-handlers-and-http-endpoints). + :::info Starting with Wolverine 3 `DocumentAttribute.Required = true` is the default behavior. In previous versions the default value was `false`. diff --git a/docs/guide/http/polecat.md b/docs/guide/http/polecat.md index e17ae09b8c..9d2bdddc59 100644 --- a/docs/guide/http/polecat.md +++ b/docs/guide/http/polecat.md @@ -62,6 +62,10 @@ In the code above, if the `Invoice` document does not exist, the route will stop If you want your handler executed even if the document does not exist, set `Required` to `false`. +Use the `OnMissing` property for any other answer — a `ProblemDetails` body, a thrown exception, or an +empty `204` with `OnMissing.EmptyContentWith204` . See +[the full `OnMissing` table](/guide/handlers/persistence#using-entity-for-message-handlers-and-http-endpoints). + ## Polecat Aggregate Workflow The HTTP endpoints can play inside the full Wolverine + Polecat combination with Wolverine's [specific diff --git a/src/Extensions/Wolverine.Http.Newtonsoft/CodeGen/NewtonsoftHttpCodeGen.cs b/src/Extensions/Wolverine.Http.Newtonsoft/CodeGen/NewtonsoftHttpCodeGen.cs index fa2062c6ed..09eaa91738 100644 --- a/src/Extensions/Wolverine.Http.Newtonsoft/CodeGen/NewtonsoftHttpCodeGen.cs +++ b/src/Extensions/Wolverine.Http.Newtonsoft/CodeGen/NewtonsoftHttpCodeGen.cs @@ -27,11 +27,12 @@ public Variable CreateReadJsonBodyVariable(Type requestType) [UnconditionalSuppressMessage("Trimming", "IL2026", Justification = "MethodCall reflects NewtonsoftHttpSerialization.GetMethod(nameof(WriteJsonAsync)) at codegen time. The target method is statically referenced via nameof; the closed-generic type is rooted at codegen time per the AOT guide.")] - public Frame CreateWriteJsonFrame(Variable resourceVariable) + public Frame CreateWriteJsonFrame(Variable resourceVariable, int missingStatusCode = 404) { var frame = new MethodCall(typeof(NewtonsoftHttpSerialization), nameof(NewtonsoftHttpSerialization.WriteJsonAsync)); frame.Arguments[1] = resourceVariable; + frame.Arguments[2] = Constant.For(missingStatusCode); return frame; } } diff --git a/src/Extensions/Wolverine.Http.Newtonsoft/NewtonsoftHttpSerialization.cs b/src/Extensions/Wolverine.Http.Newtonsoft/NewtonsoftHttpSerialization.cs index a7dc3d5a00..3e6598efcd 100644 --- a/src/Extensions/Wolverine.Http.Newtonsoft/NewtonsoftHttpSerialization.cs +++ b/src/Extensions/Wolverine.Http.Newtonsoft/NewtonsoftHttpSerialization.cs @@ -28,11 +28,11 @@ public NewtonsoftHttpSerialization(JsonSerializerSettings settings) public JsonSerializerSettings Settings { get; set; } [MethodImpl(MethodImplOptions.AggressiveInlining)] - public async Task WriteJsonAsync(HttpContext context, object? body) + public async Task WriteJsonAsync(HttpContext context, object? body, int missingStatusCode = 404) { if (body == null) { - context.Response.StatusCode = 404; + context.Response.StatusCode = missingStatusCode; return; } diff --git a/src/Http/Wolverine.Http.Marten/CompiledQueryWriterPolicy.cs b/src/Http/Wolverine.Http.Marten/CompiledQueryWriterPolicy.cs index 927438d12d..b57d2fb760 100644 --- a/src/Http/Wolverine.Http.Marten/CompiledQueryWriterPolicy.cs +++ b/src/Http/Wolverine.Http.Marten/CompiledQueryWriterPolicy.cs @@ -40,10 +40,12 @@ public bool TryApply(HttpChain chain) typeof(MartenQueryMethodCall<,>).CloseAndBuildAs(result!, arguments); chain.Postprocessors.Add(queryCall); - // This call writes the response directly to the HttpContext as a string - var writeStringCall = MethodCall.For(handler => HttpHandler.WriteString(null!, "")); + // This call writes the response directly to the HttpContext as a string. The status code has to be + // spelled out in the expression tree -- an expression tree cannot elide an optional argument. + var writeStringCall = MethodCall.For(handler => HttpHandler.WriteString(null!, "", 404)); writeStringCall.Arguments[1] = new Variable(queryCall.ReturnVariable!.VariableType, $"{queryCall.ReturnVariable.Usage}.ToString()", queryCall); + writeStringCall.Arguments[2] = Constant.For(chain.MissingResponseBodyStatusCode); chain.Postprocessors.Add(writeStringCall); } else diff --git a/src/Http/Wolverine.Http.Tests/Persistence/EmptyContentWith204Endpoints.cs b/src/Http/Wolverine.Http.Tests/Persistence/EmptyContentWith204Endpoints.cs new file mode 100644 index 0000000000..f686929068 --- /dev/null +++ b/src/Http/Wolverine.Http.Tests/Persistence/EmptyContentWith204Endpoints.cs @@ -0,0 +1,80 @@ +using Wolverine.Persistence; +using WolverineWebApi.Todos; + +namespace Wolverine.Http.Tests.Persistence; + +/// +/// Endpoints for the two independent "there is nothing to send back" paths: +/// covers a required entity that could not be loaded, while +/// covers an endpoint whose response body is simply null. +/// +// Deliberately not a static class -- HttpChain.ChainFor() needs a usable type argument +public class EmptyContentWith204Endpoints +{ + // The entity guard answers 204 instead of the default 404 + [WolverineGet("/no-content/entity/{id}")] + public static Todo2 GetEntity([Entity(OnMissing = OnMissing.EmptyContentWith204)] Todo2 todo) => todo; + + // Required = false is deliberately ignored here: on a GET, EmptyContentWith204 forces the entity to be + // treated as required, because running the endpoint with a null entity to return an empty body anyway + // buys nothing. Without that, this endpoint would NRE on todo.Name. + [WolverineGet("/no-content/entity-not-required/{id}")] + public static string GetEntityNotRequired( + [Entity(OnMissing = OnMissing.EmptyContentWith204, Required = false)] Todo2 todo) => todo.Name!; + + // No entity attribute at all -- just an endpoint whose resource comes back null + [WolverineGet("/no-content/body/{id}"), NoContentIfMissing] + public static Todo2? GetBody(string id) => id == "found" ? new Todo2 { Id = id, Name = "Found" } : null; + + // The unannotated control for GetBody + [WolverineGet("/no-content/body-default/{id}")] + public static Todo2? GetBodyDefault(string id) => id == "found" ? new Todo2 { Id = id, Name = "Found" } : null; + + // Same, but the resource type is a string, which goes through a different response writer + [WolverineGet("/no-content/string/{id}"), NoContentIfMissing] + public static string? GetString(string id) => id == "found" ? "Found" : null; + + // The unannotated control for the string writer: a null string used to throw a NullReferenceException + // and answer 500 rather than the 404 every other resource type answered + [WolverineGet("/no-content/string-default/{id}")] + public static string? GetStringDefault(string id) => id == "found" ? "Found" : null; +} + +/// +/// A class level [NoContentIfMissing] applies to every endpoint method in the class. +/// +[NoContentIfMissing] +public static class ClassLevelNoContentEndpoints +{ + [WolverineGet("/no-content/class-level/{id}")] + public static Todo2? Get(string id) => id == "found" ? new Todo2 { Id = id, Name = "Found" } : null; + + // ... unless the method opts back out + [WolverineGet("/no-content/class-level-opt-out/{id}"), NotFoundIfMissing] + public static Todo2? GetOptOut(string id) => id == "found" ? new Todo2 { Id = id, Name = "Found" } : null; +} + +/// +/// Endpoints used to prove the global WolverineHttpOptions.OnMissingResponseBody setting, which only +/// reaches GET and QUERY endpoints. +/// +public static class GlobalMissingResponseBodyEndpoints +{ + [WolverineGet("/global-no-content/get/{id}")] + public static Todo2? Get(string id) => id == "found" ? new Todo2 { Id = id, Name = "Found" } : null; + + [WolverineGet("/global-no-content/opt-out/{id}"), NotFoundIfMissing] + public static Todo2? GetOptOut(string id) => id == "found" ? new Todo2 { Id = id, Name = "Found" } : null; + + [WolverinePost("/global-no-content/post")] + public static Todo2? Post(CreateTodo2 command) => command.Id == "found" ? new Todo2 { Id = command.Id } : null; +} + +/// +/// Uses a plain [Entity], so it picks up whatever WolverineOptions.EntityDefaults.OnMissing is set to. +/// +public static class GlobalEmptyContentEntityEndpoint +{ + [WolverineGet("/global-no-content/entity/{id}")] + public static Todo2 Get([Entity] Todo2 todo) => todo; +} diff --git a/src/Http/Wolverine.Http.Tests/Persistence/empty_content_with_204.cs b/src/Http/Wolverine.Http.Tests/Persistence/empty_content_with_204.cs new file mode 100644 index 0000000000..5f6bba68c2 --- /dev/null +++ b/src/Http/Wolverine.Http.Tests/Persistence/empty_content_with_204.cs @@ -0,0 +1,367 @@ +using Alba; +using IntegrationTests; +using Marten; +using Microsoft.AspNetCore.Builder; +using Microsoft.AspNetCore.Http.Metadata; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Hosting; +using Shouldly; +using Wolverine.Marten; +using Wolverine.Persistence; +using WolverineWebApi.Todos; + +namespace Wolverine.Http.Tests.Persistence; + +// See EmptyContentWith204Endpoints +public class empty_content_with_204 : IAsyncLifetime +{ + private IAlbaHost theHost = null!; + + public async ValueTask InitializeAsync() + { + var builder = WebApplication.CreateBuilder([]); + + builder.Services.AddMarten(opts => + { + opts.Connection(Servers.PostgresConnectionString); + opts.DatabaseSchemaName = "empty_content_204"; + }).IntegrateWithWolverine().UseLightweightSessions(); + + builder.Host.UseWolverine(opts => opts.Discovery.IncludeAssembly(GetType().Assembly)); + + builder.Services.AddWolverineHttp(); + + theHost = await AlbaHost.For(builder, app => + { + app.UseDeveloperExceptionPage(); + app.MapWolverineEndpoints(); + }); + } + + async ValueTask IAsyncDisposable.DisposeAsync() + { + if (theHost != null) + { + await theHost.StopAsync(); + theHost.Dispose(); + } + } + + [Fact] + public async Task entity_miss_returns_204_with_an_empty_body() + { + var result = await theHost.Scenario(x => + { + x.Get.Url("/no-content/entity/nonexistent"); + x.StatusCodeShouldBe(204); + }); + + (await result.ReadAsTextAsync()).ShouldBeEmpty(); + } + + [Fact] + public async Task entity_hit_still_returns_the_document() + { + await using (var session = theHost.Services.GetRequiredService().LightweightSession()) + { + session.Store(new Todo2 { Id = "real-one", Name = "Kareem" }); + await session.SaveChangesAsync(TestContext.Current.CancellationToken); + } + + var result = await theHost.Scenario(x => + { + x.Get.Url("/no-content/entity/real-one"); + x.StatusCodeShouldBeOk(); + }); + + (await result.ReadAsJsonAsync())!.Name.ShouldBe("Kareem"); + } + + [Fact] + public async Task required_is_forced_true_on_a_get_so_the_handler_never_sees_a_null() + { + // Required = false on the attribute, but EmptyContentWith204 on a GET overrides it. Without that, + // the endpoint body would dereference a null Todo2 and blow up with a 500. + var result = await theHost.Scenario(x => + { + x.Get.Url("/no-content/entity-not-required/nonexistent"); + x.StatusCodeShouldBe(204); + }); + + (await result.ReadAsTextAsync()).ShouldBeEmpty(); + } + + [Fact] + public async Task no_content_if_missing_covers_a_null_response_body() + { + var result = await theHost.Scenario(x => + { + x.Get.Url("/no-content/body/missing"); + x.StatusCodeShouldBe(204); + }); + + (await result.ReadAsTextAsync()).ShouldBeEmpty(); + + await theHost.Scenario(x => + { + x.Get.Url("/no-content/body/found"); + x.StatusCodeShouldBeOk(); + }); + } + + [Fact] + public async Task no_content_if_missing_covers_a_null_string_response_body() + { + var result = await theHost.Scenario(x => + { + x.Get.Url("/no-content/string/missing"); + x.StatusCodeShouldBe(204); + }); + + (await result.ReadAsTextAsync()).ShouldBeEmpty(); + } + + [Fact] + public async Task a_null_string_resource_is_a_404_rather_than_a_500() + { + // Regression: HttpHandler.WriteString dereferenced the null for its ContentLength and threw a + // NullReferenceException, so a string returning endpoint answered 500 where every other resource + // type answered 404. + await theHost.Scenario(x => + { + x.Get.Url("/no-content/string-default/missing"); + x.StatusCodeShouldBe(404); + }); + } + + [Fact] + public async Task class_level_attribute_applies_to_the_methods() + { + await theHost.Scenario(x => + { + x.Get.Url("/no-content/class-level/missing"); + x.StatusCodeShouldBe(204); + }); + } + + [Fact] + public async Task a_method_can_opt_back_out_of_a_class_level_attribute() + { + await theHost.Scenario(x => + { + x.Get.Url("/no-content/class-level-opt-out/missing"); + x.StatusCodeShouldBe(404); + }); + } + + [Fact] + public async Task the_default_is_unchanged() + { + // Nothing about this feature is on unless you ask for it + await theHost.Scenario(x => + { + x.Get.Url("/global-no-content/get/missing"); + x.StatusCodeShouldBe(404); + }); + } +} + +// See EmptyContentWith204Endpoints +public class global_empty_content_with_204 : IAsyncLifetime +{ + private IAlbaHost theHost = null!; + + public async ValueTask InitializeAsync() + { + var builder = WebApplication.CreateBuilder([]); + + builder.Services.AddMarten(opts => + { + opts.Connection(Servers.PostgresConnectionString); + opts.DatabaseSchemaName = "global_empty_content_204"; + }).IntegrateWithWolverine().UseLightweightSessions(); + + builder.Host.UseWolverine(opts => + { + opts.Discovery.IncludeAssembly(GetType().Assembly); + + // The application wide answer for a required entity that could not be loaded + opts.EntityDefaults.OnMissing = OnMissing.EmptyContentWith204; + }); + + builder.Services.AddWolverineHttp(); + + theHost = await AlbaHost.For(builder, app => + { + app.UseDeveloperExceptionPage(); + + // ... and the application wide answer for a null response body + app.MapWolverineEndpoints(opts => + opts.OnMissingResponseBody = OnMissingResponseBody.NoContent204); + }); + } + + async ValueTask IAsyncDisposable.DisposeAsync() + { + if (theHost != null) + { + await theHost.StopAsync(); + theHost.Dispose(); + } + } + + [Fact] + public async Task global_entity_default_reaches_a_plain_entity_attribute() + { + await theHost.Scenario(x => + { + x.Get.Url("/global-no-content/entity/nonexistent"); + x.StatusCodeShouldBe(204); + }); + } + + [Fact] + public async Task global_response_body_default_reaches_a_get() + { + var result = await theHost.Scenario(x => + { + x.Get.Url("/global-no-content/get/missing"); + x.StatusCodeShouldBe(204); + }); + + (await result.ReadAsTextAsync()).ShouldBeEmpty(); + } + + [Fact] + public async Task an_endpoint_can_opt_back_out_of_the_global_default() + { + await theHost.Scenario(x => + { + x.Get.Url("/global-no-content/opt-out/missing"); + x.StatusCodeShouldBe(404); + }); + } + + [Fact] + public async Task the_global_default_does_not_reach_a_post() + { + // A 204 in place of a resource on a POST would turn a failed command into an apparent success, + // so the application wide setting stops at the safe reads. + await theHost.Scenario(x => + { + x.Post.Json(new CreateTodo2("missing", "Nope")).ToUrl("/global-no-content/post"); + x.StatusCodeShouldBe(404); + }); + } +} + +public class missing_response_body_metadata_and_validation +{ + [Fact] + public void openapi_advertises_204_instead_of_404_when_opted_in() + { + var chain = HttpChain.ChainFor(x => + EmptyContentWith204Endpoints.GetBody(null!)); + + var statuses = chain.BuildEndpoint(RouteWarmup.Lazy).Metadata + .OfType() + .Select(x => x.StatusCode) + .ToArray(); + + statuses.ShouldContain(200); + statuses.ShouldContain(204); + statuses.ShouldNotContain(404); + } + + [Fact] + public void openapi_still_advertises_404_by_default() + { + var chain = HttpChain.ChainFor(x => + EmptyContentWith204Endpoints.GetBodyDefault(null!)); + + chain.BuildEndpoint(RouteWarmup.Lazy).Metadata + .OfType() + .Select(x => x.StatusCode) + .ShouldContain(404); + } + + [Fact] + public void a_string_endpoint_advertises_204_only_when_it_opted_in() + { + // A string endpoint has never advertised a missing-resource status, so the default stays as it was + // and only the opt-in adds one. Otherwise every string returning endpoint's OpenAPI would change. + HttpChain.ChainFor(x => EmptyContentWith204Endpoints.GetStringDefault(null!)) + .BuildEndpoint(RouteWarmup.Lazy).Metadata + .OfType() + .Select(x => x.StatusCode) + .ShouldBe([200]); + + HttpChain.ChainFor(x => EmptyContentWith204Endpoints.GetString(null!)) + .BuildEndpoint(RouteWarmup.Lazy).Metadata + .OfType() + .Select(x => x.StatusCode) + .ShouldBe([200, 204]); + } + + [Fact] + public void throws_when_no_content_if_missing_is_used_on_a_post() + { + var ex = Should.Throw(() => + HttpChain.ChainFor(x => x.Post(null!))); + + ex.Message.ShouldContain("POST"); + ex.Message.ShouldContain("GET and QUERY"); + } + + [Fact] + public void throws_when_a_class_level_attribute_reaches_a_non_read_endpoint() + { + var ex = Should.Throw(() => + HttpChain.ChainFor(x => x.Delete(null!))); + + ex.Message.ShouldContain("Move it onto the individual GET/QUERY methods"); + } + + [Fact] + public void a_method_level_opt_out_rescues_a_non_read_endpoint_in_a_decorated_class() + { + // [NotFoundIfMissing] is the documented escape hatch, so this must not throw + Should.NotThrow(() => + HttpChain.ChainFor(x => x.DeleteButOptedOut(null!))); + } + + [Fact] + public void throws_when_both_attributes_are_on_the_same_member() + { + var ex = Should.Throw(() => + HttpChain.ChainFor(x => x.Get(null!))); + + ex.Message.ShouldContain("mutually exclusive"); + } +} + +public class PostWithNoContentIfMissing +{ + [Attributes.WolverineIgnore] + [WolverinePost("/no-content/invalid-post"), NoContentIfMissing] + public Todo2? Post(CreateTodo2 command) => null; +} + +[NoContentIfMissing] +public class ClassWithNoContentIfMissingAndADelete +{ + [Attributes.WolverineIgnore] + [WolverineDelete("/no-content/invalid-delete")] + public Todo2? Delete(DeleteTodo command) => null; + + [Attributes.WolverineIgnore] + [WolverineDelete("/no-content/rescued-delete"), NotFoundIfMissing] + public Todo2? DeleteButOptedOut(DeleteTodo command) => null; +} + +public class ContradictoryEndpoint +{ + [Attributes.WolverineIgnore] + [WolverineGet("/no-content/contradictory/{id}"), NoContentIfMissing, NotFoundIfMissing] + public Todo2? Get(string id) => null; +} diff --git a/src/Http/Wolverine.Http/CodeGen/INewtonsoftHttpCodeGen.cs b/src/Http/Wolverine.Http/CodeGen/INewtonsoftHttpCodeGen.cs index c2f707e8f7..5f104b5064 100644 --- a/src/Http/Wolverine.Http/CodeGen/INewtonsoftHttpCodeGen.cs +++ b/src/Http/Wolverine.Http/CodeGen/INewtonsoftHttpCodeGen.cs @@ -33,7 +33,10 @@ internal interface INewtonsoftHttpCodeGen /// /// Build the codegen frame that writes the resource value to the response body - /// via Newtonsoft.Json serialization. + /// via Newtonsoft.Json serialization. is the status written when + /// the resource is null -- 404 unless the endpoint opts into 204 with + /// or the application wide + /// WolverineHttpOptions.OnMissingResponseBody. /// - Frame CreateWriteJsonFrame(Variable resourceVariable); + Frame CreateWriteJsonFrame(Variable resourceVariable, int missingStatusCode = 404); } diff --git a/src/Http/Wolverine.Http/CodeGen/WriteJsonFrame.cs b/src/Http/Wolverine.Http/CodeGen/WriteJsonFrame.cs index 18b7bf32ac..6d5f02fbf5 100644 --- a/src/Http/Wolverine.Http/CodeGen/WriteJsonFrame.cs +++ b/src/Http/Wolverine.Http/CodeGen/WriteJsonFrame.cs @@ -7,17 +7,19 @@ namespace Wolverine.Http.CodeGen; public class WriteJsonFrame : AsyncFrame { private readonly Variable _resourceVariable; + private readonly int _missingStatusCode; - public WriteJsonFrame(Variable resourceVariable) + public WriteJsonFrame(Variable resourceVariable, int missingStatusCode = 404) { _resourceVariable = resourceVariable; + _missingStatusCode = missingStatusCode; uses.Add(resourceVariable); } public override void GenerateCode(GeneratedMethod method, ISourceWriter writer) { writer.WriteComment("Writing the response body to JSON because this was the first 'return variable' in the method signature"); - writer.Write($"await {nameof(HttpHandler.WriteJsonAsync)}(httpContext, {_resourceVariable.Usage});"); + writer.Write($"await {nameof(HttpHandler.WriteJsonAsync)}(httpContext, {_resourceVariable.Usage}, {_missingStatusCode});"); Next?.GenerateCode(method, writer); } @@ -27,7 +29,7 @@ public override void GenerateFSharpCode(GeneratedMethod method, ISourceWriter wr // WriteJsonAsync is an inherited *instance* method on HttpHandler, so it must be qualified with // the generated member's `this` self identifier (jasperfx#393). - var call = $"this.{nameof(HttpHandler.WriteJsonAsync)}(httpContext, {_resourceVariable.Usage})"; + var call = $"this.{nameof(HttpHandler.WriteJsonAsync)}(httpContext, {_resourceVariable.Usage}, {_missingStatusCode})"; writer.Write(method.AsyncMode == AsyncMode.AsyncTask ? $"do! {call}" : call); Next?.GenerateFSharpCode(method, writer); diff --git a/src/Http/Wolverine.Http/ContentNegotiation/ContentNegotiationPolicy.cs b/src/Http/Wolverine.Http/ContentNegotiation/ContentNegotiationPolicy.cs index 6d38d00e14..00d2199405 100644 --- a/src/Http/Wolverine.Http/ContentNegotiation/ContentNegotiationPolicy.cs +++ b/src/Http/Wolverine.Http/ContentNegotiation/ContentNegotiationPolicy.cs @@ -28,7 +28,8 @@ public bool TryApply(HttpChain chain) var resourceVariable = chain.ResourceVariable ?? chain.Method.Creates.First(); resourceVariable.OverrideName(resourceVariable.Usage + "_response"); - chain.Postprocessors.Add(new ContentNegotiationWriteFrame(resourceVariable, writers, chain.ConnegMode)); + chain.Postprocessors.Add(new ContentNegotiationWriteFrame(resourceVariable, writers, chain.ConnegMode, + chain.MissingResponseBodyStatusCode)); return true; } @@ -83,13 +84,16 @@ internal class ContentNegotiationWriteFrame : AsyncFrame private readonly Variable _resourceVariable; private readonly List _writers; private readonly ConnegMode _mode; + private readonly int _missingStatusCode; private Variable? _httpContext; - public ContentNegotiationWriteFrame(Variable resourceVariable, List writers, ConnegMode mode) + public ContentNegotiationWriteFrame(Variable resourceVariable, List writers, ConnegMode mode, + int missingStatusCode = 404) { _resourceVariable = resourceVariable; _writers = writers; _mode = mode; + _missingStatusCode = missingStatusCode; uses.Add(resourceVariable); } @@ -135,7 +139,8 @@ public override void GenerateCode(GeneratedMethod method, ISourceWriter writer) { writer.Write("BLOCK:else"); writer.WriteComment("Fallback to JSON serialization"); - writer.Write($"await {nameof(HttpHandler.WriteJsonAsync)}({_httpContext.Usage}, {_resourceVariable.Usage});"); + writer.Write( + $"await {nameof(HttpHandler.WriteJsonAsync)}({_httpContext.Usage}, {_resourceVariable.Usage}, {_missingStatusCode});"); writer.FinishBlock(); } else diff --git a/src/Http/Wolverine.Http/HttpChain.EndpointBuilder.cs b/src/Http/Wolverine.Http/HttpChain.EndpointBuilder.cs index d5a49d8201..625e594436 100644 --- a/src/Http/Wolverine.Http/HttpChain.EndpointBuilder.cs +++ b/src/Http/Wolverine.Http/HttpChain.EndpointBuilder.cs @@ -177,11 +177,25 @@ private void establishResourceTypeMetadata(RouteEndpointBuilder builder) if (ResourceType == typeof(string)) { Metadata.Produces(200, typeof(string), "text/plain"); + + // Unlike the JSON branch below, a string endpoint has never advertised its missing-resource status + // -- a null string used to throw out of HttpHandler.WriteString rather than answer anything at all. + // Only document the status when the endpoint explicitly opted into one, so that fixing the null + // string does not silently rewrite the OpenAPI of every string returning endpoint in the world. + if (MissingResponseBodyStatusCode != 404) + { + Metadata.Produces(MissingResponseBodyStatusCode); + } + return; } Metadata.Produces(200, ResourceType, "application/json"); - Metadata.Produces(404); + + // 404 unless this endpoint opted into an empty 204 for a null response body. Resolved by + // ResolveMissingResponseBody() before this runs, and the same value the response writer emits, + // so the generated OpenAPI cannot drift from what the endpoint actually returns. + Metadata.Produces(MissingResponseBodyStatusCode); } internal interface IApplier diff --git a/src/Http/Wolverine.Http/HttpChain.cs b/src/Http/Wolverine.Http/HttpChain.cs index ad07e79bae..83e07f7cb2 100644 --- a/src/Http/Wolverine.Http/HttpChain.cs +++ b/src/Http/Wolverine.Http/HttpChain.cs @@ -489,11 +489,158 @@ public override Frame[] AddStopConditionIfNull(Variable data, Variable? identity Metadata.Produces(404, contentType: "application/problem+json"); return [new WriteProblemDetailsIfNull(data, identity!, message, 404)]; + case OnMissing.EmptyContentWith204: + Metadata.Produces(204); + return [new SetStatusCodeAndReturnIfEntityIsNullFrame(data, 204)]; + default: return [new ThrowRequiredDataMissingExceptionFrame(data, identity!, message)]; } } + // GET and QUERY (RFC 10008) are the safe, side effect free reads where "there is nothing here" is a + // benign answer rather than a failure. Anywhere else, a 204 in place of a resource would quietly turn + // a failed command into an apparent success. + private static readonly string[] ReadOnlyHttpMethods = ["GET", "QUERY"]; + + internal bool IsReadOnlyEndpoint() + { + return _httpMethods.Count > 0 && _httpMethods.All(x => ReadOnlyHttpMethods.Contains(x)); + } + + /// + /// The status code written when this endpoint's response body is null. 404 unless + /// or WolverineHttpOptions.OnMissingResponseBody says + /// otherwise. Resolved once at bootstrapping time by . + /// + public int MissingResponseBodyStatusCode { get; private set; } = 404; + + private bool _missingResponseBodyIsExplicit; + + /// + /// Read / off the + /// endpoint method, then the endpoint class. Done at construction time rather than in + /// so that a chain built outside of -- + /// in tests, most notably -- still honors the attributes. + /// + private void readMissingResponseBodyAttributes() + { + if (tryReadMissingResponseBodyAttribute(Method.Method, out var fromMethod)) + { + MissingResponseBodyStatusCode = fromMethod; + _missingResponseBodyIsExplicit = true; + } + else if (tryReadMissingResponseBodyAttribute(Method.HandlerType, out var fromClass)) + { + MissingResponseBodyStatusCode = fromClass; + _missingResponseBodyIsExplicit = true; + } + } + + /// + /// Apply the application wide WolverineHttpOptions.OnMissingResponseBody to any endpoint that did + /// not declare its own answer. + /// + internal void ResolveMissingResponseBody(WolverineHttpOptions options) + { + if (_missingResponseBodyIsExplicit) + { + return; + } + + // The global default deliberately does NOT reach non-read endpoints. Unlike the attribute -- where the + // author is naming one endpoint and a mistake should be loud -- a single application wide setting would + // otherwise silently reshape every POST/PUT/DELETE response in the system. + MissingResponseBodyStatusCode = + options.OnMissingResponseBody == OnMissingResponseBody.NoContent204 && IsReadOnlyEndpoint() ? 204 : 404; + } + + private static bool tryReadMissingResponseBodyAttribute(MemberInfo member, out int statusCode) + { + if (member.HasAttribute()) + { + statusCode = 204; + return true; + } + + if (member.HasAttribute()) + { + statusCode = 404; + return true; + } + + statusCode = 404; + return false; + } + + /// + /// Fail fast on a [NoContentIfMissing] that could never be honored. Both checks depend only on the + /// attributes and the HTTP method, so this runs at construction time -- the same place as the GH-3648 + /// request body guard -- rather than waiting for the endpoint to be requested and answer wrongly. + /// + private void assertMissingResponseBodyAttributesAreLegal() + { + assertNotBothAttributes(Method.Method, "method"); + assertNotBothAttributes(Method.HandlerType, "class"); + + if (Method.Method.HasAttribute()) + { + assertIsReadOnlyEndpointForNoContent(false); + } + // Only reached when the method itself said nothing -- a method level [NotFoundIfMissing] is the + // documented way to keep one non-read endpoint inside a class that is otherwise all GETs. + else if (Method.HandlerType.HasAttribute() && + !Method.Method.HasAttribute()) + { + assertIsReadOnlyEndpointForNoContent(true); + } + } + + private void assertNotBothAttributes(MemberInfo member, string level) + { + if (member.HasAttribute() && member.HasAttribute()) + { + throw new InvalidOperationException( + $"HTTP endpoint {Method.HandlerType.FullNameInCode()}.{Method.Method.Name} has both " + + $"[NoContentIfMissing] and [NotFoundIfMissing] on the same {level}. These are mutually " + + "exclusive -- keep whichever one you meant."); + } + } + + private void assertIsReadOnlyEndpointForNoContent(bool fromClass) + { + if (IsReadOnlyEndpoint()) + { + return; + } + + var placement = fromClass + ? $"[NoContentIfMissing] is declared on {Method.HandlerType.FullNameInCode()}, which also holds this endpoint. Either move it onto the individual GET/QUERY methods in that class, or mark this one [NotFoundIfMissing]" + : $"[NoContentIfMissing] is declared on {Method.HandlerType.FullNameInCode()}.{Method.Method.Name}. Remove it"; + + throw new InvalidOperationException( + $"HTTP endpoint {Method.HandlerType.FullNameInCode()}.{Method.Method.Name} is mapped to " + + $"{(_httpMethods.Count == 0 ? "no HTTP method" : _httpMethods.Join("/"))} {RoutePattern?.RawText}, " + + $"but {placement}. An empty 204 in place of a response body is only meaningful on the safe, side " + + "effect free reads -- GET and QUERY. On any other HTTP method it would turn a failed request into " + + "an apparent success for the caller."); + } + + /// + /// forces the entity to be treated as required on GET or QUERY + /// endpoints. Running the endpoint with a null entity so it can return an empty body anyway buys nothing, + /// and it is the one configuration where "not required" and "answer 204" contradict each other. + /// + public override bool IsDataRequired(IDataRequirement requirement) + { + if (requirement.OnMissing == OnMissing.EmptyContentWith204 && IsReadOnlyEndpoint()) + { + return true; + } + + return requirement.Required; + } + public override string ToString() { return _fileName!; @@ -537,6 +684,9 @@ private void applyMetadata() .WithMetadata(new HttpMethodMetadata(_httpMethods)); //.WithMetadata(Method.Method); + assertMissingResponseBodyAttributesAreLegal(); + readMissingResponseBodyAttributes(); + // Checked outside the HasRequestType branch below on purpose. On a GET a complex parameter binds // from the query string rather than the body, so no Accepts metadata is produced -- but the // attribute itself still reaches the endpoint metadata through the GetCustomAttributes() loop at diff --git a/src/Http/Wolverine.Http/HttpGraph.cs b/src/Http/Wolverine.Http/HttpGraph.cs index 77b17e6543..a189db875d 100644 --- a/src/Http/Wolverine.Http/HttpGraph.cs +++ b/src/Http/Wolverine.Http/HttpGraph.cs @@ -168,6 +168,10 @@ public void DiscoverEndpoints(WolverineHttpOptions wolverineHttpOptions) wolverineHttpOptions.Middleware.Apply(_chains, Rules, Container); _optionsWriterPolicies.AddRange(wolverineHttpOptions.ResourceWriterPolicies); + // After the API versioning expansion above so the per-version clones are covered too, and before + // BuildEndpoint() below, which bakes the resolved status code into the endpoint's OpenAPI metadata. + foreach (var chain in _chains) chain.ResolveMissingResponseBody(wolverineHttpOptions); + // Apply route prefix policy before other policies so that // downstream policies see the final route patterns var routePrefixPolicy = new RoutePrefixPolicy(wolverineHttpOptions); diff --git a/src/Http/Wolverine.Http/HttpHandler.cs b/src/Http/Wolverine.Http/HttpHandler.cs index f22d9079d5..c2f793d8a7 100644 --- a/src/Http/Wolverine.Http/HttpHandler.cs +++ b/src/Http/Wolverine.Http/HttpHandler.cs @@ -100,8 +100,16 @@ public static string[] ReadManyHeaderValues(HttpContext context, string headerKe } [MethodImpl(MethodImplOptions.AggressiveInlining)] - public static Task WriteString(HttpContext context, string text) + public static Task WriteString(HttpContext context, string? text, int missingStatusCode = 404) { + // A null string resource used to dereference straight into a NullReferenceException below, so a + // string returning endpoint answered 500 where every other resource type answered 404. + if (text == null) + { + context.Response.StatusCode = missingStatusCode; + return Task.CompletedTask; + } + context.Response.ContentType = "text/plain"; context.Response.ContentLength = text.Length; return context.Response.WriteAsync(text, context.RequestAborted); @@ -215,12 +223,18 @@ private static bool acceptsJson(HttpContext context) || x.MediaType.Value!.Contains("+json"))); } + /// + /// Write the endpoint's resource as JSON, or -- when it is null -- an empty response with + /// . The status code is resolved once at bootstrapping time by + /// HttpChain.ResolveMissingResponseBody() and baked into the generated code, so this stays a + /// constant on the hot path. + /// [MethodImpl(MethodImplOptions.AggressiveInlining)] - public Task WriteJsonAsync(HttpContext context, T? body) + public Task WriteJsonAsync(HttpContext context, T? body, int missingStatusCode = 404) { if (body == null) { - context.Response.StatusCode = 404; + context.Response.StatusCode = missingStatusCode; return Task.CompletedTask; } diff --git a/src/Http/Wolverine.Http/MissingResponseBodyAttributes.cs b/src/Http/Wolverine.Http/MissingResponseBodyAttributes.cs new file mode 100644 index 0000000000..89e38b2334 --- /dev/null +++ b/src/Http/Wolverine.Http/MissingResponseBodyAttributes.cs @@ -0,0 +1,32 @@ +namespace Wolverine.Http; + +/// +/// Directs Wolverine to write an empty 204 instead of the default 404 when this endpoint's +/// response body is null, denoting "the Url is correct, but there is no body." Valid on an endpoint method or +/// on an endpoint class, where it applies to every endpoint method in that class. A method level declaration +/// wins over a class level one. +/// +/// +/// +/// This is only about the response body. It has no effect on what happens when a required entity +/// cannot be loaded -- use [Entity(OnMissing = OnMissing.EmptyContentWith204)], or the +/// WolverineOptions.EntityDefaults.OnMissing global default, for that. +/// +/// +/// Only legal on GET and QUERY (RFC 10008) endpoints. Those are the safe, side effect free reads where an +/// empty answer is a benign outcome. On any other HTTP method a 204 in place of a resource would quietly +/// turn a failed command into an apparent success on the client, so Wolverine fails fast at bootstrapping +/// time instead. +/// +/// +[AttributeUsage(AttributeTargets.Method | AttributeTargets.Class)] +public class NoContentIfMissingAttribute : Attribute; + +/// +/// Directs Wolverine to write an empty 404 when this endpoint's response body is null. This is already +/// Wolverine's default, so this attribute is only useful to opt a single endpoint or endpoint class back out of +/// an application wide WolverineHttpOptions.OnMissingResponseBody = OnMissingResponseBody.NoContent204, +/// or to opt one method out of a class level . +/// +[AttributeUsage(AttributeTargets.Method | AttributeTargets.Class)] +public class NotFoundIfMissingAttribute : Attribute; diff --git a/src/Http/Wolverine.Http/Policies/SetStatusCodeAndReturnIfEntityIsNullFrame.cs b/src/Http/Wolverine.Http/Policies/SetStatusCodeAndReturnIfEntityIsNullFrame.cs index 2c01ce2715..38713fb721 100644 --- a/src/Http/Wolverine.Http/Policies/SetStatusCodeAndReturnIfEntityIsNullFrame.cs +++ b/src/Http/Wolverine.Http/Policies/SetStatusCodeAndReturnIfEntityIsNullFrame.cs @@ -10,18 +10,21 @@ namespace Wolverine.Http.Policies; internal class SetStatusCodeAndReturnIfEntityIsNullFrame : SyncFrame { private readonly Type _entityType; + private readonly int _statusCode; private Variable? _httpResponse; private Variable? _entity; - public SetStatusCodeAndReturnIfEntityIsNullFrame(Type entityType) + public SetStatusCodeAndReturnIfEntityIsNullFrame(Type entityType, int statusCode = 404) { _entityType = entityType; + _statusCode = statusCode; } - public SetStatusCodeAndReturnIfEntityIsNullFrame(Variable entity) + public SetStatusCodeAndReturnIfEntityIsNullFrame(Variable entity, int statusCode = 404) { _entity = entity; _entityType = entity.VariableType; + _statusCode = statusCode; } public override void GenerateCode(GeneratedMethod method, ISourceWriter writer) @@ -29,12 +32,12 @@ public override void GenerateCode(GeneratedMethod method, ISourceWriter writer) ValueTypeReturnVariable.TupleVariable? problemDetailsVariable = null; if (_entity?.Creator is MethodCall { ReturnVariable: ValueTypeReturnVariable vrv }) problemDetailsVariable = vrv.Inners.FirstOrDefault(v => v.Inner.VariableType == typeof(ProblemDetails)); - writer.WriteComment("404 if this required object is null"); + writer.WriteComment($"{_statusCode} if this required object is null"); if (problemDetailsVariable != null) writer.WriteComment($"Take no action if {problemDetailsVariable.Inner.Usage}.Status == 404"); writer.Write( $"BLOCK:if ({_entity!.Usage} == null{(problemDetailsVariable == null ? "" : $" && {problemDetailsVariable.Inner.Usage}.Status != 404")})"); - writer.Write($"{_httpResponse!.Usage}.{nameof(HttpResponse.StatusCode)} = 404;"); + writer.Write($"{_httpResponse!.Usage}.{nameof(HttpResponse.StatusCode)} = {_statusCode};"); if (method.AsyncMode == AsyncMode.ReturnCompletedTask) writer.Write($"return {typeof(Task).FullNameInCode()}.{nameof(Task.CompletedTask)};"); else diff --git a/src/Http/Wolverine.Http/Resources/JsonResourceWriterPolicy.cs b/src/Http/Wolverine.Http/Resources/JsonResourceWriterPolicy.cs index f4105095aa..e9d2c7191b 100644 --- a/src/Http/Wolverine.Http/Resources/JsonResourceWriterPolicy.cs +++ b/src/Http/Wolverine.Http/Resources/JsonResourceWriterPolicy.cs @@ -13,7 +13,7 @@ public bool TryApply(HttpChain chain) if (Usage == JsonUsage.SystemTextJson) { - chain.Postprocessors.Add(new WriteJsonFrame(resourceVariable)); + chain.Postprocessors.Add(new WriteJsonFrame(resourceVariable, chain.MissingResponseBodyStatusCode)); } else { @@ -26,7 +26,8 @@ public bool TryApply(HttpChain chain) "See https://wolverinefx.net/guide/http/json.html#using-newtonsoft-json."); } - chain.Postprocessors.Add(NewtonsoftCodeGen.CreateWriteJsonFrame(resourceVariable)); + chain.Postprocessors.Add( + NewtonsoftCodeGen.CreateWriteJsonFrame(resourceVariable, chain.MissingResponseBodyStatusCode)); } return true; diff --git a/src/Http/Wolverine.Http/Resources/StringResourceWriterPolicy.cs b/src/Http/Wolverine.Http/Resources/StringResourceWriterPolicy.cs index fd0c0df7d7..64c39133fd 100644 --- a/src/Http/Wolverine.Http/Resources/StringResourceWriterPolicy.cs +++ b/src/Http/Wolverine.Http/Resources/StringResourceWriterPolicy.cs @@ -11,7 +11,8 @@ public bool TryApply(HttpChain chain) { if (chain.ResourceType == typeof(string)) { - chain.Postprocessors.Add(new WriteStringFrame(chain.Method.Creates.First())); + chain.Postprocessors.Add(new WriteStringFrame(chain.Method.Creates.First(), + chain.MissingResponseBodyStatusCode)); return true; } @@ -22,10 +23,12 @@ public bool TryApply(HttpChain chain) internal class WriteStringFrame : AsyncFrame { private readonly Variable _result; + private readonly int _missingStatusCode; - public WriteStringFrame(Variable result) + public WriteStringFrame(Variable result, int missingStatusCode = 404) { _result = result; + _missingStatusCode = missingStatusCode; uses.Add(_result); } @@ -33,7 +36,8 @@ public override void GenerateCode(GeneratedMethod method, ISourceWriter writer) { var prefix = method.AsyncMode == AsyncMode.ReturnCompletedTask ? "return" : "await"; - writer.Write($"{prefix} {nameof(HttpHandler.WriteString)}(httpContext, {_result.Usage});"); + writer.Write( + $"{prefix} {nameof(HttpHandler.WriteString)}(httpContext, {_result.Usage}, {_missingStatusCode});"); Next?.GenerateCode(method, writer); } @@ -42,7 +46,7 @@ public override void GenerateFSharpCode(GeneratedMethod method, ISourceWriter wr { // HttpHandler.WriteString is static, so it resolves cleanly in F# (no `this`). var call = - $"{typeof(HttpHandler).FSharpName()}.{nameof(HttpHandler.WriteString)}(httpContext, {_result.Usage})"; + $"{typeof(HttpHandler).FSharpName()}.{nameof(HttpHandler.WriteString)}(httpContext, {_result.Usage}, {_missingStatusCode})"; // Inside a `task { }` body await it; otherwise it IS the trailing Task expression. writer.Write(method.AsyncMode == AsyncMode.AsyncTask ? $"do! {call}" : call); diff --git a/src/Http/Wolverine.Http/WolverineHttpOptions.cs b/src/Http/Wolverine.Http/WolverineHttpOptions.cs index 42f06b0c6d..a7bd32eef7 100644 --- a/src/Http/Wolverine.Http/WolverineHttpOptions.cs +++ b/src/Http/Wolverine.Http/WolverineHttpOptions.cs @@ -123,6 +123,27 @@ public enum ServiceProviderSource FromHttpContextRequestServices } +/// +/// Governs the status code written when an HTTP endpoint's resource -- the response body -- turns out to be +/// null. This is strictly about the body an endpoint returns; the behavior when a required entity cannot be +/// loaded is controlled separately by . +/// +public enum OnMissingResponseBody +{ + /// + /// The default. A null response body is written as an empty 404, treating "there is nothing to return" as + /// "there is nothing here." + /// + NotFound404, + + /// + /// A null response body is written as an empty 204, denoting "the Url is correct, but there is no body." + /// Applied globally this only affects GET and QUERY endpoints, where an empty answer is a benign outcome + /// rather than a failed command. Override per endpoint with . + /// + NoContent204 +} + public class WolverineHttpOptions { public WolverineHttpOptions() @@ -217,6 +238,20 @@ public void UseApiVersioning(Action configure) /// public bool RejectUnparseableQueryValues { get; set; } + /// + /// The application wide default for what status code is written when an endpoint's response body is null. + /// The built in default is . Setting this to + /// only affects GET and QUERY endpoints -- a null body + /// on any other HTTP method continues to be a 404, because turning a failed command into an apparent + /// success is never what anyone wants. Individual endpoints override this with + /// or . + /// + /// + /// This governs the response body only. Use WolverineOptions.EntityDefaults.OnMissing to + /// control what happens when a required entity cannot be loaded in the first place. + /// + public OnMissingResponseBody OnMissingResponseBody { get; set; } = OnMissingResponseBody.NotFound404; + internal TenantIdDetection TenantIdDetection { get; } = new(); internal Lazy JsonSerializerOptions { get; set; } = new(() => new JsonSerializerOptions()); diff --git a/src/Persistence/MartenTests/missing_data_handling_with_entity_attributes.cs b/src/Persistence/MartenTests/missing_data_handling_with_entity_attributes.cs index 2fd65d78b1..d893a79a66 100644 --- a/src/Persistence/MartenTests/missing_data_handling_with_entity_attributes.cs +++ b/src/Persistence/MartenTests/missing_data_handling_with_entity_attributes.cs @@ -49,6 +49,15 @@ public async Task just_swallow_the_exception_and_log() await _host.InvokeAsync(new UseThing1(Guid.NewGuid().ToString())); await _host.InvokeAsync(new UseThing2(Guid.NewGuid().ToString())); await _host.InvokeAsync(new UseThing3(Guid.NewGuid().ToString())); + await _host.InvokeAsync(new UseThing6(Guid.NewGuid().ToString())); + } + + [Fact] + public async Task empty_content_with_204_just_stops_in_a_message_handler() + { + var tracked = await _host.InvokeMessageAndWaitAsync(new UseThing6(Guid.NewGuid().ToString())); + + tracked.Sent.AllMessages().Any().ShouldBeFalse(); } [Fact] @@ -143,6 +152,8 @@ public record UseThing4(string Id); public record UseThing5(string Id); +public record UseThing6(string Id); + public record UsedThing(string Id); public static class ThingHandler @@ -173,6 +184,16 @@ public static UsedThing Handle(UseThing5 command, return new UsedThing(thing.Id); } + // The 204 is meaningless outside of HTTP, but a message handler has to tolerate the setting -- an + // [Entity] configuration is routinely shared between an endpoint and a handler. Before this value joined + // the "log it and stop" group in HandlerChain.AddStopConditionIfNull it fell through to the throwing + // branch, so this handler would have thrown on a miss instead of quietly stopping. + public static UsedThing Handle(UseThing6 command, + [Entity(OnMissing = OnMissing.EmptyContentWith204)] Thing thing) + { + return new UsedThing(thing.Id); + } + public static void Handle(UsedThing msg) { Debug.WriteLine("Used thing " + msg.Id); diff --git a/src/Wolverine/Configuration/Chain.cs b/src/Wolverine/Configuration/Chain.cs index f50c01989f..2d4deed6b8 100644 --- a/src/Wolverine/Configuration/Chain.cs +++ b/src/Wolverine/Configuration/Chain.cs @@ -223,6 +223,11 @@ public virtual Frame[] AddStopConditionIfNull(Variable data, Variable? identity, return AddStopConditionIfNull(data); } + public virtual bool IsDataRequired(IDataRequirement requirement) + { + return requirement.Required; + } + private static Type[] _typesToIgnore = new Type[] { typeof(DateOnly), diff --git a/src/Wolverine/Configuration/IChain.cs b/src/Wolverine/Configuration/IChain.cs index 2e011ddda8..8551bcb56b 100644 --- a/src/Wolverine/Configuration/IChain.cs +++ b/src/Wolverine/Configuration/IChain.cs @@ -246,6 +246,14 @@ public interface IChain /// Frame[] AddStopConditionIfNull(Variable data, Variable? identity, IDataRequirement requirement); + /// + /// Is the data described by this requirement required for execution to continue? This is normally just + /// , but a chain type is allowed to force the data to be required. + /// Wolverine's HTTP chains do exactly that for on GET or QUERY + /// endpoints, where returning an empty 204 is a benign outcome. + /// + bool IsDataRequired(IDataRequirement requirement) => requirement.Required; + bool TryInferMessageIdentity(out PropertyInfo? property); /// diff --git a/src/Wolverine/Persistence/EntityAttribute.cs b/src/Wolverine/Persistence/EntityAttribute.cs index b8b392e80e..9ca97bec8e 100644 --- a/src/Wolverine/Persistence/EntityAttribute.cs +++ b/src/Wolverine/Persistence/EntityAttribute.cs @@ -174,7 +174,7 @@ public override Variable Modify(IChain chain, ParameterInfo parameter, IServiceC } Variable returnVariable; - if (Required) + if (chain.IsDataRequired(this)) { var otherFrames = chain.AddStopConditionIfNull(entity, identity, this); diff --git a/src/Wolverine/Persistence/EventSourcing/AggregateHandling.cs b/src/Wolverine/Persistence/EventSourcing/AggregateHandling.cs index 42cd878a72..b020ddcf81 100644 --- a/src/Wolverine/Persistence/EventSourcing/AggregateHandling.cs +++ b/src/Wolverine/Persistence/EventSourcing/AggregateHandling.cs @@ -369,7 +369,7 @@ internal Variable RelayAggregateToHandlerMethod(Variable eventStream, IChain cha typeof(IEventStream<>).MakeGenericType(aggregateType).GetProperty(nameof(IEventStream.Aggregate))!); - if (Requirement.Required) + if (chain.IsDataRequired(Requirement)) { var otherFrames = chain.AddStopConditionIfNull(aggregateVariable, AggregateId, Requirement); diff --git a/src/Wolverine/Persistence/EventSourcing/DcbModelAttribute.cs b/src/Wolverine/Persistence/EventSourcing/DcbModelAttribute.cs index 7ede2409c1..37b6b73440 100644 --- a/src/Wolverine/Persistence/EventSourcing/DcbModelAttribute.cs +++ b/src/Wolverine/Persistence/EventSourcing/DcbModelAttribute.cs @@ -128,7 +128,7 @@ public override Variable Modify(IChain chain, ParameterInfo parameter, IServiceC Variable modelVariable = new MemberAccessVariable(boundary, boundaryType.GetProperty(nameof(IEventBoundary.Aggregate))!); - if (Required) + if (chain.IsDataRequired(this)) { var otherFrames = chain.AddStopConditionIfNull(modelVariable, null, this); var block = new LoadEntityFrameBlock(modelVariable, otherFrames); diff --git a/src/Wolverine/Persistence/EventSourcing/ReadModelAttribute.cs b/src/Wolverine/Persistence/EventSourcing/ReadModelAttribute.cs index 5b977497cb..b36ab1d1b8 100644 --- a/src/Wolverine/Persistence/EventSourcing/ReadModelAttribute.cs +++ b/src/Wolverine/Persistence/EventSourcing/ReadModelAttribute.cs @@ -117,7 +117,7 @@ public override Variable Modify(IChain chain, ParameterInfo parameter, IServiceC aggregate.OverrideName(parameter.Name!); Variable returnVariable; - if (Required) + if (chain.IsDataRequired(this)) { var otherFrames = chain.AddStopConditionIfNull(aggregate, identity, this); diff --git a/src/Wolverine/Persistence/IDataRequirement.cs b/src/Wolverine/Persistence/IDataRequirement.cs index e3bb1715d2..188f47eeb4 100644 --- a/src/Wolverine/Persistence/IDataRequirement.cs +++ b/src/Wolverine/Persistence/IDataRequirement.cs @@ -23,7 +23,16 @@ public enum OnMissing /// /// Throws a RequiredDataMissingException using the MissingMessage /// - ThrowException + ThrowException, + + /// + /// In a message handler, the execution will just stop after logging that the data was missing -- identical to + /// . In an HTTP endpoint the request will stop w/ an empty body and a 204 status code to + /// denote "the Url was correct, but there is no content." On any GET or QUERY endpoint this value also forces + /// the data to be treated as required regardless of the setting, because + /// a 204 is a benign outcome and there is no reason to run the endpoint with a null entity. + /// + EmptyContentWith204 } public class RequiredDataMissingException : Exception diff --git a/src/Wolverine/Runtime/Handlers/HandlerChain.cs b/src/Wolverine/Runtime/Handlers/HandlerChain.cs index 5f6572e395..cf1292801a 100644 --- a/src/Wolverine/Runtime/Handlers/HandlerChain.cs +++ b/src/Wolverine/Runtime/Handlers/HandlerChain.cs @@ -509,6 +509,10 @@ public override Frame[] AddStopConditionIfNull(Variable data, Variable? identity case OnMissing.Simple404: case OnMissing.ProblemDetailsWith400: case OnMissing.ProblemDetailsWith404: + // The 204 is meaningless outside of HTTP, but the "log it and stop" behavior is identical + // to Simple404. Leaving it out of this group would drop it into the `default:` below and + // start throwing on message handlers that share an [Entity] configuration with an endpoint. + case OnMissing.EmptyContentWith204: var frame = typeof(EntityIsNotNullGuardFrame<>).CloseAndBuildAs(data, data.VariableType); if (frame is IEntityIsNotNullGuard guard) guard.Requirement = requirement;