Codit / practical-api-guidelines

Practical guidelines for building & designing APIs with .NET.
MIT License
16 stars 5 forks source link

Produces attribute #103

Open MassimoC opened 5 years ago

MassimoC commented 5 years ago

You could include the type that is returned by this operation as well (typeof(CarDto[])). Actually, my take would be to specify a Produces attribute that covers the response of a success-scenario, and specify a SwaggerResponse attribute for all other scenario's that could occur (the non-successfull statuscode that can be returned by the operation).

_Originally posted by @fgheysels

        /// <summary>
        /// Get all cars
        /// </summary>
        /// /// <param name="bodyType">Filter a specific body Type (optional)</param>
        /// <remarks>Get all cars</remarks>
        /// <returns>List of cars</returns>
        [HttpGet(Name = Constants.RouteNames.v1.GetCars)]
        [SwaggerResponse((int)HttpStatusCode.OK, "List of Cars")]
        [SwaggerResponse((int)HttpStatusCode.InternalServerError, "API is not available")]
pietersap commented 5 years ago

I don't know if adding the [produces] attribute has much value. Even with the XML formatter installed, the controllers will default to JSON in no "accept" header is included.

Only if the user requests XML explicitely via the header (Accept = Application/XML), this will make a difference. In that case, the [produces(application/json)] attribute will make sure that the controller refuses to send XML regardless the header and will... -return a 406 status code (if ReturnHttpNotAcceptable = true in the MVC configuration) -return Json (if ReturnHttpNotAcceptable = false in the MVC configuration). I don't see why we would force the User to receive a Json, if the objects that we return can as well be returned as an XML.

In General, I would suggest setting ReturnHttpNotAcceptable = true (assuming that if a user does the effort of requesting only XML, he knows what he's doing) and installing the XML formatter. In this case, the user can specificy himself which format he wants. In rare cases, we could enforce JSON with the [produces] attribute, but I would not recommend this in general. It could maybe help if we know that the returned objects cannot be serialized to XML.

About XML formatting, I have also noticed that all objects appear to be formatted to XML pretty well with the XML formatter installed. This includes datetimes and arrays/lists of custom objects. One important note is this: in the Dto objects, the lists/arrays must be intialized as lists/arrays, and not the base interface ICollection. In other words, in CarDetailsDto, we must replace...

public ICollection<CustomizationDto> Customizations { get; set; }
                = new List<CustomizationDto>();

with

public List<CustomizationDto> Customizations { get; set; }
                = new List<CustomizationDto>();

Ohterwise, requesting application/xml in the accept header will result in a 406 (ReturnHttpNotAcceptable = true) or JSON (ReturnHttpNotAcceptable = true)

MassimoC commented 5 years ago

I don't know if adding the [produces] attribute has much value. Even with the XML formatter installed, the controllers will default to JSON in no "accept" header is included.

Produces attribute should be discussed in the context of the Accept header because the guideline could be "if you are using content negotiation, do not add the produces attribute", on the other hand we need to check whether there is a way to automatically generate the produces value in the swagger file based on the Accepted media type.

I don't see why we would force the User to receive a Json, if the objects that we return can as well be returned as an XML.

The guideline should indicate what we advise to return and what no. For example if the API returns JSON, always support XML. In other cases (e.g. csv, png), we should remove the default outbound formatters? what is the guideline for the error propagation? etc...

tomkerkhove commented 4 years ago

This is actually mixed with #86 for produces, however SwaggerResponse should be added to maturity level one to document the response type.

tomkerkhove commented 4 years ago

@filipvanraemdonck is working on this. Pending invite to assign to him.