RabbitMQ ConnectionFactory wrapper

Question

This is a wrapper around RabbitMQ.Client's ConnectionFactory. Here is the simplest Producer/Consumer RabbitMQ example https://github.com/delaneybrian/jumpstartCS-rabbitmq-csharp/tree/master/1-First-RabbitMQ-App.

I would like to get a code review over that factory method and its dependencies because I feel like the properties Username, Password, CurrentConnection, etc. should not be within the factory because of the single responsibility principle (SRP). There is a lot to comment such as Lazy<T>, etc.

public sealed class RabbitConnectionFactory : IRabbitConnectionFactory
{
    private readonly IConnectionFactory _connectionFactory;
    private readonly Lazy<IConnection> _lazyConnection;
    private readonly Lazy<IModel> _lazyChannel;

    public RabbitConnectionFactory(IConnectionFactory connectionFactory)
    {
        _connectionFactory = connectionFactory;

        _lazyConnection = new Lazy<IConnection>(() =>
            _connectionFactory.CreateConnection(), LazyThreadSafetyMode.ExecutionAndPublication);

        _lazyChannel = new Lazy<IModel>(() =>
            CurrentConnection.CreateModel(), LazyThreadSafetyMode.ExecutionAndPublication);
    }

    public static IRabbitConnectionFactory From<TOptions>(TOptions options) where TOptions : RabbitBaseOptions
    {
        ArgumentNullException.ThrowIfNull(options);
        return new RabbitConnectionFactory(FromConfig(options));
    }

    private static IConnectionFactory FromConfig(RabbitBaseOptions baseOptions)
    {
        ArgumentNullException.ThrowIfNull(baseOptions);

        return new ConnectionFactory
        {
            HostName = baseOptions.Host,
            Port = baseOptions.Port,
            VirtualHost = baseOptions.VirtualHost,
            UserName = baseOptions.User,
            Password = baseOptions.Password,
            RequestedHeartbeat = baseOptions.RequestedHeartbeat,
            Ssl = new SslOption
            {
                Enabled = baseOptions.Tls.Enabled,
                ServerName = baseOptions.Host,
                Version = baseOptions.Tls.Enabled ? baseOptions.Tls.Protocols : SslProtocols.None,
                AcceptablePolicyErrors = baseOptions.Tls.AcceptablePolicyErrors,
                CertificateValidationCallback = baseOptions.Tls.CertificateValidationCallback
            }
        };
    }

    public string UserName => _connectionFactory.UserName;
    public string Password => _connectionFactory.Password;
    public string VirtualHost => _connectionFactory.VirtualHost;

    public IConnection CurrentConnection => _lazyConnection.Value;

    public IModel CurrentChannel => _lazyChannel.Value;

    public IConnection CreateConnection() => _connectionFactory.CreateConnection();

    public IModel CreateModel() => CurrentConnection.CreateModel();

    public void Dispose()
    {
        if (_lazyChannel.IsValueCreated)
        {
            _lazyChannel.Value.Dispose();
        }

        if (_lazyConnection.IsValueCreated)
        {
            _lazyConnection.Value.Dispose();
        }
    }
}

public record RabbitSslOptions
{
    public bool Enabled { get; init; }
    public SslProtocols Protocols { get; init; } = SslProtocols.Tls12;
    public SslPolicyErrors AcceptablePolicyErrors { get; init; } = SslPolicyErrors.None;
    public RemoteCertificateValidationCallback? CertificateValidationCallback { get; set; }
}

public abstract record RabbitBaseOptions
{
    public string Host { get; init; } = string.Empty;

    public string VirtualHost { get; init; } = string.Empty;

    public string User { get; init; } = string.Empty;

    public string Password { get; init; } = string.Empty;

    public int Port { get; init; } = AmqpTcpEndpoint.UseDefaultPort;
    public TimeSpan RequestedHeartbeat { get; set; } = TimeSpan.FromSeconds(60);
    public RabbitSslOptions Tls { get; init; } = new();
}

public interface IRabbitConnectionFactory : IDisposable
{
    /// <summary>
    /// Username to use when authenticating to the server.
    /// </summary>
    string UserName { get; }

    /// <summary>
    /// Password to use when authenticating to the server.
    /// </summary>
    string Password { get; }

    /// <summary>
    /// Virtual host to access during this connection.
    /// </summary>
    string VirtualHost { get; }

    IConnection CurrentConnection { get; }
    IModel CurrentChannel { get; }
    IConnection CreateConnection();
    IModel CreateModel();
}

Answer 1

IRabbitConnectionFactory

Single or Multiple interfaces

• As far as I can see this interface (in some degree) resembles/mimics the RabbitMq's IConnectionFactory
  • For me this mixture of properties (like UserName, Password, etc.) and methods (like CreateConnection) feels a bit odd
• I would suggest to make an alternative version of this, where there are two interfaces (one with the UserName, Password and VirtualHost and another with the rest)

Naming

• You have the following pairs of method - property:
  • CreateConnection, CurrentConnection
  • CreateModel, CurrentChannel
• I would suggest to do some renaming to help the consumer of your util class
  • either CreateChannel
  • or CurrentModel

Connection

• It feels a bit odd that you have the following
  • The CreateConnection uses the RabbitMq's factory directly and creates a new connection every time when you call this method
  • The CurrentConnection uses the RabbitMq's factory directly as well but creates a new connection only at the very first usage then returns the same connection no matter how many times this property is accessed

Model / Channel

• It feels a bit odd that
  • You can use the CreateModel or CurrentChannel without the need to initialize a connection
• I'm unaware of the usage of this interface, but it feels like it would be enough to expose only this method and property pair (no need for the XYZConnections)

RabbitBaseOptions

• As far as I can see you are not taking advantage of the fact that you have defined this structure as record
  • Defining it as an abstract class would be more convenient IMHO
• I assume this structure is meant to be used with ASP.NET Core's Options pattern
  • If that's the case I would consider to use DataAnnotations as well
• From the shared code piece it is unclear why the RequestedHeartbeat is the only mutable property inside this structure
  • If it is intentional then adding some documentation comment might bring clarity
  • If it in unintentional then change the set to init

RabbitSslOptions

• Yet again this could be a simple class because you are not taking advantage of any features of the record
• Why did you define the CertificateValidationCallback in a way that it has a setter rather than being an init-only property?

RabbitConnectionFactory

• Please implement the IDisposable interface as it should be
• The initialization of the lazy fields can be simplified

_lazyConnection = new(_connectionFactory.CreateConnection, LazyThreadSafetyMode.ExecutionAndPublication);
_lazyChannel = new(CurrentConnection.CreateModel, LazyThreadSafetyMode.ExecutionAndPublication);

• Inside the FromConfig I think it is unnecessary to perform a null check against the parameter
  • It is already done inside the From method
• Since the RabbitBaseOptions is defined as abstract that's why you don't need to define the From as generic, simple just

public static IRabbitConnectionFactory From(RabbitBaseOptions options)

---

UPDATE #1

Btw isn't the Dispose pattern supposed to be like that since the class is basically sealed

You are right, I've missed that part this class is marked as sealed.

By you're not getting the advantages of record you mean the properties of the positional records i.e. removing the annoying nullability suggestions/warnings and the fact that it is IEquatable by default?

Yes, you are not taking advantage any of the above (based on the shared code fragment). If you don't need these extras then don't use it.

• A record is just a class with some extra features.
• A record struct will generate a struct with some extra features.

The CreateConnection uses the RabbitMq's factory directly and creates a new connection every time when you call this method, I was kinda confused on what to do with it. What would you suggest to me in order to deal with it?

If you don't need it then do not expose it via your interface. If you do need it then try to shape it in a way that suites your needs. If it is enough to return the lazily initialized (cached) connection every time then do not create a new one for each method call.