Extract Schema Compare Contracts and Interfaces to SqlCore#2638
Extract Schema Compare Contracts and Interfaces to SqlCore#2638
Conversation
…c business logic from Microsoft.SqlTools.ServiceLayer into Microsoft.SqlTools.SqlCore. The goal is to enable code sharing between different SQL editor hosts (VS Code, ADS, SSMS) via a NuGet package, by separating core comparison logic from host-specific concerns like JSON-RPC routing, ConnectionService, and SqlTaskManager.
…nto bruzel/schema-compare
Refactored DeploymentOptions, DeploymentOptionProperty<T>, and related types from SchemaCompare.Contracts to DacFx.Contracts for better code organization and reusability. Moved DacFxUtils to DacFx as well. Updated all references and usings throughout the codebase. Fixed a recursion bug in SchemaCompareIncludeExcludeAllNodesOperation. Improved string initialization with nameof(). No changes to core logic or behavior.
…nto bruzel/schema-compare
…chema Compare contracts - Move DeploymentOptions.cs from ServiceLayer.DacFx.Contracts to SqlCore.DacFx.Contracts - Move DacFxUtils.cs from ServiceLayer.DacFx to SqlCore.DacFx - Create Schema Compare interfaces in SqlCore (ISchemaCompareConnectionProvider, ISchemaCompareScriptHandler, AccessTokenProvider) - Create Schema Compare contracts in SqlCore (SchemaCompareContracts, SchemaCompareParams, SchemaCompareResults) - Update ServiceLayer Schema Compare contracts to extend SqlCore base types - Add SchemaCompareOptionsRequest.cs and SchemaCompareGetDefaultOptionsRequest handler - Update ServiceLayer DacFx and SqlPackage files with new SqlCore using statements - Update operation files with type-specific using aliases to resolve ambiguities - Update test files for new type locations Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: bruzel <3098798+bruzel@users.noreply.github.com>
Co-authored-by: bruzel <3098798+bruzel@users.noreply.github.com>
Co-authored-by: bruzel <3098798+bruzel@users.noreply.github.com>
…SSMS overrides already handled by DeploymentScenario enum Agent-Logs-Url: https://github.com/microsoft/sqltoolsservice/sessions/6c2eb4a0-86e8-4003-b9af-3483bb467ca5 Co-authored-by: bruzel <3098798+bruzel@users.noreply.github.com>
…faultSchemaCompareOptions() The schemaCompare/getDefaultOptions endpoint duplicates existing dacfx/getDeploymentOptions with Scenario=SchemaCompare. Additionally, GetDefaultSchemaCompareOptions() was buggy - it called new DeploymentOptions() which defaults to DeploymentScenario.Deployment, not SchemaCompare. Removed: - GetDefaultSchemaCompareOptions() from DeploymentOptions.cs - SchemaCompareOptionsRequest.cs (entire file) - HandleSchemaCompareGetDefaultOptionsRequest handler from SchemaCompareService.cs - Section 2.6 and related references from migration plan doc Agent-Logs-Url: https://github.com/microsoft/sqltoolsservice/sessions/99873b20-1bb0-46d1-b098-9f346741b8e5 Co-authored-by: bruzel <3098798+bruzel@users.noreply.github.com>
…nto bruzel/SCRefactor2
There was a problem hiding this comment.
Pull request overview
Phase 2 of the Schema Compare refactor, extracting host-agnostic Schema Compare and DacFx shared types into SqlCore so operations can later be reused by multiple hosts (e.g., VSCode/ADS, SSMS), while keeping current ServiceLayer operations intact.
Changes:
- Introduces new SqlCore Schema Compare interfaces and contract types (params/results/domain models).
- Moves DacFx shared types (
DeploymentOptions,DacFxUtils) into SqlCore and updates ServiceLayer/tests to reference the new namespaces. - Adds/updates a migration plan document describing the phased architecture changes.
Reviewed changes
Copilot reviewed 39 out of 39 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/Microsoft.SqlTools.ServiceLayer.UnitTests/SqlPackage/SqlPackageServiceTests.cs | Update DeploymentOptions contract namespace import to SqlCore. |
| test/Microsoft.SqlTools.ServiceLayer.UnitTests/SchemaCompare/SchemaCompareTests.cs | Update Schema Compare contracts import to SqlCore. |
| test/Microsoft.SqlTools.ServiceLayer.IntegrationTests/SchemaCompare/SchemaCompareTestUtils.cs | Update DeploymentOptions import to SqlCore. |
| test/Microsoft.SqlTools.ServiceLayer.IntegrationTests/SchemaCompare/SchemaCompareServiceTests.cs | Switch Schema Compare contract type references to SqlCore aliases. |
| test/Microsoft.SqlTools.ServiceLayer.IntegrationTests/SchemaCompare/SchemaCompareServiceOptionsTests.cs | Update DacFx contracts import to SqlCore; add SqlCore alias usage. |
| test/Microsoft.SqlTools.ServiceLayer.IntegrationTests/DacFx/DacFxServiceTests.cs | Update DacFx contracts import to SqlCore. |
| src/Microsoft.SqlTools.SqlCore/SchemaCompare/schema-compare-migration-plan.md | New/updated migration plan for phases 1–3. |
| src/Microsoft.SqlTools.SqlCore/SchemaCompare/ISchemaCompareScriptHandler.cs | New host abstraction for script delivery. |
| src/Microsoft.SqlTools.SqlCore/SchemaCompare/ISchemaCompareConnectionProvider.cs | New host abstraction for connection string/token resolution and parsing. |
| src/Microsoft.SqlTools.SqlCore/SchemaCompare/Contracts/SchemaCompareResults.cs | New host-agnostic Schema Compare result types. |
| src/Microsoft.SqlTools.SqlCore/SchemaCompare/Contracts/SchemaCompareParams.cs | New host-agnostic Schema Compare parameter types. |
| src/Microsoft.SqlTools.SqlCore/SchemaCompare/Contracts/SchemaCompareContracts.cs | New shared Schema Compare domain types (endpoint info, diff entries, etc.). |
| src/Microsoft.SqlTools.SqlCore/SchemaCompare/AccessTokenProvider.cs | New internal DacFx auth provider wrapper for access tokens. |
| src/Microsoft.SqlTools.SqlCore/DacFx/DacFxUtils.cs | Move + namespace update; make utility publicly accessible from SqlCore. |
| src/Microsoft.SqlTools.SqlCore/DacFx/Contracts/DeploymentOptions.cs | Move + add scenario-based defaulting + publish defaults factory. |
| src/Microsoft.SqlTools.ServiceLayer/SqlPackage/SqlPackageService.cs | Update DacFxUtils namespace import to SqlCore. |
| src/Microsoft.SqlTools.ServiceLayer/SqlPackage/Contracts/GenerateSqlPackageCommandRequest.cs | Update DeploymentOptions type reference to SqlCore. |
| src/Microsoft.SqlTools.ServiceLayer/SchemaCompare/SchemaCompareUtils.cs | Alias Schema Compare domain types from SqlCore. |
| src/Microsoft.SqlTools.ServiceLayer/SchemaCompare/SchemaCompareSaveScmpOperation.cs | Update DacFxUtils namespace import to SqlCore. |
| src/Microsoft.SqlTools.ServiceLayer/SchemaCompare/SchemaCompareOperation.cs | Update DacFxUtils namespace import to SqlCore; alias DiffEntry from SqlCore. |
| src/Microsoft.SqlTools.ServiceLayer/SchemaCompare/SchemaCompareOpenScmpOperation.cs | Update DeploymentOptions import to SqlCore; alias endpoint/object id types from SqlCore. |
| src/Microsoft.SqlTools.ServiceLayer/SchemaCompare/SchemaCompareIncludeExcludeNodeOperation.cs | Alias DiffEntry from SqlCore. |
| src/Microsoft.SqlTools.ServiceLayer/SchemaCompare/SchemaCompareIncludeExcludeAllNodesOperation.cs | Alias DiffEntry from SqlCore. |
| src/Microsoft.SqlTools.ServiceLayer/SchemaCompare/Contracts/SchemaCompareSaveScmpRequest.cs | Import Schema Compare contract types from SqlCore. |
| src/Microsoft.SqlTools.ServiceLayer/SchemaCompare/Contracts/SchemaCompareRequest.cs | Remove duplicated DiffEntry/endpoint enum definitions; import SqlCore equivalents. |
| src/Microsoft.SqlTools.ServiceLayer/SchemaCompare/Contracts/SchemaComparePublishProjectChangesRequest.cs | Make params extend SqlCore base params and add TaskExecutionMode. |
| src/Microsoft.SqlTools.ServiceLayer/SchemaCompare/Contracts/SchemaComparePublishDatabaseChangesRequest.cs | Make params extend SqlCore base params and add TaskExecutionMode. |
| src/Microsoft.SqlTools.ServiceLayer/SchemaCompare/Contracts/SchemaCompareOpenScmpRequest.cs | Remove duplicated SchemaCompareObjectId; import SqlCore equivalents. |
| src/Microsoft.SqlTools.ServiceLayer/SchemaCompare/Contracts/SchemaCompareIncludeExcludeNodeRequest.cs | Make params extend SqlCore base params and add TaskExecutionMode. |
| src/Microsoft.SqlTools.ServiceLayer/SchemaCompare/Contracts/SchemaCompareIncludeExcludeAllNodesRequest.cs | Make params extend SqlCore base params and add TaskExecutionMode. |
| src/Microsoft.SqlTools.ServiceLayer/SchemaCompare/Contracts/SchemaCompareGenerateScriptRequest.cs | Make params extend SqlCore base params and add TaskExecutionMode. |
| src/Microsoft.SqlTools.ServiceLayer/DacFx/GenerateDeployScriptOperation.cs | Update DacFxUtils namespace import to SqlCore. |
| src/Microsoft.SqlTools.ServiceLayer/DacFx/DeployOperation.cs | Update DacFxUtils namespace import to SqlCore. |
| src/Microsoft.SqlTools.ServiceLayer/DacFx/DacFxService.cs | Update imports to SqlCore DacFx types. |
| src/Microsoft.SqlTools.ServiceLayer/DacFx/Contracts/SavePublishProfileRequest.cs | Update DeploymentOptions import to SqlCore. |
| src/Microsoft.SqlTools.ServiceLayer/DacFx/Contracts/GetOptionsFromProfileRequest.cs | Update DeploymentOptions import to SqlCore. |
| src/Microsoft.SqlTools.ServiceLayer/DacFx/Contracts/GetDeploymentOptionsRequest.cs | Remove local DeploymentScenario enum; use SqlCore’s version. |
| src/Microsoft.SqlTools.ServiceLayer/DacFx/Contracts/GenerateDeployScriptRequest.cs | Update DeploymentOptions import to SqlCore. |
| src/Microsoft.SqlTools.ServiceLayer/DacFx/Contracts/DeployRequest.cs | Update DeploymentOptions import to SqlCore. |
Comments suppressed due to low confidence (2)
src/Microsoft.SqlTools.SqlCore/DacFx/Contracts/DeploymentOptions.cs:123
- The XML doc for the parameterless DeploymentOptions ctor says it initializes Schema Compare defaults, but the implementation delegates to DeploymentScenario.Deployment. Either update the summary to match the behavior, or change the ctor to use the SchemaCompare scenario if that’s the intended default.
src/Microsoft.SqlTools.SqlCore/DacFx/Contracts/DeploymentOptions.cs:169 - InitializeFromProfile performs synchronous file IO (File.ReadAllText) but still returns Task and is awaited by callers. Consider either restoring async IO (ReadAllTextAsync + async/await) to avoid blocking request threads, or make the method synchronous and update call sites accordingly.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…er.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 39 out of 39 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
src/Microsoft.SqlTools.SqlCore/DacFx/Contracts/DeploymentOptions.cs:123
- The XML doc says the parameterless
DeploymentOptions()constructor initializes Schema Compare defaults, but it delegates toDeploymentScenario.Deployment. Please either update the comment to match the behavior (deployment defaults) or change the constructor to useSchemaCompareif that was intended (note this would be a behavioral change).
src/Microsoft.SqlTools.SqlCore/DacFx/Contracts/DeploymentOptions.cs:170 InitializeFromProfilereturns aTaskand is awaited by callers, but it performs a synchronousFile.ReadAllText, which can block the request handler thread. Consider using async I/O for modern targets (e.g.,ReadAllTextAsyncon net8.0) and/orTask.Runfor net472 to preserve the non-blocking behavior.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
...t.SqlTools.ServiceLayer/SchemaCompare/Contracts/SchemaComparePublishProjectChangesRequest.cs
Show resolved
Hide resolved
...t.SqlTools.ServiceLayer/SchemaCompare/Contracts/SchemaComparePublishProjectChangesRequest.cs
Show resolved
Hide resolved
src/Microsoft.SqlTools.ServiceLayer/SchemaCompare/SchemaCompareUtils.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.SqlTools.SqlCore/SchemaCompare/Contracts/SchemaCompareContracts.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.SqlTools.SqlCore/SchemaCompare/AccessTokenProvider.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.SqlTools.SqlCore/SchemaCompare/schema-compare-migration-plan.md
Show resolved
Hide resolved
Refactored AccessTokenProvider to Microsoft.SqlTools.SqlCore.Utility and updated all references accordingly. Removed duplicate implementation in SqlCore.SchemaCompare. Updated comments in SchemaCompareContracts.cs to clarify enum sync requirements with the MSSQL for VSCode Extension.
Removed the #nullable disable directive from AccessTokenProvider.cs, allowing nullable reference type analysis and warnings to be enabled or follow the project default for this file. No other changes made.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 40 out of 40 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (4)
src/Microsoft.SqlTools.SqlCore/DacFx/Contracts/DeploymentOptions.cs:123
- The parameterless DeploymentOptions constructor summary says it initializes with Schema Compare defaults, but it delegates to DeploymentScenario.Deployment. This is either a misleading doc comment or a functional bug (callers using
new DeploymentOptions()will not get the SchemaCompare-specific SSMS override defaults). Consider either changing the default ctor to useDeploymentScenario.SchemaCompare, or updating the XML docs and ensuring call sites explicitly request the intended scenario.
src/Microsoft.SqlTools.SqlCore/DacFx/DacFxUtils.cs:66 booleanOptionsDictionary = deploymentOptions.BooleanOptionsDictionary as Dictionary<...>can setbooleanOptionsDictionaryto null (e.g., ifBooleanOptionsDictionaryis null after deserialization or reassignment), which will then throw when iterating it. Since the property is alreadyDictionary<string, DeploymentOptionProperty<bool>>, consider avoiding theascast and instead null-coalescing to an empty dictionary (or validating and throwing a clearer exception).
src/Microsoft.SqlTools.SqlCore/DacFx/Contracts/DeploymentOptions.cs:169InitializeFromProfileis stillTask-returning and awaited by callers, but it now performs a synchronousFile.ReadAllText(profilePath)which will block the request handler thread. Consider restoring async I/O (ReadAllTextAsync) or changing the method to be synchronous (and updating callers) to avoid the "async method that blocks" pattern.
src/Microsoft.SqlTools.SqlCore/DacFx/Contracts/DeploymentOptions.cs:153- The
DeploymentOptions(DeploymentScenario scenario)XML doc says the Deployment scenario uses "DacFx native defaults without any modifications", butExcludeObjectTypesis always initialized with a schema-compare-specific exclude list and the ctor only initializes boolean options (no scenario-specific handling forExcludeObjectTypes). This makes it unclear what defaults callers actually get for Deployment vs SchemaCompare. Consider either makingExcludeObjectTypesscenario-dependent (or set fromDacDeployOptions) or adjusting the docs to match the current behavior.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Replaced individual type aliases (DiffEntry, SchemaCompareEndpointType, SchemaCompareObjectId) with a single CoreContracts alias for Microsoft.SqlTools.SqlCore.SchemaCompare.Contracts. Updated all references to use CoreContracts for improved clarity and consistency.
Extract Schema Compare Contracts and Interfaces to SqlCore (Phase 2/3)
Description
Phase 2 of the Schema Compare refactoring (Phase 1: PR #2635). Extracts shared type infrastructure into
SqlCoreso Schema Compare operations can be consumed by multiple hosts (VSCode/ADS, SSMS).Full plan:
schema-compare-migration-plan.md.No behavioral changes. Operations remain in ServiceLayer and work identically. This is purely structural preparation for Phase 3 (operation migration + adapter pattern).
What moved to SqlCore
DeploymentOptionsandDacFxUtils— relocated with namespace change fromServiceLayer.DacFx→SqlCore.DacFxDeploymentScenarioenum toDeploymentOptionssodacfx/getDeploymentOptionscan return scenario-appropriate defaults (SchemaCompare vs Deployment)GetDefaultPublishOptions()factory for publish-specific defaultsWhat's new in SqlCore
ISchemaCompareConnectionProvider— abstracts connection string/token retrieval (decouples fromConnectionService)ISchemaCompareScriptHandler— abstracts script delivery (decouples fromSqlTask)AccessTokenProvider— DacFxIUniversalAuthProviderimpl for Azure MFASqlCore/SchemaCompare/Contracts/:SchemaCompareContracts.cs— domain types (DiffEntry,SchemaCompareEndpointInfo,SchemaCompareEndpointType)SchemaCompareParams.cs— all parameter typesSchemaCompareResults.cs— all result typesServiceLayer contract changes
ServiceLayer contracts now extend SqlCore base types, adding only host-specific fields:
Types like
SchemaCompareEndpointType,DiffEntry, andSchemaCompareObjectIdare removed from ServiceLayer (now imported from SqlCore).Architecture: Before → After Phase 2 → After Phase 3
Before (main branch)
After Phase 2 (this PR) — shared types extracted
After Phase 3 (next PR) — operations moved, adapters added
How ServiceLayer contracts extend SqlCore types
The key design pattern: SqlCore defines host-agnostic base types, ServiceLayer adds host-specific fields (
TaskExecutionMode,ConnectionDetails):File change summary (~39 files)
DeploymentOptions.cs,DacFxUtils.cs(namespace change)usingstatement updates onlyusingstatement updates onlyusingstatement updates onlyReviewer guide
schema-compare-migration-plan.mdfor full contextSqlCore/SchemaCompare/ISchemaCompareConnectionProvider.csSqlCore/SchemaCompare/ISchemaCompareScriptHandler.csSqlCore/SchemaCompare/Contracts/SchemaCompareContracts.csSqlCore/SchemaCompare/Contracts/SchemaCompareParams.csSqlCore/SchemaCompare/Contracts/SchemaCompareResults.csDeploymentOptions.csandDacFxUtils.cs(namespace changes +DeploymentScenarioenum addition)SchemaCompareRequest.csis the most significant; others follow the same patternusing-only changes — ~24 files are purelyusingstatement additions