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

Add support for C# 11 'required' modifier. #2555

Open
joel1st opened this issue Nov 19, 2022 · 8 comments
Open

Add support for C# 11 'required' modifier. #2555

joel1st opened this issue Nov 19, 2022 · 8 comments
Labels
help-wanted A change up for grabs for contributions from the community

Comments

@joel1st
Copy link

joel1st commented Nov 19, 2022

Now that C# has inbuilt language support for required properties, it might be worth discussing how we can take advantage of this with SwashBuckle to provide more accurate, streamlined API documentation.

I think the below comment covers my thoughts on this well:
Since nullablility and required/not are different things where one does not necessarily imply the other, I think SwashBuckle should use the C# property nullability for swagger property nullable, and C# property required/not for the swagger property required.

@flostellbrink
Copy link

flostellbrink commented Dec 11, 2022

This did the trick for me:

public class SwaggerRequiredMemberFilter : ISchemaFilter
{
	public void Apply(OpenApiSchema schema, SchemaFilterContext context)
	{
		var requiredProperties = context.Type.GetProperties().Where(p => p.HasAttribute<RequiredMemberAttribute>());
		var schemaPropertyNames = schema.Properties.Keys.Where(
			sp => requiredProperties.Any(rp => string.Equals(sp, rp.Name, StringComparison.InvariantCultureIgnoreCase))
		);

		foreach (var property in schemaPropertyNames)
		{
			schema.Required.Add(property);
		}
	}
}

From a quick glance it looks like a proper implementation might get away with adding RequiredMemberAttribute here:

Edit: After a second look that is only for parameters, no idea how it works for properties.

Edit 2: Original filter did not handle nullable required classes type properties well.
Looks like openapi does not really have a concept of nullable ref types, so those just sort of end up being non-nullable.

This version handles that by not marking nullable ref types as required:

public class SwaggerRequiredMemberFilter : ISchemaFilter
{
	public void Apply(OpenApiSchema schema, SchemaFilterContext context)
	{
		var nullabilityContext = new NullabilityInfoContext();
		var properties = context.Type.GetProperties();
		foreach (var key in schema.Properties.Keys)
		{
			// Keys that do not correspond to a property are ignored
			var property = properties.SingleOrDefault(
				p => string.Equals(p.Name, key, StringComparison.OrdinalIgnoreCase)
			);
			if (property == null)
				continue;

			// Do not mark if the property is not required
			var required = property.HasAttribute<RequiredMemberAttribute>();
			if (!required)
				continue;

			// Check nullability of non primitive types
			var primitive = schema.Properties[key].Type != null;
			if (!primitive)
			{
				// Do not mark nullable ref types.
				// Ref types cannot be marked as nullable, so this would lead to them being non nullable.
				var nullabilityInfo = nullabilityContext.Create(property);
				if (nullabilityInfo.ReadState == NullabilityState.Nullable)
					continue;
			}

			schema.Required.Add(key);
		}
	}
}

@30drie
Copy link

30drie commented Jan 26, 2023

I used the solution from @flostellbrink, but found one flaw. I found that not all properties where marked required and when I looked at the code I saw that this only works as long as the name of the property is the same as the json name, but if you use JsonPropertyName to use snakecase for the API while your C# property has PascalCase naming convention, this code does not work.

So I changed the code slightly, basically do the foreach on the Properties of the class instead of Properties of the schema.
This code should work in all cases.

using System.Linq;
using System.Reflection;
using System.Runtime.CompilerServices;
using System.Text.Json.Serialization;
using Microsoft.OpenApi.Models;
using Swashbuckle.AspNetCore.SwaggerGen;

public class RequiredMemberFilter : ISchemaFilter
{
	public void Apply(OpenApiSchema schema, SchemaFilterContext context)
	{
		var nullabilityContext = new NullabilityInfoContext();
		var properties = context.Type.GetProperties();

		foreach(var property in properties)
		{
			if (property.HasAttribute<JsonIgnoreAttribute>() || !property.HasAttribute<RequiredMemberAttribute>()) continue;

			var jsonName = property.Name;
			if (property.HasAttribute<JsonPropertyNameAttribute>())
			{
				jsonName = property.GetCustomAttribute<JsonPropertyNameAttribute>()!.Name;
			}

			var jsonKey = schema.Properties.Keys.SingleOrDefault(key =>
				string.Equals(key, jsonName, StringComparison.OrdinalIgnoreCase));

			if (string.IsNullOrWhiteSpace(jsonKey)) continue;

			// Check nullability of non primitive types
			var primitive = schema.Properties[jsonKey].Type != null;
			if (!primitive)
			{
				// Do not mark nullableref types.
				// Ref types cannot be marked as nullable, so this would lead to them being non nullable.
				var nullabilityInfo = nullabilityContext.Create(property);
				if (nullabilityInfo.ReadState == NullabilityState.Nullable) continue;
			}

			schema.Required.Add(jsonKey);
		}
	}

}

@onionhammer
Copy link
Contributor

This isnt a C# 11 issue, it's a discrepancy between the ASP.NET validator treatment of non-nullable properties and swashbuckle's.

https://learn.microsoft.com/en-us/aspnet/core/mvc/models/validation?view=aspnetcore-7.0#non-nullable-reference-types-and-required-attribute

@onionhammer
Copy link
Contributor

Any movement on this issue? AFAICT this discrepancy still exists

@rthomas-rdm
Copy link

Would a PR to implement support for the C# 11 required modifier be welcome and likely to create traction on this issue?

@Havunen
Copy link

Havunen commented Feb 24, 2024

FYI this should work in DotSwashbuckle

@martincostello martincostello added the help-wanted A change up for grabs for contributions from the community label Apr 14, 2024
@mattiasnordqvist
Copy link

Is this related?
#2810

I think the required keyword has been incorrectly interpreted as not nullable

@martincostello
Copy link
Collaborator

That has already been fixed for the next release: #2879

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help-wanted A change up for grabs for contributions from the community
Projects
None yet
Development

No branches or pull requests

8 participants