Skip to content

Commit 23142a1

Browse files
sbroenneStefan Broenner
andauthored
fix: prevent orphaned Power Query connections during worksheet loading (#299)
Root Cause: ListObjects.Add() with connection STRING creates orphaned connections with generic names (Connection, Connection1, etc.) Solution: - Use Connections.Add2() to create properly named connections first - Pass connection OBJECT to ListObjects.Add() to reuse the connection - For LoadToBoth: create TWO connections with distinct names: - Worksheet: 'Query - {name}' - Data Model: 'Query - {name} (Data Model)' - Updated pattern matching in Delete/Unload/List/GetLoadConfig Test Coverage: - 14 new worksheet cleanup tests - 9 lifecycle cleanup tests - All 74+ Power Query tests passing Fixes #298 Co-authored-by: Stefan Broenner <stefan.broenner@microsoft.comm>
1 parent abbe180 commit 23142a1

File tree

8 files changed

+1100
-48
lines changed

8 files changed

+1100
-48
lines changed

src/ExcelMcp.Core/Commands/Connection/ConnectionCommands.Lifecycle.cs

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -174,6 +174,11 @@ public void Refresh(IExcelBatch batch, string connectionName, TimeSpan? timeout)
174174
// Check if this is a Power Query connection (handle separately)
175175
if (PowerQueryHelpers.IsPowerQueryConnection(conn))
176176
{
177+
// Check if this is an orphaned Power Query connection
178+
if (PowerQueryHelpers.IsOrphanedPowerQueryConnection(ctx.Book, conn))
179+
{
180+
throw new InvalidOperationException($"Connection '{connectionName}' is an orphaned Power Query connection with no corresponding query. Use excel_connection 'delete' to remove it.");
181+
}
177182
throw new InvalidOperationException($"Connection '{connectionName}' is a Power Query connection. Use excel_powerquery 'refresh' instead.");
178183
}
179184

@@ -200,7 +205,13 @@ public void Delete(IExcelBatch batch, string connectionName)
200205
// Check if this is a Power Query connection
201206
if (PowerQueryHelpers.IsPowerQueryConnection(conn))
202207
{
203-
throw new InvalidOperationException($"Connection '{connectionName}' is a Power Query connection. Use excel_powerquery with action 'Delete' instead.");
208+
// Check if this is an orphaned Power Query connection (no corresponding query exists)
209+
// Orphaned connections can be safely deleted via the connection API
210+
if (!PowerQueryHelpers.IsOrphanedPowerQueryConnection(ctx.Book, conn))
211+
{
212+
throw new InvalidOperationException($"Connection '{connectionName}' is a Power Query connection. Use excel_powerquery with action 'Delete' instead.");
213+
}
214+
// Orphaned connection - allow deletion to proceed
204215
}
205216

206217
// Remove associated QueryTables first

src/ExcelMcp.Core/Commands/PowerQuery/PowerQueryCommands.Create.cs

Lines changed: 5 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -98,23 +98,11 @@ public void Create(
9898
break;
9999

100100
case PowerQueryLoadMode.LoadToBoth:
101-
// Load to worksheet first
102-
if (LoadQueryToWorksheet(ctx.Book, queryName, targetSheet!, targetCellAddress!, result))
103-
{
104-
// Preserve worksheet properties before loading to Data Model
105-
int worksheetRows = result.RowsLoaded;
106-
string? worksheetCell = result.TargetCellAddress;
107-
string? worksheetName = result.WorksheetName;
108-
109-
// Then also load to Data Model
110-
if (LoadQueryToDataModel(ctx.Book, queryName, result))
111-
{
112-
// Restore worksheet properties (Data Model sets them to null/-1)
113-
result.RowsLoaded = worksheetRows;
114-
result.TargetCellAddress = worksheetCell;
115-
result.WorksheetName = worksheetName;
116-
}
117-
}
101+
// For LoadToBoth, create TWO separate properly-named connections:
102+
// 1. Worksheet connection: "Query - {name}" (created by LoadQueryToWorksheet)
103+
// 2. Data Model connection: "Query - {name} (Data Model)" (with suffix to avoid conflict)
104+
LoadQueryToWorksheet(ctx.Book, queryName, targetSheet!, targetCellAddress!, result);
105+
LoadQueryToDataModel(ctx.Book, queryName, result, " (Data Model)");
118106
break;
119107
}
120108

src/ExcelMcp.Core/Commands/PowerQuery/PowerQueryCommands.Lifecycle.cs

Lines changed: 28 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -149,9 +149,13 @@ public PowerQueryListResult List(IExcelBatch batch)
149149
string connName = conn.Name?.ToString() ?? "";
150150

151151
// Check if this is a Data Model connection for our query
152-
// Pattern: "Query - {queryName}" or "Query - {queryName} - suffix"
152+
// Patterns:
153+
// - "Query - {queryName}" (worksheet connection)
154+
// - "Query - {queryName} (Data Model)" (Data Model connection)
155+
// - "Query - {queryName} - suffix" (legacy pattern)
153156
if (connName.Equals($"Query - {name}", StringComparison.OrdinalIgnoreCase) ||
154-
connName.StartsWith($"Query - {name} -", StringComparison.OrdinalIgnoreCase))
157+
connName.StartsWith($"Query - {name} -", StringComparison.OrdinalIgnoreCase) ||
158+
connName.StartsWith($"Query - {name} (", StringComparison.OrdinalIgnoreCase))
155159
{
156160
// Has Data Model connection - NOT connection-only
157161
isConnectionOnly = false;
@@ -324,8 +328,15 @@ public PowerQueryLoadConfigResult GetLoadConfig(IExcelBatch batch, string queryN
324328
string connName = conn.Name?.ToString() ?? "";
325329

326330
// Check if this connection is related to our query
331+
// Patterns:
332+
// - "{queryName}" (exact match)
333+
// - "Query - {queryName}" (worksheet connection)
334+
// - "Query - {queryName} (Data Model)" (Data Model connection)
335+
// - "Query - {queryName} - suffix" (legacy pattern)
327336
bool isQueryConnection = connName.Equals(queryName, StringComparison.OrdinalIgnoreCase) ||
328-
connName.Equals($"Query - {queryName}", StringComparison.OrdinalIgnoreCase);
337+
connName.Equals($"Query - {queryName}", StringComparison.OrdinalIgnoreCase) ||
338+
connName.StartsWith($"Query - {queryName} -", StringComparison.OrdinalIgnoreCase) ||
339+
connName.StartsWith($"Query - {queryName} (", StringComparison.OrdinalIgnoreCase);
329340

330341
// Also check connection string for Power Query pattern
331342
if (!isQueryConnection)
@@ -513,9 +524,14 @@ public void Delete(IExcelBatch batch, string queryName)
513524
conn = connections.Item(c);
514525
string connName = conn.Name?.ToString() ?? "";
515526

516-
// Check if this is a Data Model connection for our query
527+
// Check if this is a connection for our query
528+
// Patterns:
529+
// - "Query - {queryName}" (worksheet connection)
530+
// - "Query - {queryName} (Data Model)" (Data Model connection)
531+
// - "Query - {queryName} - suffix" (legacy pattern)
517532
if (connName.Equals($"Query - {queryName}", StringComparison.OrdinalIgnoreCase) ||
518-
connName.StartsWith($"Query - {queryName} -", StringComparison.OrdinalIgnoreCase))
533+
connName.StartsWith($"Query - {queryName} -", StringComparison.OrdinalIgnoreCase) ||
534+
connName.StartsWith($"Query - {queryName} (", StringComparison.OrdinalIgnoreCase))
519535
{
520536
connectionsToDelete.Add(connName);
521537
}
@@ -714,10 +730,14 @@ public static OperationResult Unload(IExcelBatch batch, string queryName)
714730
conn = connections.Item(i);
715731
string connName = conn.Name?.ToString() ?? "";
716732

717-
// Check if this is a Data Model connection for our query
718-
// Pattern: "Query - {queryName}" or "Query - {queryName} - Model" etc.
733+
// Check if this is a connection for our query
734+
// Patterns:
735+
// - "Query - {queryName}" (worksheet connection)
736+
// - "Query - {queryName} (Data Model)" (Data Model connection)
737+
// - "Query - {queryName} - suffix" (legacy pattern)
719738
if (connName.Equals($"Query - {queryName}", StringComparison.OrdinalIgnoreCase) ||
720-
connName.StartsWith($"Query - {queryName} -", StringComparison.OrdinalIgnoreCase))
739+
connName.StartsWith($"Query - {queryName} -", StringComparison.OrdinalIgnoreCase) ||
740+
connName.StartsWith($"Query - {queryName} (", StringComparison.OrdinalIgnoreCase))
721741
{
722742
connectionsToDelete.Add(connName);
723743
}

src/ExcelMcp.Core/Commands/PowerQuery/PowerQueryCommands.LoadTo.cs

Lines changed: 39 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -103,21 +103,11 @@ public void LoadTo(
103103
break;
104104

105105
case PowerQueryLoadMode.LoadToBoth:
106-
// Load to worksheet first
107-
if (LoadQueryToWorksheet(ctx.Book, queryName, targetSheet!, targetCellAddress, result))
108-
{
109-
// Preserve worksheet properties before loading to Data Model
110-
int worksheetRows = result.RowsLoaded;
111-
string? worksheetCell = result.TargetCellAddress;
112-
113-
// Then also load to Data Model
114-
if (LoadQueryToDataModel(ctx.Book, queryName, result))
115-
{
116-
// Restore worksheet properties (Data Model sets them to null/-1)
117-
result.RowsLoaded = worksheetRows;
118-
result.TargetCellAddress = worksheetCell;
119-
}
120-
}
106+
// For LoadToBoth, create TWO separate properly-named connections:
107+
// 1. Worksheet connection: "Query - {name}" (created by LoadQueryToWorksheet)
108+
// 2. Data Model connection: "Query - {name} (Data Model)" (with suffix to avoid conflict)
109+
LoadQueryToWorksheet(ctx.Book, queryName, targetSheet!, targetCellAddress, result);
110+
LoadQueryToDataModel(ctx.Book, queryName, result, " (Data Model)");
121111
break;
122112

123113
case PowerQueryLoadMode.ConnectionOnly:
@@ -166,6 +156,8 @@ private static bool LoadQueryToWorksheet(
166156
dynamic? worksheets = null;
167157
dynamic? sheet = null;
168158
dynamic? destination = null;
159+
dynamic? connections = null;
160+
dynamic? connection = null;
169161
dynamic? listObjects = null;
170162
dynamic? listObject = null;
171163
dynamic? queryTable = null;
@@ -275,15 +267,30 @@ private static bool LoadQueryToWorksheet(
275267
}
276268
}
277269

278-
// Build OLE DB connection string for Power Query
279-
string connectionString = $"OLEDB;Provider=Microsoft.Mashup.OleDb.1;Data Source=$Workbook$;Location={queryName};Extended Properties=\"\"";
270+
// Step 1: Create connection with Connections.Add2() using proper naming
271+
// This ensures the connection is named "Query - {queryName}" instead of generic "Connection", "Connection1", etc.
272+
connections = workbook.Connections;
273+
string connectionName = $"Query - {queryName}";
274+
string connectionDescription = $"Connection to the '{queryName}' query in the workbook.";
275+
string connectionString = $"OLEDB;Provider=Microsoft.Mashup.OleDb.1;Data Source=$Workbook$;Location={queryName}";
276+
string commandText = $"SELECT * FROM [{queryName}]";
277+
278+
connection = connections.Add2(
279+
Name: connectionName,
280+
Description: connectionDescription,
281+
ConnectionString: connectionString,
282+
CommandText: commandText,
283+
lCmdtype: 2, // xlCmdSql
284+
CreateModelConnection: false, // Worksheet loading, NOT Data Model
285+
ImportRelationships: false
286+
);
280287

281-
// Add ListObject (Excel Table) with external data source
282-
// This is the CORRECT way to load Power Query to worksheet
288+
// Step 2: Add ListObject using the connection object (not connection string)
289+
// This reuses the properly-named connection instead of creating a new generic one
283290
listObjects = sheet.ListObjects;
284291
listObject = listObjects.Add(
285292
0, // SourceType: 0 = xlSrcExternal
286-
connectionString, // Source: connection string
293+
connection, // Source: connection object (reuses existing named connection)
287294
Type.Missing, // LinkSource
288295
1, // XlListObjectHasHeaders: xlYes
289296
destination // Destination: starting cell
@@ -317,6 +324,8 @@ private static bool LoadQueryToWorksheet(
317324
ComUtilities.Release(ref queryTable);
318325
ComUtilities.Release(ref listObject);
319326
ComUtilities.Release(ref listObjects);
327+
ComUtilities.Release(ref connection);
328+
ComUtilities.Release(ref connections);
320329
ComUtilities.Release(ref destination);
321330
ComUtilities.Release(ref sheet);
322331
ComUtilities.Release(ref worksheets);
@@ -327,10 +336,15 @@ private static bool LoadQueryToWorksheet(
327336
/// Loads query data to the Data Model using Connections.Add2.
328337
/// SHARED IMPLEMENTATION - Used by both Create and LoadTo.
329338
/// </summary>
339+
/// <param name="workbook">The workbook to load into.</param>
340+
/// <param name="queryName">The Power Query name.</param>
341+
/// <param name="result">Result object to populate.</param>
342+
/// <param name="connectionNameSuffix">Optional suffix for connection name to avoid conflicts (e.g., " (Data Model)").</param>
330343
private static bool LoadQueryToDataModel(
331344
dynamic workbook,
332345
string queryName,
333-
dynamic result)
346+
dynamic result,
347+
string? connectionNameSuffix = null)
334348
{
335349
dynamic? connections = null;
336350
dynamic? connection = null;
@@ -339,7 +353,10 @@ private static bool LoadQueryToDataModel(
339353
{
340354
connections = workbook.Connections;
341355

342-
string connectionName = $"Query - {queryName}";
356+
// Use suffix if provided (for LoadToBoth to avoid conflict with worksheet connection)
357+
string connectionName = string.IsNullOrEmpty(connectionNameSuffix)
358+
? $"Query - {queryName}"
359+
: $"Query - {queryName}{connectionNameSuffix}";
343360
string description = $"Connection to the '{queryName}' query in the workbook.";
344361
string connectionString = $"OLEDB;Provider=Microsoft.Mashup.OleDb.1;Data Source=$Workbook$;Location={queryName}";
345362
string commandText = $"\"{queryName}\"";

src/ExcelMcp.Core/Commands/PowerQuery/PowerQueryHelpers.cs

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,65 @@ namespace Sbroenne.ExcelMcp.Core.PowerQuery;
77
/// </summary>
88
public static class PowerQueryHelpers
99
{
10+
/// <summary>
11+
/// Determines if a Power Query connection is orphaned (no corresponding query exists).
12+
/// An orphaned connection is one that appears to be a Power Query connection (based on
13+
/// connection string or naming pattern) but has no matching entry in the Queries collection.
14+
/// This commonly occurs after query deletions, renames, or copy/paste operations in Excel.
15+
///
16+
/// A connection is considered orphaned if:
17+
/// 1. It's a Power Query connection (uses Microsoft.Mashup provider)
18+
/// 2. AND EITHER:
19+
/// a. It doesn't follow the standard "Query - {queryName}" naming pattern (e.g., "Connection", "Connection1")
20+
/// b. OR it follows the pattern but the corresponding query no longer exists in Workbook.Queries
21+
/// </summary>
22+
/// <param name="workbook">Excel workbook COM object</param>
23+
/// <param name="connection">Connection COM object</param>
24+
/// <returns>True if connection is a Power Query connection with no corresponding query</returns>
25+
public static bool IsOrphanedPowerQueryConnection(dynamic workbook, dynamic connection)
26+
{
27+
// First check if this is even a Power Query connection
28+
if (!IsPowerQueryConnection(connection))
29+
{
30+
return false;
31+
}
32+
33+
string connectionName = connection.Name?.ToString() ?? "";
34+
35+
// Check if connection follows the standard "Query - {queryName}" naming pattern
36+
// Only connections with this pattern are considered "proper" Power Query connections
37+
if (!connectionName.StartsWith("Query - ", StringComparison.OrdinalIgnoreCase))
38+
{
39+
// Generic names like "Connection", "Connection1", etc. are ALWAYS orphaned
40+
// even if their Location= points to an existing query.
41+
// The proper connection for a query is always named "Query - {queryName}".
42+
return true;
43+
}
44+
45+
// Extract the query name from the "Query - {queryName}" pattern
46+
string expectedQueryName = connectionName["Query - ".Length..];
47+
48+
// Handle potential suffixes like "Query - Name - Model" (though rare)
49+
int dashIndex = expectedQueryName.IndexOf(" -", StringComparison.Ordinal);
50+
if (dashIndex > 0)
51+
{
52+
expectedQueryName = expectedQueryName[..dashIndex];
53+
}
54+
55+
// Check if a query with this name exists
56+
dynamic? query = null;
57+
try
58+
{
59+
query = ComUtilities.FindQuery(workbook, expectedQueryName);
60+
// If query is null, the connection is orphaned
61+
return query == null;
62+
}
63+
finally
64+
{
65+
ComUtilities.Release(ref query);
66+
}
67+
}
68+
1069
/// <summary>
1170
/// Determines if a connection is a Power Query connection
1271
/// </summary>

0 commit comments

Comments
 (0)