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

[NancyFx] Inheritance #7079

Open
craffael opened this issue Nov 30, 2017 · 2 comments
Open

[NancyFx] Inheritance #7079

craffael opened this issue Nov 30, 2017 · 2 comments

Comments

@craffael
Copy link
Contributor

Description

Currently the Nancy Generator does not support inheritance at all. I'll briefly discuss the current status of nancy w.r.t. inheritance based on the simple Pet/Cat model found further down.

Models:

The models inherit correctly from each other:

public class Pet:  IEquatable<Pet>{...}
public sealed class Cat: Pet,  IEquatable<Cat>{...}

However, the builder of the Cat currently doesn't allow setting the Pet properties (with immutable=true):

public sealed class CatBuilder {
  public CatBuilder HuntingSkil(string value) {...}
  public Cat Build() {...} 
  ... private members....
}
Deserialization:

The /pet endpoint deserializes subtypes (such as a Cat) currently wrong, i.e. it just serializes them as a Pet and ignores all additional properties of Cat:

Post["/pet"] = parameters =>
{
    var body = this.Bind<Pet>();
    Preconditions.IsNotNull(body, "Required parameter: 'body' is missing at 'PetPost'");
    
    return service.PetPost(Context, body);
};
Serialization:

Serialization works out of the box. That is a Cat is serialized as a Cat. The only problem is, that the discriminator is not automatically set (and because of the problem with model described above, it can in fact not be set at all).

Swagger-codegen version

2.3-Snapshot

Swagger declaration file content or url

Here is the swagger file that I have used above to describe the current status:

swagger: "2.0"
info:
  version: "1.0.0"
  title: Demo


paths:
  /pet:
    post:
      summary: "post a bar"
      parameters:
        - name: body
          in: body
          required: true
          schema: 
            $ref: '#/definitions/Pet'
      responses:
        201:
          description: successful.
          schema:
            $ref: '#/definitions/Pet'
  
          
definitions:
  Pet:
    discriminator: type
    type: object
    required:
      - type
    properties:
      type:
        type: string
      name:
        type: string
        
  Cat:
    allOf:
      - $ref: '#/definitions/Pet'
      - properties: 
          huntingSkil:
            type: string

Command line used for generation

-l nancyfx --additional-properties packageContext=v1,interfacePrefix=I

Related issues/PRs

I have not found a related issue/PR.

Suggest a fix/enhancement

I would be interested very much to fix this problem but I was hoping to get a bit of feedback before I start with it (@jimschubert ?, @mandrean ?).

Especially the deserialization issue is not so easy. At the moment I'm considering two possible solutions but maybe there are even better ideas...:

  1. Rely on json.net and the nancy integration. This seems to be the industry standard for c# and is also used by the Restsharp swagger client. However it would be an additional dependency. This was also suggested by the folks from the #nancyfx slack channel.
  2. Build a custom ModelBinder for every type with a discriminator. This will probably be a bit slower than the json.net implementation but it would avoid the introduction of an additional dependency.

In order to solve the problem with the model builder, I suggest to use the approach suggested here.

@jimschubert
Copy link
Contributor

jimschubert commented Dec 19, 2017

@craffael It sounds like most of the issue is in the implementation of the NancyFX server generated code, which I'm not that familiar with.

I'd recommend if you take this up, sticking with the JSON.net route. This is likely going to be affected by similar to #6969 (which was merged into master about a week before you opened this PR), and #7043 (if you're targeting 3.5).

I've had some computer issues over the last few weeks, but I'm back in business and I could try to assist if you get stuck. I have a friend who is somewhat familiar with NancyFX, and I may be able to lean on him a bit for advice.

@craffael
Copy link
Contributor Author

@jimschubert Thanks for the helpful pointers. Especially PR #6969 should be very helpful. I think .Net 3.5 is not needed as it is the nancy generator supports only .net 4.5 so far.

I will look into the implementation this week and can hopefully already make a proposal by the end of it. Also good to know that you're computer is working again ;)

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