Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change [Document] Required to true in Wolverine 3.0 #1007

Closed
haefele opened this issue Aug 12, 2024 · 0 comments
Closed

Change [Document] Required to true in Wolverine 3.0 #1007

haefele opened this issue Aug 12, 2024 · 0 comments

Comments

@haefele
Copy link
Contributor

haefele commented Aug 12, 2024

Is your feature request related to a problem? Please describe.
The Wolverine docs make it seem like the [Document] attribute returns status code 404 for not-found NULL documents by default.

The first example for the [Document] is this:

[WolverineGet("/invoices/longhand/id")]
[ProducesResponseType(404)]
[ProducesResponseType(200, Type = typeof(Invoice))]
public static async Task<IResult> GetInvoice(
    Guid id,
    IQuerySession session,
    CancellationToken cancellationToken)
{
    var invoice = await session.LoadAsync<Invoice>(id, cancellationToken);
    if (invoice == null) return Results.NotFound();

    return Results.Ok(invoice);
}

Turns into this way simpler and shorter code:

[WolverineGet("/invoices/{id}")]
public static Invoice Get([Document] Invoice invoice)
{
    return invoice;
}

That is a bit misleading tho, because there is actually no check for NULL in the second code.
When the document does not exist, the Get(...) handler will still be called, but with NULL for the invoice variable.

A while back the DocumentAttribute.Required property was added to change this behavior, but for backward compatibility reasons the property is false by default.

Notably, the second handler will still return 404 for NULL documents, but only because the return value of the handler is null.

Another misleading example from the docs is this:

[WolverinePost("/invoices/{number}/approve")]
public static IMartenOp Approve([Document("number")] Invoice invoice)
{
    invoice.Approved = true;
    return MartenOps.Store(invoice);
}

This code will actually throw a NullReferenceException at invoice.Approved = true when the invoice document with the given number doesn't exist.

Describe the solution you'd like
I suggest the DocumentAttribute.Required property should be changed to true by default.
This corresponds to what I expect from this functionality after reading the docs.

Describe alternatives you've considered
Change the docs to make it more clear, that the handler might be called with NULL.

Additional context
Briefly talked about this suggestion here before.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant