Skip to content

chore: Skip unnecessary work for empty or null sequences#1601

Merged
HofmeisterAn merged 1 commit intodevelopfrom
feature/avoid-allocation-for-builder-configurations
Dec 7, 2025
Merged

chore: Skip unnecessary work for empty or null sequences#1601
HofmeisterAn merged 1 commit intodevelopfrom
feature/avoid-allocation-for-builder-configurations

Conversation

@HofmeisterAn
Copy link
Collaborator

What does this PR do?

The PR slightly adjusts the build configuration (Combine(...) method) implementation and skips unnecessary work for empty or null sequences. These methods are called frequently, so this change slightly improves performance.

Why is it important?

-

Related issues

-

@HofmeisterAn HofmeisterAn added the chore A change that doesn't impact the existing functionality, e.g. internal refactorings or cleanups label Dec 7, 2025
@netlify
Copy link

netlify bot commented Dec 7, 2025

Deploy Preview for testcontainers-dotnet ready!

Name Link
🔨 Latest commit bd190a1
🔍 Latest deploy log https://app.netlify.com/projects/testcontainers-dotnet/deploys/6935d530a69cfe0008526bd3
😎 Deploy Preview https://deploy-preview-1601--testcontainers-dotnet.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@coderabbitai
Copy link

coderabbitai bot commented Dec 7, 2025

Summary by CodeRabbit

  • Removed Features

    • Removed EventStoreDb project from the solution
  • Improvements

    • Enhanced configuration combination logic with improved null and empty value handling for greater robustness

✏️ Tip: You can customize this high-level summary in your review settings.

Walkthrough

This PR removes the EventStoreDb project reference from the solution file and enhances configuration merging logic with explicit null and emptiness handling across multiple overloads. Supporting changes include parameter documentation, LINQ simplification, and internal field renaming in test utilities.

Changes

Cohort / File(s) Summary
Solution Configuration
Testcontainers.slnx
Removed project reference to src/Testcontainers.EventStoreDb/Testcontainers.EventStoreDb.csproj
Configuration Merging Logic
src/Testcontainers/Builders/BuildConfiguration.cs
Enhanced null and empty collection handling in four Combine overloads: explicit checks for null/empty values before concatenation or composition, and normalization of non-collection inputs to arrays
Configuration Documentation
src/Testcontainers/Configurations/Containers/ContainerConfiguration.cs
Added XML documentation for connectionStringProvider constructor parameter
Test Utilities
tests/Testcontainers.Commons/DockerfileParser.cs
Simplified LINQ chain by replacing DefaultIfEmpty().First() with FirstOrDefault(string.Empty)
Test Utilities
tests/Testcontainers.Commons/TestSession.cs
Renamed private field from Cache to Stages with consistent updates to access points

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • BuildConfiguration.cs: Review null-handling logic and empty-collection checks across all four Combine overloads for consistency and correctness; verify composition behavior change in ComposableEnumerable<T> overload
  • EventStoreDb removal: Confirm no orphaned references remain in other solution files or project configurations

Possibly related PRs

  • chore: Remove EventStoreDb module #1599: Removes the Testcontainers.EventStoreDb module entirely, directly aligning with this PR's removal of the project reference from the solution file.
  • feat: Add KurrentDb module #1583: Deprecates EventStoreDb and introduces KurrentDb replacement; related through the same EventStoreDb project removal timeline.
  • feat: Add Grafana module #1509: Modifies BuildConfiguration.Combine overloads with similar null-handling enhancements; impacts configuration merging used throughout the codebase.

Poem

🐰 EventStore has hopped away,
Config logic shines more bright today,
Null checks dance and empties clear—
Cleaner merges drawing near! ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
Description check ❓ Inconclusive The description addresses the mandatory sections (WHAT and WHY), though the WHY section only contains a dash. Missing details on testing approach and no related issues linked despite performance optimization changes. Expand the WHY section to explain the performance rationale and provide testing guidance for reviewers validating the performance improvements.
✅ Passed checks (1 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the main optimization change (skipping work for empty/null sequences) which is the primary focus of the PR across multiple Combine() method overloads.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feature/avoid-allocation-for-builder-configurations

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between dc28b26 and bd190a1.

📒 Files selected for processing (5)
  • Testcontainers.slnx (0 hunks)
  • src/Testcontainers/Builders/BuildConfiguration.cs (3 hunks)
  • src/Testcontainers/Configurations/Containers/ContainerConfiguration.cs (1 hunks)
  • tests/Testcontainers.Commons/DockerfileParser.cs (1 hunks)
  • tests/Testcontainers.Commons/TestSession.cs (2 hunks)
💤 Files with no reviewable changes (1)
  • Testcontainers.slnx
🧰 Additional context used
🧬 Code graph analysis (1)
tests/Testcontainers.Commons/TestSession.cs (1)
tests/Testcontainers.Commons/DockerfileParser.cs (3)
  • Dictionary (19-50)
  • DockerfileParser (3-51)
  • DockerfileParser (9-17)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (66)
  • GitHub Check: ci (Testcontainers.Azurite, ubuntu-24.04)
  • GitHub Check: ci (Testcontainers.Cassandra, ubuntu-24.04)
  • GitHub Check: ci (Testcontainers.Bigtable, ubuntu-24.04)
  • GitHub Check: ci (Testcontainers.BigQuery, ubuntu-24.04)
  • GitHub Check: ci (Testcontainers.ActiveMq, ubuntu-24.04)
  • GitHub Check: ci (Testcontainers.ArangoDb, ubuntu-24.04)
  • GitHub Check: analyze (csharp)
  • GitHub Check: ci (Testcontainers.Azurite, ubuntu-24.04)
  • GitHub Check: ci (Testcontainers.Cassandra, ubuntu-24.04)
  • GitHub Check: ci (Testcontainers.Bigtable, ubuntu-24.04)
  • GitHub Check: ci (Testcontainers.BigQuery, ubuntu-24.04)
  • GitHub Check: ci (Testcontainers.ActiveMq, ubuntu-24.04)
  • GitHub Check: ci (Testcontainers.ArangoDb, ubuntu-24.04)
  • GitHub Check: analyze (csharp)
  • GitHub Check: ci (Testcontainers.Azurite, ubuntu-24.04)
  • GitHub Check: ci (Testcontainers.Cassandra, ubuntu-24.04)
  • GitHub Check: ci (Testcontainers.Bigtable, ubuntu-24.04)
  • GitHub Check: ci (Testcontainers.ActiveMq, ubuntu-24.04)
  • GitHub Check: ci (Testcontainers.ArangoDb, ubuntu-24.04)
  • GitHub Check: analyze (csharp)
  • GitHub Check: ci (Testcontainers.Azurite, ubuntu-24.04)
  • GitHub Check: ci (Testcontainers.Cassandra, ubuntu-24.04)
  • GitHub Check: ci (Testcontainers.Bigtable, ubuntu-24.04)
  • GitHub Check: ci (Testcontainers.ActiveMq, ubuntu-24.04)
  • GitHub Check: ci (Testcontainers.ArangoDb, ubuntu-24.04)
  • GitHub Check: analyze (csharp)
  • GitHub Check: ci (Testcontainers.ClickHouse, ubuntu-24.04)
  • GitHub Check: ci (Testcontainers.Azurite, ubuntu-24.04)
  • GitHub Check: ci (Testcontainers.Cassandra, ubuntu-24.04)
  • GitHub Check: ci (Testcontainers.Bigtable, ubuntu-24.04)
  • GitHub Check: ci (Testcontainers.ActiveMq, ubuntu-24.04)
  • GitHub Check: ci (Testcontainers.ArangoDb, ubuntu-24.04)
  • GitHub Check: analyze (csharp)
  • GitHub Check: ci (Testcontainers.ClickHouse, ubuntu-24.04)
  • GitHub Check: ci (Testcontainers.Azurite, ubuntu-24.04)
  • GitHub Check: ci (Testcontainers.Cassandra, ubuntu-24.04)
  • GitHub Check: ci (Testcontainers.Bigtable, ubuntu-24.04)
  • GitHub Check: ci (Testcontainers.ActiveMq, ubuntu-24.04)
  • GitHub Check: ci (Testcontainers.ArangoDb, ubuntu-24.04)
  • GitHub Check: analyze (csharp)
  • GitHub Check: ci (Testcontainers.ClickHouse, ubuntu-24.04)
  • GitHub Check: ci (Testcontainers.Azurite, ubuntu-24.04)
  • GitHub Check: ci (Testcontainers.Cassandra, ubuntu-24.04)
  • GitHub Check: ci (Testcontainers.Bigtable, ubuntu-24.04)
  • GitHub Check: ci (Testcontainers.ActiveMq, ubuntu-24.04)
  • GitHub Check: ci (Testcontainers.ArangoDb, ubuntu-24.04)
  • GitHub Check: analyze (csharp)
  • GitHub Check: ci (Testcontainers.ClickHouse, ubuntu-24.04)
  • GitHub Check: ci (Testcontainers.Azurite, ubuntu-24.04)
  • GitHub Check: ci (Testcontainers.Cassandra, ubuntu-24.04)
  • GitHub Check: ci (Testcontainers.Bigtable, ubuntu-24.04)
  • GitHub Check: ci (Testcontainers.ActiveMq, ubuntu-24.04)
  • GitHub Check: ci (Testcontainers.ArangoDb, ubuntu-24.04)
  • GitHub Check: analyze (csharp)
  • GitHub Check: ci (Testcontainers.ClickHouse, ubuntu-24.04)
  • GitHub Check: ci (Testcontainers.Cassandra, ubuntu-24.04)
  • GitHub Check: ci (Testcontainers.Bigtable, ubuntu-24.04)
  • GitHub Check: ci (Testcontainers.ActiveMq, ubuntu-24.04)
  • GitHub Check: ci (Testcontainers.ArangoDb, ubuntu-24.04)
  • GitHub Check: analyze (csharp)
  • GitHub Check: ci (Testcontainers.ClickHouse, ubuntu-24.04)
  • GitHub Check: ci (Testcontainers.Cassandra, ubuntu-24.04)
  • GitHub Check: ci (Testcontainers.Bigtable, ubuntu-24.04)
  • GitHub Check: ci (Testcontainers.ActiveMq, ubuntu-24.04)
  • GitHub Check: ci (Testcontainers.ArangoDb, ubuntu-24.04)
  • GitHub Check: analyze (csharp)
🔇 Additional comments (6)
src/Testcontainers/Configurations/Containers/ContainerConfiguration.cs (1)

42-42: LGTM!

The XML documentation for the connectionStringProvider parameter follows the established pattern and correctly documents the existing constructor parameter.

tests/Testcontainers.Commons/TestSession.cs (1)

6-6: LGTM!

The rename from Cache to Stages improves clarity by accurately describing what the dictionary stores—Dockerfile stage mappings.

Also applies to: 19-19

src/Testcontainers/Builders/BuildConfiguration.cs (3)

45-68: LGTM! Good defensive handling for null and empty sequences.

The changes correctly:

  1. Short-circuit when newValue is null (return oldValue)
  2. Short-circuit when oldValue is null (return newValue)
  3. Skip concatenation when newValue is an empty collection
  4. Materialize non-collection enumerables to arrays before concatenation to avoid multiple enumeration issues

This aligns well with the PR objective to avoid unnecessary allocations.


91-104: LGTM!

The null and empty handling for IReadOnlyList<T> is consistent with the IEnumerable<T> overload and correctly skips unnecessary concatenation.


161-174: LGTM!

The null and empty handling for IReadOnlyDictionary<TKey, TValue> correctly avoids dictionary allocation and merge operations when newValue is empty.

tests/Testcontainers.Commons/DockerfileParser.cs (1)

41-41: LGTM!

Using FirstOrDefault(string.Empty) is more idiomatic and readable than the previous DefaultIfEmpty().First() pattern while maintaining the same behavior.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@HofmeisterAn HofmeisterAn merged commit 92af06f into develop Dec 7, 2025
148 checks passed
@HofmeisterAn HofmeisterAn deleted the feature/avoid-allocation-for-builder-configurations branch December 7, 2025 19:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

chore A change that doesn't impact the existing functionality, e.g. internal refactorings or cleanups

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant