ikyriak / IdempotentAPI

A .NET library that handles the HTTP write operations (POST and PATCH) that can affect only once for the given request data and idempotency-key by using an ASP.NET Core attribute (filter).
MIT License
253 stars 38 forks source link

Http Status code of the response is not returned correctly from the cache #39

Closed MohamadTahir closed 1 year ago

MohamadTahir commented 2 years ago

Hey again, I found what I believe to be a small bug :) here is the case, in my project, I have the need to send a NotAccepted response and since I couldn't find a method in the ControllerBase that returns a NotAccepted status code, I decided to make my own ObjectResult (let me know if .net has a NotAccepted response that I missed somehow), so I went ahead and made the following function that returns the response that I want

    public static ObjectResult NotAcceptedObjectResult(string message)
    {
        return new ObjectResult(new ErrorModel
        {
            Title = HttpStatusCode.NotAcceptable,
            StatusCode = StatusCodes.Status406NotAcceptable,
            Errors = new[]{
                    message
                },
        })
        {
            StatusCode = StatusCodes.Status406NotAcceptable,
        };
    }

the ErrorModel is just a record that has the properties that you can see, either way, the response that I get from the first call (not from the cache) is correct, but if I repeat the call with the same idempotency key (to get the result from the cache ) the response that I get back from cache is not correct, the response body is the same but the HTTP Status code of the call is 200 instead of 406.

I ended up making these changes

[DefaultStatusCode(DefaultStatusCode)]
public class NotAcceptableObjectResult : ObjectResult
{
    private const int DefaultStatusCode = StatusCodes.Status406NotAcceptable;
    public NotAcceptableResult([ActionResultObjectValue] object? value) : base(value)
    {
        StatusCode = DefaultStatusCode;
    }
}

which is the same as BadRequestObjectResult from .det, and now I return the following from my function

public ObjectResult NotAcceptableResult(string message)
    {
        return new NotAcceptableObjectResult(new ErrorModel
        {
            Title = HttpStatusCode.Conflict,
            StatusCode = StatusCodes.Status409Conflict,
            Errors = new[]{
                    message
                },
        });
    }

after these changes, the first response and the response I get from the cache, are both correct. I am not sure what is causing the bug but here is the bug and the workaround for it.

I hope you can fix it :) If you need more information about the bug let me know.

ikyriak commented 1 year ago

Hello @MohamadTahir Thanks for taking the time to report this issue and the workaround! šŸ‘ šŸ™

MohamadTahir commented 1 year ago

Always welcome @ikyriak

there is another thing, after doing some search I realized that unsuccessful responses should not be cached when implementing idempotency, why? what if a transient error occurred? the user should be able to process the same quest with the same idempotency key, this package behaves correctly when the error occurs before reaching the controller, what do I mean by this?

so let's say that we have a request model like the following

public record SampleModel
{
    [MinLength(3)]
    public int[]  { get; set; } = Array.Empty<int>();
}

if the user sends a request and the array size is 2, then the request won't go inside the controller and will return 400, in this case, the package won't cache the response, which is the correct behavior.

but what if the user sent an array of size 4, there will be no validation errors and the request will go into the controller, in there we do our own validation on the data that has been provided in the array then decide that we need to return the NotAcceptableResult response for whatever reason, in this case, since the request is returned from within the controller it will be cached by the package, which is not the behavior that we look for.

ikyriak commented 1 year ago

Hello @MohamadTahir

The issue of the NotAcceptedObjectResult will be fixed in the following major release, as you have provided a workaround for the moment.

Regarding the "unsuccessful responses should not be cached":

In the 1.0.2-prerelease-01 version, you can set the CacheOnlySuccessResponses attribute option to true/false. Currently, the default is false not to change the current behavior. However, the default value will be true in the following major release. You can see more details in issue https://github.com/ikyriak/IdempotentAPI/issues/37.

Is this behavior working for your case?

MohamadTahir commented 1 year ago

Perfect, that's what we want, we want to cache only successfully responses

ikyriak commented 1 year ago

Hello @MohamadTahir,

The fix for the NotAcceptedObjectResul issue has been released in v2.0.0-RC.01 šŸŽ† šŸŽ‰ . Could you please verify that it's OK?