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

Nullability with using [FromForm] on a class doesn't work #2157

Closed
quails4Eva opened this issue Jul 5, 2021 · 5 comments
Closed

Nullability with using [FromForm] on a class doesn't work #2157

quails4Eva opened this issue Jul 5, 2021 · 5 comments

Comments

@quails4Eva
Copy link

V6.1.4

When using the [FromForm] attribute for a class parameter, it doesn't seem to correctly apply nullable attributes to the properties e.g. [FromForm] MyClass myObj . Though if you pass an array it does seem to do it correctly (as it uses a reference to the definition) [FromForm] MyClass[] myObjs. Though in this case using an IFormFile property on the array didn't seem to work.

@John-Paul-R
Copy link

Can confirm this is happening. Optional props on a param marked with [FromForm] are correctly excluded in the "required" array in the generated swagger doc, but are not marked with nullable: true.

This causes downstream problems with interpreting the generated swagger doc. (Things like NSwag will treat the props as non-nullable.)

Example:

Here is an example of a controller method and a mock of the generated swagger doc, along with the
expected swagger doc. If you feel it is necessary, I can see about creating a small repo to reproduce, though I think this illustrates the issue fairly clearly.

It is worth noting that the props of this ExampleItemCreateRequest are correctly marked as nullable elsewhere in the swagger doc. It is only when it is used for multipart/form-data that prop nullability is not respected.

C# Controller

// Request Data Obj
public class ExampleItemCreateRequest
{
	// This is correctly required and non-nullable.
	[Required]
	public string Name { get; set; } = null!;

	// This should be nullable in the generated swagger doc, but is not.
	// It is not explicitly required, however.
	public string? Description { get; set; }
}
// Controller
[HttpPost]
public async Task<ActionResult> CreateExampleItem([FromForm] ExampleItemCreateRequest request)
{
	// Process request
}

Generated Swagger

"requestBody": {
  "content": {
    "multipart/form-data": {
	  "schema": {
	    "required": [
		  "Name"
		],
		"type": "object",
		"properties": {
		  "Name": {
			"type": "string"
		  },
		  "Description": {
			"type": "string"
		  }
		},
		"encoding": {
		  "Title": {
			"style": "form"
		  },
		  "Description": {
			"style": "form"
		  }
		}
	  }
	}
  }
}

Expected Swagger

"requestBody": {
  "content": {
    "multipart/form-data": {
	  "schema": {
	    "required": [
		  "Name"
		],
		"type": "object",
		"properties": {
		  "Name": {
			"type": "string"
		  },
		  "Description": {
			"type": "string",
			"nullable": true // This should be nullable
		  }
		},
		"encoding": {
		  "Title": {
			"style": "form"
		  },
		  "Description": {
			"style": "form"
		  }
		}
	  }
	}
  }
}

@John-Paul-R
Copy link

John-Paul-R commented Oct 4, 2021

For those who come after me, an OperationFilter to patch this problem:

using Microsoft.OpenApi.Models;
using Swashbuckle.AspNetCore.SwaggerGen;

public class OptionalFormDataFilter : IOperationFilter
{
	public void Apply(OpenApiOperation operation, OperationFilterContext context)
	{
		if (operation.RequestBody?.Content.ContainsKey("multipart/form-data") ?? false) {
			foreach (var content in operation.RequestBody.Content) {
				var requiredProps = content.Value.Schema.Required;
				var props = content.Value.Schema.Properties;
				foreach (var prop in props) {
					if (!requiredProps.Contains(prop.Key)) {
						prop.Value.Nullable = true;
					}
				}
			}

		}
	}
}

@berhir
Copy link
Contributor

berhir commented Jan 12, 2022

We updated to .NET 6 and the latest version of Swashbuckle and now we are having this issue in multiple places.

I looked at the code and the bug was introduced with this commit 05432a9 ("Only apply nullable, readOnly, writeOnly to Schema properties").
At least the nullable information must be applied to models used with [FromForm] too.
@domaindrivendev can you revert this change partially to fix the issue?

@berhir
Copy link
Contributor

berhir commented Jan 12, 2022

@John-Paul-R Thanks for sharing your solution. I modified it slightly, because in our case, not all fields without a required attribute are nullable.

using Microsoft.OpenApi.Models;
using Swashbuckle.AspNetCore.SwaggerGen;
using System.Linq;

namespace TruckerApp.Web.Common.Swagger
{
    public class SwaggerOptionalFormDataFilter : IOperationFilter
    {
        public void Apply(OpenApiOperation operation, OperationFilterContext context)
        {
            if (operation.RequestBody?.Content.ContainsKey("multipart/form-data") ?? false)
            {
                foreach (var content in operation.RequestBody.Content)
                {
                    var requiredProps = content.Value.Schema.Required;
                    var props = content.Value.Schema.Properties;
                    foreach (var prop in props)
                    {
                        var paramterDescription = context.ApiDescription.ParameterDescriptions.FirstOrDefault(paramDesc => paramDesc.Name == prop.Key);
                        if (!requiredProps.Contains(prop.Key) && paramterDescription != null && paramterDescription.Type.IsReferenceOrNullableType())
                        {
                            prop.Value.Nullable = true;
                        }
                    }
                }
            }
        }
    }
}

@martincostello
Copy link
Collaborator

To make issue tracking a bit less overwhelming for the new maintainers (see #2778), I've created a new tracking issue to roll-up various nullability issues here: #2793.

We'll refer back to this issue from there and include it as part of resolving that issue, but I'm going to close this one to help prune the backlog.

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

4 participants