Skip to content
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

Check List: Validate arguments of public methods #5047

Open
20 of 56 tasks
Zombach opened this issue Jul 24, 2024 · 30 comments
Open
20 of 56 tasks

Check List: Validate arguments of public methods #5047

Zombach opened this issue Jul 24, 2024 · 30 comments
Labels
area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication untriaged New issue has not been triaged

Comments

@Zombach
Copy link
Contributor

Zombach commented Jul 24, 2024

Is your feature request related to a problem? Please describe the problem.

Check List to #2649
Source CA1062

Cause
An externally visible method dereferences one of its reference arguments without verifying whether that argument is null (Nothing in Visual Basic).

You can configure this rule to exclude certain types and parameters from analysis. You can also indicate null-check validation methods.

Rule description
All reference arguments that are passed to externally visible methods should be checked against null. If appropriate, throw an ArgumentNullException when the argument is null.

If a method can be called from an unknown assembly because it is declared public or protected, you should validate all parameters of the method. If the method is designed to be called only by known assemblies, mark the method internal and apply the InternalsVisibleToAttribute attribute to the assembly that contains the method.

Describe the solution you'd like

I suggest covering this with additional tests.
Check every reference argument in the public API ArgumentNullException.ThrowIfNull(source).

Using the example of Aspire.Hosting.Redis
RedisBuilderExtensions.cs

public static IResourceBuilder<RedisResource> AddRedis(this IDistributedApplicationBuilder builder, string name, int? port = null)
{
    ArgumentNullException.ThrowIfNull(builder);

    var redis = new RedisResource(name);
    return builder.AddResource(redis)
                  .WithEndpoint(port: port, targetPort: 6379, name: RedisResource.PrimaryEndpointName)
                  .WithImage(RedisContainerImageTags.Image, RedisContainerImageTags.Tag)
                  .WithImageRegistry(RedisContainerImageTags.Registry);
}

[Fact]
public void AddRedisContainerShouldThrowsWhenBuilderIsNull()
{
    IDistributedApplicationBuilder builder = null!;
    const string name = "Redis";

    var action = () => builder.AddRedis(name);

    Assert.Multiple(() =>
    {
        var exception = Assert.Throws<ArgumentNullException>(action);
        Assert.Equal(nameof(builder), exception.ParamName);
    });
}

Attached as an full example CA1062#Aspire.Hosting.Redis

Additional context

When checking, we get a NullReferenceException
image

Components.*

Hosting.*

ServiceDiscovery.*

Frozen*

  • - Adding public API test coverage for Aspire.Hosting.AppHost
  • - Adding public API test coverage for Aspire.Hosting.Orleans
  • - Adding public API test coverage for Aspire.Hosting.Seq
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication label Jul 24, 2024
@Zombach
Copy link
Contributor Author

Zombach commented Jul 24, 2024

@davidfowl @eerhardt
Helloy. If you don't mind, could you familiarize yourself with this task?

@Alirexaa
Copy link
Contributor

It seems to duplicate of #2649

@Zombach Zombach changed the title Validate arguments of public methods Check List: Validate arguments of public methods Jul 24, 2024
@Zombach
Copy link
Contributor Author

Zombach commented Jul 24, 2024

It seems to duplicate of #2649

Really. Then you need to understand whether this solution option is suitable and make adjustments if necessary.

@Zombach
Copy link
Contributor Author

Zombach commented Jul 26, 2024

@DamianEdwards
Good afternoon.
Because you are the original creator of this issue #2649.
Could you please rate this topic?
I would like to resolve this issue and track progress status using a checklist.

But this requires a go-ahead,
And also successful Merge CA1062#Aspire.Hosting.Redis
To have a working example taking into account the necessary edits

@Alirexaa
Copy link
Contributor

I will work on Aspire.Hosting.Elasticsearch.

@Zombach
Copy link
Contributor Author

Zombach commented Jul 31, 2024

I will work on Aspire.Hosting.Nats

@Zombach
Copy link
Contributor Author

Zombach commented Aug 1, 2024

I will work on Aspire.Hosting.Keycloak

@Alirexaa
Copy link
Contributor

Alirexaa commented Aug 1, 2024

I will work on Aspire.Hosting.Milvus

@Zombach
Copy link
Contributor Author

Zombach commented Aug 2, 2024

I will work on Aspire.Hosting.PosgreSQL

@Alirexaa
Copy link
Contributor

Alirexaa commented Aug 2, 2024

I will work on Aspire.Hosting.RabbitMQ

@Alirexaa
Copy link
Contributor

Alirexaa commented Aug 2, 2024

I will work on Aspire.Hosting.MySql

@Alirexaa
Copy link
Contributor

Alirexaa commented Aug 2, 2024

I will work on Adding public API test coverage for Aspire.Hosting.Kafka

@Zombach
Copy link
Contributor Author

Zombach commented Aug 2, 2024

I will work on Adding public API test coverage for Aspire.Hosting.Garnet

@Alirexaa
Copy link
Contributor

Alirexaa commented Aug 2, 2024

I will work in Aspire.Hosting.NodeJs

@Zombach
Copy link
Contributor Author

Zombach commented Aug 4, 2024

I will work in Microsoft.Extensions.ServiceDiscovery.Yarp

@Zombach
Copy link
Contributor Author

Zombach commented Aug 4, 2024

Add groups Components.* and ServiceDiscovery.*

@Alirexaa
Copy link
Contributor

Alirexaa commented Aug 4, 2024

Add groups Components.* and ServiceDiscovery.*

hmm, a lot of work :)
Thanks.

@Alirexaa
Copy link
Contributor

Alirexaa commented Aug 4, 2024

I will work on Aspire.Elastic.Clients.Elasticsearch

@Alirexaa
Copy link
Contributor

Alirexaa commented Aug 4, 2024

I will work on Aspire.Milvus.Client

@Alirexaa
Copy link
Contributor

Alirexaa commented Aug 4, 2024

I will work on Aspire.MongoDB.Driver

@Alirexaa
Copy link
Contributor

Alirexaa commented Aug 6, 2024

I will work on Aspire.Hosting.MongoDB

@Zombach
Copy link
Contributor Author

Zombach commented Aug 8, 2024

I will work on Microsoft.Extensions.ServiceDiscovery.Dns

@Alirexaa
Copy link
Contributor

Alirexaa commented Aug 9, 2024

I will work in Aspire.NATS.Net

@Alirexaa
Copy link
Contributor

Alirexaa commented Aug 9, 2024

I will work on Aspire.Hosting.SqlServer

@sebastienros
Copy link
Member

sebastienros commented Aug 9, 2024

@Alirexaa @Zombach Could you make these in batches, it might help reduce the noise and we are on agreement with how these should be done so the back-and-forth should be minimal. Thanks. Mayube one for clients and one for hosting, and if one of you owns it in the fork then they could invite the other so they can work on the same branch if necessary (I can already as a contributor of this repos). Then when I merge I will keep both names as contributors for internet points.

@Zombach
Copy link
Contributor Author

Zombach commented Aug 9, 2024

@Alirexaa @Zombach Could you make these in batches, it might help reduce the noise and we are on agreement with how these should be done so the back-and-forth should be minimal. Thanks. Mayube one for clients and one for hosting, and if one of you owns it in the fork then they could invite the other so they can work on the same branch if necessary (I can already as a contributor of this repos). Then when I merge I will keep both names as contributors for internet points.

Yes, okay. We will do this in blocks.

@Zombach
Copy link
Contributor Author

Zombach commented Aug 9, 2024

I'm working with @Alirexaa on #5250
Microsoft.Extensions.ServiceDiscovery.Abstractions
Microsoft.Extensions.ServiceDiscovery
Aspire.Keycloak.Authentication
Aspire.Microsoft.Azure.Cosmos
Aspire.Azure.AI.OpenAI
Aspire.Azure.Data.Tables
Aspire.Hosting.AWS
Aspire.Confluent.Kafka
Aspire.Azure.Search.Documents
Aspire.Azure.Security.KeyVault
Aspire.Azure.Storage.Queues
Aspire.Hosting.Dapr

@Zombach
Copy link
Contributor Author

Zombach commented Aug 23, 2024

I'm working on #5402
Aspire.Azure.Messaging.EventHubs
Aspire.Azure.Messaging.ServiceBus
Aspire.Azure.Messaging.WebPubSub
Aspire.Azure.Storage.Blobs
Aspire.Microsoft.Data.SqlClient
Aspire.Microsoft.EntityFrameworkCore.Cosmos
Aspire.Microsoft.EntityFrameworkCore.SqlServer
Aspire.MySqlConnector
Aspire.Npgsql
Aspire.Npgsql.EntityFrameworkCore.PostgreSQL
Aspire.Oracle.EntityFrameworkCore

@Zombach
Copy link
Contributor Author

Zombach commented Aug 23, 2024

I'm working on #5407
Aspire.Pomelo.EntityFrameworkCore.MySql
Aspire.Qdrant.Client
Aspire.RabbitMQ.Client
Aspire.Seq
Aspire.StackExchange.Redis
Aspire.StackExchange.Redis.DistributedCaching
Aspire.StackExchange.Redis.OutputCaching

@Zombach
Copy link
Contributor Author

Zombach commented Aug 25, 2024

These projects are frozen for now, since they have not yet moved the test projects into a separate project.

Aspire.Hosting.AppHost
Aspire.Hosting.Orleans
Aspire.Hosting.Seq

@joperezr joperezr added the untriaged New issue has not been triaged label Oct 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication untriaged New issue has not been triaged
Projects
None yet
Development

No branches or pull requests

4 participants