Important update to DAB toolkit, fixed a few things, updated many#1138
Important update to DAB toolkit, fixed a few things, updated many#1138JerryNixon wants to merge 6 commits intomainfrom
Conversation
- Updated ContainerResourceCreationTests to improve exception handling and validation. - Removed outdated configuration files (dab-config.json, dab-config-2.json). - Added new configuration files with updated schemas and entities (dab-config-anonymous.json, dab-config-authenticated.json, dab-config-anonymous-2.json). - Introduced SQL script for creating a Star Trek database with relevant tables and data. - Enhanced test coverage for configuration file handling and mount behavior. - Added GlobalSuppressions.cs for code analysis suppression in BlazorApp.
…te test to reflect response type change
There was a problem hiding this comment.
Pull request overview
This PR updates the CommunityToolkit Aspire Azure Data API Builder (DAB) integration plus its sample AppHost/Blazor demo and tests, aligning config/schema versions, improving container configuration options, and simplifying the SQL Server demo initialization.
Changes:
- Updated DAB container/schema versions and expanded the integration API to support adding config files/folders via read-only bind mounts.
- Simplified the demo SQL Server initialization by switching to a single creation script, and added MCP Inspector wiring to the demo AppHost.
- Enhanced demo client response handling via a new
DabResponse<T>model and expanded test coverage around mounts/endpoints/image overrides.
Reviewed changes
Copilot reviewed 21 out of 22 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/CommunityToolkit.Aspire.Hosting.Azure.DataApiBuilder.Tests/dab-config-authenticated.json | Adds authenticated DAB config used by tests (schema updated to v1.6.87). |
| tests/CommunityToolkit.Aspire.Hosting.Azure.DataApiBuilder.Tests/dab-config-anonymous.json | Updates schema URL to v1.6.87 for anonymous test config. |
| tests/CommunityToolkit.Aspire.Hosting.Azure.DataApiBuilder.Tests/dab-config-anonymous-2.json | Updates schema URL to v1.6.87 for second anonymous test config. |
| tests/CommunityToolkit.Aspire.Hosting.Azure.DataApiBuilder.Tests/config-folder/dab-folder-config-anonymous.json | Adds folder-based config file for directory-mount tests. |
| tests/CommunityToolkit.Aspire.Hosting.Azure.DataApiBuilder.Tests/config-folder/dab-folder-config-anonymous-2.json | Adds second folder-based config file for directory-mount tests. |
| tests/CommunityToolkit.Aspire.Hosting.Azure.DataApiBuilder.Tests/ContainerResourceCreationTests.cs | Expands tests for mount behavior, health checks, endpoints, and image overrides. |
| tests/CommunityToolkit.Aspire.Hosting.Azure.DataApiBuilder.Tests/CommunityToolkit.Aspire.Hosting.Azure.DataApiBuilder.Tests.csproj | Ensures new JSON assets are copied to output for tests. |
| tests/CommunityToolkit.Aspire.Hosting.Azure.DataApiBuilder.Tests/AppHostTests.cs | Updates integration test deserialization to use DabResponse<T>. |
| src/CommunityToolkit.Aspire.Hosting.Azure.DataApiBuilder/README.md | Updates docs/examples to prefer WithConfigFile and adds MCP Inspector guidance. |
| src/CommunityToolkit.Aspire.Hosting.Azure.DataApiBuilder/DataApiBuilderHostingExtension.cs | Adds WithConfigFile/WithConfigFolder, removes default config auto-mount, and tweaks defaults/URLs. |
| src/CommunityToolkit.Aspire.Hosting.Azure.DataApiBuilder/DataApiBuilderContainerResource.cs | Seals resource and adds a PrimaryEndpoint accessor for service discovery. |
| src/CommunityToolkit.Aspire.Hosting.Azure.DataApiBuilder/DataApiBuilderContainerImageTags.cs | Updates default image tag to 1.6.87 and adds XML docs. |
| src/CommunityToolkit.Aspire.Hosting.Azure.DataApiBuilder/CommunityToolkit.Aspire.Hosting.Azure.DataApiBuilder.csproj | Removes unused health checks package reference. |
| examples/data-api-builder/CommunityToolkit.Aspire.Hosting.Azure.DataApiBuilder.BlazorApp/TrekApiClient.cs | Adds DabResponse<T>/DabError and changes error/logging behavior. |
| examples/data-api-builder/CommunityToolkit.Aspire.Hosting.Azure.DataApiBuilder.BlazorApp/GlobalSuppressions.cs | Adds a CA1873 suppression for logging performance analysis. |
| examples/data-api-builder/CommunityToolkit.Aspire.Hosting.Azure.DataApiBuilder.BlazorApp/Components/Pages/Home.razor | Updates demo UI copy. |
| examples/data-api-builder/CommunityToolkit.Aspire.Hosting.Azure.DataApiBuilder.AppHost/sql-server/entrypoint.sh | Removes custom SQL Server entrypoint script (no longer needed). |
| examples/data-api-builder/CommunityToolkit.Aspire.Hosting.Azure.DataApiBuilder.AppHost/sql-server/configure-db.sh | Removes custom SQL Server init script (no longer needed). |
| examples/data-api-builder/CommunityToolkit.Aspire.Hosting.Azure.DataApiBuilder.AppHost/sql-server.sql | Adds consolidated SQL creation/seed script for the demo database. |
| examples/data-api-builder/CommunityToolkit.Aspire.Hosting.Azure.DataApiBuilder.AppHost/dab-config.json | Updates schema URL and enables MCP in DAB config. |
| examples/data-api-builder/CommunityToolkit.Aspire.Hosting.Azure.DataApiBuilder.AppHost/Program.cs | Switches to WithCreationScript, adds MCP Inspector resource, and updates DAB setup. |
| examples/data-api-builder/CommunityToolkit.Aspire.Hosting.Azure.DataApiBuilder.AppHost/CommunityToolkit.Aspire.Hosting.Azure.DataApiBuilder.AppHost.csproj | Bumps AppHost SDK, adds MCP Inspector reference, and copies sql-server.sql to output. |
Comments suppressed due to low confidence (2)
src/CommunityToolkit.Aspire.Hosting.Azure.DataApiBuilder/DataApiBuilderHostingExtension.cs:59
AddDataAPIBuilderbind-mounts config files to/App/{fileName}but doesn’t guard against duplicate filenames across different paths. If two inputs share the same filename, this will create conflicting mounts. Consider reusingThrowIfMountTargetExists(or otherwise detecting duplicates) in this loop as well, to match the behavior documented forWithConfigFile/WithConfigFolder.
foreach (var configFilePath in configFilePaths)
{
var configFileName = File.Exists(configFilePath)
? Path.GetFileName(configFilePath)
: throw new FileNotFoundException($"Config file not found: {configFilePath}");
rb.WithBindMount(configFilePath, $"/App/{configFileName}", true);
}
tests/CommunityToolkit.Aspire.Hosting.Azure.DataApiBuilder.Tests/ContainerResourceCreationTests.cs:20
- Test name says "NameShouldNotBeNullOrWhiteSpace", but the assertion only checks
null(not empty/whitespace). Either extend the test to cover empty/whitespace inputs (and update implementation accordingly) or rename the test to reflect the actual validation.
[Fact]
public void AddDataApiBuilderNameShouldNotBeNullOrWhiteSpace()
{
IDistributedApplicationBuilder builder = DistributedApplication.CreateBuilder();
Assert.Throws<ArgumentNullException>(() => builder.AddDataAPIBuilder(null!));
}
src/CommunityToolkit.Aspire.Hosting.Azure.DataApiBuilder/README.md
Outdated
Show resolved
Hide resolved
...builder/CommunityToolkit.Aspire.Hosting.Azure.DataApiBuilder.BlazorApp/GlobalSuppressions.cs
Outdated
Show resolved
Hide resolved
...-api-builder/CommunityToolkit.Aspire.Hosting.Azure.DataApiBuilder.BlazorApp/TrekApiClient.cs
Show resolved
Hide resolved
...les/data-api-builder/CommunityToolkit.Aspire.Hosting.Azure.DataApiBuilder.AppHost/Program.cs
Show resolved
Hide resolved
...r/CommunityToolkit.Aspire.Hosting.Azure.DataApiBuilder.BlazorApp/Components/Pages/Home.razor
Outdated
Show resolved
Hide resolved
src/CommunityToolkit.Aspire.Hosting.Azure.DataApiBuilder/DataApiBuilderHostingExtension.cs
Outdated
Show resolved
Hide resolved
src/CommunityToolkit.Aspire.Hosting.Azure.DataApiBuilder/DataApiBuilderContainerResource.cs
Show resolved
Hide resolved
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| @@ -1,4 +1,4 @@ | |||
| <Project Sdk="Aspire.AppHost.Sdk/13.1.0"> | |||
| <Project Sdk="Aspire.AppHost.Sdk/13.1.1"> | |||
There was a problem hiding this comment.
Let's revert this change as we should keep them all in sync and we haven't had time to update everything else.
| @@ -1,5 +1,5 @@ | |||
| { | |||
| "$schema": "https://github.com/Azure/data-api-builder/releases/download/v1.3.19/dab.draft.schema.json", | |||
| "$schema": "https://github.com/Azure/data-api-builder/releases/download/v1.7.86-rc/dab.draft.schema.json", | |||
There was a problem hiding this comment.
This doesn't match the container image listed, shouldn't they match?
|
|
||
| <ItemGroup> | ||
| <PackageReference Include="Aspire.Hosting" /> | ||
| <PackageReference Include="Microsoft.Extensions.Diagnostics.HealthChecks" /> |
| name: DataApiBuilderContainerResource.HttpEndpointName) | ||
| .WithDataApiBuilderDefaults(); | ||
|
|
||
| // Use default config file path if no paths are provided |
There was a problem hiding this comment.
Do we no longer want to work on the expectation of a default config file, we're going to require one to be provided?
| if (!file.Exists) | ||
| { | ||
| throw new FileNotFoundException($"Config file not found: {file.FullName}"); | ||
| } |
There was a problem hiding this comment.
should we do an aggregation and raise all the missing files rather than just the first one missing? Given it's a runtime error, I think it'd be better to do something like var missing = configFiles.Where(f => !f.Exists); if (missing.Any()) throw...
| if (!folder.Exists) | ||
| { | ||
| throw new DirectoryNotFoundException($"Config directory not found: {folder.FullName}"); | ||
| } |
There was a problem hiding this comment.
Same as the comment on the file info version
| public static IResourceBuilder<DataApiBuilderContainerResource> WithConfigFile( | ||
| this IResourceBuilder<DataApiBuilderContainerResource> builder, | ||
| params FileInfo[] configFiles) |
There was a problem hiding this comment.
We have no param string[] configFiles (other than the initial entrypoint) - should we have that too?
|
|
||
| ### Example 1: Single data source | ||
|
|
||
| The docs for a basic configuration file are at [MS Learn](https://learn.microsoft.com/en-us/azure/data-api-builder/configuration/). |
There was a problem hiding this comment.
| The docs for a basic configuration file are at [MS Learn](https://learn.microsoft.com/en-us/azure/data-api-builder/configuration/). | |
| The docs for a basic configuration file are at [MS Learn](https://learn.microsoft.com/azure/data-api-builder/configuration/). |
best to drop locale so it directs people to the right localised version
|
|
||
| ### Example 2: Multiple data sources | ||
|
|
||
| The docs for multi-source configuration are at [MS Learn](https://learn.microsoft.com/en-us/azure/data-api-builder/concept/config/multi-data-source). |
There was a problem hiding this comment.
| The docs for multi-source configuration are at [MS Learn](https://learn.microsoft.com/en-us/azure/data-api-builder/concept/config/multi-data-source). | |
| The docs for multi-source configuration are at [MS Learn](https://learn.microsoft.com/azure/data-api-builder/concept/config/multi-data-source). |
best to drop locale so it directs people to the right localised version
This pull request introduces several improvements and updates to the Data API Builder Aspire integration and its demo applications. The most significant changes include upgrading dependencies, simplifying SQL Server initialization, adding MCP Inspector integration in the demo app, and enhancing error handling and logging in the Blazor client. Below are the most important changes grouped by theme:
Dependency and configuration updates:
Aspire.AppHost.Sdkversion inCommunityToolkit.Aspire.Hosting.Azure.DataApiBuilder.AppHost.csprojfrom 13.1.0 to 13.1.1, and updated the Data API Builder container image tag to 1.6.87 inDataApiBuilderContainerImageTags.cs. [1] [2]$schemareference indab-config.jsonto match the new Data API Builder version and enabled MCP integration in the configuration. [1] [2]SQL Server initialization simplification:
configure-db.shandentrypoint.sh) and replaced bind mounts with a direct SQL script (sql-server.sql) loaded and executed during container setup inProgram.cs. [1] [2] [3] [4]MCP Inspector integration:
Program.cs. [1] [2]Blazor client improvements:
TrekApiClient.csby introducing a new response model (DabResponse<T>,DabError) and logging errors for failed deserialization or API errors. Also updated UI text inHome.razorand added a code analysis suppression for expensive logging. [1] [2] [3]Codebase cleanup and documentation: