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

Generalize Api responses #615

Closed
CanMehmetK opened this issue Apr 18, 2022 · 9 comments
Closed

Generalize Api responses #615

CanMehmetK opened this issue Apr 18, 2022 · 9 comments

Comments

@CanMehmetK
Copy link

Is your feature request related to a problem? Please describe.
For Front End (specially angular) getting different result models is a problem

Describe the solution you'd like
There should be a general result to standardize all reponses.. Simple PaginationResponse does the job but
some result may not require to return data.

Describe alternatives you've considered

Something like this classes and also changes needed in Exception middleware accordingly.

    public class PaginationResponse
    {
        [JsonConverter(typeof(JsonStringEnumConverter))]
        public toast Toast { get; set; }
        public bool HasError { get; set; } = false;

        public string? MessageTitle { get; set; }
        public string? Message { get; set; }
    }

    public class PaginationResponse<T> : PaginationResponse
    {

        public PaginationResponse(List<T> data, int count, int page, int pageSize)
        {
            Data = data;
            CurrentPage = page;
            PageSize = pageSize;
            TotalPages = (int)Math.Ceiling(count / (double)pageSize);
            TotalCount = count;
        }
        public PaginationResponse(List<T> data, toast toast, string message, bool hasError = false)
        {
            Data = data;
            Toast = toast;
            Message = message;
            HasError = hasError;
        }

        public PaginationResponse(string message)
        {
            Message = message;
        }

        public PaginationResponse(toast toast, string message, bool hasError = false)
        {
            Toast = toast;
            Message = message;
            HasError = hasError;
        }

        public List<T>? Data { get; set; }

        public int? CurrentPage { get; set; }

        public int? TotalPages { get; set; }

        public int? TotalCount { get; set; }

        public int? PageSize { get; set; }

        public bool? HasPreviousPage => CurrentPage > 1;

        public bool? HasNextPage => CurrentPage < TotalPages;
    }
@snax4a
Copy link
Contributor

snax4a commented Jun 6, 2022

Has anyone managed to generalize error results?

Currently exception middleware produces this response:

{
    "messages": [
        "Some error message"
    ],
    "source": "FSH.WebApi.Application.Catalog.Brands.CreateBrandRequestHandler+<Handle>d__5",
    "exception": "The exception message",
    "errorId": "d5a86f52-1859-43f4-a4bc-2c862dec5840",
    "supportMessage": "Provide the ErrorId to the support team for further analysis.",
    "statusCode": 404
}

But this does not catch fluent validation ValidationException which produces this response:

{
    "type": "https://tools.ietf.org/html/rfc7231#section-6.5.1",
    "title": "One or more validation errors occurred.",
    "status": 400,
    "traceId": "00-e3eb4e0c174ebed8898e6a3df9c26f2d-106e54ff47355f70-00",
    "errors": {
        "Name": [
            "The Name field is required.",
            "'Name' must not be empty."
        ]
    }
}

It would be nice to have validation error transformed to the ErrorResult

@fretje
Copy link
Contributor

fretje commented Jun 11, 2022

@snax4a

I have actually eliminated the ErrorResult and ExceptionMiddleware, and replaced it with a combination of ProblemDetails Middleware from https://github.com/khellang/Middleware and a "SerilogLoggingActionFilter" (Mvc Actionfilter) to add some action properties and the tenant to the serilog logcontext.

I was planning to make a pr for it, but my codebase has diverged a lot at this point, so making pr's is taking longer. Also I'm still very busy with my daytime job for the foreseeable future and really need to spend the free time I have left AFK ;-)

I can post some code snippits though:

// relevant part of program.cs in the Host project (could also be integrated into infrastructure)
    ...
    builder.Services.AddProblemDetails(options =>
    {
        options.ValidationProblemStatusCode = 400;
        options.Map<Exception>(ex => FSHProblemDetails.Create(ex));
    });

    builder.Services
        .AddControllers(options => options
                .Filters.Add<SerilogLoggingActionFilter>()) // Adds some action properties and the tenant to the log context
            .AddFluentValidation()
            .AddProblemDetailsConventions(); // Adds MVC conventions to work better with the ProblemDetails middleware.

    builder.Services.AddInfrastructure(builder.Configuration);
    builder.Services.AddApplication();

    var app = builder.Build();

    await app.Services.InitializeDatabasesAsync();

    app.UseProblemDetails();
    ...
public class FSHProblemDetails : StatusCodeProblemDetails
{
    protected FSHProblemDetails(int statusCode, Exception ex)
        : base(statusCode)
    {
        Detail = ex.Message;
        if (ex.TargetSite?.DeclaringType?.FullName is { } source)
        {
            Extensions["Source"] = source;
        }
    }

    public static ProblemDetails Create(Exception exception)
    {
        var statusCode = exception is CustomException customException
            ? customException.StatusCode
            : HttpStatusCode.InternalServerError;

        return new FSHProblemDetails((int)statusCode, exception);
    }
}

public class SerilogLoggingActionFilter : IActionFilter
{
    private readonly LoggerSettings _loggerSettings;
    private readonly IDiagnosticContext _diagnosticContext;

    public SerilogLoggingActionFilter(IOptions<LoggerSettings> loggerSettings, IDiagnosticContext diagnosticContext) =>
        (_loggerSettings, _diagnosticContext) = (loggerSettings.Value, diagnosticContext);

    public void OnActionExecuting(ActionExecutingContext context)
    {
        if (_loggerSettings.EnableRequestLogging)
        {
            _diagnosticContext.Set("RouteData", context.ActionDescriptor.RouteValues);
            _diagnosticContext.Set("ActionName", context.ActionDescriptor.DisplayName);
            _diagnosticContext.Set("ActionId", context.ActionDescriptor.Id);
            _diagnosticContext.Set("ValidationState", context.ModelState.IsValid);
            if (context.HttpContext.GetMultiTenantContext<FSHTenantInfo>()?.TenantInfo?.Identifier is { } tenant)
            {
                _diagnosticContext.Set("Tenant", tenant);
            }
        }
    }

    // Required by the interface
    public void OnActionExecuted(ActionExecutedContext context)
    {
    }
}

There's probably more to it... but that's what I can come up with in short notice ;-)

@gkhedekar5758
Copy link

Hey can i take this issue? and work on it?

@fretje
Copy link
Contributor

fretje commented Jun 16, 2022

@gkhedekar5758 sure! I'll gladly review a pr for this ;-)

@gkhedekar5758
Copy link

@fretje thanks a lot sir, I am new to git, I would like to understand more about the problem/requirement here. how can i reach out to someone for clarification. @snax4a @CanMehmetK

@fretje
Copy link
Contributor

fretje commented Jun 17, 2022

@gkhedekar5758 Not sure this is the best issue to get started...

As I understand it, we're actually still in the "design" phase here... If I read it right, @snax4a would like to have all "error" responses as "ErrorResult" (which is a custom object we "rolled our own"), while my suggestion is the other way around... having all "error" responses as "ProblemDetails" (which is the more "standard" way for handling errors in rest api's and for which there actually exists infrastructure within aspnetcore mvc itself).

Actually, if you want to have the error messages shown with their respective controls in the blazor front-end you need to have a problemdetails response (which includes de propertyname with its individual validation messages). If that would be "transformed" to an "ErrorResult", there wouldn't be a way to map the error messages to their respective properties anymore...

@frankyjquintero
Copy link
Contributor

#693 Since we are with this Pr, could we say that the error messages for validation and exceptions could be normalized?

@iammukeshm
Copy link
Member

iammukeshm commented Apr 7, 2023

AddFluentValidation

fixed this by adding validation behavior and catching it at the middleware

image

@iammukeshm
Copy link
Member

Frontend can deserialize the response into ErrorResult if the status code is 400, 500, 404 or similar.

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

6 participants