Skip to content

Update accessibility check for types in same assembly #61082

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Mar 24, 2025
Merged

Update accessibility check for types in same assembly #61082

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
13 changes: 8 additions & 5 deletions src/OpenApi/gen/Helpers/AssemblyTypeSymbolsVisitor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ public override void VisitNamedType(INamedTypeSymbol type)
{
_cancellationToken.ThrowIfCancellationRequested();

if (!IsDeclaredInAssembly(type) || !_exportedTypes.Add(type))
if (!IsAccessibleType(type) || !_exportedTypes.Add(type))
{
return;
}
Expand All @@ -61,7 +61,7 @@ public override void VisitNamedType(INamedTypeSymbol type)
foreach (var property in properties)
{
_cancellationToken.ThrowIfCancellationRequested();
if (IsDeclaredInAssembly(property) && _exportedProperties.Add(property))
if (IsAccessibleType(property) && _exportedProperties.Add(property))
{
property.Type.Accept(this);
}
Expand All @@ -70,13 +70,16 @@ public override void VisitNamedType(INamedTypeSymbol type)
foreach (var method in methods)
{
_cancellationToken.ThrowIfCancellationRequested();
if (IsDeclaredInAssembly(method) && _exportedMethods.Add(method))
if (IsAccessibleType(method) && _exportedMethods.Add(method))
{
method.Accept(this);
}
}
}

private bool IsDeclaredInAssembly(ISymbol symbol) =>
SymbolEqualityComparer.Default.Equals(symbol.ContainingAssembly, assemblySymbol);
private bool IsAccessibleType(ISymbol symbol) =>
SymbolEqualityComparer.Default.Equals(symbol.ContainingAssembly, assemblySymbol)
&& (symbol.DeclaredAccessibility == Accessibility.Public ||
Copy link
Member

@BrennanConroy BrennanConroy Mar 21, 2025

Choose a reason for hiding this comment

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

Is it simpler to check for != Accessibility.Private? Or are there more accessibility states I'm forgetting?

Copy link
Member Author

@captainsafia captainsafia Mar 22, 2025

Choose a reason for hiding this comment

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

The way the Accessibility enum is modeled is kinda weird. It does it based on the modifiers in the class so protected class Foo will have Accessibility.Protected even though it is private.

symbol.DeclaredAccessibility == Accessibility.Internal ||
symbol.DeclaredAccessibility == Accessibility.ProtectedOrInternal);
}
59 changes: 43 additions & 16 deletions .../test/Microsoft.AspNetCore.OpenApi.SourceGenerators.Tests/AdditionalTextsTests.Schemas.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Globalization;
using System.Text.Json.Nodes;
using Microsoft.OpenApi.Models;

Expand Down Expand Up @@ -67,6 +68,34 @@ public class BoardItem
/// </summary>
public string Name { get; set; }
}

private class Element
{
/// <summary>
/// The unique identifier for the element.
/// </summary>
/// <remarks>
/// This won't be emitted since it is a public
/// property on a private class.
/// </remarks>
public string Name { get; set; }
}

protected internal class ProtectedInternalElement
{
/// <summary>
/// The unique identifier for the element.
/// </summary>
public string Name { get; set; }
}

protected class ProtectedElement
{
/// <summary>
/// The unique identifier for the element.
/// </summary>
public string Name { get; set; }
}
}

/// <summary>
Expand Down Expand Up @@ -201,18 +230,17 @@ await SnapshotTestHelper.VerifyOpenApi(compilation, additionalAssemblies, docume
var longTypeExample = Assert.IsAssignableFrom<JsonNode>(typeWithExamples.Properties["longType"].Example);
Assert.Equal(1234567890123456789, longTypeExample.GetValue<long>());

// Broken due to https://github.com/microsoft/OpenAPI.NET/issues/2137
// var doubleTypeExample = Assert.IsAssignableFrom<JsonNode>(typeWithExamples.Properties["doubleType"].Example);
// Assert.Equal("3.14", doubleTypeExample.GetValue<string>());
var doubleTypeExample = Assert.IsAssignableFrom<JsonNode>(typeWithExamples.Properties["doubleType"].Example);
Assert.Equal(3.14, doubleTypeExample.GetValue<double>());

// var floatTypeExample = Assert.IsAssignableFrom<JsonNode>(typeWithExamples.Properties["floatType"].Example);
// Assert.Equal(3.14f, floatTypeExample.GetValue<float>());
var floatTypeExample = Assert.IsAssignableFrom<JsonNode>(typeWithExamples.Properties["floatType"].Example);
Assert.Equal(3.14f, floatTypeExample.GetValue<float>());

// var dateTimeTypeExample = Assert.IsAssignableFrom<JsonNode>(typeWithExamples.Properties["dateTimeType"].Example);
// Assert.Equal(DateTime.Parse("2022-01-01T00:00:00Z", CultureInfo.InvariantCulture), dateTimeTypeExample.GetValue<DateTime>());
var dateTimeTypeExample = Assert.IsAssignableFrom<JsonNode>(typeWithExamples.Properties["dateTimeType"].Example);
Assert.Equal(new DateTime(2022, 01, 01), dateTimeTypeExample.GetValue<DateTime>());

// var dateOnlyTypeExample = Assert.IsAssignableFrom<JsonNode>(typeWithExamples.Properties["dateOnlyType"].Example);
// Assert.Equal(DateOnly.Parse("2022-01-01", CultureInfo.InvariantCulture), dateOnlyTypeExample.GetValue<DateOnly>());
var dateOnlyTypeExample = Assert.IsAssignableFrom<JsonNode>(typeWithExamples.Properties["dateOnlyType"].Example);
Assert.Equal("2022-01-01", dateOnlyTypeExample.GetValue<string>());

var stringTypeExample = Assert.IsAssignableFrom<JsonNode>(typeWithExamples.Properties["stringType"].Example);
Assert.Equal("Hello, World!", stringTypeExample.GetValue<string>());
Expand All @@ -223,15 +251,14 @@ await SnapshotTestHelper.VerifyOpenApi(compilation, additionalAssemblies, docume
var byteTypeExample = Assert.IsAssignableFrom<JsonNode>(typeWithExamples.Properties["byteType"].Example);
Assert.Equal(255, byteTypeExample.GetValue<int>());

// Broken due to https://github.com/microsoft/OpenAPI.NET/issues/2137
// var timeOnlyTypeExample = Assert.IsAssignableFrom<JsonNode>(typeWithExamples.Properties["timeOnlyType"].Example);
// Assert.Equal(TimeOnly.Parse("12:30:45", CultureInfo.InvariantCulture), timeOnlyTypeExample.GetValue<TimeOnly>());
var timeOnlyTypeExample = Assert.IsAssignableFrom<JsonNode>(typeWithExamples.Properties["timeOnlyType"].Example);
Assert.Equal("12:30:45", timeOnlyTypeExample.GetValue<string>());

// var timeSpanTypeExample = Assert.IsAssignableFrom<JsonNode>(typeWithExamples.Properties["timeSpanType"].Example);
// Assert.Equal(TimeSpan.Parse("P3DT4H5M", CultureInfo.InvariantCulture), timeSpanTypeExample.GetValue<TimeSpan>());
var timeSpanTypeExample = Assert.IsAssignableFrom<JsonNode>(typeWithExamples.Properties["timeSpanType"].Example);
Assert.Equal("P3DT4H5M", timeSpanTypeExample.GetValue<string>());

// var decimalTypeExample = Assert.IsAssignableFrom<JsonNode>(typeWithExamples.Properties["decimalType"].Example);
// Assert.Equal(3.14159265359m, decimalTypeExample.GetValue<decimal>());
var decimalTypeExample = Assert.IsAssignableFrom<JsonNode>(typeWithExamples.Properties["decimalType"].Example);
Assert.Equal(3.14159265359m, decimalTypeExample.GetValue<decimal>());

var uriTypeExample = Assert.IsAssignableFrom<JsonNode>(typeWithExamples.Properties["uriType"].Example);
Assert.Equal("https://example.com", uriTypeExample.GetValue<string>());
Expand Down
75 changes: 58 additions & 17 deletions src/OpenApi/test/Microsoft.AspNetCore.OpenApi.SourceGenerators.Tests/SchemaTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ public async Task SupportsXmlCommentsOnSchemas()
app.MapPost("/todo", (Todo todo) => { });
app.MapPost("/project", (Project project) => { });
app.MapPost("/board", (ProjectBoard.BoardItem boardItem) => { });
app.MapPost("/protected-internal-element", (ProjectBoard.ProtectedInternalElement element) => { });
app.MapPost("/project-record", (ProjectRecord project) => { });
app.MapPost("/todo-with-description", (TodoWithDescription todo) => { });
app.MapPost("/type-with-examples", (TypeWithExamples typeWithExamples) => { });
Expand Down Expand Up @@ -58,6 +59,43 @@ public class BoardItem
{
public string Name { get; set; }
}

/// <summary>
/// No XML comment processed here.
/// </summary>
private class Element
Copy link
Member

@BrennanConroy BrennanConroy Mar 21, 2025

Choose a reason for hiding this comment

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

Doesn't this type need to be used somewhere so we can check the lack of description?

Copy link
Member Author

@captainsafia captainsafia Mar 22, 2025

Choose a reason for hiding this comment

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

We have to rely on the check for compilation errors for this one. Since the type is private, we can't actually use it in the parameter list or return list for a method handler that is internal and/or public.

{
/// <summary>
/// The unique identifier for the element.
/// </summary>
/// <remarks>
/// This won't be emitted since it is a public
/// property on a private class.
/// </remarks>
public string Name { get; set; }
}

/// <summary>
/// Can find this XML comment.
/// </summary>
protected internal class ProtectedInternalElement
{
/// <summary>
/// The unique identifier for the element.
/// </summary>
public string Name { get; set; }
}

/// <summary>
/// No XML comment processed here.
/// </summary>
protected class ProtectedElement
{
/// <summary>
/// The unique identifier for the element.
/// </summary>
public string Name { get; set; }
}
}

/// <summary>
Expand Down Expand Up @@ -130,7 +168,7 @@ public interface IUser
}

/// <inheritdoc/>
public class User : IUser
internal class User : IUser
{
/// <inheritdoc/>
public int Id { get; set; }
Expand All @@ -155,6 +193,11 @@ await SnapshotTestHelper.VerifyOpenApi(compilation, document =>
var board = path.RequestBody.Content["application/json"].Schema;
Assert.Equal("An item on the board.", board.Description);

path = document.Paths["/protected-internal-element"].Operations[OperationType.Post];
var element = path.RequestBody.Content["application/json"].Schema;
Assert.Equal("The unique identifier for the element.", element.Properties["name"].Description);
Assert.Equal("Can find this XML comment.", element.Description);

path = document.Paths["/project-record"].Operations[OperationType.Post];
project = path.RequestBody.Content["application/json"].Schema;

Expand All @@ -179,18 +222,17 @@ await SnapshotTestHelper.VerifyOpenApi(compilation, document =>
var longTypeExample = Assert.IsAssignableFrom<JsonNode>(typeWithExamples.Properties["longType"].Example);
Assert.Equal(1234567890123456789, longTypeExample.GetValue<long>());

// Broken due to https://github.com/microsoft/OpenAPI.NET/issues/2137
// var doubleTypeExample = Assert.IsAssignableFrom<JsonNode>(typeWithExamples.Properties["doubleType"].Example);
// Assert.Equal("3.14", doubleTypeExample.GetValue<string>());
var doubleTypeExample = Assert.IsAssignableFrom<JsonNode>(typeWithExamples.Properties["doubleType"].Example);
Assert.Equal(3.14, doubleTypeExample.GetValue<double>());

// var floatTypeExample = Assert.IsAssignableFrom<JsonNode>(typeWithExamples.Properties["floatType"].Example);
// Assert.Equal(3.14f, floatTypeExample.GetValue<float>());
var floatTypeExample = Assert.IsAssignableFrom<JsonNode>(typeWithExamples.Properties["floatType"].Example);
Assert.Equal(3.14f, floatTypeExample.GetValue<float>());

// var dateTimeTypeExample = Assert.IsAssignableFrom<JsonNode>(typeWithExamples.Properties["dateTimeType"].Example);
// Assert.Equal(DateTime.Parse("2022-01-01T00:00:00Z", CultureInfo.InvariantCulture), dateTimeTypeExample.GetValue<DateTime>());
var dateTimeTypeExample = Assert.IsAssignableFrom<JsonNode>(typeWithExamples.Properties["dateTimeType"].Example);
Assert.Equal(new DateTime(2022, 01, 01), dateTimeTypeExample.GetValue<DateTime>());

// var dateOnlyTypeExample = Assert.IsAssignableFrom<JsonNode>(typeWithExamples.Properties["dateOnlyType"].Example);
// Assert.Equal(DateOnly.Parse("2022-01-01", CultureInfo.InvariantCulture), dateOnlyTypeExample.GetValue<DateOnly>());
var dateOnlyTypeExample = Assert.IsAssignableFrom<JsonNode>(typeWithExamples.Properties["dateOnlyType"].Example);
Assert.Equal("2022-01-01", dateOnlyTypeExample.GetValue<string>());

var stringTypeExample = Assert.IsAssignableFrom<JsonNode>(typeWithExamples.Properties["stringType"].Example);
Assert.Equal("Hello, World!", stringTypeExample.GetValue<string>());
Expand All @@ -201,15 +243,14 @@ await SnapshotTestHelper.VerifyOpenApi(compilation, document =>
var byteTypeExample = Assert.IsAssignableFrom<JsonNode>(typeWithExamples.Properties["byteType"].Example);
Assert.Equal(255, byteTypeExample.GetValue<int>());

// Broken due to https://github.com/microsoft/OpenAPI.NET/issues/2137
// var timeOnlyTypeExample = Assert.IsAssignableFrom<JsonNode>(typeWithExamples.Properties["timeOnlyType"].Example);
// Assert.Equal(TimeOnly.Parse("12:30:45", CultureInfo.InvariantCulture), timeOnlyTypeExample.GetValue<TimeOnly>());
var timeOnlyTypeExample = Assert.IsAssignableFrom<JsonNode>(typeWithExamples.Properties["timeOnlyType"].Example);
Assert.Equal("12:30:45", timeOnlyTypeExample.GetValue<string>());

// var timeSpanTypeExample = Assert.IsAssignableFrom<JsonNode>(typeWithExamples.Properties["timeSpanType"].Example);
// Assert.Equal(TimeSpan.Parse("P3DT4H5M", CultureInfo.InvariantCulture), timeSpanTypeExample.GetValue<TimeSpan>());
var timeSpanTypeExample = Assert.IsAssignableFrom<JsonNode>(typeWithExamples.Properties["timeSpanType"].Example);
Assert.Equal("P3DT4H5M", timeSpanTypeExample.GetValue<string>());

// var decimalTypeExample = Assert.IsAssignableFrom<JsonNode>(typeWithExamples.Properties["decimalType"].Example);
// Assert.Equal(3.14159265359m, decimalTypeExample.GetValue<decimal>());
var decimalTypeExample = Assert.IsAssignableFrom<JsonNode>(typeWithExamples.Properties["decimalType"].Example);
Assert.Equal(3.14159265359m, decimalTypeExample.GetValue<decimal>());

var uriTypeExample = Assert.IsAssignableFrom<JsonNode>(typeWithExamples.Properties["uriType"].Example);
Assert.Equal("https://example.com", uriTypeExample.GetValue<string>());
Expand Down
2 changes: 2 additions & 0 deletions ...s/SchemaTests.SupportsXmlCommentsOnSchemas#OpenApiXmlCommentSupport.generated.verified.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,8 +189,10 @@ private static Dictionary<MemberKey, XmlComment> GenerateCacheEntries()
_cache.Add(new MemberKey(typeof(global::Todo), MemberType.Type, null, null, []), new XmlComment(@"This is a todo item.", null, null, null, null, false, null, null, null));
_cache.Add(new MemberKey(typeof(global::Project), MemberType.Type, null, null, []), new XmlComment(@"The project that contains Todo items.", null, null, null, null, false, null, null, null));
_cache.Add(new MemberKey(typeof(global::ProjectBoard.BoardItem), MemberType.Type, null, null, []), new XmlComment(@"An item on the board.", null, null, null, null, false, null, null, null));
_cache.Add(new MemberKey(typeof(global::ProjectBoard.ProtectedInternalElement), MemberType.Type, null, null, []), new XmlComment(@"Can find this XML comment.", null, null, null, null, false, null, null, null));
_cache.Add(new MemberKey(typeof(global::ProjectRecord), MemberType.Type, null, null, []), new XmlComment(@"The project that contains Todo items.", null, null, null, null, false, null, [new XmlParameterComment(@"Name", @"The name of the project.", null, false), new XmlParameterComment(@"Description", @"The description of the project.", null, false)], null));
_cache.Add(new MemberKey(typeof(global::User), MemberType.Type, null, null, []), new XmlComment(null, null, null, null, null, false, null, null, null));
_cache.Add(new MemberKey(typeof(global::ProjectBoard.ProtectedInternalElement), MemberType.Property, "Name", null, []), new XmlComment(@"The unique identifier for the element.", null, null, null, null, false, null, null, null));
_cache.Add(new MemberKey(typeof(global::ProjectRecord), MemberType.Property, "Name", null, []), new XmlComment(@"The name of the project.", null, null, null, null, false, null, null, null));
_cache.Add(new MemberKey(typeof(global::ProjectRecord), MemberType.Property, "Description", null, []), new XmlComment(@"The description of the project.", null, null, null, null, false, null, null, null));
_cache.Add(new MemberKey(typeof(global::TodoWithDescription), MemberType.Property, "Id", null, []), new XmlComment(@"The identifier of the todo.", null, null, null, null, false, null, null, null));
Expand Down
Loading