Skip to content

Conversation

captainsafia
Copy link
Member

Closes #62757.

@Copilot Copilot AI review requested due to automatic review settings August 3, 2025 04:34
@github-actions github-actions bot added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Aug 3, 2025
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR modifies the validation generator to skip non-public properties and types during code generation, addressing issue #62757. The changes ensure that only publicly accessible members are included in the generated validation code.

Key Changes

  • Modified the type parser to skip non-public types and properties in validation generation
  • Updated test cases to verify that only public properties are included in generated validation code
  • Added comprehensive test coverage for accessibility scenarios

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File Description
ValidationsGenerator.TypesParser.cs Added accessibility checks to skip non-public types and properties during validation generation
ValidationsGenerator.ComplexType.cs Added new test case to verify non-public properties are skipped while public properties remain validated
ValidationsGeneratorTests.SkipsClassesWithNonAccessibleTypes#ValidatableInfoResolver.g.verified.cs New snapshot showing generated code only includes public properties
ValidationsGeneratorTests.CanValidateParameters#ValidatableInfoResolver.g.verified.cs Updated snapshot removing non-public properties from Dictionary validation

return false;
}

// Skip types that are not accessible from generated code
Copy link
Preview

Copilot AI Aug 3, 2025

Choose a reason for hiding this comment

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

Consider adding a comment explaining why non-public types are skipped, as this is a significant behavioral change that affects which types get validation generation.

Suggested change
// Skip types that are not accessible from generated code
// Skip types that are not accessible from generated code
// Only public types are included for validation generation because generated code can only access public types.
// This is a significant behavioral decision: non-public types will not have validation generated.

Copilot uses AI. Check for mistakes.

continue;
}

// Skip properties that are not accessible from generated code
Copy link
Preview

Copilot AI Aug 3, 2025

Choose a reason for hiding this comment

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

Consider adding a comment explaining why non-public properties are skipped, similar to the type-level check, to maintain consistency in documentation.

Suggested change
// Skip properties that are not accessible from generated code
// Skip properties that are not accessible from generated code
// Only public properties can be accessed by generated code, so non-public properties are skipped.

Copilot uses AI. Check for mistakes.

continue;
}

// Skip properties that are not accessible from generated code
Copy link
Preview

Copilot AI Aug 3, 2025

Choose a reason for hiding this comment

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

Consider adding a comment explaining why non-public properties are skipped, to maintain consistency with the other accessibility checks in this file.

Suggested change
// Skip properties that are not accessible from generated code
// Skip properties that are not accessible from generated code
// Non-public properties are skipped because they cannot be accessed by the generated code.

Copilot uses AI. Check for mistakes.

@captainsafia captainsafia added area-blazor Includes: Blazor, Razor Components area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc feature-validation Issues related to model validation in minimal and controller-based APIs and removed area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework labels Aug 3, 2025
@captainsafia
Copy link
Member Author

/azp run

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

Copy link
Member

@javiercn javiercn left a comment

Choose a reason for hiding this comment

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

LGTM

@BrennanConroy
Copy link
Member

So, best option might be to skip over the private types here to avoid the compilation errors -- then the runtime checks should kick in after the fact.

Non-public properties are validated at run-time still? Is there test coverage for that?

@captainsafia
Copy link
Member Author

captainsafia commented Aug 4, 2025

Non-public properties are validated at run-time still? Is there test coverage for that?

I was specifically refering to types that are resolved from the DI container but not annotated with the [FromServices] attribute here.

app.MapGet("/", (CatalogDbContext dbContext) => ...);

In the example above, there's no way to determine at compile-time that CatalogDbContext is a type that will be resolved from the DI container so we will end up creating a ValidatableTypeInfo for it. Prior to this PR, the compile-time code would walk both public and non-public properties on that type and emit ValidatablePropertyInfos for them. That's what caused the compile-time error.

In this new model, we'll still create a ValidatableTypeInfo for it, but it won't include problematic ValidatablePropertyInfo so there will be no compile-time errors. Then at runtime it will be ignored from validation via this check which uses the IServiceProviderIsService to check if it is registered in DI and skip actually validating it.

@captainsafia captainsafia enabled auto-merge (squash) August 4, 2025 21:49
@captainsafia captainsafia disabled auto-merge August 4, 2025 22:40
@captainsafia captainsafia enabled auto-merge (squash) August 4, 2025 22:49
@captainsafia captainsafia merged commit e7ebb34 into main Aug 5, 2025
28 of 30 checks passed
@captainsafia captainsafia deleted the safia/fix-vsg-accessibility branch August 5, 2025 00:17
@dotnet-policy-service dotnet-policy-service bot added this to the 10.0-rc1 milestone Aug 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-blazor Includes: Blazor, Razor Components area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc feature-validation Issues related to model validation in minimal and controller-based APIs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[.NET 10] ValidationsGenerator throws some errors due to inaccessibility
3 participants