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
5 changes: 5 additions & 0 deletions src/OpenApi/gen/Helpers/AssemblyTypeSymbolsVisitor.cs
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,11 @@ public override void VisitNamespace(INamespaceSymbol symbol)
public override void VisitNamedType(INamedTypeSymbol type)
{
_cancellationToken.ThrowIfCancellationRequested();
if (type.IsGenericType)
{
// Because type comments are based on the Unbound generic type, i.e. List`1, we want to use the unbound generic type
type = type.ConstructUnboundGenericType();
}

if (!IsAccessibleType(type) || !_exportedTypes.Add(type))
{
Expand Down
27 changes: 26 additions & 1 deletion src/OpenApi/gen/XmlCommentGenerator.Parser.cs
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ internal static string NormalizeDocId(string docId)
return comments;
}

internal static IEnumerable<(string, XmlComment?)> ParseComments(
internal static IEnumerable<(string, XmlComment?)> ParseCommentsApplicationAssembly(
(List<(string, string)> RawComments, Compilation Compilation) input,
CancellationToken cancellationToken)
{
Expand All @@ -136,6 +136,31 @@ internal static string NormalizeDocId(string docId)
return comments;
}

internal static IEnumerable<(string, XmlComment?)> ParseCommentsReference(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If someone has a better idea for this, let me know. The main difference compared to ParseCommentsApplicationAssembly is that it only allows "accessible" types

(List<(string, string)> RawComments, Compilation Compilation) input,
CancellationToken cancellationToken)
{
var compilation = input.Compilation;
var comments = new List<(string, XmlComment?)>();
foreach (var (name, value) in input.RawComments)
{
if (DocumentationCommentId.GetFirstSymbolForDeclarationId(name, compilation) is ISymbol symbol &&
// Only include symbols that are accessible from the application assembly.
symbol.IsAccessibleType() &&
Comment on lines +148 to +149
Copy link
Contributor

Choose a reason for hiding this comment

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

@desjoerd this seems okay, but should this PR also revisit the use of symbol.IsAccessible() up on L124?

Specifically, it seems like this source generator should default to excluding types marked with EmbeddedAttribute, since those are accessible from the application assembly, but seem to be of dubious worth w.r.t. embedding constants into the assembly for (what I assume are largely going to be) marker attributes for source generators, dev dependencies, etc.

(e.g. thinking about this from the aot perspective where I don't want paragraphs and paragraphs xml docs from internal/dev-time-only marker attributes getting baked into the executable)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the application assembly types with [Embedded] are accessible and could even be used within your Api I think.

For referenced assemblies they will be already excluded as it will only emit xml comments for types marked as public.
https://github.com/dotnet/aspnetcore/blob/main/src%2FOpenApi%2Fgen%2FHelpers%2FISymbolExtensions.cs#L193

// Skip static classes that are just containers for members with annotations
// since they cannot be instantiated.
symbol is not INamedTypeSymbol { TypeKind: TypeKind.Class, IsStatic: true })
{
var parsedComment = XmlComment.Parse(symbol, compilation, value, cancellationToken);
if (parsedComment is not null)
{
comments.Add((name, parsedComment));
}
}
}
return comments;
}

internal static bool FilterInvocations(SyntaxNode node, CancellationToken _)
=> node is InvocationExpressionSyntax { Expression: MemberAccessExpressionSyntax { Name.Identifier.ValueText: "AddOpenApi" } };

Expand Down
4 changes: 2 additions & 2 deletions src/OpenApi/gen/XmlCommentGenerator.cs
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,10 @@ public void Initialize(IncrementalGeneratorInitializationContext context)
// and the target assembly.
var parsedCommentsFromXmlFile = commentsFromXmlFile
.Combine(context.CompilationProvider)
.Select(ParseComments);
.Select(ParseCommentsReference);
Copy link
Member

Choose a reason for hiding this comment

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

Hmmm...is this change related to the fix for generic types?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This one is to fix the comments coming from a documentation file when a type also exists in the main compilation.

When an internal type with the same namespace and class name exists in the Application assembly.

In the unit tests it's the Sample.Duplicate which is internal in the Application assembly. When it processes it from a different assembly it would resolve to Sample.Duplicate.

I duplicated the ParseComments into ParseCommentsReference and ParseCommentsApplicationAssembly to have a different condition. ParseCommentsReference only includes public types. ParseCommentsApplicationAssembly includes public types and types available in the compilation.

var parsedCommentsFromCompilation = commentsFromTargetAssembly
.Combine(context.CompilationProvider)
.Select(ParseComments);
.Select(ParseCommentsApplicationAssembly);
// Discover AddOpenApi invocations so that we can intercept them with an implicit
// registration of the transformers for mapping XML doc comments to the OpenAPI file.
var groupedAddOpenApiInvocations = context.SyntaxProvider
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -87,4 +87,84 @@ public void NormalizeDocId_ReturnsExpectedResult(string input, string expected)
var result = XmlCommentGenerator.NormalizeDocId(input);
Assert.Equal(expected, result);
}
}

[Fact]
public async Task ShouldNotDuplicateDocumentationIds()
{
var source = """
using System;
using System.Threading.Tasks;
using Microsoft.AspNetCore.Builder;
using Microsoft.Extensions.DependencyInjection;

var builder = WebApplication.CreateBuilder(args);

builder.Services.AddOpenApi();

var app = builder.Build();
app.MapOpenApi();

app.MapPost("/", (GenericFoo<bool> fooBool) => new Bar());

app.Run();

/// <summary>
/// Should not be duplicated. <see cref="T" />
/// </summary>
public class GenericFoo<T>
{
}

public class Bar
{
/// <summary>
/// FooString property xml comment.
/// </summary>
public GenericFoo<string> FooString { get; set; }

/// <summary>
/// FooInt property xml comment.
/// </summary>
public GenericFoo<int> FooInt { get; set; }

/// <summary>
/// Method with GenericFoo<double> parameter.
/// </summary>
/// <param name="fooDouble">The GenericFoo of double.</param>
public void Test(GenericFoo<double> fooDouble)
{

}
}

namespace Sample {

/// <summary>
/// Duplicated internal class.
/// </summary>
internal class Duplicate {
}
}
""";

var internalDuplicatedSource = """
namespace Sample {

/// <summary>
/// Duplicated internal class.
/// </summary>
internal class Duplicate {
}
}
""";
var references = new Dictionary<string, List<string>>
{
{ "InternalDuplicateLibrary1", [internalDuplicatedSource] },
{ "InternalDuplicateLibrary2", [internalDuplicatedSource] }
};

var generator = new XmlCommentGenerator();
await SnapshotTestHelper.Verify(source, generator, references, out var compilation, out var additionalAssemblies);
await SnapshotTestHelper.VerifyOpenApi(compilation, additionalAssemblies, Assert.NotNull);
}
}
Loading
Loading