Nancyfx codegen (C#)

[mstefaniuk]

Here you have codegen for .NET NancyFX framework. It generates code for library that should be shared as compiled artifact (NuGet). So there is no tests but Nancy REST module, service interface and simplified abstract implementation. It is heavily tested within private projects.

[jimschubert]

@mstefaniuk

To your point about sample code, that's not a common approach in many open source projects (including Swagger Codegen). Samples are provided as part of the github repository, and generated by bash scripts. They're not only useful for users to see the generated output, but are also used to programmatically check the validity of the generated samples. For .NET generated sources intended to only run on Windows, we wouldn't programmatically execute them in non-Windows environments but maintainers do run bash scripts and compile the generated code on non-Windows environments to quickly verify changes. The samples aren't included in compiled binary output. I don't disagree with you, though. I think a separate location for examples could be a cleaner approach for many projects, and certainly wouldn't clutter source with auto-generated code.

The reason to include a build.sh and build.bat file is because not every developer is using Visual Studio with integrated NuGet. Some developers (like myself) use Xamarin on OS X, in which only the beta version 6.0 currently automates NuGet package management. XBuild and MSBuild can build .sln and .csproj directly, but they have no knowledge of NuGet. In addition, many developers on Windows are now switching to Visual Studio Code as a lightweight and less memory intensive alternative to Visual Studio. I haven't checked version 1.2 of Visual Studio Code, but I believe version 1.1 hasn't yet integrated NuGet. Build scripts would help automate this NuGet restoration process. For example, the C# Client batch file downloads NuGet, references the dependencies, and compile sources.

I agree with your last point about moving any bootstrapper helper to a future addition. Once this PR is merged, I think we should create an issue to track that. I think the use case you presented would be helpful in a README.md output with the generated source.

[wing328]

@mstefaniuk it's definitely a good start 👍

I'll merge tomorrow (Tuesday) and also share my views on some of your questions/feedback above.

[jimschubert]

@mstefaniuk I also agree, it's a great start.

When I created the aspnet5 generator, I was originally going to contribute a nancyfx generator against vNext but decided against it because things were in such flux that I could generate a server using the aspnet yeoman generator one day and it would work then generate the same thing three days later and it wouldn't!

Somewhat unrelated to the PR, a friend of mine wrote a sort of dynamic REST api abstraction on top of NancyFX. gwely/Napi if you're interested in checking it out.

[wing328]

@jimschubert thanks for reviewing the change.

@mstefaniuk thanks for the PR.

About enum naming, I agree yours is better. The one you see in C# is done by the one who contributed the enum support in C#. I'll give that contributor a heads-up before switching to yours.

[jimschubert]

@wing328 no problem.

I don't think we can easily switch from the Enum suffix to this implementation, although I agree @mstefaniuk has a better implementation. The reason is that changing ColorsEnum to Colors will be a breaking change for all consumers of the C# client. I think changing the default to be @mstefaniuk's implementation and maintaining an option for an enum prefix/suffix would still be a breaking change, but at least there'd be a workaround for teams generating the C# client as an SDK for their APIs.

[wing328]

One fallback I can think of is to use vendor extension (e.g. x-csharp-enum-prefix, x-csharp-enum-suffix) and developers can then customize the enum prefix/suffix per property/parameter/class

[jimschubert]

That's a great idea.

[jchannon]

NancyFX core contributor here - what is Parameters.ValueOf doing. If trying to get the username from the querstring then we have different syntax to do that.

[wing328]

@jchannon thanks for reviewing the code (and sorry for late reply as I missed your question)

I think it's trying to obtain username from the path. e.g. if path is /user/wing328, then username will store "wing328". In other words, it's not a query string.

[jchannon]

You could do parameters.username to get that value. I assume your Parameters.ValueOf is some wrapper around it all. Not sure why but will assume you have a good reason

[wing328]

@jchannon I'm not the author so I don't know the exact reason.

My guess is that username can be of different type (defined in swagger spec), e.g. date, double, string, etc and that's why it needs to call Parameters.ValueOf<datatype> to cast the value into the type defined in the spec.

Ref: https://github.com/swagger-api/swagger-codegen/blob/master/modules/swagger-codegen/src/main/resources/nancyfx/api.mustache#L29

[razzeee]

A basic README.md would have been great :/

[razzeee]

I really struggle to understand how this is supposed to work, I know I need to implement either the interface or the abstraction. But how do I handle errors and return my error message using this generated code?

[wing328]

@simo9000 please submit a PR to update Nancyfx to the latest version if you've not done so.

[wing328]

@razzeee I agree with you that we should add a README for Nancyfx (just like other auto-generated code)

But how do I handle errors and return my error message using this generated code?

I think this is more of a question for Nancyfx itself as swagger codegen simply generates the code and you can then perform the error handling with respect to Nancyfx best practices.

[razzeee]

@wing328
Well it really seems like I need to find an example implementation.
It's quiet weird to me, that the server generates the code to answer 200 responses but not any 400+ responses I have defined in my swagger config. That's just where I stop to understand it :(

[jimschubert]

@razzeee This generator doesn't currently support non-200 response types. This PR is the initial implementation to get the stubbed server generated code.

For example, the petstore.yaml used to generate the sample has the following PUT operation on /pets:

 put:
      tags:
        - pet
      summary: Update an existing pet
      description: ''
      operationId: updatePet
      consumes:
        - application/json
        - application/xml
      produces:
        - application/xml
        - application/json
      parameters:
        - in: body
          name: body
          description: Pet object that needs to be added to the store
          required: true
          schema:
            $ref: '#/definitions/Pet'
      responses:
        '400':
          description: Invalid ID supplied
        '404':
          description: Pet not found
        '405':
          description: Validation exception
      security:
        - petstore_auth:
            - 'write:pets'
            - 'read:pets'

The generated code is:

Put["/pet"] = parameters =>
{
    var body = this.Bind<Pet>();
    Preconditions.IsNotNull(body, "Required parameter: 'body' is missing at 'UpdatePet'");
                
    service.UpdatePet(Context, body);
    return new Response { ContentType = "application/xml"};
};

It looks like this is handling:

• Parameters (including required checks)
• produces
• consumes (implicitly via NancyFX framework)

Things missing from the swagger definition:

• Summary
• Description
• operationId
• Custom responses
• Security

We could add summary, description, and operationId to the generated code. Custom responses and security are implementation details which may be difficult. Using the example above:

• 400: Invalid ID supplied is a requirement check that would require the generator to have follow some contextual clues in the response message and generate code to inspect the posted body for an invalid id parameter, where "invalid" isn't documented.
• 404: Pet not found would require knowledge of how a pet is found by the service implementation. This would be difficult with our logicless templates.
• 405: Validation exception is a non-standard API response for a validation exception. 405 is used for "Method Not Allowed" and doesn't really make a lot of sense in the example swagger. This would also require contextual knowledge of the implementation.

Because these things are all contextual and based on your domain/implementation, it would be difficult to "get it right" from a generator's perspective. There are things that could be done by following community standards or best practices. For instance, a common set of exceptions could be generated to be thrown by services, and a global exception handling mechanism could respond with those 4xx and 5xx exceptions.

A user of the swagger generation tool can add rules to .swagger-codegen-ignore to prevent existing files from being overwritten on future generations.

I hope that answers some of your questions or concerns.

[razzeee]

@jimschubert
yes, that really helps.
what I've always done before is returning a Nancy Response and Package the Result good or bad into it, but I don't see how I could do this with the generated code. Or how I would profit from the generated code then.

I'm not whining that the generator doesn't generate error responses. I'm just wondering how I need to implement them in a nice way using this.

[jimschubert]

@razzeee I definitely didn't think you were whining ;). Do you have a project on Github that follows your pattern? I'm wondering if there's a way to modify the existing generator to account for your code goals. I have pretty limited experience with NancyFX, so I'm not familiar with what's considered a best practice.

Also, it's possible for you to edit the templates to meet your needs, then specify the custom templates when you run the CLI. Is that something you'd be willing to try? This way, you should be able to get the Response output as you posted and have the files structured in a way you're more familiar with.

[simo9000]

@wing328 see #4482 for nancy update

[razzeee]

@jimschubert
Unfortunatly nothing of the work I've done with nancy is public :/
We might be able to just switch out the concrete types with dynamic like this implementation does for e.g. https://github.com/shawty/nancyfxbook/blob/master/NancyFxBook/Modules/MP3PlayerRoutes.cs#L73
That should be backwards compatible with the current implementation, in theory. It might not be as straight forward, but that's something that we could address in the readme.

Then the content negotiation of nancyfx, should handle whatever you throw it's way.
See: https://github.com/NancyFx/Nancy/wiki/Defining-routes#action

The response can be any model and the final result will be determined by Content Negotiation. However, if it is of the type Response then content negotiation will be ignored and the response will be sent straight back to the host.

I'll have a look if I can modify the generator in a way to do that, and create a petstore example.

[razzeee]

Here is a draft https://github.com/Razzeee/swagger-codegen/tree/nancyfx-dynamic
I will try to do some tests in the next days

[mstefaniuk]

@razzeee In NancyFX you can handle exceptions and return error JSON messages. You can add OnError handler do IPipelines in NancyFX bootstrap. We are using following handler:

((context, exception) =>
  {
    var response = context.Response ?? new Response();
    var result = mapper.Handle(context, exception);
    response.Write(result);
    if (!string.IsNullOrEmpty(response.ContentType))
    {
      response.Headers.Put(HttpHeaders.ContentType, response.ContentType);
    }
    return response;
})

In above code mapper is simply mapping exception to a proper handler. Particular handler generates error response as follows:

namespace Commons.Nancy.Error.Handlers
{
    public class PreconditionFailedHandler: ExceptionHandler
    {
        public ErrorResponse Handle(NancyContext context, Exception e)
        {
            var exception = (PreconditionFailed) e;
            var body = new ErrorMessage(exception.Code, exception.Message);
            return new ErrorResponse(HttpStatusCode.PreconditionFailed, body);
        }
    }
}

I'm not sure what can dynamic approach help in error handling. Typed is much better. Maybe we will open our exception handling commons for NancyFX to make it easier.

[razzeee]

@msolujic
sounds nice, but should ideally be created by the swagger codegen, as it's probably the best place for it, right?
I will try to test what you suggested, as I would prefere to have concrete types for each endpoint and just handle errors like you suggested.

Edit: Ah, you said bootstraper, so it might be hard to get it into codegen :/

[simo9000]

@razzeee

Edit: Ah, you said bootstraper, so it might be hard to get it into codegen :/

Actually it is not hard at all. Nancy is designed to allow easy subclassing of the bootstrapper. The codegen for nancyfx doesn't currently have a bootstrapper.mustache but it would be very easy to add one in a PR.

I agree with @msolujic that exception handling is a preferred means to dynamic return values. I would suggest making a SwaggerException with a field for the response code and then using the OnError pipeline handler to convert the exception to JSON as @msolujic indicates.

[wing328]

I would suggest making a SwaggerException with a field for the response code and then using the OnError pipeline handler to convert the exception to JSON as @msolujic indicates.

For API clients generated by Swagger Codegen, there's usually an ApiException class. The API server may also want to use ApiException to deliver a more consistent developer experience.

[razzeee]

Well I'm toying with this right now

        protected override void RequestStartup(TinyIoCContainer container, IPipelines pipelines, NancyContext context)
        {
            pipelines.OnError.AddItemToEndOfPipeline((ctx, ex) =>
            {
                DefaultJsonSerializer serializer = new DefaultJsonSerializer();
                Response error = new JsonResponse(ex.Message, serializer);

                if (ex is ArgumentException)
                {
                    error.StatusCode = HttpStatusCode.PreconditionFailed;
                }
                else
                {
                    error.StatusCode = HttpStatusCode.InternalServerError;
                }
                
                return error;
            });

            base.RequestStartup(container, pipelines, context);
        }

But it's not as elegant as I would like it.
If you want to specify an "error code" like "No content", you need to first find a exception that you can "abuse" and map to that one. (given that there really is no exception named no content)
Well I could decide by exception payload, but meh :/

[mstefaniuk]

@razzeee We considered to provide error handling within codegen but it would force users to use particular implementation of that. Those services can be part of bigger solution so it would be wrong decision.

I'm not sure what mean to abuse exception but you can simply create your own exceptions as we created PreconditionFailed thrown by precondition checker. You can simply build a map (dictionary in C#) with keys as exception classes and values as response providers. We use only two fields (code and message) but you can add more for specific ones.

[razzeee]

That might work nicely. Thanks!
I understand the rational for not having it in codegen, the real problem for me is, the lack of any documentation on any of this topics. Would be really good to have some good pointers.

[simo9000]

@razzeee @mstefaniuk what might be a good compromise is to create a bootstrap.mustache and an apiexception.mustache to forward exceptions as JSON with the response code. Then to ensure backward capability, create a generator option which, if true, will conditionally add the onError pipeline handler. This handler should check to see if the exception thrown is of type apiexception prior to sending the serialized version to the client. The above mentioned generator option can default to false which will preserve the current behavior.

[mikob]

Building on @razzeee 's points, I'm not understanding how to get the nancy swagger auto generated code running and some documentation would save everyone some frustration. It seems @razzeee was finally able to get it running, but I'm still stuck. The wiki doesn't have any useful information about using the auto generated code. I will happily put in a pull-request with an update to the docs if I can get the instructions straight.

Here's what I have so far (of course I haven't gotten it working yet so any help would be greatly appreciated):
The autogenerated nancy code is intended to be installed as a library, so the autogenerated code can be updated without overwriting any existing code you may have.

1. Make an nuget package using the nuspec file or solution file included in the autogenerated zip.
   eg. nuget pack IO.Swagger.nuspec
2. Install the package in your existing project or a new project. To quickly generate a new boilerplate project, you can use the yeoman generator dotnetcore-nancy.
3. ???
   I can't see my routes here, so it seems something else needs to be configured
4. Test that the routes are working. You can enable Nancy diagnostics and check which routes it has registered by adding this to your bootstrap constructor:

For Nancy-2.0.0

 public override void Configure(Nancy.Configuration.INancyEnvironment environment)
{
    environment.Diagnostics(password: "secret");
}

Then accessing /_Nancy/ on your locally running server -> interactive diagnostics -> route cache -> getAllRoutes.

5. Implement the services that are stubbed and referenced in modules??

[mstefaniuk]

@mikob Generated code contains Nancy module so it is autowired by IoC bootstraper.

Please look at UserModule from PetStore sample. There is UserModule that deploy routes, UserService interface for logic implementation that gives access to NancyContext and AbstractUserService that simplifies implementation of the previous service. When you implement interface or abstract class that implements logic of module everything should work without hassle.

I'm not sure how it is related but generated code was focused on Nancy 1.4.1 and you mention about version 2.0.0.