Resolve Azure.Core.Expressions.DataFactory identities in AzureTypeFactory.CreateFrameworkType#59365
Conversation
…tory.CreateFrameworkType Agent-Logs-Url: https://github.com/Azure/azure-sdk-for-net/sessions/6129c2eb-fa15-474f-97e0-14f33da46f49 Co-authored-by: haiyuazhang <1924967+haiyuazhang@users.noreply.github.com>
| /// <inheritdoc/> | ||
| protected override Type? CreateFrameworkType(string fullyQualifiedTypeName) | ||
| { | ||
| // Resolve types from the Azure.Core.Expressions.DataFactory assembly. These types are |
There was a problem hiding this comment.
This approach is brittle and not a direction that I'm comfortable with. We shouldn't be special casing libraries via hardcode. We'll want to come up with a more generic approach before moving forward.
There was a problem hiding this comment.
I'm not sure this is needed as DFE is already handled through the CreateCSharpType override.
There was a problem hiding this comment.
Also MTG already has a fallback for unknown types - https://github.com/microsoft/typespec/blob/main/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/TypeFactory.cs#L287-L294
There was a problem hiding this comment.
This approach is brittle and not a direction that I'm comfortable with. We shouldn't be special casing libraries via hardcode. We'll want to come up with a more generic approach before moving forward.
Azure.Core.Expressions.DataFactory is a core library that the azure generator already takes a dependency on. However based on my other two comments, I'm not sure why this change is needed.
There was a problem hiding this comment.
@copilot can you clarify why the existing code path in CreateCSharpType is not sufficient? DFE is used as a union type so we need the additional union metadata to resolve the generic arg.
There was a problem hiding this comment.
Per offline conversation with Josh, I've come to learn that this is an efficiency pattern already established, not a new pattern. Clearing my blocker.
There was a problem hiding this comment.
You're right on both counts — I've reverted the change in 8086b37.
To clarify the original concern: the issue is about non-DFE types (DataFactorySecret, DataFactoryLinkedServiceReference, DataFactorySecretString) appearing as InputModelType.External.Identity, not as union variants — so the CreateCSharpType union override doesn't apply here. However, the base TypeFactory.CreateExternalType (decompiled) does already have the generic fallback you pointed at: when CreateFrameworkType returns null, it calls ExternalTypeReferenceResolver.TryResolve(externalProperties) using the Package/MinVersion metadata, and reports an unsupported-external-type diagnostic only when both fail.
So my fix was special-casing what the generic resolver should handle. The real gap is on the emitter side — @@alternateType(..., { identity: "..." }, "csharp") needs to also emit package (and ideally minVersion) metadata on the InputExternalTypeMetadata so ExternalTypeReferenceResolver can locate the type without hardcoded assembly lookups in the generator. Happy to look at the emitter side if you'd like, but wanted to confirm direction before moving forward.
There was a problem hiding this comment.
Since the Azure Generatory is already aware of DataFactory types I think we can extend to include them here. Though, I would prefer just hardcoding the fully qualified name for each type rather than using the startswith and looking up the type from the assembly.
@copilot
There was a problem hiding this comment.
Done in 3d25ee1 — added the three DataFactory FQNs (DataFactorySecret, DataFactoryLinkedServiceReference, DataFactorySecretString) as explicit cases in the existing switch alongside ResourceIdentifier/AzureLocation/ResponseError/ETag, no StartsWith or reflection-based assembly lookup. All 41 AzureTypeFactoryTests pass.
Agent-Logs-Url: https://github.com/Azure/azure-sdk-for-net/sessions/d1020932-3aeb-4b15-8a64-9ad5ca54ccc2 Co-authored-by: JoshLove-msft <54595583+JoshLove-msft@users.noreply.github.com>
…tory Agent-Logs-Url: https://github.com/Azure/azure-sdk-for-net/sessions/18ee4980-e45a-466b-b10b-776acb76e984 Co-authored-by: JoshLove-msft <54595583+JoshLove-msft@users.noreply.github.com>
| "Azure.Core.AzureLocation" => typeof(AzureLocation), | ||
| "Azure.ResponseError" => typeof(ResponseError), | ||
| "Azure.ETag" => typeof(ETag), | ||
| // Types from Azure.Core.Expressions.DataFactory are commonly referenced via |
Agent-Logs-Url: https://github.com/Azure/azure-sdk-for-net/sessions/d30b2f52-3232-4ed6-bbe1-c5ea9d9fd17d Co-authored-by: JoshLove-msft <54595583+JoshLove-msft@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Fixes a generator type-resolution gap where @@alternateType(..., { identity: "Azure.Core.Expressions.DataFactory.*" }, "csharp") identities could not be resolved via Type.GetType, causing external/aliased model types to fail resolution and (downstream) generated properties to be dropped.
Changes:
- Added explicit
CreateFrameworkTypeswitch mappings forDataFactorySecret,DataFactoryLinkedServiceReference, andDataFactorySecretString. - Extended the test
InputFactory.Modelhelper to allow settingInputModelType.Externalmetadata. - Added tests covering external identity resolution through
CreateCSharpTypeand directCreateFrameworkTyperesolution for the DataFactory identities.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| eng/packages/http-client-csharp/generator/Azure.Generator/src/AzureTypeFactory.cs | Adds explicit framework-type mappings for DataFactory identities to avoid Type.GetType resolution failures. |
| eng/packages/http-client-csharp/generator/Azure.Generator/test/common/InputFactory.cs | Enables constructing InputModelType instances with External metadata for test scenarios. |
| eng/packages/http-client-csharp/generator/Azure.Generator/test/AzureTypeFactoryTests.cs | Adds regression tests for external identity → framework type resolution, including DataFactory types. |
| "Azure.ETag" => typeof(ETag), | ||
| "Azure.Core.Expressions.DataFactory.DataFactorySecret" => typeof(DataFactorySecret), | ||
| "Azure.Core.Expressions.DataFactory.DataFactoryLinkedServiceReference" => typeof(DataFactoryLinkedServiceReference), | ||
| "Azure.Core.Expressions.DataFactory.DataFactorySecretString" => typeof(DataFactorySecretString), | ||
| _ => base.CreateFrameworkType(fullyQualifiedTypeName) |
Picks up upstream generator fixes: - LRO OperationSource rename: *OperationSource -> *ResourceOperationSource (mgmt codegen LRO array action surface + generic OperationSource hardening, Azure#59522) - Array body parameter handling (Azure#59566) - Azure.Core.Expressions.DataFactory identity resolution (Azure#59365) - Custom base type override for inheritable system types (Azure#56634) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Description
Properties whose model type is identity-aliased to an
Azure.Core.Expressions.DataFactory.*type via@@alternateType(..., { identity: "..." }, "csharp")were silently dropped from generated SDKs (e.g.SecretBase,LinkedServiceReference,SecureStringinAzure.ResourceManager.DataFactory).The base generator routes
InputType.External != nullthroughCreateExternalType→CreateFrameworkType(Identity). The baseCreateFrameworkTyperesolves viaType.GetType, which cannot find types inAzure.Core.Expressions.DataFactorywithout an assembly-qualified name. Resolution returnsnull,CreateExternalTypefails, and the property is dropped with no diagnostic.Changes
AzureTypeFactory.CreateFrameworkType: added three explicit fully-qualified-name cases to the existing switch —Azure.Core.Expressions.DataFactory.DataFactorySecret,Azure.Core.Expressions.DataFactory.DataFactoryLinkedServiceReference, andAzure.Core.Expressions.DataFactory.DataFactorySecretString— mapped to their respectivetypeof(...)values, matching the existing style used forResourceIdentifier,AzureLocation,ResponseError, andETag.ManagementTypeFactoryinherits the fix.InputFactory.Model(test helper): added an optionalexternalTypeMetadataparameter so tests can constructInputModelTypes withExternalset (mirrors the existingUnionhelper).AzureTypeFactoryTests: end-to-end coverage throughCreateCSharpTypefor anAzure.Coreidentity and the issue'sDataFactorySecretcase, plus directCreateFrameworkTypecases for all three documented DataFactory identities.This checklist is used to make sure that common guidelines for a pull request are followed.
General Guidelines
Testing Guidelines
SDK Generation Guidelines
*.csprojandAssemblyInfo.csfiles have been updated with the new version of the SDK.