Refactor name validation to NameValidationPolicyAnnotation with configurable rules#15728
Refactor name validation to NameValidationPolicyAnnotation with configurable rules#15728
Conversation
JamesNK
requested review from
danegsta,
davidfowl,
karolz-ms and
mitchdenny
as code owners
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 15728Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 15728" |
JamesNK
added
the
area-app-model
There was a problem hiding this comment.
Pull request overview
This PR moves resource name validation (64-character limit enforced by ModelName.ValidateName) out of Resource construction and into DistributedApplicationBuilder.AddResource() and Build(), adding an opt-out marker annotation (SuppressNameValidationAnnotation) for internal “suffix-appending” resources like rebuilders/installers.
Changes:
- Remove name validation from
Resource/ExecutableResourceconstructors and validate inDistributedApplicationBuilder.AddResource()andBuild(). - Introduce
SuppressNameValidationAnnotationand apply it to rebuilder/installer resources to avoid failures when suffixes push names over 64 characters. - Add/adjust tests to cover AddResource/Build validation and suppression behavior, plus annotation presence on affected resources.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Aspire.Hosting.Tests/ProjectResourceTests.cs | Adds test asserting the project rebuilder resource carries the suppression annotation. |
| tests/Aspire.Hosting.Tests/ModelNameTests.cs | Removes prior test that relied on constructor-level validation behavior. |
| tests/Aspire.Hosting.Tests/DistributedApplicationBuilderTests.cs | Adds tests for name validation in AddResource/Build, including suppression opt-out. |
| tests/Aspire.Hosting.Python.Tests/AddPythonAppTests.cs | Adds tests asserting Python installer/venv creator resources carry the suppression annotation. |
| tests/Aspire.Hosting.JavaScript.Tests/PackageInstallationTests.cs | Adds test asserting JavaScript installer resource carries the suppression annotation. |
| src/Aspire.Hosting/ProjectResourceBuilderExtensions.cs | Applies suppression annotation to the generated ProjectRebuilderResource. |
| src/Aspire.Hosting/DistributedApplicationBuilder.cs | Adds name validation in AddResource() and Build(), skipping when suppression annotation exists. |
| src/Aspire.Hosting/ApplicationModel/SuppressNameValidationAnnotation.cs | Introduces new public marker annotation type. |
| src/Aspire.Hosting/ApplicationModel/Resource.cs | Removes name validation from base Resource construction. |
| src/Aspire.Hosting/ApplicationModel/ProjectRebuilderResource.cs | Removes use of skip-validation constructor pattern. |
| src/Aspire.Hosting/ApplicationModel/ExecutableResource.cs | Removes skip-validation constructor overload; uses base Resource(name) directly. |
| src/Aspire.Hosting.Python/PythonVenvCreatorResource.cs | Removes use of skip-validation constructor pattern. |
| src/Aspire.Hosting.Python/PythonInstallerResource.cs | Removes use of skip-validation constructor pattern. |
| src/Aspire.Hosting.Python/PythonAppResourceBuilderExtensions.cs | Applies suppression annotation to created Python installer/venv creator resources. |
| src/Aspire.Hosting.JavaScript/JavaScriptInstallerResource.cs | Refactors to primary-constructor style (public surface unchanged). |
| src/Aspire.Hosting.JavaScript/JavaScriptHostingExtensions.cs | Applies suppression annotation to created JavaScript installer resources. |
src/Aspire.Hosting/ApplicationModel/SuppressNameValidationAnnotation.cs
Outdated
Show resolved
Hide resolved
JamesNK
force-pushed
the
jamesnk/name-validation-suppression-v2
branch
from
fa56879 to
ac8add0
Compare
|
Much cleaner. |
JamesNK
force-pushed
the
jamesnk/name-validation-suppression-v2
branch
from
e1838b8 to
39f6d3e
Compare
|
Re-running the failed jobs in the CI workflow for this pull request because 1 job was identified as retry-safe transient failures in the CI run attempt.
|
JamesNK
force-pushed
the
jamesnk/name-validation-suppression-v2
branch
2 times, most recently
from
5314738 to
5f5cf68
Compare
karolz-ms
left a comment
karolz-ms
left a comment
There was a problem hiding this comment.
This solution completely disables name validation for several "system" resources. I am not sure this is the best approach. I think it makes bugs related to naming harder to catch early.
One alternative would be to introduce "name validation policy" concept, with user resources subject to standard naming policy vs system resources having a different/more relaxed policy. An annotation could be used to indicate the desired policy.
src/Aspire.Hosting.Azure.WebPubSub/Aspire.Hosting.Azure.WebPubSub.csproj
Show resolved
Hide resolved
This aligns with another proposal I made a short while back to support extensible naming policies that resources could be applied based on usage, so e.g. if you add Azure then the naming policy of compute resources would be further restricted to ensure they map to Azure resource naming cleanly. |
|
What do yall want here? I don't think having a Alternatively, if |
|
I just think suppressing name validation (and introducing an annotation for doing so) is not a good idea in general. If system resources need longer names, then we should have a different name validation policy for them, one that accepts longer names, say 96- or 128-character long. Whether you want to introduce the name validation policies as part of fixing this issue is related, but different question. If you decide to do that, I would expect to have a If this is too much for now, the suggested |
This reverts commit 30f3928.
…alidationAnnotation Resource names have a 64-character limit enforced by ModelName.ValidateName. Internal resources like ProjectRebuilderResource, JavaScriptInstallerResource, PythonInstallerResource, and PythonVenvCreatorResource append suffixes to user-provided resource names, which can push them over the limit. Previously, validation happened in the Resource constructor, requiring internal skip-validation constructors that couldn't be accessed from external assemblies. This change moves validation from the Resource constructor to DistributedApplicationBuilder.AddResource() and Build(), and introduces a public SuppressNameValidationAnnotation that any resource can use to opt out of name validation before being added to the app model.
…tation Replace the boolean SuppressNameValidationAnnotation with a configurable NameValidationPolicyAnnotation that exposes individual validation rules (MaxLength, ValidateStartsWithLetter, ValidateAllowedCharacters, ValidateNoConsecutiveHyphens, ValidateNoTrailingHyphen) as init-only properties. Add static readonly fields None (disables all rules) and Default (enforces all standard rules). Add public default-value constants to ModelName and use them in both the simple overloads and the annotation property initializers.
JamesNK
force-pushed
the
jamesnk/name-validation-suppression-v2
branch
from
5f5cf68 to
f5279b7
Compare
JamesNK
changed the title
|
@karolz-ms @DamianEdwards I added Right now the code analysis still uses default rules for resource and endpoint arguments. When needed, properties could be placed on |
|
Re-running the failed jobs in the CI workflow for this pull request because 1 job was identified as retry-safe transient failures in the CI run attempt.
|
IEvangelist
left a comment
IEvangelist
left a comment
There was a problem hiding this comment.
This looks clean! Nice job.
|
🎬 CLI E2E Test Recordings — 56 recordings uploaded (commit View recordings
📹 Recordings uploaded automatically from CI run #24067278937 |
|
@JamesNK I logged #13404 previously to cover expanding name validation to be more flexible. The thought there was that the validation facets would be set via MSBuild so that they could be discovered at both build time and runtime, although it could also be achieved via attribute constructor parameters on |
As far as I know, the only place where the current resource name rules don't work is length. And that happens when we take a valid name and create another resource from it and add more characters. So it doesn't involve the analyzer. The problem with an MSBuild property is it would apply globally. You couldn't add a new name rule and only apply it to certain resources. Or disable a rule for existing resources. Although maybe globally setting rules is what's wanted when thinking about deploying to an environment. Should the approach in this PR change? |
|
@JamesNK I think this is fine for now. We could maybe do an inherited attribute to enable specific resources to have specific rules that are still visible by the analyzer by having the validation logic be callable by the analyzer on a static method on the derived attribute or some-such. But no need to block for now. |
Summary
Replaces the boolean
SuppressNameValidationAnnotationand theskipValidationconstructor parameter onResource/ExecutableResourcewith a configurableNameValidationPolicyAnnotationthat can individually control each validation rule.Changes
New:
NameValidationPolicyAnnotationinit-only properties) for customizing name validation rules per resourceMaxLength,ValidateStartsWithLetter,ValidateAllowedCharacters,ValidateNoConsecutiveHyphens,ValidateNoTrailingHyphenDefault— enforces all standard rulesNone— disables all rules (replaces the old suppress annotation)Validation moved to
DistributedApplicationBuilderAddResource()andBuild()via aValidateResourceNamehelperNameValidationPolicyAnnotationfrom the resource (falling back toDefault) and passes its settings toModelName.ValidateNameResourceconstructor no longer validates names (just rejects null/whitespace)skipValidationinternal constructor fromResourceandExecutableResourceModelNameupdatesDefaultMaxLength(64),DefaultValidateStartsWithLetter,DefaultValidateAllowedCharacters,DefaultValidateNoConsecutiveHyphens,DefaultValidateNoTrailingHyphenValidateName/TryValidateNameoverloads that accept individual rule parametersNameValidationPolicyAnnotationproperty initializers reference these constants as the single source of truthUsage sites updated
PythonInstallerResource,PythonVenvCreatorResource,JavaScriptInstallerResource,ProjectRebuilderResourcenow addNameValidationPolicyAnnotation.Noneas an annotation instead of usingskipValidation: trueorSuppressNameValidationAnnotationTests
Assert.Same(NameValidationPolicyAnnotation.None, policy)to verify the exact static instance