yahehe / Nancy.Swagger

Nancy plugin for generated API documentation in Swagger format.
MIT License
133 stars 60 forks source link

SwaggerMetadataProvider should call 'SwaggerTypeMapping.GetMappedType' #113

Open DGuidi opened 7 years ago

DGuidi commented 7 years ago

When mapping a model I think TypeMapping should be called, like in SwaggerExtensions.ToDataType

jnallard commented 7 years ago

Can you give some context for the consequences of this issue? It would be helpful for understanding you request.

When you add a mapping, the only thing it should do is change the model that is generated for the type. You might need to make sure the target type is manually added to the ISwaggerModelCatalog.

DGuidi commented 7 years ago

It would be helpful for understanding you request.

Yep, of course. I've a DTO structure like

RootDTO
{
   public DetailsDTO { get; set; }
}

DetailsDTO : List<object>
{

}

I need this structure for my purposes, but this it's outside of swagger specifications, so I added a mapping to a known type SwaggerTypeMapping.AddTypeMapping(typeof(DetailsDTO), typeof(string));

Simply to avoid errors, and using descriptions to describe the actual type.

Result is that SwaggerModule ignores mapping and throws ArgumentException in Type GetType(Type type), because DetailsDTO is actually a container but GetGenericArguments.First() doesn't handle DetailsDTO structure.

yahehe commented 7 years ago

1) Swagger should handle that data structure just fine 2) User specified mappings should definitely override the default without errors, I'll look into this

jnallard commented 7 years ago

Actually, I'm not sure how Nancy.Swagger would handle a List. I assume it would just show a list of empty objects.

The problem is that the base class is the class with the generic arguments, not the class you're actually using. So it is a Container, but when it tries to get the generic arguments for the container, it fails because it's part of the base class. You might be able to solve this if you define your DetailsDTO class as actually a DetailsDTO<T> class.

I originally designed the Mapping feature to basically just change the model that is returned for a type, because that minimized the scope of the change. If we used your suggestion, we would need to change the lines inside this foreach block. We would need to look for any other consequences of the change too. (Not saying that is a bad thing)

DGuidi commented 7 years ago

Swagger should handle that data structure just fine

I see an exception using 1.4.3-stable exactly here, because 'DetailsDTO' it's a container but GetGenericArguments returns an empty array.

User specified mappings should definitely override the default without errors, I'll look into this

I need to specify a thing that I'm missing: I see json generated but Swagger.UI show errors when consuming the generated json. As example: mapping a property to "object" show an error in UI, and I'm not sure if this is a good thing or a bad thing

jnallard commented 7 years ago

@DGuidi Did you read my post?

In regards to your error, can you show me the Swagger json generated causing this issue? Feel free to obfuscate whatever you need to.

DGuidi commented 7 years ago

@jnallard readed after my comment

You might be able to solve this if you define your DetailsDTO class as actually a DetailsDTO class.

fixing DetailsDTO to DetailsDTO isn't praticable, I prefer to have a valid but poorly documented dto. I'd like to fix using type mappings, but you're right: this can introduce consequences.

can you show me the Swagger json generated causing this issue?

Here the gist, not sure this can be useful

jnallard commented 7 years ago

Using this, I'm seeing an error about your responses for your paths. The Response Object should map from a status code to the description object, not from a status text. This may be because of something you're doing or we may have fixed it in later versions.

I'm not sure if that's the same error you're seeing though.

DGuidi commented 7 years ago

This may be because of something you're doing

My code looks ok, I think.

Response(HttpStatusCode.BadRequest, r => r
   .Description("TextStuff"))

maybe is simply something fixed il later versions

jnallard commented 7 years ago

In your JSON, it looks like HttpStatusCode.BadRequest is resolving to "BadRequest" and not 400. Here it is set via using the actual number. https://github.com/yahehe/Nancy.Swagger/blob/1.4.3-stable/samples/Nancy.Swagger.Demo/Modules/HomeMetadataModule.cs

You should make sure your HttpStatusCode.BadRequest is being changed into an int. (I'm not sure why we take a string parameter)


After I fix that, I get an error with this: { "name": "search", "in": "query", "description": "La ricerca da eseguire, valida per tutte le proprietà del DTO", "type": "SearchGroupDTO" }

The type should not be SearchGroupDTO since it's only used for primitives. What does your code look like for this?


I changed that to a string to validate the JSON more.

For some reason your types are String and Int32, when they should be string and integer. What's your code for that too? And you're missing path parameters for your {ambience} and {verticale}

DGuidi commented 7 years ago

your HttpStatusCode.BadRequest is being changed into an int

I'm pretty sure I'm using explicit method that takes HttpStatusCode enum

The type should not be SearchGroupDTO since it's only used for primitives

Ok, clear. the parameter is actually a JSON string of my 'SearchGroupDTO', so I should use 'string'

your types are String and Int32

I'm using typeif(int).Name, now it's clear to me that I shoud use string constants

you're missing path paramaters for your {ambience} and {verticale}

You're right. Thanks

jnallard commented 7 years ago

I'm pretty sure I'm using explicit method that takes HttpStatusCode enum

Your "/v1/{ambiente}/lookup" path prints out 200, but your other path prints out OK for the response key. It seems like you're doing two different things somehow.

I'm using typeif(int).Name, now it's clear to me that I shoud use string constants

How are you setting the types?

DGuidi commented 7 years ago

It seems like you're doing two different things somehow.

yep, I've used integer only in the single declaration you've seen

How are you setting the types?

Describe[controller.GetByIdOpId] = _ => _
                .AsSwagger(with => with
                    .Operation(op => op
                            .OperationId(controller.GetByIdOpId)
                            .Tag(tag.Name)
                            .ConsumeMimeTypes(new[] { "application/json", "application/xml" })
                            .ProduceMimeTypes(new[] { "application/json", "application/xml" })
                            .Parameters(new[]
                            {
                                new Parameter
                                {
                                    In = ParameterIn.Path,
                                    Name = "id",
                                    Required = true,
                                    Type = typeof(string).Name // now changed to "string"
                                }
                            })
                    ));
jnallard commented 7 years ago

You should probably always use the integer for the declarations. I'm not sure why the enum is returning a string.

For the Type property, you can use Primitive.FromType(type).Type or something like that. You can also use that to set the Format too.