RDMP-355 SQL Merge Extraction Destination Component

[JFriel]

Proposed Change

Create new component to simplify meging extraction data into existing database tables

Type of change

What types of changes does your code introduce? Tick all that apply.

• Bugfix (non-breaking change which fixes an issue)
• New Feature (non-breaking change which adds functionality)
• Breaking Change (fix or feature that would cause existing functionality to not work as expected)
• Documentation-Only Update
• Other (if none of the other choices apply)

Checklist

By opening this PR, I confirm that I have:

• Ensured that the PR branch is in sync with the target branch (i.e. it is automatically merge-able)
• Created or updated any tests if relevant
• Have validated this change against the Test Plan
• Requested a review by one of the repository maintainers
• Have written new documentation or updated existing documentation to detail any new or updated functionality and how to use it
• Have added an entry into the changelog

In general, to fix this class of issue, ensure that any IDisposable object is disposed even if an exception occurs between its creation and the intended Dispose call. The idiomatic C# solution is to wrap such objects in a using statement (or using declaration in newer C#) so that Dispose is called automatically in a finally-like manner. If a using is not suitable, a try/finally block explicitly calling Dispose in finally is appropriate.

Here, the problem is _bulkcopy, created on line 263 and disposed on line 265. The single best fix with minimal functional change is to move the _bulkcopy variable into a using statement, calling Upload inside the using block and removing the explicit _bulkcopy.Dispose() call. This keeps the same logic and order of operations, while guaranteeing disposal if Upload or any subsequent code in that block throws. No new methods or external libraries are needed. The change is localized to Rdmp.Core/DataExport/DataExtraction/Pipeline/Destinations/MSSqlMergeDestination.cs around lines 263–265.

In general, the correct fix is to ensure that the SqlCommand instance is disposed deterministically, typically via a using statement or using declaration that wraps its creation and use. This guarantees that any unmanaged resources associated with the command are released promptly, even if exceptions occur.

For this specific code, the simplest and least intrusive change is to wrap the creation and use of cmd in a using block. We only need to modify the block starting at line 281 in Rdmp.Core/DataExport/DataExtraction/Pipeline/Destinations/MSSqlMergeDestination.cs. Replace the standalone creation of cmd and its use with a using (var cmd = new SqlCommand(...)) { ... } block that contains the ExecuteNonQuery, the success logging, and the optional temp table drop. This keeps the existing behavior, ensures cmd.Dispose() is always called, and does not require any new imports or changes elsewhere. _managedConnection.Dispose() should remain after the using block so that the command is disposed before the connection/transaction wrapper is disposed.

In general, to fix this kind of issue you ensure that every IDisposable resource is disposed even if an exception occurs between its creation and its intended Dispose call. In C#, that is normally achieved with using statements or, if using can’t be used, a try/finally that calls Dispose in the finally block. This guarantees cleanup regardless of how control leaves the block.

For this specific method, we need to guarantee disposal of _managedConnection, _bulkcopy, and also SqlCommand cmd (which currently is never disposed at all). The minimal change that preserves existing functionality is to introduce nested using blocks: one around the managed connection, one around the bulk copy, and one around the SqlCommand. All existing logic (creating the temp table, performing the bulk copy, building mergeSql, logging, executing the command, optionally dropping the temp table) can remain the same, just indented inside these using scopes. This ensures that if any of the operations flagged in the alert (e.g., BeginBulkInsert, Upload, building mergeSql, OnNotify, ExecuteNonQuery, Drop) throw, the using blocks will still dispose the relevant objects.

Concretely, in MSSqlMergeDestination.WriteRows (in Rdmp.Core/DataExport/DataExtraction/Pipeline/Destinations/MSSqlMergeDestination.cs), replace the section starting at the creation of _managedConnection (current line 262) through the end of the method (current line 286) with code that:

• Uses using (var _managedConnection = tmpTbl.Database.Server.GetManagedConnection()) instead of a bare var.
• Inside that, uses using (var _bulkcopy = tmpTbl.BeginBulkInsert(...)) instead of a bare var plus explicit Dispose.
• After building mergeSql, uses using (var cmd = new SqlCommand(...)).
• Keeps the job.OnNotify, ExecuteNonQuery, tmpTbl.Drop(), and logic unchanged, only indented.
• Removes the explicit _bulkcopy.Dispose(); and _managedConnection.Dispose(); calls, since using will now handle disposal automatically.

This single restructuring addresses all 19 variants: any exception from any of the flagged calls will still result in the connection, bulk copy, and command being disposed.

In general, to address this kind of issue you transform the foreach to iterate over a projected sequence using LINQ’s Select, so that the loop variable directly represents the derived value you actually use inside the loop. This removes the extra local mapping step and better communicates the loop’s intent.

Here, the loop currently iterates over dleListener instances and immediately maps each to its DataLoadInfo:

foreach (var dleListener in listeners)
{
    IDataLoadInfo dataLoadInfo = dleListener.DataLoadInfo;
    ...
}

The best fix is to change the foreach to iterate directly over IDataLoadInfo by applying .Select(l => l.DataLoadInfo) to listeners, and then remove the now-unnecessary local variable. Concretely, replace the foreach header and the first line in the loop body with:

foreach (IDataLoadInfo dataLoadInfo in listeners.Select(l => l.DataLoadInfo))
{
    DataColumn newColumn = new(SpecialFieldNames.DataLoadRunID, typeof(int))
    {
        DefaultValue = dataLoadInfo.ID
    };
    ...
}

We already have using System.Linq; at line 24, so no new imports are needed. No other files or regions need changes; everything is contained within the shown block in MSSqlMergeDestination.cs.