Schema Compare: Fabric Warehouse platform detection + constraint script preservation - #2722
Schema Compare: Fabric Warehouse platform detection + constraint script preservation#2722roramsay wants to merge 10 commits into
Conversation
For Feature 1847587 (Schema Compare for Fabric Warehouse). The vscode-mssql client needs to know which DacFx platform a comparison ran under so it can display the dialect badge in the diff view header (e.g. `SqlDwUnified` for Fabric Warehouse vs. `Sql160` for Azure SQL Database). Today the result has no way to convey this without the client re-walking the DacFx model. Adds `SourcePlatform` and `TargetPlatform` (nullable `string`) to both `SchemaCompareResult` types (`Microsoft.SqlTools.SqlCore` for SSMS direct-call consumers; `Microsoft.SqlTools.ServiceLayer` for the JSON-RPC `schemaCompare/compare` response). Populated from `ComparisonResult.SourceModel.Version.ToString()` and the target equivalent after the comparison runs -- pass-through reads, no new I/O. Null-safe when the comparison didn't produce a model. Wired through `SchemaCompareOperation`: - New `SourcePlatform` / `TargetPlatform` properties on the operation set by `Execute()` from the DacFx `ComparisonResult` models. - `SchemaCompareService` reads these into the JSON-RPC `SchemaCompareResult` it returns to clients. Tests: two new unit tests in `SchemaCompareTests.cs` covering the contract shape (both `SchemaCompareResult` and the operation expose the properties, default to `null`, and accept writes). End-to-end behaviour against live comparisons is covered by integration tests for Feature 1847587. No DacFx API change. No version bump required for this commit. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The public TSqlModel.Version surface maps SqlPlatforms.SqlDwUnified to
SqlServerVersion.Sql150 inside DacFx (InternalModelUtils.CalculateVersionsForPlatform),
so reading .Version.ToString() always returned 'Sql150' for Fabric Warehouse
models and mislabelled the platform pill in Schema Compare.
Read the comparison's normalized DSP from
SchemaComparisonResult.DataModel.DatabaseSchemaProvider.Platform via reflection.
DatabaseSchemaProvider is internal-abstract in DacFx but its .Platform property
returns the public SqlPlatforms enum, whose values map directly to the DSP-name
strings clients expect ('Sql160', 'SqlAzure', 'SqlDwUnified', etc.). Lookups
are cached statically; any reflection failure is logged and falls back to a
null platform string (the pill silently hides).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…diffs CreateDiffEntry stripped any child script that started with `alter` to avoid duplicating constraint DDL that the parent's CREATE TABLE already inlined for legacy SQL Server. SqlDwUnified (Fabric Warehouse) never inlines PK/FK/UNIQUE/ CHECK/DEFAULT constraints into CREATE TABLE; they are always emitted as standalone `ALTER TABLE ... ADD CONSTRAINT ...` statements. The filter therefore discarded the only script that defined the constraint, so the diff editor showed CREATE TABLE without the PK and the generated script was missing the PK definition entirely. Detect constraint children (PK/FK/UNIQUE/CHECK/DEFAULT) under a SqlDwUnified comparison and preserve their scripts in that case. Behavior is unchanged for all other platforms. Move the cached reflection-based platform detector from SchemaCompareOperation into SchemaCompareUtils so CreateDiffEntry can use it; add a ConditionalWeakTable cache keyed by SchemaComparisonResult so the recursive walk pays the reflection cost once per comparison, not once per diff entry. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
@microsoft-github-policy-service agree company="Microsoft" |
…e 1847587) Implements the test design from the Schema Compare for Fabric Warehouse spec (Feature 1847587). Unit tests (SchemaCompareTests.cs): Extracts SchemaCompareUtils.ShouldPreserveAlterScriptForConstraint(typeName, platform) as a pure helper so the SqlDwUnified strip-on-alter carve-out is testable without manufacturing a sealed DacFx SchemaComparisonResult graph. Existing IsConstraintChildOnSqlDwUnified is now a thin wrapper. Adds 18 TestCase rows covering every constraint suffix under SqlDwUnified, non-constraint types under SqlDwUnified, every non-Fabric platform string a comparison can emit (Sql160/Sql150/SqlAzure/SqlAzureV12/SqlDw), null/empty type and platform inputs, substring-vs-suffix guard, and GetComparisonPlatform null-result short-circuit. Integration tests (SchemaCompareFabricTests.cs): Full 4-endpoint-pair x 7-scenario matrix (28 cells) via [TestCaseSource]. Scenarios: AllSupportedTypes, CompatibleAlterColumn, IncompatibleAlterColumn, PrimaryKey, Unique, ForeignKey, SelectionScope. Endpoint pairs: DacpacToDacpac, DacpacToProject, ProjectToDacpac, ProjectToProject. Fabric dacpacs are built offline at fixture setup via new TSqlModel(SqlServerVersion.SqlDwUnified, ...) + DacPackageExtensions.BuildPackage so the suite requires no live Fabric Warehouse. Per-cell assertions (per spec lines 309-316): comparison validity + IsEqual=false + Differences.Any(); SourcePlatform == TargetPlatform == SqlDwUnified; constraints folded under parent SqlTable diff (not surfaced as top-level entries); constraint child carries ADD CONSTRAINT NONCLUSTERED NOT ENFORCED script; selection-scope IncludeOnly(OrdersScope) yields a Generate Script output that references zero unrelated tables or constraints (locks in the Prototype 3 empirical regression that motivated DacFx PR 2143938); project-target round-trip converges to IsEqual after publish. Live Fabric publish (per-cell assertion 7) is intentionally manual-only per the spec; it is exercised by the WSR matrix, not automated here. Fixtures (test/.../SchemaCompare/Fabric/*.sql): 14 minimal T-SQL scripts confined to the Fabric-supported data type list, with all constraints scripted as NONCLUSTERED NOT ENFORCED. Plus emptyFabricTemplate.sqlproj with DSP=SqlDwUnifiedDatabaseSchemaProvider for the Project endpoint cases. Requires DacFx >= 170.5.20 (the published build that ships PR 2143938's cascade fix). The sts-version-bump that flips Packages.props onto that DacFx is a separate change and is not included here. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Added the test scenarios from the spec (Feature 1847587 §Test design) in commit 40f96df: Unit tests ( Integration tests ( Fabric dacpacs are built offline at fixture setup via Per-cell assertions implemented per spec lines 309-316:
Local run: 28/28 Fabric integration tests pass (~17 s) against a locally-built DacFx
|
Public DacFx 170.5.38-preview ships the constraint cascade-exclusion fix (DacFx PR 2143938) that the SqlDwUnified Schema Compare integration tests depend on. Replaces the temporary 170.4.80-preview pin. Local run: 26/26 SchemaCompare unit tests + 28/28 Fabric integration tests pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Bumped Local verification against 170.5.38-preview:
CI dependency called out in the previous review comment is resolved. |
|
As part of updating the dependencies in Packages.props we require that any PRs opened also verify that Please respond to this comment verifying that you've done the appropriate validation (or explain why it's not necessary) before merging in the PR
|
|
Responding to the Packages.props update checklist:
|
|
Cross-referenced the DacFx Directly validates this bump — the intended fix is called out verbatim:
That is DacFx PR 2143938 (the cascade-exclusion fix). Our Also relevant, no code change needed:
Not applicable to this PR (informational):
Net: the release notes strengthen the case for this bump (they're the public confirmation of the fix our tests were waiting on) and none of the other changes require action in this PR. |
There was a problem hiding this comment.
Pull request overview
Adds Fabric Warehouse (SqlDwUnified) awareness to Schema Compare results and preserves Fabric-specific constraint ALTER scripts so diffs and generated deployment scripts don’t drop constraint definitions.
Changes:
- Expose
SourcePlatform/TargetPlatformon Schema Compare operation + result contracts, populated via reflected DSP platform detection. - Preserve child
ALTER TABLE ... ADD CONSTRAINTscripts for SqlDwUnified constraint diffs (PK/FK/UQ/CHECK/DEFAULT) instead of stripping “alter”-prefixed scripts. - Add an offline Fabric ↔ Fabric integration test matrix (dacpac/project combinations) plus Fabric SQL fixtures and an SDK-style Fabric
.sqlprojtemplate; bump DacFx package version.
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| test/Microsoft.SqlTools.ServiceLayer.UnitTests/SchemaCompare/SchemaCompareTests.cs | Adds unit coverage for platform properties and SqlDwUnified constraint-script preservation helper. |
| test/Microsoft.SqlTools.ServiceLayer.IntegrationTests/SchemaCompare/SqlProjects/emptyFabricTemplate.sqlproj | Adds a Fabric-targeted SQL project template (SqlDwUnified DSP) for integration testing. |
| test/Microsoft.SqlTools.ServiceLayer.IntegrationTests/SchemaCompare/SchemaCompareFabricTests.cs | Adds Fabric integration matrix tests validating platform detection, constraint folding, script preservation, selection-scope behavior, and project round-trip. |
| test/Microsoft.SqlTools.ServiceLayer.IntegrationTests/SchemaCompare/Fabric/FabricWidenColumn_Target.sql | Adds Fabric fixture for widening-column target schema. |
| test/Microsoft.SqlTools.ServiceLayer.IntegrationTests/SchemaCompare/Fabric/FabricWidenColumn_Source.sql | Adds Fabric fixture for widening-column source schema. |
| test/Microsoft.SqlTools.ServiceLayer.IntegrationTests/SchemaCompare/Fabric/FabricUnique_Target.sql | Adds Fabric fixture for UNIQUE scenario target schema. |
| test/Microsoft.SqlTools.ServiceLayer.IntegrationTests/SchemaCompare/Fabric/FabricUnique_Source.sql | Adds Fabric fixture for UNIQUE scenario source schema. |
| test/Microsoft.SqlTools.ServiceLayer.IntegrationTests/SchemaCompare/Fabric/FabricSelectionScope_Target.sql | Adds Fabric fixture for selection-scope target schema. |
| test/Microsoft.SqlTools.ServiceLayer.IntegrationTests/SchemaCompare/Fabric/FabricSelectionScope_Source.sql | Adds Fabric fixture for selection-scope source schema. |
| test/Microsoft.SqlTools.ServiceLayer.IntegrationTests/SchemaCompare/Fabric/FabricPrimaryKey_Target.sql | Adds Fabric fixture for PK scenario target schema. |
| test/Microsoft.SqlTools.ServiceLayer.IntegrationTests/SchemaCompare/Fabric/FabricPrimaryKey_Source.sql | Adds Fabric fixture for PK scenario source schema. |
| test/Microsoft.SqlTools.ServiceLayer.IntegrationTests/SchemaCompare/Fabric/FabricNarrowColumn_Target.sql | Adds Fabric fixture for narrowing-column target schema. |
| test/Microsoft.SqlTools.ServiceLayer.IntegrationTests/SchemaCompare/Fabric/FabricNarrowColumn_Source.sql | Adds Fabric fixture for narrowing-column source schema. |
| test/Microsoft.SqlTools.ServiceLayer.IntegrationTests/SchemaCompare/Fabric/FabricForeignKey_Target.sql | Adds Fabric fixture for FK scenario target schema. |
| test/Microsoft.SqlTools.ServiceLayer.IntegrationTests/SchemaCompare/Fabric/FabricForeignKey_Source.sql | Adds Fabric fixture for FK scenario source schema. |
| test/Microsoft.SqlTools.ServiceLayer.IntegrationTests/SchemaCompare/Fabric/FabricAllTypes_Target.sql | Adds Fabric fixture for supported-types target schema. |
| test/Microsoft.SqlTools.ServiceLayer.IntegrationTests/SchemaCompare/Fabric/FabricAllTypes_Source.sql | Adds Fabric fixture for supported-types source schema. |
| src/Microsoft.SqlTools.SqlCore/SchemaCompare/SchemaCompareUtils.cs | Adds SqlDwUnified constraint ALTER preservation logic and reflected/cached platform detection. |
| src/Microsoft.SqlTools.SqlCore/SchemaCompare/SchemaCompareOperation.cs | Hoists detected platform onto operation for propagation to result contract. |
| src/Microsoft.SqlTools.SqlCore/SchemaCompare/Contracts/SchemaCompareResults.cs | Adds platform fields to SqlCore result contract. |
| src/Microsoft.SqlTools.ServiceLayer/SchemaCompare/SchemaCompareService.cs | Wires operation platform fields into the ServiceLayer response payload. |
| src/Microsoft.SqlTools.ServiceLayer/SchemaCompare/Contracts/SchemaCompareRequest.cs | Adds platform fields to ServiceLayer JSON-RPC response contract. |
| Packages.props | Bumps Microsoft.SqlServer.DacFx to 170.5.38-preview. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
- SchemaCompareResults / SchemaCompareRequest: XML docs now correctly identify SourcePlatform / TargetPlatform as short names of DacFx SqlPlatforms values sourced from DatabaseSchemaProvider.Platform (not SqlServerVersion / TSqlModel.Version, which reports Sql150 for Fabric Warehouse). - SchemaCompareTests: updated rationale comment to match the reflection-based GetComparisonPlatform implementation used by this PR. - SchemaCompareFabricTests: simplified redundant IndexOf + CountOccurrences assertion for excluded-table check to a single IndexOf, and removed the now-unused CountOccurrences helper. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Reviewed the six PR-review comments from @copilot-pull-request-reviewer and addressed the five with actionable, valid suggestions in commit c2f73df. The DacFx-bump comment (#6) is dismissed as its premise is empirically false — see rationale below. Addressed (commit c2f73df):
Dismissed:
Post-fix verification against
|
- SchemaCompareUtils.cs: Logger.Warning now logs the full exception (type + message + stack) via ex.ToString(), not just ex.Message. Matches the Logger.Error(Exception) pattern already used elsewhere in Logger.cs. - SchemaCompareTests.cs: SchemaCompareOperationExposesSourceAndTargetPlatformProperties comment corrected to describe the actual population path (SchemaCompareUtils.GetComparisonPlatform via reflection), not the pre-PR TSqlModel.Version pass-through. - SchemaCompareFabricTests.cs: removed three unused SqlPrimaryKeyConstraintType / SqlForeignKeyConstraintType / SqlUniqueConstraintType constants (dead code — assertions use ConstraintTypeSuffixes). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Second-pass review fixes shipped in commit a13fa63 and all applicable threads resolved: New fixes in a13fa63:
Thread status:
Post-fix verification (
|
…mpat Type.GetProperty(string) defaults to BindingFlags.Public|Instance only, so if DacFx ever flips DataModel from non-public to public, or flips DatabaseSchemaProvider/Platform from public to non-public, the reflection lookup silently fails and permanently disables platform detection via s_reflectionInitFailed. All three GetProperty callsites (DataModel, DatabaseSchemaProvider, Platform) now use Public|NonPublic|Instance so the lookup tolerates accessibility changes across DacFx versions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Addressed all three reflection binding-flag comments in commit 339d8d6: All three Verification (
Threads resolved. |
…-merge-revalidate
| // Child scripts that do not use alter need to be added if they are being changed, ex: "EXECUTE sp_addextendedproperty...". | ||
| // Don't add scripts that start with alter because those are handled by a top level element's create | ||
| if (!sourceScript.ToLowerInvariant().StartsWith("alter")) | ||
| if (keepAlterScript || !sourceScript.ToLowerInvariant().StartsWith("alter")) | ||
| { | ||
| diffEntry.SourceScript = FormatScript(sourceScript); |
| PropertyInfo dspProp = s_dspProp ?? dataModel.GetType().GetProperty( | ||
| "DatabaseSchemaProvider", | ||
| BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance); | ||
| if (dspProp == null) | ||
| { | ||
| return null; | ||
| } | ||
| s_dspProp = dspProp; | ||
|
|
||
| object dsp = dspProp.GetValue(dataModel); | ||
| if (dsp == null) | ||
| { | ||
| return null; | ||
| } |
| PropertyInfo platformProp = s_platformProp ?? dsp.GetType().GetProperty( | ||
| "Platform", | ||
| BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance); | ||
| if (platformProp == null) | ||
| { | ||
| return null; | ||
| } | ||
| s_platformProp = platformProp; | ||
|
|
||
| object platformValue = platformProp.GetValue(dsp); | ||
| return platformValue?.ToString(); |
| /// Null if the | ||
| /// platform could not be detected (e.g. the comparison was never run). |
llali
left a comment
llali
left a comment
There was a problem hiding this comment.
both changes in this PR can be fixed in dacfx directly and that's what I recommend doing
| // .Platform is a public abstract SqlPlatforms property (a public enum), and | ||
| // the cascade fix in PR 2143938 confirms this value is reliably populated. | ||
| string comparisonPlatform = SchemaCompareUtils.GetComparisonPlatform(this.ComparisonResult); | ||
| this.SourcePlatform = comparisonPlatform; |
|
Closing this PR and moving the changes into DacFx |
Three commits on this branch ship the sqltoolsservice changes for Schema Compare
on Microsoft Fabric Warehouse (Feature 1847587):
30d90abe— surfaceSourcePlatform/TargetPlatformon the result contract.23a8ef56— read platform from the comparison''s DSP via cached reflection(TSqlModel.Version returns
Sql150for SqlDwUnified due to an internalDacFx mapping bug — see InternalModelUtils.CalculateVersionsForPlatform).
aa9a0761— keep Fabric Warehouse constraint scripts under table diffs(SqlDwUnified-scoped carve-out in CreateDiffEntry''s
starts-with-alterfilter; behaviour unchanged for every other platform).
Companion PRs:
constraints under SqlDwUnified.
affected-constraints banner.
Spec: https://dev.azure.com/powerbi/Trident/_git/specs-dw/pullrequest/989687
Verified end-to-end against a live Fabric Warehouse + a Fabric .sqlproj:
SqlDwUnifiedon both endpoints.for an Add table with a PK constraint.
CREATE TABLEfollowed bythe standalone
ALTER TABLE ... ADD CONSTRAINT [PK_...] PRIMARY KEY NONCLUSTERED ([col]) NOT ENFORCED;statement.Test gap: a new unit test for
CreateDiffEntry_PreservesConstraintAlterScriptUnderSqlDwUnifiedis in the spec''s test design but not added in this PR — the SchemaCompare
unit-test project (
Microsoft.SqlTools.ServiceLayer.UnitTests) currentlyfails to compile against the DacFx 170.5 overlay (
TSqlModelBuilderremoved inIntelliSense/SqlProjectIntelliSenseTests.cs). The test will be added once
the DacFx package version is officially bumped (gated on PR 2143938).