Skip to content

Fix sanitization of type names in validations generator #61952

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

Merged
merged 1 commit into from
May 16, 2025

Conversation

captainsafia
Copy link
Member

This pull request introduces several updates to the ValidationsGenerator in the Microsoft.AspNetCore.Http project, focusing on improving type name sanitization.

  • Replaced the custom logic for sanitizing type names in SanitizeTypeName with a compiled Regex (InvalidNameCharsRegex) for improved clarity and maintainability. (src/Http/Http.Extensions/gen/Microsoft.AspNetCore.Http.ValidationsGenerator/Emitters/ValidationsGenerator.Emitter.cs, [1] [2]

Fixes #61388.

Note: I realize that as a follow-up to this change we need to make updates to the implementation to support validating dictionary values. Since that's a bit more involved I'll pull it out into a separate PR.

@github-actions github-actions bot added the needs-area-label Used by the dotnet-issue-labeler to label those issues which couldn't be triaged automatically label May 15, 2025
@captainsafia captainsafia added 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 needs-area-label Used by the dotnet-issue-labeler to label those issues which couldn't be triaged automatically labels May 15, 2025
@@ -92,6 +97,38 @@ private ValidatableTypeInfo CreateTestService()
]
);
}
private ValidatableTypeInfo CreateDictionary_2()
Copy link
Member Author

Choose a reason for hiding this comment

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

#61953 will change how this looks but we validate that compilable names are emitted here.

.Replace(",", "_")
.Replace(" ", "_");
// Replace invalid characters with underscores
return InvalidNameCharsRegex.Replace(typeName, "_");
Copy link
Member

Choose a reason for hiding this comment

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

It can't start with a digit presumably?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think there's a restriction on metadata names starting with digits per the CLR spec. However, since we derive this MetadataName that we pass to this method from a TypeSymbol resolve via static analysis, it's probably come from C# code where an identifier starting with a number is invalid.

If we do hit that case, I think emitting a method name starting with _ is OK syntax so the generator shouldn't emit invalid syntax.

Copy link
Contributor

@mikekistler mikekistler left a comment

Choose a reason for hiding this comment

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

Looks good! 👍

@captainsafia
Copy link
Member Author

Gonna merge this so it lands before the preview.5 cut-off. Happy to address feedback after merge in a follow-up PR.

@captainsafia captainsafia merged commit 944f0f5 into main May 16, 2025
28 checks passed
@captainsafia captainsafia deleted the safia/vsg-generic-types branch May 16, 2025 21:02
@dotnet-policy-service dotnet-policy-service bot added this to the 10.0-preview5 milestone May 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

Min API Validation fails for some generic types
3 participants