microsoft / microsoft/typespec
[http-client-csharp] AddTypeToKeep stores stale FQN after visitor namespace changes, causing public models to be internalized
- Dominant language
- Java
- Stars
- 5.9k
- Forks
- 394
- Avg merge
- 1d 23h
- Merged PRs (30d)
- 104
Description
## Bug Report
### Description
`AddTypeToKeep(TypeProvider)` eagerly resolves `type.Type.FullyQualifiedName` to a string at call time. When a visitor (e.g. `NamespaceVisitor`) later changes the type's namespace, the stored FQN becomes stale. `PostProcessor.ShouldKeepType()` then fails to match the type, causing it to be internalized even though `@@access(public)` was set.
This was discovered during the **ProviderHub** management SDK migration, where models like `ManifestLevelPropertyBag` and `ProviderFeaturesRule` were silently internalized despite explicit `@@access(Access.public)` decorators.
### Root Cause
In `CodeModelGenerator.cs`:
```csharp
public void AddTypeToKeep(TypeProvider type, bool isRoot = true)
=> AddTypeToKeep(type.Type.FullyQualifiedName, isRoot);
```
This is called from `ModelProvider` constructor (for `Access == "public"`) and `TypeFactory` (for public enums) — both run **before** visitors execute.
The pipeline order in `CSharpGen.ExecuteAsync()`:
1. Build TypeProviders (constructors run → `AddTypeToKeep` stores pre-visitor FQN)
2. Run visitors (`NamespaceVisitor` changes namespace → FQN changes)
3. Write generated files
4. `PostProcessAsync()` → `PostProcessor` receives stale FQNs in `typesToKeep`
In `PostProcessor.ShouldKeepType()`, both checks fail:
- Simple name `"MyModel"` vs typesToKeep containing `"OldNamespace.MyModel"` → no match
- Roslyn FQN `"NewNamespace.Models.MyModel"` vs `"OldNamespace.MyModel"` → no match
### Impact
Models with `@@access(public)` get silently internalized when `model-namespace` is true (the mgmt default). This affects management SDK migrations where models like `ManifestLevelPropertyBag` (ProviderHub service) become internal despite explicit public access decorators.
### Reproduction
```csharp
[Test]
public void PublicModelTypeToKeepUpdatesAfterNamespaceChange()
{
var inputModel = InputFactory.Model(
"MockInputModel",
access: "public");
MockHelpers.LoadMockGenerator(
inputModelTypes: [inputModel]);
var modelProvider = CodeModelGenerator.Instance.OutputLibrary.TypeProviders
.SingleOrDefault(t => t.Name == "MockInputModel") as ModelProvider;
Assert.IsNotNull(modelProvider);
var originalFqn = modelProvider!.Type.FullyQualifiedName;
Assert.AreEqual("Sample.Models.MockInputModel", originalFqn);
// Simulate a visitor changing the namespace (e.g., NamespaceVisitor appending ".Models")
modelProvider.Update(@namespace: "NewNamespace.Models");
var newFqn = modelProvider.Type.FullyQualifiedName;
Assert.AreEqual("NewNamespace.Models.MockInputModel", newFqn);
// After namespace change, the resolved root types should contain the updated FQN
// so that PostProcessor.ShouldKeepType can match it against the generated code.
var rootTypes = CodeModelGenerator.Instance.AdditionalRootTypes;
Assert.IsTrue(rootTypes.Contains(newFqn),
$"AdditionalRootTypes should contain the post-visitor FQN '{newFqn}' "
+ $"but only contains: [{string.Join(", ", rootTypes)}]");
}
```
### Proposed Fix
Store `TypeProvider` references instead of eagerly resolving FQNs. Resolve lazily when `AdditionalRootTypes`/`NonRootTypes` are accessed (after visitors have run).
Contributor guide
Assessment
This issue has not been assessed yet.