saaratrix / asp.net-core-mvc-localized-routing

The Unlicense
11 stars 4 forks source link

Upgrade to ASP.NET Core 3.0 #36

Open saaratrix opened 5 years ago

saaratrix commented 5 years ago

ASP.NET Core 3.0 introduces Dynamic routing by using MapDynamicControllerRoute so hopefully the whole solution can be rewritten using that instead of the current one with creating new routes based on the attributes.

So the new solution would hopefully work like this:

Implementation using dynamic routing can be seen here, but they don't use attributes inside the controllers. https://www.strathweb.com/2019/08/dynamic-controller-routing-in-asp-net-core-3-0/

Also by using the new MapDynamicControllerRoute areas will hopefully work. Hopefully also the TagHelper can go away because I assume the MapDynamicControllerRoute will solve that automatically.

Finally also add default functionality for issues reported by others, such as #29,

So if everything goes well the whole solution should be much simpler and work better for 3.0,

saaratrix commented 4 years ago

MapDynamicControllerRoute turned out to be a seemingly nice feature but also one not working fully as I expected it to.

It doesn't work well out of the box with parameters. {culture}/{controller}/{action}/{id?} and then if you have an action with 2 parameters it doesn't trigger because it's 2 parameters instead of 1.

This solves that issue, but now we don't have the names of the parameters which we need to pass into the route. So now the convention needs to get the parameters from the action & controller models. {culture}/{controller}/{action}/{params*}

So many edge cases to solve but I think it's still better than manually constructing all the extra routes. But originally I was hoping many of the features could go away and be solved by .net core 3.0, but that doesn't seem to be the case.

saaratrix commented 4 years ago

Need to rework how the localization attributes work to solve this use case with the dynamic map routing: route/{param1}/otherRoute/{param2}

BrightSoul commented 4 years ago

@saaratrix Thank you for these comments, I managed to create a demo project based on MapDynamicControllerRoute. I link it here in case you want to give it a look and maybe give me some opinion. https://github.com/BrightSoul/aspnetcore-localization-demo

My localizations are in resource files (both content and URL localizations).

It was both physically and mentally challenging to create such a project because of maaany obstacles I found along the way. And I'm not too satisfied with the result. First of all, there's an issue with dynamic controller routes which prevents link generation. https://github.com/dotnet/aspnetcore/issues/16965

In addition to the dynamic controller route, I had to configure a "regular" controller route. It's pretty ugly that way and I'm looking for better solutions to work around the issue. And finally, I provided my own implementation of a LinkGenerator which wraps the DefaultLinkGenerator so I could localize route values before links are actually generated. Of course, I couldn't derive from DefaultLinkGenerator because it's an internal sealed class.

There were so many road bumps... I wish there was one single extensibility point I could use for localizing both incoming requests and url generation.

saaratrix commented 4 years ago

Hi @BrightSoul your message was a pleasant surprise :) And it is indeed many obstacles to overcome. It's been quite some times since I took a break and never got the motivation to continue with the 3.0 Upgrade. I got stuck on handling parameters and GET/POST etc, in your demo I can't find any parameter so maybe you haven't even tried to solve that? Or maybe it just works from the re-mapping you're doing which would be very nice!

But as far as the solution go it seems to be much less code than mine and if it's easy to edit inside the resource file I think it's probably better. The benefit of the attributes approach was that you could edit it directly in the controller.

If it solves the parameters I think it's a very neat solution and if I resume the 3.0+ upgrade I will be looking at your solution because I like the rewrite concept for the link generator.

One thing that might simplify the CreateReverseMap method might be to either store the controllerModels somewhere or maybe ASP.NET allows you to fetch them instead of using Assembly & Reflections. Also I think your Actions ForEach loop can throw an exception if you have a GET & POST method since you're just doing Dictionary.Add();

If you don't mind I can add a link to your repository on the landing page of this repository as a 3.0 solution using resource files?

BrightSoul commented 4 years ago

Hello there and thanks for your reply! Maybe we'll be able to crack this together :D

If you don't mind I can add a link to your repository on the landing page of this repository as a 3.0 solution using resource files?

Yeah, sure!

Also I think your Actions ForEach loop can throw an exception if you have a GET & POST method since you're just doing Dictionary.Add();

Good catch, it would indeed throw an exception.

One thing that might simplify the CreateReverseMap method might be to either store the controllerModels somewhere or maybe ASP.NET allows you to fetch them instead of using Assembly & Reflections.

Now that you make me think of it, I could just leverage the IActionDescriptorCollectionProvider. No need to use reflection at all. I'll update the project soon enough.

I got stuck on handling parameters and GET/POST etc, in your demo I can't find any parameter so maybe you haven't even tried to solve that?

Not yet, this was only a first attempt. I'll do a few tests though.

I'll keep you updated!

BrightSoul commented 4 years ago

Ok, I got rid of the reflection code. The IActionDescriptorCollectionProvider worked nicely. https://github.com/BrightSoul/aspnetcore-localization-demo/blob/9e1b03988b6842bf07a4a4c0d73db1b4079fcd87/Models/Localization/LocalizationTransformer.cs#L58

saaratrix commented 4 years ago

Great and hopefully your tests with GET & POST requests turn out well. Otherwise you said it best yourself :)

It was both physically and mentally challenging to create such a project because of maaany obstacles I found along the way.

BrightSoul commented 4 years ago

@saaratrix I updated my demo, GET & POST seem to be working fine, even though I've not yet added proper unit/integration tests. In this revision, I added support for localized query parameter names since they're visibile to the user in the address bar.

capitolo3

Basically, you just create a link as usual, with as many asp-route-* attributes as needed.

<a asp-route-comments="1">@localizer["Comments.Show"]</a>

The name comments will be localized by the DefaultLinkGenerator wrapper if the resource file has an entry like this.

<data name="Query.comments" xml:space="preserve">
  <value>commenti</value>
</data>

Input field names inside GET forms are not translated though, since they don't go through the DefaulLinkGenerator and this might be an issue for search forms.

I added a small commenting section I used to test POST requests. In my opinion, POST keys should not be localized since they're not visibile to the user anyway.

If we really wanted to localize input field names, I guess one option would be to create a custom tag helper for asp-for.

The demo is still lacking Areas & Razor Page support.

saaratrix commented 4 years ago

@BrightSoul Nice :) Well done!

I do remember having to do something special for forms but that was with a different technique so hopefully you don't have to. And I agree, there is no need to localize POST requests and input field names are also hidden to a user unless a GET form adds it but that seems like a very niche use case. At least I haven't used a GET form personally in a long time.

If you do write tests I recommend integration tests because then you can actually test the final value and you can debug all your classes to see the input & output values. It was very useful when I was working on 2.0.