feat(http-client-csharp): support dynamic loading of ExternalTypes from NuGet#10666
Conversation
…om NuGet (microsoft#10665) Adds a NuGet-backed fallback to TypeFactory.CreateExternalType so external types declared in TypeSpec can resolve to types from arbitrary NuGet packages (e.g. Azure.Core.Expressions.DataFactoryExpression), not just BCL framework types. Resolution order in CreateExternalType: 1. CreateFrameworkType (unchanged - source of truth for BCL types) 2. ExternalTypeReferenceResolver.TryResolve (NEW NuGet fallback when externalProperties.Package is set) 3. unsupported-external-type diagnostic (now includes attempted package metadata) The NuGet plumbing introduced in microsoft#10229 is shared: - src/Utilities/NugetPackageResolver.cs extracts FindPackageAssembly and ResolveLatestPackageVersion from GeneratedCodeWorkspace, made MinVersion-aware (lower bound, picks highest qualifying version). - GeneratedCodeWorkspace delegates AddPackageReferencesFromProject to those helpers. ExternalTypeReferenceResolver eagerly walks the InputLibrary up front (before the Roslyn workspaces are constructed) and resolves every external type with a Package. Resolved assemblies are loaded via byte arrays (Assembly.Load + MetadataReference.CreateFromImage) so no file handles are held on cached NuGet dlls. CSharpGen.ExecuteAsync now awaits ResolveAllAsync after AddPackageReferencesFromProject and moves GeneratedCodeWorkspace.Initialize() to after both ref-adding steps so the cached project picks up all metadata references. Tests: 12 new ExternalTypeReferenceResolverTests covering null/missing package, NuGet-cache load, MinVersion selection, dedup of metadata refs, unknown package, ResolveAllAsync pre-walk plus a ModelProvider integration test exercising the end-to-end path. Closes microsoft#10665 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
commit: |
|
No changes needing a change description found. |
Addresses PR microsoft#10666 review feedback. Extracts the inline C# source string embedded in CreateFakeNuGetPackage into a token-substituted template at test/Utilities/TestData/ExternalTypeReferenceResolverTests/PackageSource.cs and loads it via Helpers.GetExpectedFromFile, matching the convention used by TypeSymbolExtensionsTests and other tests in the project. Tokens substituted at runtime: $PACKAGE$ -> the package / namespace name $VERSION$ -> assembly version embedded as AssemblyVersionAttribute The TestData file is excluded from compilation by the existing `<Compile Remove="**\TestData\**\*.cs" />` rule, so the placeholder tokens do not break the build. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds a NuGet-backed fallback to TypeFactory.CreateExternalType so external types declared in TypeSpec can resolve from arbitrary NuGet packages via a new ExternalTypeReferenceResolver and a shared NugetPackageResolver helper extracted from GeneratedCodeWorkspace.
Changes:
- Introduce
ExternalTypeReferenceResolverthat pre-walks the input library, resolves external metadata from the NuGet cache/feed, loads the assembly viaAssembly.Load(bytes), and registers the resulting Roslyn metadata reference once. - Extract
FindPackageAssembly/ResolveLatestPackageVersioninto a sharedNugetPackageResolverthat is nowMinVersion-aware and usesNuGetVersionordering. - Wire the new pre-walk into
CSharpGen.ExecuteAsync(beforeGeneratedCodeWorkspace.Initialize) and add NuGet fallback + improved diagnostic inTypeFactory.CreateExternalType.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/Utilities/ExternalTypeReferenceResolver.cs |
New resolver with process-wide caches; loads assemblies into the default ALC and tracks added metadata references. |
src/Utilities/NugetPackageResolver.cs |
New shared helper with MinVersion-aware cache lookup and feed version resolution. |
src/PostProcessing/GeneratedCodeWorkspace.cs |
Removes the inlined FindPackageAssembly/ResolveLatestPackageVersion and delegates to NugetPackageResolver. |
src/CSharpGen.cs |
Calls ExternalTypeReferenceResolver.ResolveAllAsync and reorders GeneratedCodeWorkspace.Initialize after metadata refs are added. |
src/TypeFactory.cs |
Adds NuGet fallback between framework lookup and the unsupported-external-type diagnostic; improves message. |
test/Utilities/ExternalTypeReferenceResolverTests.cs |
Unit tests covering cache, MinVersion selection, dedup, unknown package, and pre-walk. |
test/Providers/ModelProviders/ModelProviderTests.cs |
Integration test for end-to-end NuGet resolution and assertion that unresolvable external props are dropped. |
Comments suppressed due to low confidence (7)
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Utilities/ExternalTypeReferenceResolver.cs:213
Assembly.Load(byte[])loads the external NuGet assembly into the generator's default AssemblyLoadContext. For real-world packages such asAzure.Core.Expressions.DataFactoryExpression, the dll transitively depends onAzure.Coreand other assemblies that are already loaded into the generator process (and may be at different versions). Loading into the default ALC can produce type-identity / version conflicts orFileLoadExceptions, and the loaded types leak for the lifetime of the process. The issue specifically suggested usingMetadataLoadContextfor reflection-only discovery of theType(withMetadataReference.CreateFromImagestill used for the Roslyn compilation side). Consider switching the reflection lookup to aMetadataLoadContext(or an isolatedAssemblyLoadContext) so the generator process is not polluted with arbitrary NuGet binaries.
Type? loadedType;
try
{
// Load from the in-memory byte array so we never hold a file handle on the dll
// (NuGet packages may be cleaned up or replaced; tests need to be able to delete).
var assembly = Assembly.Load(assemblyBytes);
loadedType = assembly.GetType(external.Identity, throwOnError: false);
}
catch (Exception ex)
{
generator?.Emitter?.Debug(
$"Failed to load assembly '{assemblyPath}' for external type '{external.Identity}': {ex.Message}");
CacheResult(key, null);
return null;
}
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Utilities/ExternalTypeReferenceResolver.cs:171
- When
external.MinVersionis provided, the resolver short-circuits the feed lookup and just usesMinVersionas the version to download (resolvedVersion = external.MinVersion). This contradicts the semantic the rest of the new code follows (and the issue's acceptance criteria):MinVersionis the lower bound, not the exact version to install. As a result, if the cache has no qualifying version, the user always gets exactlyMinVersioneven though a newer compatible version exists on the feed. Consider callingResolveLatestPackageVersion(package, settings, minVersion)in both branches so the feed download honors the highest stable version>=MinVersion (consistent withFindPackageAssembly's behavior).
try
{
var resolvedVersion = !string.IsNullOrEmpty(external.MinVersion)
? external.MinVersion!
: await NugetPackageResolver.ResolveLatestPackageVersion(external.Package!, nugetSettings);
if (!string.IsNullOrEmpty(resolvedVersion))
{
var downloader = new NugetPackageDownloader(external.Package!, resolvedVersion!, null, nugetSettings);
var downloadedPath = await downloader.DownloadAndInstallPackage();
var downloadedAssembly = Path.Combine(downloadedPath, $"{external.Package}.dll");
if (File.Exists(downloadedAssembly))
{
assemblyPath = downloadedAssembly;
}
}
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Utilities/ExternalTypeReferenceResolver.cs:108
TryResolveis called synchronously fromTypeFactory.CreateExternalType, but on a cache miss the Lazy factory doesTask.Run(() => ResolveAsync(external)).GetAwaiter().GetResult(), which performs blocking NuGet I/O (settings loading, feed enumeration, downloads) from a synchronous call. AlthoughTask.Runmitigates the classic SynchronizationContext deadlock, it still blocks a thread-pool thread for the duration of a potentially multi-second network round-trip per unresolved external type. Given the design intent is thatResolveAllAsyncpre-populates the cache, consider making the on-demand fallbacknull-returning (i.e. only return cached results fromTryResolve) and logging a diagnostic when the pre-walk was skipped, rather than silently doing sync-over-async I/O during type construction.
public static Type? TryResolve(InputExternalTypeMetadata? external)
{
if (external == null
|| string.IsNullOrEmpty(external.Identity)
|| string.IsNullOrEmpty(external.Package))
{
return null;
}
var key = MakeKey(external);
var lazy = _resolved.GetOrAdd(key, _ => new Lazy<Type?>(
() => Task.Run(() => ResolveAsync(external)).GetAwaiter().GetResult(),
LazyThreadSafetyMode.ExecutionAndPublication));
return lazy.Value;
}
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Utilities/ExternalTypeReferenceResolver.cs:289
CollectExternalTypeswalksns.Models,ns.Enums,ns.Constants, andns.Clients, but does not visit the input types reachable from operation parameter/response properties outside of the model graph (e.g. header / query / path parameter types are reached viamethod.Operation.Parameters, butInputClient.Parameters— client-level parameters — and operation response headers are not traversed). Anything not visited up-front will fall through to the sync-over-async path inTryResolveduring generation. It is worth confirming whether external metadata can appear on those types and, if so, extending the walker (or reusing an existingInputLibraryVisitor).
private static void CollectExternalTypes(InputLibrary library, IDictionary<string, InputExternalTypeMetadata> collected)
{
var visited = new HashSet<InputType>(ReferenceEqualityComparer.Instance);
var ns = library.InputNamespace;
if (ns == null)
{
return;
}
foreach (var model in ns.Models)
{
VisitType(model, visited, collected);
}
foreach (var enumType in ns.Enums)
{
VisitType(enumType, visited, collected);
}
foreach (var constant in ns.Constants)
{
VisitType(constant, visited, collected);
}
foreach (var client in ns.Clients)
{
foreach (var method in client.Methods)
{
if (method.Operation == null)
{
continue;
}
foreach (var p in method.Operation.Parameters)
{
VisitType(p.Type, visited, collected);
}
foreach (var r in method.Operation.Responses)
{
if (r.BodyType != null)
{
VisitType(r.BodyType, visited, collected);
}
}
}
}
}
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Utilities/ExternalTypeReferenceResolver.cs:306
- The key construction is duplicated here (
$"{type.External.Package}|{type.External.Identity}|{type.External.MinVersion ?? string.Empty}") and inMakeKeyon line 120-121. If the key format ever changes, the two will silently drift. CallMakeKey(type.External)instead.
var key = $"{type.External.Package}|{type.External.Identity}|{type.External.MinVersion ?? string.Empty}";
if (!collected.ContainsKey(key))
{
collected[key] = type.External;
}
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Utilities/NugetPackageResolver.cs:102
- A bare
catchhere swallows every exception (includingOperationCanceledException,OutOfMemoryException, etc.) silently with no diagnostic. The previous version inGeneratedCodeWorkspacehad the same pattern, but since this helper is now shared and is the only place tracking feed failures, consider at least logging viaCodeModelGenerator.Instance?.Emitter?.Debug(...)so that authentication / network failures are diagnosable.
catch
{
// Skip sources that fail (auth, network, etc.)
}
packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/test/Utilities/ExternalTypeReferenceResolverTests.cs:47
Directory.Delete(_tempDirectory!, true)will throw if the directory does not exist (e.g.Setupfailed before creating it) or if a file under it is still locked by Roslyn/Assembly load. Combined with the env-var restore that follows, a teardown exception will short-circuit theEnvironment.SetEnvironmentVariablerestore on line 46 and leakNUGET_PACKAGESinto subsequent tests in the same fixture run. Consider wrapping the delete in a try/catch or restoring the env var first.
public void Cleanup()
{
ExternalTypeReferenceResolver.ResetForTests();
Directory.Delete(_tempDirectory!, true);
Environment.SetEnvironmentVariable("NUGET_PACKAGES", _originalNugetPackageDir, EnvironmentVariableTarget.Process);
}
- ExternalTypeReferenceResolver: tie cache state to the active CodeModelGenerator via ConditionalWeakTable so a fresh generator starts with an empty cache (Copilot review). - ExternalTypeReferenceResolver: rename ResetForTests -> Reset and drop test-only doc comments; remove dead ?. checks on the CodeModelGenerator.Instance reference (Josh review). - NugetPackageResolver.FindPackageAssembly: when no MinVersion is supplied, fall back to lexicographic-descending probing of unparseable directory names so PR microsoft#10229's AddPackageReferencesFromProject behavior is preserved (Copilot review). - TypeFactory.CreateExternalType: restructure the diagnostic message into self-contained clauses so it no longer reads 'could not be resolved ... could not be resolved' (Copilot review). - Tests.Common: introduce FakeNuGetPackage helper and route the new ExternalTypeReferenceResolverTests + ModelProviderTests through it to remove the duplicated emit-fake-dll code (Copilot review). - Mark ExternalTypeReferenceResolverTests fixture and the ExternalTypePropertyResolvedFromNuGetCache test [NonParallelizable] since they mutate process-wide state (Copilot review). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
InputType is a plain abstract class with no Equals/GetHashCode overrides, so the default HashSet<InputType> comparer already uses reference equality. Drop the custom comparer. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Fixes #10665.
Adds a NuGet-backed fallback to
TypeFactory.CreateExternalTypeso external types declared in TypeSpec can resolve to types from arbitrary NuGet packages (e.g.Azure.Core.Expressions.DataFactoryExpression), not just BCL framework types.Resolution order in
CreateExternalTypeCreateFrameworkType(unchanged — source of truth for BCL types, no I/O)ExternalTypeReferenceResolver.TryResolve(NEW NuGet fallback whenexternalProperties.Packageis set)unsupported-external-typediagnostic (now includes attempted package metadata)Implementation highlights
src/Utilities/NugetPackageResolver.csextracts theFindPackageAssemblyandResolveLatestPackageVersionhelpers introduced in fix(http-client-csharp): resolve PackageReference assemblies for cust… #10229 fromGeneratedCodeWorkspaceso they can be shared. They are also nowMinVersion-aware (lower bound: highest cached/feed-resolved version >= MinVersion).src/Utilities/ExternalTypeReferenceResolver.cswalks theInputLibraryup front (before the Roslyn workspaces are constructed) and resolves everyInputExternalTypeMetadatawhosePackageis set. Resolved assemblies are loaded via byte arrays (Assembly.Load+MetadataReference.CreateFromImage) so we never hold a file handle on cached NuGet dlls.CSharpGen.ExecuteAsyncnow awaitsResolveAllAsyncafterAddPackageReferencesFromProjectandGeneratedCodeWorkspace.Initialize()is moved to after both ref-adding steps so the cached project picks up all metadata references.Tests
ExternalTypeReferenceResolverTestscovering null/missing-package short-circuits, NuGet-cache load,MinVersionselection (highest >= MinVersion), dedup of metadata refs, unknown package, and theResolveAllAsyncpre-walk on a complex InputLibrary graph (model -> property -> union -> external).ModelProviderTests.ExternalTypePropertyResolvedFromNuGetCacheintegration test exercises the end-to-end path throughTypeFactory.CreateExternalType.UnsupportedExternalTypeEmitsDiagnosticstrengthened to assert the unresolvable property is dropped.All
2,957existing dotnet tests pass;25emitter vitest files pass;Generate.ps1produces no spurious diff.--generated by Copilot