RicoSuter / NSwag

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

FileResponse instead of FileParameter from WebAPI Post action #692

Open Liero opened 7 years ago

Liero commented 7 years ago

NSwag generates wrong operation definition from following WebApi action:

[HttpPost]
public async Task<IActionResult> Post()
{
    if (!Request.ContentLength.HasValue)
    {
        return BadRequest("null content");
    }

    byte[] array = new byte[Request.ContentLength.Value];

    using (MemoryStream memoryStream = new MemoryStream(array))
    {
        await Request.Body.CopyToAsync(memoryStream);
    }

    return Ok();
}

notice that the generated operation does not specify any parameter, but defines a return schema, which is obviosly wrong

    "/api/file": {
      "post": {
        "tags": [
          "Files"
        ],
        "operationId": "Files_Post",
        "parameters": [],
        "responses": {
          "200": {
            "description": "",
            "schema": {
              "type": "file"
            },
            "x-nullable": true
          }
        }
      }
    }
dgioulakis commented 7 years ago

That's what nswag defaults to when it can't determine a return type, but the method signature's return type is not void or Task. See: https://github.com/NSwag/NSwag/blob/master/src/NSwag.SwaggerGeneration.WebApi/Processors/OperationResponseProcessor.cs

dgioulakis commented 7 years ago

My suspicion is that this is to remain in compliance with the standard: The Responses Object MUST contain at least one response code, and it SHOULD be the response for a successful operation call.

RicoSuter commented 7 years ago

You need to add

[SwaggerResponse(200, typeof(void))]

But I think we also need a new attribute like

[SwaggerParameter("body", ParamterType.Body, typeof(byte[]))]

so that we can define additional parameters (which are not declared in the method signature).

Important: NSwag uses only reflection to generate the spec, it cannot read the procedural code to determine the possible response types and dynamically read parameters.

Liero commented 7 years ago

SwaggerParameter attribute makes sense, but I've already found limitations of attributes, for example:

public abstract class DataControllerBase<TData> : Controller where TData : class 
{
    [SwaggerResponse(typeof(TData)] //does not work
    [SwaggerResponse(404, typeof(void)]
    [HttpGet("{id}")]
    public IActionResult Get(string id)
    {
         TData item = Find(id);
         if (item == null) return NotFound(id);
         Ok(item);
    }
}

Another possible issue:

I've written very simple very simple IInputFormatter that allows me to post application/octet-stream and bind it to Stream:

[HttpPost]
public async Task<IActionResult> Post([FromBody]Stream stream)

I see two issues: how to tell that request content type is "application/octet-stream" and what should be input type in generated swagger document. Currently it defines new type 'Stream'

RicoSuter commented 7 years ago

Try

    [SwaggerResponse(200, typeof(T)]

is this also not working?

grovesNL commented 7 years ago

The generic parameter is also TData not T

Liero commented 7 years ago

Type arguments cannot be used in attributes - it's C# limitation unfortunatelly.

I also tried something like this: [CustomSwaggerResponse(200, () => typeof(T))]

Also not working.

@grovesNL: I've edited code on github manually, my fault.

RicoSuter commented 7 years ago

Maybe we can add the ability to specify a type factory method: Something like:

[SwaggerResponse(200, "GetResponseType") void Foo() {

}

Type GetResponseType() { return typeof(T); }

BlackGad commented 7 years ago

It is conveniently to use nameof(op) operator in situations like this.

also if u control code attributes extractor (NSwag.SwaggerGeneration i suppose) and have all reflection information u could do like this:

 [AttributeUsage(AttributeTargets.Method, Inherited = false, AllowMultiple = true)]
    sealed class SwaggerResponseAttribute : Attribute
    {
        #region Constructors

        //Simple usage
        public SwaggerResponseAttribute(int code, Type responseType)
        {
            Code = code;
            ResponseType = responseType;
        }

        //Internal property/method specification
        public SwaggerResponseAttribute(int code, string propertyName) : this(code, null, propertyName)
        {
        }

        //External static type property/method specification
        public SwaggerResponseAttribute(int code, Type classType, string propertyName)
        {
            if (propertyName == null) throw new ArgumentNullException(nameof(propertyName));

            ClassType = classType;
            Code = code;
            PropertyName = propertyName;
        }

        #endregion

        #region Properties

        public Type ClassType { get; }

        public int Code { get; }
        public string PropertyName { get; }

        public Type ResponseType { get; }

        #endregion
    }

    class ExternalResolver
    {
        #region Static members

        public static Type SomeProperty
        {
            get { return typeof(byte[]); }
        }

        #endregion
    }

    class Controller<T>
    {
        #region Members

        [SwaggerResponse(200, nameof(SomeProperty))]
        [SwaggerResponse(400, typeof(ExternalResolver), nameof(ExternalResolver.SomeProperty))]
        [SwaggerResponse(404, typeof(int))]
        public IHttpActionResult Action(string id = null)
        {
            return null;
        }

        public static Type SomeProperty
        {
            get { return typeof(T); }
        }
        #endregion
    }

    class SwaggerGeneration
    {
        #region Constructors

        public SwaggerGeneration()
        {
            var realControllerType = typeof(Controller<int>);
            var extractedAttributeFromAction = realControllerType.GetMethod(nameof(Action)).GetCustomAttribute<SwaggerResponseAttribute>();
            if (extractedAttributeFromAction?.ResponseType != null)
            {
                //got it
                return;
            }
            if (!string.IsNullOrWhiteSpace(extractedAttributeFromAction?.PropertyName))
            {
                var responseTypeStorageType = extractedAttributeFromAction.ClassType ?? realControllerType;
                var propertyInfo = responseTypeStorageType.GetProperty(extractedAttributeFromAction.PropertyName,
                                                                       BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Static);
                if (propertyInfo == null) throw new InvalidOperationException();
                var responseType = propertyInfo.GetValue(null) as Type;
                //got it
            }
        }

        #endregion
    }
BlackGad commented 7 years ago

If u will use method, u can also pass some parse information to it(controller info, action info etc)

RicoSuter commented 7 years ago

For extreme customization, we also have the SwaggerOperationProcessorAttribute

Liero commented 7 years ago

@ruster: well, you would have to invoke the GetResponseType() method on the controller, which doesn't sound like a good idea.

I would follow System.ComponentModel.DataAnnotation.Validation attribute approach:

Let's SwaggerResponseAttribute inherit from SwaggerResponseAttributeBase, e.g.:

public abstract class SwaggerResponseAttributeBase : Attribute
{
    public abstract Type GetReturnType(OperationProcessorContext context);
    public virtual int GetResponseCode() => 200;
}

then we could easily define our own attributes:

public abstract class DataControllerBase<TData> : Controller where TData : class 
{
    [GenericSwaggerResponse(nameof(TData))
    IActionResult Get(string id)...
}
public class GenericSwaggerResponseAttribute : SwaggerResponseAttributeBase
{
    public GenericSwaggerResponseAttribute(string genericArgumentName)
    {
        GenericArgumentName = genericArgumentName;
    }

    public string GenericArgumentName { get; }

    public override Type GetReturnType(OperationProcessorContext context)
    {
        Type declaringType = context.MethodInfo.DeclaringType;
        Type declaringGenericType = declaringType.GetGenericTypeDefinition();
        if (declaringGenericType == null)
        {
            throw new InvalidOperationException($"'{this.GetType().FullName}' can be used on generic classes only.\n{declaringType.FullName} is not generic type");
        }

        Type genericArgumentDefinition = declaringGenericType.GetGenericArguments()
            .FirstOrDefault(t => t.Name == GenericArgumentName); 
        if (genericArgumentDefinition == null)
        {
            throw new InvalidOperationException($"Generic argument '{GenericArgumentName}' is invalid for type '{this.GetType().FullName}'");
        }

        Type valueOfGenericArgument = declaringType.GenericTypeArguments[genericArgumentDefinition.GenericParameterPosition];
        return valueOfGenericArgument;
    }
}

@BlackGad: when it comes to external resolver, I would also take inspiration from System.ComponentModel.DataAnnotations.CustomValidationAttribute

there's no need to specify both type and method. Just specify type, and the method can be determined by interface, or some naming convention in case of static classes like in the CustomValidationAttribute samle.


I also think this is quite special scenario. You will never be able to cover every possible scenario. What I suggest is to focus on extensibility in future releases, so for example I will be able to replace OperationResponseProcessor with my own CustomOperationResponseProcessor, that inherits from it it. I would just override a method public virtual Type GetResponseType(ICollection<Attribute> attributes, OperationProcessorContext context);

this would allow me to use my own convention on my entire project, so I don't need to specify attributes at all.