Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
122 changes: 93 additions & 29 deletions src/OpenApi/gen/XmlCommentGenerator.Emitter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ namespace Microsoft.AspNetCore.OpenApi.Generated
using System.Threading.Tasks;
using Microsoft.AspNetCore.OpenApi;
using Microsoft.AspNetCore.Mvc.Controllers;
using Microsoft.AspNetCore.Mvc.ModelBinding;
using Microsoft.AspNetCore.Mvc.ModelBinding.Metadata;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.OpenApi;
Expand Down Expand Up @@ -153,30 +154,6 @@ public static string CreateDocumentationId(this PropertyInfo property)
return sb.ToString();
}

/// <summary>
/// Generates a documentation comment ID for a property given its container type and property name.
/// Example: P:Namespace.ContainingType.PropertyName
/// </summary>
public static string CreateDocumentationId(Type containerType, string propertyName)
{
if (containerType == null)
{
throw new ArgumentNullException(nameof(containerType));
}
if (string.IsNullOrEmpty(propertyName))
{
throw new ArgumentException("Property name cannot be null or empty.", nameof(propertyName));
}

var sb = new StringBuilder();
sb.Append("P:");
sb.Append(GetTypeDocId(containerType, includeGenericArguments: false, omitGenericArity: false));
sb.Append('.');
sb.Append(propertyName);

return sb.ToString();
}

/// <summary>
/// Generates a documentation comment ID for a method (or constructor).
/// For example:
Expand Down Expand Up @@ -389,7 +366,7 @@ public Task TransformAsync(OpenApiOperation operation, OpenApiOperationTransform
foreach (var parameterComment in methodComment.Parameters)
{
var parameterInfo = methodInfo.GetParameters().SingleOrDefault(info => info.Name == parameterComment.Name);
var operationParameter = operation.Parameters?.SingleOrDefault(parameter => parameter.Name == parameterComment.Name);
var operationParameter = GetOperationParameter(operation, parameterInfo, parameterComment);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure we care too much, but if it's easy to fix could we avoid calling GetModelNames multiple times per parameter?

if (operationParameter is not null)
{
var targetOperationParameter = UnwrapOpenApiParameter(operationParameter);
Expand All @@ -400,7 +377,10 @@ public Task TransformAsync(OpenApiOperation operation, OpenApiOperationTransform
}
targetOperationParameter.Deprecated = parameterComment.Deprecated;
}
else
// Only fall back to the request body when the parameter is actually bound to it.
// This avoids applying documentation for parameters that aren't part of the
// OpenAPI surface (e.g. a `CancellationToken`) to the request body.
else if (IsRequestBodyParameter(context, parameterInfo, parameterComment))
{
var requestBody = operation.RequestBody;
if (requestBody is not null)
Expand Down Expand Up @@ -449,10 +429,14 @@ public Task TransformAsync(OpenApiOperation operation, OpenApiOperationTransform
&& metadata.ContainerType is { } containerType
&& metadata.PropertyName is { } propertyName)
{
var propertyDocId = DocumentationCommentIdHelper.CreateDocumentationId(containerType, propertyName);
if (XmlCommentCache.Cache.TryGetValue(DocumentationCommentIdHelper.NormalizeDocId(propertyDocId), out var propertyComment))
var propertyInfo = containerType.GetProperty(propertyName);
if (propertyInfo is null)
{
continue;
}
if (XmlCommentCache.Cache.TryGetValue(DocumentationCommentIdHelper.NormalizeDocId(propertyInfo.CreateDocumentationId()), out var propertyComment))
{
var parameter = operation.Parameters?.SingleOrDefault(p => p.Name == metadata.Name);
var parameter = GetOperationParameter(operation, propertyInfo);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to apply IsRequestBodyParameter to this piece of code as well?

var description = propertyComment.Summary;
if (!string.IsNullOrEmpty(description) && !string.IsNullOrEmpty(propertyComment.Value))
{
Expand Down Expand Up @@ -499,6 +483,86 @@ public Task TransformAsync(OpenApiOperation operation, OpenApiOperationTransform
return Task.CompletedTask;
}

private static IOpenApiParameter? GetOperationParameter(OpenApiOperation operation, PropertyInfo propertyInfo)
{
return GetOperationParameter(operation, propertyInfo, propertyInfo?.Name);
}
Comment thread
khellang marked this conversation as resolved.

private static IOpenApiParameter? GetOperationParameter(OpenApiOperation operation, ParameterInfo? parameterInfo, XmlParameterComment comment)
{
return GetOperationParameter(operation, parameterInfo, parameterInfo?.Name ?? comment.Name);
}

private static IOpenApiParameter? GetOperationParameter(OpenApiOperation operation, ICustomAttributeProvider? attributeProvider, string? name)
{
var parameters = operation.Parameters;
if (parameters is null || parameters.Count == 0)
{
return null;
}

var modelNames = GetModelNames(attributeProvider, name);

foreach (var parameter in parameters)
{
var parameterName = parameter.Name;

if (string.IsNullOrEmpty(parameterName))
{
continue;
}

if (modelNames.Contains(parameterName))
{
return parameter;
}
}

return null;
}

private static bool IsRequestBodyParameter(OpenApiOperationTransformerContext context, ParameterInfo? parameterInfo, XmlParameterComment comment)
{
var modelNames = GetModelNames(parameterInfo, parameterInfo?.Name ?? comment.Name);

foreach (var parameterDescription in context.Description.ParameterDescriptions)
{
if (parameterDescription.Source == BindingSource.Body

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This excludes IFormFile (and other form types) from the description. Should those be excluded?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But those are bound to the body, right?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, so if there is a description on the form param it should end up as the description in openapi?
#67078 calls this out as well.

&& parameterDescription.Name is { } parameterName
&& modelNames.Contains(parameterName))
{
return true;
}
}

return false;
}

private static IReadOnlySet<string> GetModelNames(ICustomAttributeProvider? attributeProvider, string? name)
{
var modelNames = new HashSet<string>();

if (!string.IsNullOrEmpty(name))
{
modelNames.Add(name);
}

if (attributeProvider is null)
{
return modelNames;
}

foreach (var attribute in attributeProvider.GetCustomAttributes(inherit: false))
{
if (attribute is IModelNameProvider modelNameProvider && !string.IsNullOrEmpty(modelNameProvider.Name))
{
modelNames.Add(modelNameProvider.Name);
}
}

return modelNames;
}

private static OpenApiParameter UnwrapOpenApiParameter(IOpenApiParameter sourceParameter)
{
if (sourceParameter is OpenApiParameterReference parameterReference)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -141,4 +141,173 @@ await SnapshotTestHelper.VerifyOpenApi(compilation, document =>
Assert.Equal("The id of the user.", path.Parameters[0].Description);
});
}

[Fact]
public async Task SupportsParametersWithCustomNamesFromControllers()
{
var source =
"""
using System.Collections.Generic;
using Microsoft.AspNetCore.Builder;
using Microsoft.AspNetCore.Mvc;
using Microsoft.Extensions.DependencyInjection;

var builder = WebApplication.CreateBuilder();

builder.Services
.AddControllers()
.AddApplicationPart(typeof(TestController).Assembly);
builder.Services.AddOpenApi();

var app = builder.Build();

app.MapControllers();

app.Run();

[ApiController]
[Route("[controller]")]
public class TestController : ControllerBase
{
/// <param name="userId">The id of the user.</param>
[HttpGet("{user_id}")]
public string Get([FromRoute(Name = "user_id")] int userId)
{
return "Hello, World!";
}

[HttpGet]
public IEnumerable<Person> Search(Query query)
{
return [];
}
}

public partial class Program {}

public record Person(int Id, string Name);

public class Query
{
/// <summary>
/// The full name of the person.
/// </summary>
[FromQuery(Name = "full_name")]
public string? Name { get; init; }
}
""";
var generator = new XmlCommentGenerator();
await SnapshotTestHelper.Verify(source, generator, out var compilation);
await SnapshotTestHelper.VerifyOpenApi(compilation, document =>
{
var getOperation = document.Paths["/Test/{user_id}"].Operations[HttpMethod.Get];
Assert.Equal("user_id", getOperation.Parameters[0].Name);
Assert.Equal("The id of the user.", getOperation.Parameters[0].Description);

var searchOperation = document.Paths["/Test"].Operations[HttpMethod.Get];
Assert.Equal("full_name", searchOperation.Parameters[0].Name);
Assert.Equal("The full name of the person.", searchOperation.Parameters[0].Description);
});
}

[Fact]
public async Task ShouldNotApplyCancellationTokenDocumentationToRequestBody()
{
var source =
"""
using System.Collections.Generic;
using System.Threading;
using Microsoft.AspNetCore.Builder;
using Microsoft.AspNetCore.Mvc;
using Microsoft.Extensions.DependencyInjection;

var builder = WebApplication.CreateBuilder();

builder.Services
.AddControllers()
.AddApplicationPart(typeof(TestController).Assembly);
builder.Services.AddOpenApi();

var app = builder.Build();

app.MapControllers();

app.Run();

[ApiController]
[Route("[controller]")]
public class TestController : ControllerBase
{
/// <param name="cancellationToken">The cancellation token.</param>
[HttpGet]
public ActionResult Create(Person person, CancellationToken cancellationToken)
{
return Created();
}
}

public partial class Program {}

public record Person(int Id, string Name);
""";
var generator = new XmlCommentGenerator();
await SnapshotTestHelper.Verify(source, generator, out var compilation);
await SnapshotTestHelper.VerifyOpenApi(compilation, document =>
{
var getOperation = document.Paths["/Test"].Operations[HttpMethod.Get];
Assert.Null(getOperation.RequestBody.Description);
});
}

[Fact]
public async Task ShouldNotApplyFromServicesDocumentationToRequestBody()
{
var source =
"""
using System.Collections.Generic;
using Microsoft.AspNetCore.Builder;
using Microsoft.AspNetCore.Mvc;
using Microsoft.Extensions.DependencyInjection;

var builder = WebApplication.CreateBuilder();

builder.Services
.AddControllers()
.AddApplicationPart(typeof(TestController).Assembly);
builder.Services.AddOpenApi();

var app = builder.Build();

app.MapControllers();

app.Run();

[ApiController]
[Route("[controller]")]
public class TestController : ControllerBase
{
/// <param name="service">The service used to create the resource.</param>
[HttpGet]
public ActionResult Create(Person person, [FromServices] ITestService service)
{
return Created();
}
}

public interface ITestService {}

public partial class Program {}

public record Person(int Id, string Name);
""";
var generator = new XmlCommentGenerator();
await SnapshotTestHelper.Verify(source, generator, out var compilation);
await SnapshotTestHelper.VerifyOpenApi(compilation, document =>
{
var getOperation = document.Paths["/Test"].Operations[HttpMethod.Get];
// The `service` parameter is bound from DI, not the request body, so its
// documentation must not be applied to the request body description.
Assert.Null(getOperation.RequestBody.Description);
});
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -239,7 +239,7 @@ public static IResult Post19(string regularParam, [AsParameters] MixedParameters
/// <param name="bindingParams">Parameters from different sources.</param>
public static IResult Get20([AsParameters] BindingSourceParametersClass bindingParams)
{
return TypedResults.Ok($"Query: {bindingParams.QueryParam}, Header: {bindingParams.HeaderParam}");
return TypedResults.Ok($"Query: {bindingParams.QueryParam}, Query with custom name: {bindingParams.QueryParamWithCustomName}, Header: {bindingParams.HeaderParam}");
}

/// <summary>
Expand Down Expand Up @@ -348,6 +348,12 @@ public class BindingSourceParametersClass
[FromQuery]
public string? QueryParam { get; set; }

/// <summary>
/// Query parameter from URL with custom name.
/// </summary>
[FromQuery(Name = "custom_name")]
public string? QueryParamWithCustomName { get; set; }

/// <summary>
/// Header value from request.
/// </summary>
Expand Down Expand Up @@ -496,7 +502,10 @@ await SnapshotTestHelper.VerifyOpenApi(compilation, document =>
var path20 = document.Paths["/20"].Operations[HttpMethod.Get];
Assert.Equal("Tests AsParameters with different binding sources.", path20.Summary);
Assert.Equal("Query parameter from URL.", path20.Parameters[0].Description);
Assert.Equal("Header value from request.", path20.Parameters[1].Description);
var customNameParam = path20.Parameters[1];
Assert.Equal("custom_name", customNameParam.Name);
Assert.Equal("Query parameter from URL with custom name.", customNameParam.Description);
Assert.Equal("Header value from request.", path20.Parameters[2].Description);

// Test XML documentation priority order: value > returns > summary
var path22 = document.Paths["/21"].Operations[HttpMethod.Get];
Expand Down
Loading
Loading