RicoSuter / NSwag

The Swagger/OpenAPI toolchain for .NET, ASP.NET Core and TypeScript.
http://NSwag.org
MIT License
6.7k stars 1.24k forks source link

Route prefixing does not appear to work for dotnet core #1386

Open DannyRyman opened 6 years ago

DannyRyman commented 6 years ago

We are attempting to use SwaggerToCSharpControllerGenerator to generate a dotnet core compatible web api. Unfortunately we have a slight problem with route prefixing. The generator is generating the code [routeprefix('v1')] whereas, I believe that using dotnet core this needs to be [route('v1)]

RicoSuter commented 6 years ago

Did you enable the IsAspNetCore setting?

DannyRyman commented 6 years ago

Thanks for your help. Unless I am misunderstanding, I think you misread my question. I am trying to do contract/schema first development using the SwaggerToCSharpControllerGenerator. As I understand it, the IsAspNetCore setting is used when wiring up swagger to your web api on startup? My problem is that the code generated by the SwaggerToCSharpControllerGenerator does not build because it uses routeprefix which is not valid for dotnet core projects, and there does not appear to be a configuration option to change this?

RicoSuter commented 6 years ago

Sorry, the setting does not exist and RoutePrefix is always used: https://github.com/RSuter/NSwag/blob/master/src/NSwag.CodeGeneration.CSharp/Templates/Controller.liquid#L28

cromo commented 6 years ago

Our team is running in to this as well, and we're using NSwag.MSBuild to automate the generation of the controller code. We're working around this issue by running a string substitution on the result via powershell, e.g.

  <Target Name="NSwag" BeforeTargets="Build">
    <Exec Command="$(NSwagExe_Core21) run swagger.nswag" />
    <Exec Command="powershell -Command &quot;(Get-Content LoggingController.cs).replace('RoutePrefix', 'Route') | Set-Content LoggingController.cs&quot;" />
  </Target>

That said, there's interest here to make this work out of the box. How involved would that be? The change to the liquid template looks minor, but I haven't looked at how much supporting code would need to be changed nor accounted for testing nor NSwagStudio.

altso commented 6 years ago

Another way to make it work is to define RoutePrefixAttribute:

// ReSharper disable once CheckNamespace
namespace Microsoft.AspNetCore.Mvc
{
    public class RoutePrefixAttribute : RouteAttribute
    {
        public RoutePrefixAttribute(string template)
            : base(template)
        {
        }
    }
}
RicoSuter commented 6 years ago

I think we should collect all missing stuff and then decide how to proceed.

Simplest way is to remove the namespace setting and add isaspnetcore instead and handle this in a single tpl.

Another option is to have two or four tpls (aspnet/aspnetcore, partial/interface). But this means lots of duplication.

cromo commented 6 years ago

I think that adding an flag to specify ASP.NET Core would save a lot of headache when generating code for it. This is one of the most painful parts to work with right now because they require workarounds, but including other ASP.NET Core needs into a single flag would tidily wrap up all our wants in one package. (The other big one we want is #1547 mentioned above, which has similar workarounds.)

I've only glanced at the template files, but the changes required for ASP.NET Core are not drastic - most of them are adjusting namespaces and altering a few identifiers here and there. The main structure is the same, though I can understand wanting to split templates to keep them concise.

RicoSuter commented 6 years ago

It’s not just asp.net core but also a version maybe, see #1547 which is only available in core 2.1+...

cromo commented 6 years ago

True, ActionResult<TValue> was added in 2.1. That may further complicate a compound template. In general, I'd prefer the complexity of a single template (up to a point) over having to maintain multiple templates in parallel. Though maybe there's a way to split out the differences into different partials that get included in the same skeleton, reducing duplication while still splitting out the details?

altso commented 6 years ago

@RSuter Please note that action method could also return IActionResult since version 1.0 and it's also not supported.