Implement Connection Properties and supporting expressions#11938
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 11938Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 11938" |
| /// <param name="qdrantResource">The Qdrant server resource</param> | ||
| /// <param name="connectionName">An override of the source resource's name for the connection string. The resulting connection string will be "ConnectionStrings__connectionName" if this is not null.</param> | ||
| /// <returns>The <see cref="IResourceBuilder{T}"/>.</returns> | ||
| public static IResourceBuilder<TDestination> WithReference<TDestination>(this IResourceBuilder<TDestination> builder, IResourceBuilder<QdrantServerResource> qdrantResource, string? connectionName = null) |
There was a problem hiding this comment.
@davidfowl this is an important change, this resource has a custom WithReference that didn't add the relationship. Now I am invoking the common one.
| // Renders a custom operation in the manifest expression to indicate that | ||
| // the value should be URI-encoded. | ||
|
|
||
| // if (expression.StartsWith('{') && |
There was a problem hiding this comment.
This is where the manifest could be updated with the uri formatting information.
| /// <param name="resource">The resource that provides the connection properties. Cannot be null.</param> | ||
| /// <param name="key">The key of the connection property to retrieve. Cannot be null.</param> | ||
| /// <returns>The value associated with the specified connection property key.</returns> | ||
| public static ReferenceExpression GetConnectionProperty(this IResourceWithConnectionString resource, string key) |
There was a problem hiding this comment.
Maybe this should be on the interface?
| certKeyFile: process.env['HTTPS_CERT_KEY_FILE'] ?? '', | ||
| cacheAddress: process.env['ConnectionStrings__cache'] ?? '', | ||
| cacheUrl: process.env['CACHE_URI'] ?? '', | ||
| apiServer: process.env['services__weatherapi__https__0'] ?? process.env['services__weatherapi__http__0'] |
There was a problem hiding this comment.
We gotta work on this next 😄
|
@aaronpowell big changes for polyglot connection strings. This is non breaking but we'll need a similar update on the community toolkit. @sebastienros Lets generate a spec as part of this PR that explains the well known keys by convention:
|
There was a problem hiding this comment.
Pull Request Overview
This PR implements a comprehensive connection properties system that allows resources to expose structured metadata (host, port, credentials, URIs, etc.) beyond just raw connection strings. Resources can now opt into injecting individual connection properties as environment variables, giving consuming applications more granular access to connection information.
- Connection Properties API: New
GetConnectionProperties()method onIResourceWithConnectionStringexposes key-value pairs for structured connection metadata - Environment Injection Control: New annotation system (
ReferenceEnvironmentInjectionFlags) allows resources to specify whether they want connection strings, individual properties, or both when referenced - URI Format Support:
ReferenceExpressionnow supports:uriformatting to ensure proper URL encoding of interpolated values
Reviewed Changes
Copilot reviewed 62 out of 62 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/Aspire.Hosting/ApplicationModel/IResourceWithConnectionString.cs |
Added GetConnectionProperties() method to interface |
src/Aspire.Hosting/ApplicationModel/ReferenceExpression.cs |
Added format specifier support (:uri) and Empty static field |
src/Aspire.Hosting/ApplicationModel/ReferenceEnvironmentInjectionFlags.cs |
New enum for controlling what gets injected as environment variables |
src/Aspire.Hosting/ApplicationModel/ReferenceEnvironmentInjectionAnnotation.cs |
New annotation to configure injection behavior per resource |
src/Aspire.Hosting/ResourceBuilderExtensions.cs |
Updated WithReference to support connection properties splatting |
src/Aspire.Hosting/ConnectionPropertiesExtensions.cs |
Helper methods for combining connection properties |
| Multiple resource files (Redis, PostgreSQL, MySQL, etc.) | Implemented connection properties for database and messaging resources |
src/Aspire.Hosting.NodeJs/NodeExtensions.cs |
Added default All injection flags for NPM apps |
| Test files | Comprehensive test coverage for new functionality |
Comments suppressed due to low confidence (1)
tests/Aspire.Hosting.Tests/WithReferenceTests.cs:1
- [nitpick] Consider using a more secure test password or avoiding special characters in test passwords that could cause encoding issues. The '@' symbol in passwords requires careful handling in URI encoding scenarios.
// Licensed to the .NET Foundation under one or more agreements.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| /// <returns>An enumerable collection of key/value pairs, where each key is the name of a connection property and each value | ||
| /// is its corresponding <see cref="ReferenceExpression"/>. The collection is empty if there are no connection | ||
| /// properties.</returns> | ||
| IEnumerable<KeyValuePair<string, ReferenceExpression>> GetConnectionProperties() => []; |
There was a problem hiding this comment.
One nice thing about using a Dictionary here is that we can initialize it with a comparer. The design doc says
Keys are emitted in PascalCase; lookups are case insensitive and duplicates should be avoided.
But there is no real way to enforce "lookups are case insensitive and duplicates should be avoided".
But I agree that just doing IEnumerable will work fine, and should be lower overhead than a Dictionary.
There was a problem hiding this comment.
We can always reconsider after some usage
| /// <param name="source">The resource that exposes the base connection properties.</param> | ||
| /// <param name="additional">The additional connection properties to merge into the values supplied by <paramref name="source"/>.</param> | ||
| /// <returns>A sequence that contains the combined set of connection properties with duplicate keys resolved in favor of <paramref name="additional"/>.</returns> | ||
| public static IEnumerable<KeyValuePair<string, ReferenceExpression>> CombineProperties(this IResourceWithConnectionString source, IEnumerable<KeyValuePair<string, ReferenceExpression>> additional) |
There was a problem hiding this comment.
This method is going to show up in intellisense everywhere, right? Should we instead not make it an extension method?
There was a problem hiding this comment.
More like a utility method? Can you think of another example so I move it these? I think it should remain public since we expect other integrations to reuse it.
There was a problem hiding this comment.
Yes, public is appropriate. Just not an extension method. I don't think the method needs to be moved necessarily.
Other examples of this is creating generated parameters -
| /// <remarks> | ||
| /// Format: <c>mongodb://[user:password@]{host}:{port}/{database}[?authSource=admin&authMechanism=SCRAM-SHA-256]</c>. The credential and query segments are included only when a password is configured. | ||
| /// </remarks> | ||
| public ReferenceExpression UriExpression => Parent.BuildConnectionString(DatabaseName); |
There was a problem hiding this comment.
How is this different than ConnectionStringExpression? Looks like they both do the same implementation.
There was a problem hiding this comment.
I thought this could be useful if we need to change one or the other to have a separate property. Or do you mean UriExpression => ConnectionStringExpression ?
There was a problem hiding this comment.
I guess I was surprised that there are 2 public properties that return the same information. If it is a common pattern to have both, then that's fine.
Or do you mean UriExpression => ConnectionStringExpression ?
That would be a nice to have change to show that they are the same.
| IEnumerable<KeyValuePair<string, ReferenceExpression>> IResourceWithConnectionString.GetConnectionProperties() => | ||
| Parent.CombineProperties([ | ||
| new("Database", ReferenceExpression.Create($"{DatabaseName}")), | ||
| new("JdbcConnectionString", JdbcConnectionString), |
There was a problem hiding this comment.
If both the Parent and the Child have the same JdbcConnectionString, the child's value wins, right?
There was a problem hiding this comment.
Yes, there is a unit test to ensure that. It also happens for Uri in other resources.
eerhardt
left a comment
There was a problem hiding this comment.
Let's get this in so we can play with it and we can tweak as we go.
davidfowl
left a comment
There was a problem hiding this comment.
Lots of follow up expected after we merge this.
Description
WithReferencenow honors a newReferenceEnvironmentInjectionAnnotation, letting resources opt into injecting connection strings, individual connection properties, or both viaReferenceEnvironmentInjectionFlags. Project, executable resources, Python and NpmApp default to injecting everything; others stay connection-string only unless annotated.ReferenceExpressionget a format specifier (currently:uri) on both literals and resource providers. A new encoder helper ensures URI-safe interpolation without double encoding. The manifest expressions are not encoded but the code is commented for reference.IResourceWithConnectionStringexposesGetConnectionProperties, andResourceBuilderExtensionsnow uses it to splat environment variables (with configurable prefixes) and to query specific properties viaGetConnectionProperty.playground/nodesample was updated to use the new connection metadata.I used this document to define which properties need to be exposed and when UriExpression make sense:
connection-reference-pre-lastcols.md
Contributes to #2111
The Azure resources will be handled in a subsequent PR.
Checklist
<remarks />and<code />elements on your triple slash comments?