From 452f7564aab882360def278e6e76dbaea82ca262 Mon Sep 17 00:00:00 2001 From: Malcolm Nixon Date: Wed, 17 Jun 2026 21:50:42 -0400 Subject: [PATCH 1/2] Fix code quality warnings from CodeQL, Roslyn, and SonarQube - Add CodeQL exclusions for cs/useless-assignment-to-local and cs/missed-ternary-operator targeting generated obj/ files - Inline archiveEntry variable in ZipAssertTests to fix cs/linq/missed-select - Use string interpolation in ProgramTests to fix cs/useless-tostring-call - Add using var to StringWriter locals in CliTests to fix cs/local-not-disposed - Simplify using statement block to using var in CliTests (IDE0063) - Change Errors property return type to ReadOnlyCollection in four CapturingContext test helpers (CA1859) - Extract RunDocumentAssertions and BuildPageText helpers from FileAssertPdfAssert.Run to reduce cognitive complexity (S3776) - Extract RunEntryChecks helper from FileAssertFile.Run to reduce cognitive complexity (S3776) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .github/codeql-config.yml | 10 ++ .../Modeling/FileAssertFile.cs | 59 +++++++----- .../Modeling/FileAssertPdfAssert.cs | 93 ++++++++++++------- .../Cli/CliTests.cs | 25 +++-- .../Modeling/FileAssertHtmlAssertTests.cs | 3 +- .../Modeling/FileAssertJsonAssertTests.cs | 3 +- .../Modeling/FileAssertYamlAssertTests.cs | 3 +- .../Modeling/FileAssertZipAssertTests.cs | 6 +- .../ProgramTests.cs | 2 +- 9 files changed, 127 insertions(+), 77 deletions(-) diff --git a/.github/codeql-config.yml b/.github/codeql-config.yml index d3b36b2..62a97ce 100644 --- a/.github/codeql-config.yml +++ b/.github/codeql-config.yml @@ -16,3 +16,13 @@ query-filters: id: cs/path-combine paths: - src/DemaConsulting.FileAssert/PathHelpers.cs + # Exclude useless-assignment warnings in generated code under obj/ directories + - exclude: + id: cs/useless-assignment-to-local + paths: + - "**/obj/**" + # Exclude missed-ternary-operator warnings in generated code under obj/ directories + - exclude: + id: cs/missed-ternary-operator + paths: + - "**/obj/**" diff --git a/src/DemaConsulting.FileAssert/Modeling/FileAssertFile.cs b/src/DemaConsulting.FileAssert/Modeling/FileAssertFile.cs index 0251a4b..c474b89 100644 --- a/src/DemaConsulting.FileAssert/Modeling/FileAssertFile.cs +++ b/src/DemaConsulting.FileAssert/Modeling/FileAssertFile.cs @@ -239,34 +239,45 @@ internal void Run(IContext context, IFileContainer container) { foreach (var entryPath in files) { - // Enforce size constraints when specified - if (MinSize.HasValue || MaxSize.HasValue) - { - var size = container.GetEntrySize(entryPath); - var displayPath = container.GetDisplayPath(entryPath); + RunEntryChecks(context, container, entryPath); + } + } + } - if (MinSize.HasValue && size < MinSize.Value) - { - context.WriteError( - $"File '{displayPath}' is {size} byte(s), which is less than the minimum {MinSize.Value} bytes"); - } + /// + /// Runs size and file-type assertions for a single matched entry, reporting violations. + /// + /// The context used for reporting errors. + /// The container that holds the entry. + /// The relative path of the entry within the container. + private void RunEntryChecks(IContext context, IFileContainer container, string entryPath) + { + // Enforce size constraints when specified + if (MinSize.HasValue || MaxSize.HasValue) + { + var size = container.GetEntrySize(entryPath); + var displayPath = container.GetDisplayPath(entryPath); - if (MaxSize.HasValue && size > MaxSize.Value) - { - context.WriteError( - $"File '{displayPath}' is {size} byte(s), which exceeds the maximum {MaxSize.Value} bytes"); - } - } + if (MinSize.HasValue && size < MinSize.Value) + { + context.WriteError( + $"File '{displayPath}' is {size} byte(s), which is less than the minimum {MinSize.Value} bytes"); + } - // Delegate to each file-type assert unit when declared - TextAssert?.Run(context, container, entryPath); - PdfAssert?.Run(context, container, entryPath); - XmlAssert?.Run(context, container, entryPath); - HtmlAssert?.Run(context, container, entryPath); - YamlAssert?.Run(context, container, entryPath); - JsonAssert?.Run(context, container, entryPath); - ZipAssert?.Run(context, container, entryPath); + if (MaxSize.HasValue && size > MaxSize.Value) + { + context.WriteError( + $"File '{displayPath}' is {size} byte(s), which exceeds the maximum {MaxSize.Value} bytes"); } } + + // Delegate to each file-type assert unit when declared + TextAssert?.Run(context, container, entryPath); + PdfAssert?.Run(context, container, entryPath); + XmlAssert?.Run(context, container, entryPath); + HtmlAssert?.Run(context, container, entryPath); + YamlAssert?.Run(context, container, entryPath); + JsonAssert?.Run(context, container, entryPath); + ZipAssert?.Run(context, container, entryPath); } } diff --git a/src/DemaConsulting.FileAssert/Modeling/FileAssertPdfAssert.cs b/src/DemaConsulting.FileAssert/Modeling/FileAssertPdfAssert.cs index 1ee603f..f672c10 100644 --- a/src/DemaConsulting.FileAssert/Modeling/FileAssertPdfAssert.cs +++ b/src/DemaConsulting.FileAssert/Modeling/FileAssertPdfAssert.cs @@ -24,6 +24,7 @@ using DemaConsulting.FileAssert.Configuration; using DemaConsulting.FileAssert.Utilities; using UglyToad.PdfPig; +using UglyToad.PdfPig.Content; namespace DemaConsulting.FileAssert.Modeling; @@ -275,43 +276,69 @@ internal void Run(IContext context, IFileContainer container, string entryPath) using (document) { - // Apply metadata assertions to the document information - foreach (var rule in _metadata) - { - var value = GetMetadataField(document, rule.Field); - rule.Apply(context, displayPath, value); - } + RunDocumentAssertions(context, displayPath, document); + } + } + + /// + /// Applies all configured metadata, page-count, and text assertions to an already-opened + /// PDF document, reporting violations via . + /// + /// The context used for reporting errors. + /// The display path of the file, used in error messages. + /// The opened PDF document to assert against. + private void RunDocumentAssertions(IContext context, string displayPath, PdfDocument document) + { + // Apply metadata assertions to the document information + foreach (var rule in _metadata) + { + var value = GetMetadataField(document, rule.Field); + rule.Apply(context, displayPath, value); + } + + // Skip page and text checks when no such constraints are configured + if (_pages == null && _text.Count == 0) + { + return; + } - // Apply page count constraints and collect pages only when needed - if (_pages != null || _text.Count > 0) + // Apply page count constraints and collect pages only when needed + var pageList = document.GetPages().ToList(); + _pages?.Apply(context, displayPath, pageList.Count); + + // Apply text rules to the extracted body text when rules are defined + if (_text.Count == 0) + { + return; + } + + var content = BuildPageText(pageList); + foreach (var rule in _text) + { + rule.Apply(context, displayPath, content); + } + } + + /// + /// Concatenates the text from all pages into a single string, separating pages with newlines + /// so that text rules do not see words from adjacent pages merged together. + /// + /// The ordered list of pages from the PDF document. + /// A single string containing all page text joined with newline separators. + private static string BuildPageText(IReadOnlyList pages) + { + var sb = new StringBuilder(); + for (var i = 0; i < pages.Count; i++) + { + if (i > 0) { - var pageList = document.GetPages().ToList(); - _pages?.Apply(context, displayPath, pageList.Count); - - // Apply text rules to the extracted body text when rules are defined - if (_text.Count > 0) - { - var sb = new StringBuilder(); - for (var i = 0; i < pageList.Count; i++) - { - if (i > 0) - { - // Separate page text with newlines so that text rules don't - // see two adjacent words from different pages glued together. - sb.Append('\n'); - } - - sb.Append(pageList[i].Text); - } - - var content = sb.ToString(); - foreach (var rule in _text) - { - rule.Apply(context, displayPath, content); - } - } + sb.Append('\n'); } + + sb.Append(pages[i].Text); } + + return sb.ToString(); } /// diff --git a/test/DemaConsulting.FileAssert.Tests/Cli/CliTests.cs b/test/DemaConsulting.FileAssert.Tests/Cli/CliTests.cs index 70e9288..b176193 100644 --- a/test/DemaConsulting.FileAssert.Tests/Cli/CliTests.cs +++ b/test/DemaConsulting.FileAssert.Tests/Cli/CliTests.cs @@ -40,21 +40,20 @@ public void Cli_CreateContext_ParsesSilentValidateAndLogFlags() var logPath = tempDir.GetFilePath("out.log"); // Act - create a context with the silent, validate, and log flags - using (var context = Context.Create( + using var context = Context.Create( [ "--silent", "--validate", "--log", logPath - ])) - { - // Assert - all flags are reflected in the context properties - Assert.True(context.Silent); - Assert.True(context.Validate); - Assert.False(context.Version); - Assert.False(context.Help); - Assert.Equal(".fileassert.yaml", context.ConfigFile); - Assert.Equal(0, context.ExitCode); - } + ]); + + // Assert - all flags are reflected in the context properties + Assert.True(context.Silent); + Assert.True(context.Validate); + Assert.False(context.Version); + Assert.False(context.Help); + Assert.Equal(".fileassert.yaml", context.ConfigFile); + Assert.Equal(0, context.ExitCode); } @@ -137,8 +136,8 @@ public void Cli_OutputPipeline_WithoutSilentFlag_WritesMessagesToConsole() // Arrange var originalOut = Console.Out; var originalError = Console.Error; - var outWriter = new System.IO.StringWriter(); - var errorWriter = new System.IO.StringWriter(); + using var outWriter = new System.IO.StringWriter(); + using var errorWriter = new System.IO.StringWriter(); try { diff --git a/test/DemaConsulting.FileAssert.Tests/Modeling/FileAssertHtmlAssertTests.cs b/test/DemaConsulting.FileAssert.Tests/Modeling/FileAssertHtmlAssertTests.cs index 36d50b6..37b4714 100644 --- a/test/DemaConsulting.FileAssert.Tests/Modeling/FileAssertHtmlAssertTests.cs +++ b/test/DemaConsulting.FileAssert.Tests/Modeling/FileAssertHtmlAssertTests.cs @@ -18,6 +18,7 @@ // OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE // SOFTWARE. +using System.Collections.ObjectModel; using DemaConsulting.FileAssert.Cli; using DemaConsulting.FileAssert.Configuration; using DemaConsulting.FileAssert.Modeling; @@ -471,7 +472,7 @@ private sealed class CapturingContext : IContext private readonly List _errors = []; /// Gets all error messages captured since this context was created. - public IReadOnlyList Errors => _errors.AsReadOnly(); + public ReadOnlyCollection Errors => _errors.AsReadOnly(); /// public void WriteLine(string message) { } diff --git a/test/DemaConsulting.FileAssert.Tests/Modeling/FileAssertJsonAssertTests.cs b/test/DemaConsulting.FileAssert.Tests/Modeling/FileAssertJsonAssertTests.cs index 309f708..f5c9611 100644 --- a/test/DemaConsulting.FileAssert.Tests/Modeling/FileAssertJsonAssertTests.cs +++ b/test/DemaConsulting.FileAssert.Tests/Modeling/FileAssertJsonAssertTests.cs @@ -18,6 +18,7 @@ // OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE // SOFTWARE. +using System.Collections.ObjectModel; using DemaConsulting.FileAssert.Cli; using DemaConsulting.FileAssert.Configuration; using DemaConsulting.FileAssert.Modeling; @@ -400,7 +401,7 @@ private sealed class CapturingContext : IContext private readonly List _errors = []; /// Gets all error messages captured since this context was created. - public IReadOnlyList Errors => _errors.AsReadOnly(); + public ReadOnlyCollection Errors => _errors.AsReadOnly(); /// public void WriteLine(string message) { } diff --git a/test/DemaConsulting.FileAssert.Tests/Modeling/FileAssertYamlAssertTests.cs b/test/DemaConsulting.FileAssert.Tests/Modeling/FileAssertYamlAssertTests.cs index fd4cc89..27333f7 100644 --- a/test/DemaConsulting.FileAssert.Tests/Modeling/FileAssertYamlAssertTests.cs +++ b/test/DemaConsulting.FileAssert.Tests/Modeling/FileAssertYamlAssertTests.cs @@ -18,6 +18,7 @@ // OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE // SOFTWARE. +using System.Collections.ObjectModel; using DemaConsulting.FileAssert.Cli; using DemaConsulting.FileAssert.Configuration; using DemaConsulting.FileAssert.Modeling; @@ -454,7 +455,7 @@ private sealed class CapturingContext : IContext private readonly List _errors = []; /// Gets all error messages captured since this context was created. - public IReadOnlyList Errors => _errors.AsReadOnly(); + public ReadOnlyCollection Errors => _errors.AsReadOnly(); /// public void WriteLine(string message) { } diff --git a/test/DemaConsulting.FileAssert.Tests/Modeling/FileAssertZipAssertTests.cs b/test/DemaConsulting.FileAssert.Tests/Modeling/FileAssertZipAssertTests.cs index 37c3a60..3cba266 100644 --- a/test/DemaConsulting.FileAssert.Tests/Modeling/FileAssertZipAssertTests.cs +++ b/test/DemaConsulting.FileAssert.Tests/Modeling/FileAssertZipAssertTests.cs @@ -18,6 +18,7 @@ // OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE // SOFTWARE. +using System.Collections.ObjectModel; using System.IO.Compression; using DemaConsulting.FileAssert.Cli; using DemaConsulting.FileAssert.Configuration; @@ -83,8 +84,7 @@ private static void CreateZipFile(string path, IEnumerable entries) using var archive = ZipFile.Open(path, ZipArchiveMode.Create); foreach (var entry in entries) { - var archiveEntry = archive.CreateEntry(entry); - using var stream = archiveEntry.Open(); + using var stream = archive.CreateEntry(entry).Open(); // Write a single placeholder byte so the entry is not an empty-stream edge case stream.WriteByte(0x00); @@ -184,7 +184,7 @@ private sealed class CapturingContext : IContext private readonly List _errors = []; /// Gets all error messages captured since this context was created. - public IReadOnlyList Errors => _errors.AsReadOnly(); + public ReadOnlyCollection Errors => _errors.AsReadOnly(); /// public void WriteLine(string message) { } diff --git a/test/DemaConsulting.FileAssert.Tests/ProgramTests.cs b/test/DemaConsulting.FileAssert.Tests/ProgramTests.cs index 375e8ad..79e5a34 100644 --- a/test/DemaConsulting.FileAssert.Tests/ProgramTests.cs +++ b/test/DemaConsulting.FileAssert.Tests/ProgramTests.cs @@ -203,7 +203,7 @@ public void Program_Run_ExplicitConfigMissing_WritesError() Program.Run(context); // Assert - var combined = outWriter.ToString() + errWriter.ToString(); + var combined = $"{outWriter}{errWriter}"; Assert.Contains("Configuration file not found", combined); Assert.Equal(1, context.ExitCode); } From f88b4fb52768d98d066226a7f3322fc8a7768c48 Mon Sep 17 00:00:00 2001 From: Malcolm Nixon Date: Wed, 17 Jun 2026 22:10:43 -0400 Subject: [PATCH 2/2] Avoid materializing Page objects when only page count is needed When only a page-count constraint is configured (no text rules), enumerate the pages with Count() rather than ToList() to avoid allocating Page objects unnecessarily for large PDFs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../Modeling/FileAssertPdfAssert.cs | 23 ++++++++++--------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/src/DemaConsulting.FileAssert/Modeling/FileAssertPdfAssert.cs b/src/DemaConsulting.FileAssert/Modeling/FileAssertPdfAssert.cs index f672c10..113af30 100644 --- a/src/DemaConsulting.FileAssert/Modeling/FileAssertPdfAssert.cs +++ b/src/DemaConsulting.FileAssert/Modeling/FileAssertPdfAssert.cs @@ -302,20 +302,21 @@ private void RunDocumentAssertions(IContext context, string displayPath, PdfDocu return; } - // Apply page count constraints and collect pages only when needed - var pageList = document.GetPages().ToList(); - _pages?.Apply(context, displayPath, pageList.Count); - - // Apply text rules to the extracted body text when rules are defined - if (_text.Count == 0) + // When text rules are present, materialize pages once for both count and content. + // When only a page-count constraint is configured, enumerate without allocating Page objects. + if (_text.Count > 0) { - return; + var pageList = document.GetPages().ToList(); + _pages?.Apply(context, displayPath, pageList.Count); + var content = BuildPageText(pageList); + foreach (var rule in _text) + { + rule.Apply(context, displayPath, content); + } } - - var content = BuildPageText(pageList); - foreach (var rule in _text) + else { - rule.Apply(context, displayPath, content); + _pages?.Apply(context, displayPath, document.GetPages().Count()); } }