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

Support for non mvc web apis #26

Closed
petterhoel opened this issue Dec 15, 2019 · 8 comments
Closed

Support for non mvc web apis #26

petterhoel opened this issue Dec 15, 2019 · 8 comments

Comments

@petterhoel
Copy link

I have used this package to get language specific validation messages in dotnet core web apis. The client is usually a SPA on a separate js-framework app running on a separate domain. We expose the messages from ForEvolve on a custom property on an extended ProblemDetails object. This is handled by error handler middleware.

Dotnet core 3 does not use mvc anymore for these kinds of projects, so we are back to basic error messages provided by Microsoft.

I would love a general approach or custom extension for this use case. Something not scoped to mvc.

As a consumer something that looks something like this:
services.AddForEvolveLocalization(): in place of services.AddMvc().AddForEvolveMvcLocalization();

Would love to know if there are any plans for anything like this?

@Carl-Hugo
Copy link
Member

Dotnet core 3 does not use mvc anymore

What do you mean exactly by that? As far as I know, Asp.Net Core MVC is still there, but they allow developers to cherry-pick the components they load. For example, calling services.AddController() instead of services.AddMvc() does not load Razor Pages, Views, etc. but only the controller-related features.

All of those new, more targetted helpers should still return an IMvcBuilder, so I'm not so sure about what is missing in the MVC pipeline for your use-case to work/not work.

As a consumer something that looks something like this:
services.AddForEvolveLocalization(): in place of services.AddMvc().AddForEvolveMvcLocalization();

As a matter of fact, I should be able to remove that second call for all versions of .Net Core, which is something I should do sooner rather than later (now that I think about it).

Would love to know if there are any plans for anything like this?

I'll answer this one in two parts:

  1. Yes, I want this project to support .Net Core 3, and I did not know that anything was broken until today. The official documentation is pretty much the same as pre-3.0, so I'll have to investigate I guess.
  2. No, I don't have plans for this, but I would not mind looking into it if you can contribute two minimal repro projects that I can test against.

Why two projects?

  1. A project targetting .Net Core 2.1+ (say samples/NetCore2WebApi). Since 2.1+ will reach its end of life on August 21, 2021, I'd like to validate the current state of the library against it first (for regression testing purposes).
  2. A second project targetting .Net Core 3.1+ (say samples/NetCore3WebApi) which is the new LTS version, to implement the required changes for your use cases to work.

As a bonus, those two libraries would become projects that peoples can use to get started.

If you have some more time to invest, please let me know.

@petterhoel
Copy link
Author

petterhoel commented Dec 15, 2019

Dotnet core 3 does not use mvc anymore

What do you mean exactly by that?

Sorry, bad wording on my part. Yes, mvc is still here for sure. But as you say it was required to use it in an api project before. For controllers and routing I believe. Now services.AddMvc() may very well be omitted, and as you describe a bare bones setup might look something like this:

image

services.AddControllers() does indeed return a IMvcBuilder. So does services.AddControllersWithViews(), but System.NulLReferenceException is thrown when chaining with AddForEvolveMvcLocalization();

Glad to hear you want to add support. I'm happy to help by providing minimal repros 😊 But forthcoming week is not looking good for time. I will look into it over the holidays and keep you posted.

Thanks!

@Carl-Hugo
Copy link
Member

but System.NulLReferenceException is thrown when chaining with AddForEvolveMvcLocalization();

Ok, that's a good starting point.

Glad to hear you want to add support. I'm happy to help by providing minimal repros 😊 But forthcoming week is not looking good for time. I will look into it over the holidays and keep you posted.

Perfect, I'm busy too, but with repro projects, it should be easy to fix (I hope)

@petterhoel
Copy link
Author

@Carl-Hugo Happy new year 🥳

Sorry this took a while, but the holidays got the better of me 😴

I have added the sample projects. They reside over at this fork for now.

Starting the 3.1 api will reproduce the NullReferenceException we discussed before.
image

When you have some time, let me know what you'd like to do.

@Carl-Hugo
Copy link
Member

@petterhoel thanks and happy new year to you too!

I'll take a look as fast as possible and come back to you with a more solid ETA.

@Carl-Hugo
Copy link
Member

I looked at your repro projects, and you forgot the first services.AddForEvolveLocalization();. The fixed version is:

public void ConfigureServices(IServiceCollection services)
{
    services.AddForEvolveLocalization(); // <-- was missing
    services.AddControllers()
        .AddForEvolveMvcLocalization();

    services.Configure<ApiBehaviorOptions>(options =>
    {
        options.InvalidModelStateResponseFactory = context => new ValidationProblemDetailsResult();
    });
}

Then, your helpers need to be async; for some reason, you cannot Response.Write() anymore, you must Response.WriteAsync() (in ValidationProblemDetailsResult.cs).

To make it work, I updated your extension to:

public static class HttpExtentions
{
    public static async Task WriteJsonAsync<T>(this HttpResponse response, T obj, string contentType = null)
    {
        response.ContentType = contentType ?? "application/json";
        await System.Text.Json.JsonSerializer.SerializeAsync(
            response.Body,
            obj,
            new System.Text.Json.JsonSerializerOptions
            {
                IgnoreNullValues = true,
                PropertyNamingPolicy = System.Text.Json.JsonNamingPolicy.CamelCase
            }
        );
    }
}

Here is the result in Postman:
image

I'll really try to find the time to reduce the number of calls needed to one, as it can be confusing.


Moreover, app.UseForEvolveRequestLocalization(); already calls app.UseRequestLocalization(localizationOptions);. You can configure RequestLocalizationOptions when adding the dependencies in the ConfigureServices() method, like the following:

public void ConfigureServices(IServiceCollection services)
{
    services.AddForEvolveLocalization(options =>
    {
        var noCulture = new CultureInfo("nb");
        options.RequestLocalizationOptions = new RequestLocalizationOptions
        {
            SupportedCultures = new List<CultureInfo>()
            {
                noCulture
            },
            SupportedUICultures = new List<CultureInfo>()
            {
                noCulture
            },
            DefaultRequestCulture = new RequestCulture(noCulture),
            FallBackToParentCultures = false,
            FallBackToParentUICultures = false,
            RequestCultureProviders = null
        };
    });
    services
        .AddControllers()
        .AddForEvolveMvcLocalization();

    services.Configure<ApiBehaviorOptions>(options =>
    {
        options.InvalidModelStateResponseFactory = context => new ValidationProblemDetailsResult();
    });
}

public void Configure(IApplicationBuilder app, IWebHostEnvironment env)
{
    if (!env.IsDevelopment())
    {
        app.UseHsts();
    }

    app.UseForEvolveRequestLocalization();
    app.UseHttpsRedirection();
    app.UseRouting();
    app.UseAuthorization();
    app.UseEndpoints(endpoints =>
    {
        endpoints.MapControllers();
    });
}

As you most likely know, overriding SupportedCultures and SupportedUICultures will remove all supported languages and replace them with your new value (nb in this case).

Unless you mind having a small array or two in memory, you could also simplify the settings to:

services.AddForEvolveLocalization(options =>
{
    var noCulture = new CultureInfo("nb");
    options.RequestLocalizationOptions = new RequestLocalizationOptions
    {
        DefaultRequestCulture = new RequestCulture(noCulture),
        FallBackToParentCultures = false,
        FallBackToParentUICultures = false,
        RequestCultureProviders = null
    };
});

If there is something else, feel free to reply here so we can reopen the issue or to open a new issue if that's unrelated. 🍻

@petterhoel
Copy link
Author

petterhoel commented Jan 5, 2020

Thanks @Carl-Hugo !

Sorry I missed such a basic thing, but it was not clear to me. On the up side, I guess it motivates an effort to merge services.AddForEvolveLocalization(); and services.AddControllers().AddForEvolveMvcLocalization();into one call.

Re the extention and async issue: I wanted to keep all surrounding code the same and was expecting the async-issue to arise as I've run into it before. The reason is a breaking change where kestrel and iis-servers default setting now is to not allow sync writes. More info can be found here if you are curious.

Thanks again for your time and help ☺️

@Carl-Hugo
Copy link
Member

The reason is a breaking change where kestrel and iis-servers default setting now is to not allow sync writes. More info can be found here if you are curious.

I encountered that while upgrading .Net Core 2.2 apps to 3.0 as well, and assumed it was a breaking change. Thanks for the link, I'll take a look at it (I like to stay informed, but there is so much to keep up to date about that it's easy to miss some stuff out).

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

2 participants