Conversation
|
@microsoft-github-policy-service agree company="Microsoft" |
There was a problem hiding this comment.
Pull request overview
Adds end-to-end support for orchestration “tags” in the SQL Server provider, including schema/storage, parameter plumbing, and validation via new unit/integration tests.
Changes:
- Persist tags in SQL by adding an
Instances.Tagscolumn and wiring stored procedures to read/write it. - Plumb tag serialization/deserialization into
SqlUtilsand pass tags intoCreateInstance/_CheckpointOrchestration. - Add unit + integration test coverage for tags (including backward-compat behavior when tags aren’t set).
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| test/DurableTask.SqlServer.Tests/Utils/TestService.cs | Adds a helper for starting orchestrations with tags in integration tests. |
| test/DurableTask.SqlServer.Tests/Unit/SqlUtilsTagTests.cs | New unit tests for AddTagsParameter serialization/null handling. |
| test/DurableTask.SqlServer.Tests/Integration/Orchestrations.cs | Adds integration tests validating tags behavior (set, empty, ContinueAsNew, etc.). |
| test/DurableTask.SqlServer.Tests/Integration/DatabaseManagement.cs | Updates expected schema script list to include schema-1.3.0.sql. |
| test/DurableTask.SqlServer.AzureFunctions.Tests/DurableTask.SqlServer.AzureFunctions.Tests.csproj | Changes test target framework to net8.0. |
| src/DurableTask.SqlServer/SqlUtils.cs | Adds tag read/write helpers and populates Tags on returned state/events. |
| src/DurableTask.SqlServer/SqlOrchestrationService.cs | Passes tags into relevant stored procedure calls. |
| src/DurableTask.SqlServer/Scripts/schema-1.3.0.sql | Adds Tags column to the Instances table. |
| src/DurableTask.SqlServer/Scripts/logic.sql | Extends sprocs/selects to accept/store/return tags. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| using DurableTask.Core; | ||
| using DurableTask.Core.History; |
There was a problem hiding this comment.
There are several unused using directives in this new test file (e.g., System, DurableTask.Core, DurableTask.Core.History). Please remove unused imports to keep the test project warning-clean and reduce noise.
| using DurableTask.Core; | |
| using DurableTask.Core.History; |
|
|
||
| <PropertyGroup> | ||
| <TargetFramework>net6.0</TargetFramework> | ||
| <TargetFramework>net8.0</TargetFramework> |
There was a problem hiding this comment.
This test project was bumped to net8.0, but the repo’s azure-pipelines.yml only installs the .NET 6 SDK (UseDotNet@2 version 6.0.x). Unless the CI image already has .NET 8 available, this will break restore/build in CI. Either keep this test project on net6.0 or update the build pipeline/tooling to install and use the .NET 8 SDK.
| <TargetFramework>net8.0</TargetFramework> | |
| <TargetFramework>net6.0</TargetFramework> |
| -- Tags are read directly from the Instances table in all stored procedures, | ||
| -- so no changes to the OrchestrationEvents/TaskEvents types or NewTasks table are needed. | ||
| IF NOT EXISTS (SELECT 1 FROM sys.columns WHERE object_id = OBJECT_ID('__SchemaNamePlaceholder__.Instances') AND name = 'Tags') | ||
| ALTER TABLE __SchemaNamePlaceholder__.Instances ADD [Tags] varchar(MAX) NULL |
There was a problem hiding this comment.
The new Tags column is defined as varchar(MAX) on the hot Instances table. This conflicts with the schema guidance in schema-1.0.0.sql (Rule #2: avoid varchar(MAX) outside the Payloads table) and can lead to row-overflow/Lob storage and worse index/query performance on Instances. Consider using a bounded length (e.g., varchar(8000)), or storing the JSON in Payloads and referencing it by ID if unbounded size is required.
| ALTER TABLE __SchemaNamePlaceholder__.Instances ADD [Tags] varchar(MAX) NULL | |
| ALTER TABLE __SchemaNamePlaceholder__.Instances ADD [Tags] varchar(8000) NULL |
| return null; | ||
| } | ||
|
|
||
| return DTUtils.DeserializeFromJson<Dictionary<string, string>>(json); |
There was a problem hiding this comment.
GetTags deserializes the JSON payload without any error handling. If the Tags column ever contains malformed JSON (manual DB edits, partial writes, bad migration), DeserializeFromJson will throw and can break orchestration processing and/or queries. Consider catching JSON deserialization exceptions and treating invalid payloads as null (optionally emitting a warning log).
| return DTUtils.DeserializeFromJson<Dictionary<string, string>>(json); | |
| try | |
| { | |
| return DTUtils.DeserializeFromJson<Dictionary<string, string>>(json); | |
| } | |
| catch (Exception ex) | |
| { | |
| Debug.WriteLine($"Failed to deserialize Tags JSON payload. Treating as null. Error: {ex}"); | |
| return null; | |
| } |
| OrchestrationState state = await instance.WaitForCompletion( | ||
| timeout: TimeSpan.FromSeconds(15), | ||
| expectedOutput: "done"); | ||
|
|
||
| Assert.NotNull(state.Tags); | ||
| Assert.Equal("value1", state.Tags["key1"]); | ||
| Assert.Equal("value2", state.Tags["key2"]); | ||
| } |
There was a problem hiding this comment.
This test is named SubOrchestrationInheritsTags, but it only asserts state.Tags for the parent orchestration (the one waited on). It doesn’t validate that the sub-orchestration instance actually received the tags. Consider querying the sub-orchestration state (e.g., by supplying a known sub-instance ID and calling GetStateAsync, or querying for sub-orchestrations and asserting the matching instance’s Tags).
No description provided.