Settings cleanup and improvements (#1901)

* Make initialization of setings less error prone

* Throw expression

* Unneeded nullable

* Range

* Remove no longer needed log error

---------

Co-authored-by: Daniel Marbach <danielmarbach@users.noreply.github.com>

Author: danielmarbach

src/ScriptBuilder.Tests/ApprovalFiles/APIApprovals.Approve.approved.txt

@@ -88,7 +88,7 @@ public class ScriptGenerator
 {
     public ScriptGenerator(string assemblyPath, string destinationDirectory, bool clean = true, bool overwrite = true, System.Collections.Generic.IReadOnlyList<NServiceBus.Persistence.Sql.ScriptBuilder.BuildSqlDialect>? dialectOptions = null, System.Func<string, string>? promotionFinder = null) { }
     public void Generate() { }
-    public static void Generate(string assemblyPath, string targetDirectory, System.Action<string, string> logError, System.Func<string, string> promotionPathFinder) { }
+    public static void Generate(string assemblyPath, string targetDirectory, System.Func<string, string> promotionPathFinder) { }
 }
 public abstract class ScriptWriter
 {

src/ScriptBuilder.Tests/SqlAttributeParametersReadersTest.cs

@@ -15,7 +15,7 @@ public void Minimal()
                         //At least one is required
                         "MsSqlServerScripts", true
                     }
-                });
+                }, []);
 
         Assert.That(result, Is.Not.Null);
         Approver.Verify(result);
@@ -24,7 +24,7 @@ public void Minimal()
     [Test]
     public void Defaults()
     {
-        var result = SettingsAttributeReader.ReadFromProperties([]);
+        var result = SettingsAttributeReader.ReadFromProperties([], []);
         Assert.That(result, Is.Not.Null);
         Approver.Verify(result);
     }
@@ -59,10 +59,8 @@ public void NonDefaults()
                     {
                         "ProduceOutboxScripts", false
                     }
-                });
+                }, []);
         Assert.That(result, Is.Not.Null);
         Approver.Verify(result);
     }
-
-
 }

src/ScriptBuilder/AttributeReading/Settings.cs

@@ -3,19 +3,19 @@
 using System.Collections.Generic;
 using NServiceBus.Persistence.Sql.ScriptBuilder;
 
-class Settings
+sealed class Settings
 {
-    public List<BuildSqlDialect> BuildDialects { get; set; } = [];
+    public required IReadOnlyCollection<BuildSqlDialect> BuildDialects { get; init; }
 
-    public string? ScriptPromotionPath { get; set; }
+    public string? ScriptPromotionPath { get; init; }
 
-    public bool ProduceOutboxScripts { get; set; }
+    public required bool ProduceOutboxScripts { get; init; }
 
-    public bool ProduceSubscriptionScripts { get; set; }
+    public required bool ProduceSubscriptionScripts { get; init; }
 
-    public bool ProduceTimeoutScripts { get; set; }
+    public required bool ProduceTimeoutScripts { get; init; }
 
-    public bool ProduceSagaScripts { get; set; }
+    public required bool ProduceSagaScripts { get; init; }
 
-    public List<SagaDefinition> SagaDefinitions { get; set; } = [];
+    public required IReadOnlyCollection<SagaDefinition> SagaDefinitions { get; init; }
 }

src/ScriptBuilder/AttributeReading/SettingsAttributeReader.cs

@@ -13,8 +13,8 @@ static class SettingsAttributeReader
 {
     public static Settings Read(string assemblyPath)
     {
-        Settings? settings = null;
-        List<SagaDefinition> sagas = [];
+        List<SagaDefinition> sagaDefinitions = [];
+        Dictionary<string, object?>? properties = null;
 
         using var stream = File.OpenRead(assemblyPath);
         using var peReader = new PEReader(stream);
@@ -37,8 +37,12 @@ public static Settings Read(string assemblyPath)
                     if (attNamespace == "NServiceBus.Persistence.Sql")
                     {
                         var args = attribute.DecodeValue(AttributeTypeProvider.Instance);
-                        var properties = args.NamedArguments.ToDictionary(o => o.Name!, o => o.Value);
-                        settings = ReadFromProperties(properties);
+                        // This assembly level attribute can only exists once per assembly. This is an additional safe guard to make this code more intention revealing
+                        if (properties is not null)
+                        {
+                            throw new InvalidOperationException("Only one SqlPersistenceSettingsAttribute can be defined per assembly");
+                        }
+                        properties = args.NamedArguments.ToDictionary(o => o.Name!, o => o.Value);
                     }
                 }
             }
@@ -54,48 +58,43 @@ public static Settings Read(string assemblyPath)
                     if (string.IsNullOrEmpty(attNamespace))
                     {
                         var args = attribute.DecodeValue(AttributeTypeProvider.Instance);
-                        var properties = args.NamedArguments.ToDictionary(o => o.Name!, o => o.Value);
+                        var sagaProperties = args.NamedArguments.ToDictionary(o => o.Name!, o => o.Value);
 
-                        var sagaType = properties.GetValueOrDefault("SagaType") as string;
-                        var corrName = properties.GetValueOrDefault("CorrelationPropertyName") as string;
-                        var corrType = properties.GetValueOrDefault("CorrelationPropertyType") as string;
-                        var transName = properties.GetValueOrDefault("TransitionalCorrelationPropertyName") as string;
-                        var transType = properties.GetValueOrDefault("TransitionalCorrelationPropertyType") as string;
-                        var tableSuffix = properties.GetValueOrDefault("TableSuffix") as string;
+                        var sagaType = sagaProperties.GetValueOrDefault("SagaType") as string;
+                        var corrName = sagaProperties.GetValueOrDefault("CorrelationPropertyName") as string;
+                        var corrType = sagaProperties.GetValueOrDefault("CorrelationPropertyType") as string;
+                        var transName = sagaProperties.GetValueOrDefault("TransitionalCorrelationPropertyName") as string;
+                        var transType = sagaProperties.GetValueOrDefault("TransitionalCorrelationPropertyType") as string;
+                        var tableSuffix = sagaProperties.GetValueOrDefault("TableSuffix") as string;
 
                         var correlation = GetCorrelation(corrName, corrType);
                         var transitionalCorrelation = GetCorrelation(transName, transType);
 
                         if (tableSuffix is not null && sagaType is not null)
                         {
                             var definition = new SagaDefinition(tableSuffix, sagaType, correlation, transitionalCorrelation);
-                            sagas.Add(definition);
+                            sagaDefinitions.Add(definition);
                         }
                     }
                 }
             }
         }
 
         // If no attribute, generate the default
-        settings ??= ReadFromProperties([]);
-
-        // Then apply the saga definitions
-        settings.SagaDefinitions = sagas;
-        return settings;
+        return ReadFromProperties(properties ?? [], sagaDefinitions);
     }
 
-    public static Settings ReadFromProperties(Dictionary<string, object?> properties)
-    {
-        return new Settings
+    public static Settings ReadFromProperties(Dictionary<string, object?> properties, IReadOnlyCollection<SagaDefinition> sagas) =>
+        new()
         {
             BuildDialects = ReadBuildDialects(properties).ToList(),
             ScriptPromotionPath = ReadScriptPromotionPath(properties),
             ProduceSagaScripts = GetBoolProperty(properties, "ProduceSagaScripts", true),
             ProduceTimeoutScripts = GetBoolProperty(properties, "ProduceTimeoutScripts", true),
             ProduceSubscriptionScripts = GetBoolProperty(properties, "ProduceSubscriptionScripts", true),
             ProduceOutboxScripts = GetBoolProperty(properties, "ProduceOutboxScripts", true),
+            SagaDefinitions = sagas
         };
-    }
 
     static bool GetBoolProperty(Dictionary<string, object?> properties, string key, bool defaultValue = false)
         => properties.GetValueOrDefault(key) as bool? ?? defaultValue;
@@ -157,12 +156,7 @@ static IEnumerable<BuildSqlDialect> ReadBuildDialects(Dictionary<string, object?
             return null;
         }
 
-        if (!Enum.TryParse<CorrelationPropertyType>(type, out var propType))
-        {
-            throw new Exception($"Invalid correlation property type '{type}' found in metadata attribute.");
-        }
-
-        return new CorrelationProperty(name, propType);
+        return !Enum.TryParse<CorrelationPropertyType>(type, out var propType) ? throw new Exception($"Invalid correlation property type '{type}' found in metadata attribute.") : new CorrelationProperty(name, propType);
     }
 
     // Minimal implementation that only supports primitive types
@@ -172,8 +166,8 @@ sealed class AttributeTypeProvider : ICustomAttributeTypeProvider<object?>
 
         public static readonly AttributeTypeProvider Instance = new();
 
-        public object? GetPrimitiveType(PrimitiveTypeCode typeCode) => typeCode;
-        public object? GetSystemType() => typeof(Type);
+        public object GetPrimitiveType(PrimitiveTypeCode typeCode) => typeCode;
+        public object GetSystemType() => typeof(Type);
         public object? GetTypeFromDefinition(MetadataReader r, TypeDefinitionHandle h, byte raw) => null;
         public object? GetTypeFromReference(MetadataReader r, TypeReferenceHandle h, byte raw) => null;
         public object? GetSZArrayType(object? elementType) => null;

src/ScriptBuilder/ScriptGenerator.cs

@@ -14,8 +14,7 @@ public class ScriptGenerator(string assemblyPath,
     IReadOnlyList<BuildSqlDialect>? dialectOptions = null,
     Func<string, string>? promotionFinder = null)
 {
-    public static void Generate(string assemblyPath, string targetDirectory,
-        Action<string, string> logError, Func<string, string> promotionPathFinder)
+    public static void Generate(string assemblyPath, string targetDirectory, Func<string, string> promotionPathFinder)
     {
         var writer = new ScriptGenerator(assemblyPath, targetDirectory, promotionFinder: promotionPathFinder);
         writer.Generate();

src/ScriptBuilder/Writers/SagaWriter.cs

@@ -7,7 +7,7 @@
 class SagaWriter(bool clean,
     bool overwrite,
     string scriptPath,
-    List<SagaDefinition> sagaDefinitions)
+    IReadOnlyCollection<SagaDefinition> sagaDefinitions)
     : ScriptWriter(clean, overwrite, scriptPath)
 {
     public override void WriteScripts(BuildSqlDialect dialect)
@@ -21,7 +21,7 @@ public override void WriteScripts(BuildSqlDialect dialect)
             var maximumNameLength = 244 - ScriptPath.Length;
             if (sagaFileName.Length > maximumNameLength)
             {
-                sagaFileName = $"{sagaFileName.Substring(0, maximumNameLength)}_{index}";
+                sagaFileName = $"{sagaFileName[..maximumNameLength]}_{index}";
                 index++;
             }
 

src/ScriptBuilderTask.Tests/InnerTaskTests.cs

@@ -1,5 +1,4 @@
-using System;
-using System.IO;
+using System.IO;
 using System.Linq;
 using NUnit.Framework;
 using Particular.Approvals;
@@ -25,8 +24,7 @@ public void IntegrationTest()
             assemblyPath: Path.Combine(testDirectory, "ScriptBuilderTask.Tests.Target.dll"),
             intermediateDirectory: intermediatePath,
             projectDirectory: "TheProjectDir",
-            solutionDirectory: Path.Combine(temp, "PromotePath"),
-            logError: (error, type) => throw new Exception(error));
+            solutionDirectory: Path.Combine(temp, "PromotePath"));
         innerTask.Execute();
         var files = Directory.EnumerateFiles(temp, "*.*", SearchOption.AllDirectories)
                         .Select(s => s.Replace(temp, "temp").ConvertPathSeparators("/"))

src/ScriptBuilderTask/InnerTask.cs

@@ -1,28 +1,9 @@
-using System;
-using System.IO;
+using System.IO;
 using NServiceBus.Persistence.Sql;
 
-class InnerTask
+sealed class InnerTask(string assemblyPath, string intermediateDirectory, string projectDirectory, string solutionDirectory)
 {
-    string assemblyPath;
-    string intermediateDirectory;
-    string projectDirectory;
-    string solutionDirectory;
-    Action<string, string> logError;
-
-    public InnerTask(string assemblyPath, string intermediateDirectory, string projectDirectory, string solutionDirectory, Action<string, string> logError)
-    {
-        this.assemblyPath = assemblyPath;
-        this.intermediateDirectory = intermediateDirectory;
-        this.projectDirectory = projectDirectory;
-        this.solutionDirectory = solutionDirectory;
-        this.logError = logError;
-    }
-
-    public void Execute()
-    {
-        ScriptGenerator.Generate(assemblyPath, intermediateDirectory, logError, FindPromotionPath);
-    }
+    public void Execute() => ScriptGenerator.Generate(assemblyPath, intermediateDirectory, FindPromotionPath);
 
     string FindPromotionPath(string promotionPath)
     {

src/ScriptBuilderTask/SqlPersistenceScriptBuilderTask.cs

@@ -34,11 +34,7 @@ public override bool Execute()
             try
             {
                 ValidateInputs();
-                var innerTask = new InnerTask(AssemblyPath, IntermediateDirectory, ProjectDirectory, SolutionDirectory,
-                    logError: (error, file) =>
-                    {
-                        logger.LogError(error, file);
-                    });
+                var innerTask = new InnerTask(AssemblyPath, IntermediateDirectory, ProjectDirectory, SolutionDirectory);
                 innerTask.Execute();
             }
             catch (ErrorsException exception)