Files
Stefan Broenner 9e7d532196 refactor: eliminate exception wrapping across all command layers (205+ catch blocks removed) (#224)
* refactor: centralize Excel shutdown logic and add early path validation

- Extract SaveWorkbookWithTimeout() to eliminate code duplication (~70 lines)
- Create ExcelShutdownService for unified workbook close and Excel quit
- Remove GC.Collect() calls per Microsoft guidance (RCW finalizers handle cleanup)
- Add early path length validation (218 char limit) before Excel COM operations
- Convert COM path errors to PathTooLongException with clear messages
- Update all exception catch blocks to handle nested AggregateException
- Add Microsoft.Extensions.Resilience for COM busy retry with exponential backoff
- Update documentation to reflect batch API patterns and remove deprecated references

Benefits:
- Fail fast: Path validation before spawning Excel process
- DRY: Single shutdown implementation used by ExcelBatch, ExcelSession, FileCommands
- Performance: No forced GC, automatic RCW cleanup
- Resilience: 6-attempt retry for transient COM busy errors
- Clear errors: PathTooLongException → JSON error messages for LLMs

* refactor: remove unnecessary try-catch blocks for cleaner code

* Potential fix for code scanning alert no. 2575: Generic catch clause

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>

* Potential fix for code scanning alert no. 2576: Generic catch clause

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>

* refactor: clean up Excel process termination logic in tests

* refactor: update documentation for ExcelSession tests to clarify cleanup process

* Refactor Excel Tool Classes and Tests

- Removed unused variables and comments in ExcelNamedRangeTool, ExcelPowerQueryTool, ExcelRangeTool, ExcelVbaTool, and ExcelWorksheetTool to clean up the code.
- Updated tests in ExcelComSmokeTests, ExcelBatchTests, ExcelSessionTests, and SessionManagerTests to improve readability and maintainability by eliminating unnecessary variable assignments.
- Consolidated sheet command tests into a single file, SheetCommandsTests.Move.cs, and removed redundant tests from the previous SheetCommandsTests.MoveAndCrossWorkbook.cs file.
- Ensured all tests maintain functionality while improving structure and clarity.

* refactor: streamline loading logic in Power Query commands

* refactor: update Power Query refresh methods to enforce timeout and improve error handling

* refactor: remove unused result properties and exception wrapping from PowerQuery commands

- Removed IsRetryable, RetryGuidance, OperationContext from ResultBase
  These fields were never consumed by CLI, MCP Server, or LLM layers

- Refactored PowerQuery commands to let exceptions bubble naturally
  Removed outer try-catch wrapping from 6 partial files:
  - PowerQueryCommands.Create.cs
  - PowerQueryCommands.View.cs
  - PowerQueryCommands.Update.cs
  - PowerQueryCommands.Lifecycle.cs
  - PowerQueryCommands.Refresh.cs
  - PowerQueryCommands.LoadTo.cs

- Pattern: batch.Execute() already captures exceptions via TaskCompletionSource
  No need to wrap at method level. Keep finally blocks for COM cleanup.

Test Results:
- PowerQuery: 14/14 passed
- CLI: 2/2 passed
- Connection: 13/13 passed
- MCP: 44/46 passed (2 pre-existing PivotTable enum failures)
- Build: 0 errors, 0 warnings

Architecture validation confirms sound design:
- Exception propagation works end-to-end
- COM resource cleanup via finally blocks still functions
- No loss of functionality

* refactor: remove exception wrapping from all remaining Core commands

Applied the same pattern across 21 additional command files:
- Connection (3 files)
- VBA (3 files)
- Table (6 files)
- Sheet (4 files)
- Range (4 files)
- DataModel (3 files)
- PivotTable (6 files)
- NamedRange (2 files)
- ConditionalFormat (1 file)

Changes:
- Removed outer try-catch wrappers around batch.Execute() calls
- Preserved all finally blocks for critical COM resource cleanup
- Preserved inner try-catch for specific exception handling
- Exceptions now flow naturally to batch.Execute() via TaskCompletionSource

Statistics:
- 21 files refactored (+ 8 from PowerQuery = 29 total)
- 41 methods updated
- 553 net lines removed (982 deleted, 429 inserted)
- 0 compilation errors
- Build: SUCCESS

Test Results (spot checks):
- PowerQuery: 14/14 PASSED
- Worksheets: 36/36 PASSED

Architecture validated across entire Core command layer.

* refactor: remove all suppressions from RegularPivotTableFieldStrategy.cs

- Removed 11 exception-suppressing catch blocks (Success=false returns)
- Preserved all finally blocks for COM resource cleanup
- Methods refactored: ListFields, AddRowField, AddColumnField, AddValueField,
  AddFilterField, RemoveField, SetFieldName, SetFieldFunction, SetFieldFormat,
  SetFieldFilter, SortField, SetLayout, SetSubtotals, SetGrandTotals
- Exceptions now flow naturally to callers
- Build: 0 errors confirmed
- Architecture validated - batch.Execute() TaskCompletionSource properly receives exceptions

* refactor: remove exception suppressions from OlapPivotTableFieldStrategy.cs

- Removed all exception-suppressing catch blocks (Success=false returns)
- Preserved safe fallback patterns (continue in loop, conditional rethrrows)
- Methods affected: ListFields and related field manipulation methods
- Build: 0 errors confirmed
- 17 safe fallback catches remaining (architectural patterns - KEEP)
- OlapPivotTableFieldStrategy refactoring complete

* refactor: remove exception suppression from PowerQuery commands (21 patterns)

- PowerQueryCommands.LoadTo.cs: 6 patterns removed
  * Parameter validation (targetCellAddress) → ArgumentException
  * Query not found → InvalidOperationException
  * Sheet access, table overlap, cell data validation → InvalidOperationException
  * Removed catch block suppressing Data Model exceptions

- PowerQueryCommands.Update.cs: 4 patterns removed
  * Query name validation → ArgumentException
  * Empty M code validation → ArgumentException
  * Query not found → InvalidOperationException
  * Removed catch block suppressing refresh exceptions

- PowerQueryCommands.View.cs: 2 patterns removed
  * Query name validation → ArgumentException
  * Query not found → InvalidOperationException

- PowerQueryCommands.Refresh.cs: 2 patterns removed
  * Query name validation → ArgumentException
  * Query not found with suggestion → InvalidOperationException

- PowerQueryCommands.Lifecycle.cs: 4 patterns removed
  * GetLoadConfig validation → ArgumentException
  * GetLoadConfig query not found → InvalidOperationException
  * Delete validation → ArgumentException
  * Delete query not found → InvalidOperationException

- PivotTableCommandsTests: 3 tests corrected
  * Fixed GetInfo_NonExistent, Delete_NonExistent to use Assert.Throws
  * Updated CreateFromDataModel_NoDataModel error message expectation

All exceptions now propagate naturally through batch.Execute().
Build: 0 errors, 0 warnings
Tests: All 14 PowerQuery tests pass, 3 PivotTable tests pass

* refactor: remove exception suppression from Range commands (61 patterns)

Converted all error suppression patterns in RangeCommands to proper exception propagation:

**Files Fixed (61 patterns total):**

- RangeCommands.Editing.cs: 21 patterns removed
  * Copy, CopyValues, CopyFormulas: Convert null checks → InvalidOperationException, remove catch blocks
  * InsertCells, DeleteCells, InsertRows, DeleteRows, InsertColumns, DeleteColumns: Convert null checks → InvalidOperationException, remove catch blocks

- RangeCommands.Search.cs: 8 patterns removed
  * Find: Convert null check → InvalidOperationException, remove catch block
  * Replace: Convert null check → InvalidOperationException, remove catch block
  * Sort: Convert null checks + validations → InvalidOperationException/ArgumentException, remove catch block

- RangeCommands.NumberFormat.cs: 8 patterns removed
  * GetNumberFormats, SetNumberFormat: Convert null checks → InvalidOperationException, remove catch blocks
  * SetNumberFormats: Convert null checks + validation errors → InvalidOperationException/ArgumentException, remove catch blocks

- RangeCommands.Hyperlinks.cs: 8 patterns removed
  * AddHyperlink, RemoveHyperlink, ListHyperlinks, GetHyperlink: Convert null checks → InvalidOperationException, remove catch blocks

- RangeCommands.Values.cs: 4 patterns removed
  * GetValues, SetValues: Convert null checks → InvalidOperationException
  * Convert specific COM exceptions (E_OUTOFMEMORY) → throw InvalidOperationException (not suppress)

- RangeCommands.Formulas.cs: 4 patterns removed
  * GetFormulas, SetFormulas: Convert null checks → InvalidOperationException
  * Convert specific COM exceptions (E_OUTOFMEMORY) → throw InvalidOperationException (not suppress)

- RangeCommands.Discovery.cs: 6 patterns removed
  * GetUsedRange, GetCurrentRegion, GetInfo: Convert null checks → InvalidOperationException, remove catch blocks

- RangeHelpers.cs: 2 patterns removed
  * ClearRange helper: Convert null check → InvalidOperationException, remove catch block

**Pattern Applied:**
- Parameter validation (BEFORE batch.Execute()): null → ArgumentException
- Not found / Invalid state (INSIDE batch.Execute()): null → InvalidOperationException
- Specific COM exceptions: Rethrow as InvalidOperationException with context (not suppress)
- Remove all catch blocks that return OperationResult { Success = false }

All exceptions now propagate naturally through batch.Execute().
Build: 0 errors, 0 warnings

* refactor: remove exception suppression from Range commands (61 patterns)

Converted all error suppression patterns in RangeCommands to proper exception propagation:

**Files Fixed (61 patterns total):**

- RangeCommands.Editing.cs: 21 patterns removed
  * Copy, CopyValues, CopyFormulas: Convert null checks → InvalidOperationException, remove catch blocks
  * InsertCells, DeleteCells, InsertRows, DeleteRows, InsertColumns, DeleteColumns: Convert null checks → InvalidOperationException, remove catch blocks

- RangeCommands.Search.cs: 8 patterns removed
  * Find: Convert null check → InvalidOperationException, remove catch block
  * Replace: Convert null check → InvalidOperationException, remove catch block
  * Sort: Convert null checks + validations → InvalidOperationException/ArgumentException, remove catch block

- RangeCommands.NumberFormat.cs: 8 patterns removed
  * GetNumberFormats, SetNumberFormat: Convert null checks → InvalidOperationException, remove catch blocks
  * SetNumberFormats: Convert null checks + validation errors → InvalidOperationException/ArgumentException, remove catch blocks

- RangeCommands.Hyperlinks.cs: 8 patterns removed
  * AddHyperlink, RemoveHyperlink, ListHyperlinks, GetHyperlink: Convert null checks → InvalidOperationException, remove catch blocks

- RangeCommands.Values.cs: 4 patterns removed
  * GetValues, SetValues: Convert null checks → InvalidOperationException
  * Convert specific COM exceptions (E_OUTOFMEMORY) → throw InvalidOperationException (not suppress)

- RangeCommands.Formulas.cs: 4 patterns removed
  * GetFormulas, SetFormulas: Convert null checks → InvalidOperationException
  * Convert specific COM exceptions (E_OUTOFMEMORY) → throw InvalidOperationException (not suppress)

- RangeCommands.Discovery.cs: 6 patterns removed
  * GetUsedRange, GetCurrentRegion, GetInfo: Convert null checks → InvalidOperationException, remove catch blocks

- RangeHelpers.cs: 2 patterns removed
  * ClearRange helper: Convert null check → InvalidOperationException, remove catch block

**Pattern Applied:**
- Parameter validation (BEFORE batch.Execute()): null → ArgumentException
- Not found / Invalid state (INSIDE batch.Execute()): null → InvalidOperationException
- Specific COM exceptions: Rethrow as InvalidOperationException with context (not suppress)
- Remove all catch blocks that return OperationResult { Success = false }

All exceptions now propagate naturally through batch.Execute().
Build: 0 errors, 0 warnings

* refactor(core): Convert DataModel validation errors to exceptions (31 patterns)

- DataModelCommands.Write.cs: 18 patterns fixed
  - DeleteMeasure, DeleteRelationship, CreateMeasure, UpdateMeasure
  - CreateRelationship, UpdateRelationship
  - NoDataModelTables, MeasureNotFound, TableNotFound, RelationshipNotFound
  - All validation errors before batch.Execute() → InvalidOperationException

- DataModelCommands.Read.cs: 10 patterns fixed
  - ListMeasures, Read, ListRelationships, ListColumns, ReadTable, ReadInfo
  - NoDataModelTables, MeasureNotFound, TableNotFound
  - All validation errors → InvalidOperationException

- DataModelCommands.Refresh.cs: 2 patterns fixed
  - NoDataModelTables, TableNotFound → InvalidOperationException
  - Kept Model.Refresh() catch (line 71) - valid recoverable COM exception

Part of exception propagation refactoring. Let batch.Execute() handle
exceptions via TaskCompletionSource for proper STA thread propagation.

Related: PowerQuery (SHA cbf4441), Range (SHA 1a0cb35)

* refactor(core): Convert Table validation errors to exceptions (15/56 patterns - Part 1)

- TableCommands.Data.cs: 2 patterns fixed
  - Append: TableNotFound, NoData validation

- TableCommands.StructuredReferences.cs: 2 patterns fixed
  - GetStructuredReference: TableNotFound
  - Removed outer catch block

- TableCommands.DataModel.cs: 5 patterns fixed
  - AddToDataModel: TableNotFound, TableInDataModel (2x), Add2Exception
  - Removed outer catch block

- TableCommands.Lifecycle.cs: 6 patterns fixed
  - Create: SheetNotFound, TableExists
  - Rename: TableNotFound, NewNameExists
  - Delete: TableNotFound
  - Read: TableNotFound

Part of exception propagation refactoring. Remaining 41 patterns in:
Filters (11), Structure (9), Columns (8), Sort (8), NumberFormat (7)

Related: PowerQuery (SHA cbf4441), Range (SHA 1a0cb35), DataModel (SHA 64929e4)

* refactor(core): Convert Table validation errors to exceptions (30/56 patterns - Part 2)

- TableCommands.NumberFormat.cs: 7 patterns fixed
  - GetColumnNumberFormat, SetColumnNumberFormat helper methods
  - TableNotFound, ColumnNotFound, NoDataRows validations
  - Removed catch blocks

- TableCommands.Sort.cs: 8 patterns fixed
  - Sort (single column): TableNotFound, ColumnNotFound
  - Sort (multi column): NoSortColumns, TooManySortColumns, TableNotFound, ColumnNotFound
  - Removed catch blocks

Total Table progress: 30/56 patterns complete. Remaining: 26 patterns in
Filters (10), Columns (7), Structure (9)

Related: PowerQuery (SHA cbf4441), Range (SHA 1a0cb35), DataModel (SHA 64929e4), Table Part 1 (SHA dce889a)

* refactor(core): Convert Table validation errors to exceptions (56/56 patterns - Part 3)

Complete final Table command files with exception propagation pattern:
- Columns: 7 patterns (AddColumn, RemoveColumn, RenameColumn)
- Structure: 9 patterns (Resize, ToggleTotals, SetColumnTotal, SetStyle)
- Filters: 10 patterns (ApplyFilter x2, ClearFilters, GetFilters)

All Table validation errors now throw InvalidOperationException before batch.Execute().
All catch blocks removed - batch.Execute() handles exceptions via TaskCompletionSource.

Progress: 169 patterns complete (PowerQuery 21, Range 61, DataModel 31, Table 56)
Remaining: 48 patterns (Sheet 12, VBA & Other 36)

* refactor(core): Convert Sheet validation errors to exceptions (12 patterns)

Convert all Sheet command validation errors to exceptions:
- Lifecycle: 6 patterns (parameter validation, SheetNotFound checks)
- TabColor: 4 patterns (RGB validation, SheetNotFound checks)
- Visibility: 2 patterns (SheetNotFound checks)

All Sheet validation errors now throw ArgumentException/InvalidOperationException before batch.Execute().

Progress: 181 patterns complete (PowerQuery 21, Range 61, DataModel 31, Table 56, Sheet 12)
Remaining: 36 patterns (VBA 14, NamedRange 9, Connection 14, PowerQuery.Create 3)

* refactor(core): Convert PowerQuery.Create validation errors to exceptions (3 patterns)

Convert validation errors in PowerQueryCommands.Create to exceptions:
- Empty query name \u2192 ArgumentException
- Empty M code \u2192 ArgumentException
- Existing query \u2192 InvalidOperationException

Progress: 184 patterns complete (PowerQuery 24, Range 61, DataModel 31, Table 56, Sheet 12)
Remaining: 33 patterns (VBA 14, NamedRange 9, Connection 14) - investigating pattern types

* refactor(core): Convert VBA validation errors to exceptions (10 patterns)

Convert VBA command validation errors to exceptions:
- Lifecycle: 7 patterns (VBA file validation, trust checks, module not found)
- Operations: 3 patterns (Run/Delete validation, module not found)

Note: Kept 7 safe catch blocks for COM trust exceptions (per Rule 1b).

Progress: 194 patterns complete (PowerQuery 24, Range 61, DataModel 31, Table 56, Sheet 12, VBA 10)
Remaining: 23 patterns (NamedRange 9, Connection 14)

* refactor(core): Convert Connection, NamedRange, PowerQuery, and VBA validation errors to exceptions (20 patterns)

**Changes:**
- ConnectionCommands.Lifecycle.cs: 5 patterns (View, Refresh, Delete - connection not found + PowerQuery redirect checks)
- ConnectionCommands.Operations.cs: 4 patterns (LoadTo, Test - connection not found + PowerQuery redirect + empty connection string)
- ConnectionCommands.Properties.cs: 5 patterns (GetProperties, SetProperties - connection not found + PowerQuery redirect + removed 2 catch-all blocks)
- NamedRangeCommands.Operations.cs: 9 patterns (Write, Read, Create, Update, Delete - parameter not found + validation checks converted to ArgumentException)
- PowerQueryCommands.Lifecycle.cs: 2 patterns (Unload - query name validation + query not found)
- VbaCommands.Operations.cs: 1 pattern (Run - VBA file validation before batch.Execute)

**Pattern Conversion:**
- Validation BEFORE batch.Execute → ArgumentException
- Validation INSIDE batch.Execute → InvalidOperationException
- Catch-all Exception blocks → Removed entirely (let batch.Execute handle)

**Safe Patterns Preserved (5 total):**
- DataModelCommands.Refresh.cs: Fallback logic when Model.Refresh() unsupported
- PowerQueryCommands.Refresh.cs: COM exception handling for refresh errors (2)
- VbaCommands.Lifecycle.cs: COM trust disabled during operation (2)

**Progress:**
- Completed: 211 patterns across 7 commits
- Remaining: 5 safe exception handlers (intentionally kept)
- Build: Successful, 0 warnings, 0 errors

* refactor(core): Convert 2 remaining VBA Lifecycle validation errors to exceptions

**Changes:**
- VbaCommands.Lifecycle.cs: 2 patterns
  - List: VBA trust check before batch.Execute → InvalidOperationException
  - Update: Module not found inside batch.Execute → InvalidOperationException

**Pattern Conversion:**
- Validation BEFORE batch.Execute → InvalidOperationException (trust check is pre-condition)
- Validation INSIDE batch.Execute → InvalidOperationException (module not found)

**Note:** These 2 patterns were fixed in same session as SHA 35def63 but accidentally not staged with that commit. Including them now for completeness.

**Progress:**
- Total completed: 217 patterns across 8 commits
- Remaining: 5 safe exception handlers (intentionally kept)
- Build: Successful, 0 warnings, 0 errors

* fix(mcp-server): add missing PivotTableAction enum mappings

Adds ToActionString mappings for 6 PivotTableAction values:
- GroupByDate → 'group-by-date'
- GroupByNumeric → 'group-by-numeric'
- CreateCalculatedField → 'create-calculated-field'
- SetLayout → 'set-layout'
- SetSubtotals → 'set-subtotals'
- SetGrandTotals → 'set-grand-totals'

Fixes test failure in AllActionEnums_HaveCompleteToActionStringMappings.

Test results:
- MCP Server: 5/5 tests passed
- CLI: 2/2 tests passed

* feat(cli): add 6 missing PivotTable actions for full Core parity

Adds CLI support for advanced PivotTable operations:
- group-by-date: Group date/time fields (Days, Months, Quarters, Years)
- group-by-numeric: Create numeric range groups with interval
- create-calculated-field: Custom formulas for derived fields
- set-layout: Compact/Tabular/Outline layout forms
- set-subtotals: Show/hide subtotals for row fields
- set-grand-totals: Control row/column grand totals

Implementation:
- Added 6 new actions to PivotTableCommand switch statement
- Implemented 6 execution methods with validation
- Added 9 new command options to Settings class
- Added TryParseDateGroupingInterval helper method

Coverage: PivotTable CLI now has 25/25 actions (100% parity with Core/MCP)

* feat(cli): add 28 missing Range actions for full Core parity

Adds comprehensive Range CLI support:

INSERT/DELETE OPERATIONS (6):
- insert-cells: Insert blank cells with shift direction
- delete-cells: Delete cells with shift direction
- insert-rows: Insert entire rows
- delete-rows: Delete entire rows
- insert-columns: Insert entire columns
- delete-columns: Delete entire columns

FIND/REPLACE OPERATIONS (2):
- find: Search for values/formulas with options
- replace: Replace text/values with options

SORT OPERATION (1):
- sort: Multi-column sorting with headers support

RANGE QUERIES (3):
- get-used-range: Get worksheet's used range
- get-current-region: Get contiguous data block
- get-info: Get range information (address, dimensions)

NUMBER FORMAT OPERATIONS (3):
- get-number-formats: Get format codes (2D array)
- set-number-format: Set uniform format for range
- set-number-formats: Set per-cell formats (2D array)

STYLE OPERATIONS (2):
- get-style: Get built-in style name
- set-style: Apply built-in Excel style

FORMATTING OPERATION (1):
- format-range: Visual formatting (font, fill, border, alignment)

VALIDATION OPERATIONS (3):
- validate-range: Add data validation rules
- get-validation: Get validation settings
- remove-validation: Remove validation

AUTO-FIT OPERATIONS (2):
- autofit-columns: Auto-fit column widths
- autofit-rows: Auto-fit row heights

MERGE OPERATIONS (3):
- merge-cells: Merge cells into single cell
- unmerge-cells: Unmerge previously merged cells
- get-merge-info: Check if range has merged cells

CELL LOCK OPERATIONS (2):
- set-cell-lock: Lock/unlock cells
- get-cell-lock: Get cell lock status

Implementation:
- Added 28 switch case entries
- Implemented 28 execution methods with validation
- Added 4 helper methods (direction parsers, format loaders)
- Added 49 new command options to Settings class

Coverage: Range CLI now has 42/42 actions (100% parity with Core/MCP)

* feat(cli): add 4 missing PowerQuery actions for full Core parity

Adds PowerQuery CLI support for:
- delete: Remove Power Query from workbook
- refresh: Update query data with configurable timeout
- get-load-config: Get current load destination settings
- refresh-all: Refresh all Power Queries in workbook

Implementation:
- Added 4 switch case entries
- Implemented 4 execution methods
- Added --refresh-timeout option for refresh action
- Updated help message with all 9 actions

Coverage: PowerQuery CLI now has 9/9 actions (100% parity with Core/MCP)

* feat(cli): add action aliases for DataModel and Table (full Core parity)

DataModel CLI: Already complete with 14/14 actions
- All Core methods already exposed with proper aliases
- No changes needed

Table CLI: Add action aliases for consistency
- Added 'read' alias for 'get' (table info)
- Added 'get-column-number-format' alias for 'get-column-format'
- Added 'set-column-number-format' alias for 'set-column-format'

Implementation:
- Updated switch statement to support both naming conventions
- Maintains backward compatibility with existing action names

Coverage: Table CLI now has 23/23 actions (100% parity with Core/MCP)

---------

Co-authored-by: Stefan Broenner <stefan.broenner@microsoft.comm>
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
2025-11-21 17:32:24 +01:00
..
2025-11-17 09:27:59 +00:00