Add argument null validation to Aspire.Hosting #6278
Conversation
|
There is a test failure from Which is wrong: |
|
|
|
Ok. So, that means On the surface that seems strange to me. Why would someone call it with null rather than just not call it? If it is changed to accept null, what should the doc comment say if you pass null to it? I'm not fimilar with this API. I'd like some actionable feedback. |
| @@ -185,7 +185,7 @@ public async Task EnsureContainerWithVolumesEmitsVolumes() | |||
| builder.AddContainer("containerwithvolumes", "image/name") | |||
| .WithVolume("myvolume", "/mount/here") | |||
| .WithVolume("myreadonlyvolume", "/mount/there", isReadOnly: true) | |||
| .WithVolume(null! /* anonymous volume */, "/mount/everywhere"); | |||
| .WithVolume("/mount/everywhere"); | |||
There was a problem hiding this comment.
Should we update the API so allow for name to be null? If you follow the call path, it gets passed into source below:
/// <param name="source">The source path if a bind mount or name if a volume. Can be <c>null</c> if the mount is an anonymous volume.</param>
/// <param name="target">The target path of the mount.</param>
/// <param name="type">The type of the mount.</param>
/// <param name="isReadOnly">A value indicating whether the mount is read-only.</param>
public ContainerMountAnnotation(string? source, string target, ContainerMountType type, bool isReadOnly)
Also, these WithVolume APIs don't meet .NET Design Guidelines:
https://learn.microsoft.com/en-us/dotnet/standard/design-guidelines/member-overloading
❌ AVOID being inconsistent in the ordering of parameters in overloaded members. Parameters with the same name should appear in the same position in all overloads.
There was a problem hiding this comment.
WithVolume without name has already shipped.
There was a problem hiding this comment.
Should we update the API so allow for name to be null?
There was a problem hiding this comment.
I don't see why when there is an overload without a name.
There was a problem hiding this comment.
Because it makes it easier to write code like this:
string? volumeName = GetVolumeName(); // may return null
builder.AddContainer("containerwithvolumes", "image/name")
.WithVolume(volumeName, "/mount/everywhere");There was a problem hiding this comment.
See also: https://learn.microsoft.com/en-us/dotnet/standard/design-guidelines/member-overloading
✔️ DO allow null to be passed for optional arguments.
| @@ -168,6 +178,10 @@ public static IResourceBuilder<ProjectResource> AddProject(this IDistributedAppl | |||
| /// </example> | |||
| public static IResourceBuilder<ProjectResource> AddProject(this IDistributedApplicationBuilder builder, [ResourceName] string name, string projectPath, string? launchProfileName) | |||
| { | |||
| ArgumentNullException.ThrowIfNull(builder); | |||
| ArgumentNullException.ThrowIfNull(name); | |||
There was a problem hiding this comment.
Several of these should probably throw if empty, as in:
| ArgumentNullException.ThrowIfNull(name); | |
| ArgumentException.ThrowIfNullOrEmpty(name); |
There was a problem hiding this comment.
They could. However, there is more validations of names that ensures they're not empty, Technically this null check isn't nesseccary, but I like the consistency of checking all references for null in public API calls.
@mitchdenny What is the answer here? |
|
Given the API has shipped allow null to be passed to WithImageRegistry. |
|
I don't think it being shipped should stop us. It shipped without a nullable argument. What is the best behavior? |
|
/azp run |
|
Kicking new build to see what the test failures were. |
|
Azure Pipelines successfully started running 1 pipeline(s). |
95d7184 to
233f1fd
Compare
|
Flagging this as a breaking change as we did technically change the signature, although most folks won't notice if they just recompile. |
Description
Fixes #2649
Checklist
<remarks />and<code />elements on your triple slash comments?Microsoft Reviewers: Open in CodeFlow