Add argument null validation to Aspire.Hosting

[JamesNK]

Description

Fixes #2649

Checklist

• Is this feature complete?
  • Yes. Ready to ship.
  • No. Follow-up changes expected.
• Are you including unit tests for the changes and scenario tests if relevant?
  • Yes
  • No
• Did you add public API?
  • Yes
    • If yes, did you have an API Review for it?
      • Yes
      • No
    • Did you add <remarks /> and <code /> elements on your triple slash comments?
      • Yes
      • No
  • No
• Does the change make any security assumptions or guarantees?
  • Yes
    • If yes, have you done a threat model and had a security review?
      • Yes
      • No
  • No
• Does the change require an update in our Aspire docs?
  • Yes
    • Link to aspire-docs issue:
  • No

Microsoft Reviewers: Open in CodeFlow

[JamesNK]

There is a test failure from

aspire/src/Aspire.Hosting/ContainerResourceBuilderExtensions.cs

Line 317 in 4e7e0de

.WithImageRegistry(null!)

Which is wrong: WithImageRegistry not allowing null or WithDockerfile calling WithImageRegistry with a null value?

[mitchdenny]

WithImageRegistry(null!) is valid because sometimes you don't have an image registry.

[JamesNK]

Ok. So, that means WithImageRegistry should allow a null argument?

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.

[eerhardt]

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:

aspire/src/Aspire.Hosting/ContainerResourceBuilderExtensions.cs

Line 50 in 4e7e0de

public static IResourceBuilder<T> WithVolume<T>(this IResourceBuilder<T> builder, string name, string target, bool isReadOnly = false) where T : ContainerResource

aspire/src/Aspire.Hosting/ContainerResourceBuilderExtensions.cs

Line 63 in 4e7e0de

public static IResourceBuilder<T> WithVolume<T>(this IResourceBuilder<T> builder, string target) where T : ContainerResource

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.

[JamesNK]

WithVolume without name has already shipped.

[eerhardt]

Should we update the API so allow for name to be null?

[JamesNK]

I don't see why when there is an overload without a name.

[eerhardt]

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");

[eerhardt]

See also: https://learn.microsoft.com/en-us/dotnet/standard/design-guidelines/member-overloading

✔️ DO allow null to be passed for optional arguments.

[JamesNK]

Ok

[drewnoakes]

Several of these should probably throw if empty, as in:

Suggested change

	ArgumentNullException.ThrowIfNull(name);
	ArgumentException.ThrowIfNullOrEmpty(name);

[JamesNK]

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.

[JamesNK]

Ok. So, that means WithImageRegistry should allow a null argument?

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.

@mitchdenny What is the answer here?

[mitchdenny]

Given the API has shipped allow null to be passed to WithImageRegistry.

[JamesNK]

I don't think it being shipped should stop us. It shipped without a nullable argument.

What is the best behavior?

[mitchdenny]

LGTM. I think its good to tighten this up.

[mitchdenny]

/azp run

[mitchdenny]

Kicking new build to see what the test failures were.

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[mitchdenny]

Flagging this as a breaking change as we did technically change the signature, although most folks won't notice if they just recompile.