Skip to content

Commit cf1c72d

Browse files
fix(http-client-csharp): override MRW *Core methods when external base declares them (#11044)
Fixes #11042 ## Problem Models derived from an external/referenced MRW-generated base type (e.g. `Azure.AI.Extensions.OpenAI` types deriving from OpenAI's `ResponseItem`) generated `PersistableModelCreateCore` and the other serialization `*Core` methods as `protected virtual` instead of `protected override`. The compiler then reports CS0114 (`hides inherited member ... add the override keyword`). ## Root cause PR #11002 routes external `InputModelType`s through `SystemObjectModelProvider` so derived models get a real `ModelProvider` base. However `SystemObjectModelProvider.ShouldSkipDerivedSerializationMethodOverrides` unconditionally returns `true` (introduced in #9862 for hand-authored ARM bases like `ResourceData` that do **not** declare the generated `*Core` methods). Reusing it for *all* external bases forced derived models to hide rather than override even when the external base is itself MRW-generated and exposes the virtual `*Core` methods. ## Fix `MrwSerializationTypeDefinition` now decides whether to skip the derived overrides by reflecting on the wrapped framework type when the base is a `SystemObjectModelProvider`: if it declares all four generated `*Core` methods (`JsonModelWriteCore`, `JsonModelCreateCore`, `PersistableModelWriteCore`, `PersistableModelCreateCore`), the derived model emits `protected override`; otherwise existing behavior is preserved (bases without those methods, e.g. `object`/`ResourceData`, still emit `virtual`). ## Tests Added regression tests in `SystemObjectModelSerializationTests` covering an external base that follows the generated MRW pattern, asserting the four `*Core` methods are emitted as `override`. Existing tests (object base -> virtual) remain unchanged. Full ClientModel suite (1435 tests) passes. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent ea6c31d commit cf1c72d

2 files changed

Lines changed: 176 additions & 1 deletion

File tree

packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/src/Providers/MrwSerializationTypeDefinition.cs

Lines changed: 52 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,7 @@ public MrwSerializationTypeDefinition(InputModelType inputModel, ModelProvider m
9494
_additionalBinaryDataProperty = new(GetAdditionalBinaryDataPropertiesProp);
9595
_additionalProperties = new(() => [.. _model.Properties.Where(p => p.IsAdditionalProperties)]);
9696
_shouldOverrideMethods = _model.BaseModelProvider != null && !_isStruct;
97-
_shouldSkipDerivedSerializationMethodOverrides = _model.BaseModelProvider?.ShouldSkipDerivedSerializationMethodOverrides == true;
97+
_shouldSkipDerivedSerializationMethodOverrides = ShouldSkipDerivedSerializationMethodOverrides(_model.BaseModelProvider);
9898
_utf8JsonWriterSnippet = _utf8JsonWriterParameter.As<Utf8JsonWriter>();
9999
_mrwOptionsParameterSnippet = _serializationOptionsParameter.As<ModelReaderWriterOptions>();
100100
_jsonElementParameterSnippet = _jsonElementDeserializationParam.As<JsonElement>();
@@ -157,6 +157,57 @@ private static bool IsModelType(CSharpType type)
157157
=> ScmCodeModelGenerator.Instance.TypeFactory.CSharpTypeMap.TryGetValue(type, out var baseProvider) &&
158158
baseProvider is ModelProvider;
159159

160+
/// <summary>
161+
/// Determines whether a derived model should skip overriding the generated serialization
162+
/// <c>*Core</c> methods of its base.
163+
/// </summary>
164+
/// <remarks>
165+
/// External/system base models are represented by a <see cref="SystemObjectModelProvider"/>.
166+
/// Such a base only participates in the generated MRW <c>*Core</c> override chain when the
167+
/// wrapped framework/referenced type itself follows the model-reader-writer serialization
168+
/// pattern (i.e. implements <see cref="IJsonModel{T}"/> or <see cref="IPersistableModel{T}"/>,
169+
/// and therefore exposes the overridable <c>*Core</c> methods). When it does, derived models
170+
/// must override those methods rather than hide them (otherwise the compiler reports CS0114).
171+
/// When it does not (for example a hand-authored base such as <c>ResourceData</c>), derived
172+
/// models re-introduce the methods as <c>virtual</c>.
173+
/// </remarks>
174+
private static bool ShouldSkipDerivedSerializationMethodOverrides(ModelProvider? baseModelProvider)
175+
{
176+
if (baseModelProvider is null)
177+
{
178+
return false;
179+
}
180+
181+
if (baseModelProvider is SystemObjectModelProvider systemBase)
182+
{
183+
return !SystemTypeImplementsModelReaderWriter(systemBase.SystemType);
184+
}
185+
186+
return baseModelProvider.ShouldSkipDerivedSerializationMethodOverrides;
187+
}
188+
189+
private static bool SystemTypeImplementsModelReaderWriter(CSharpType systemType)
190+
{
191+
if (!systemType.IsFrameworkType)
192+
{
193+
return false;
194+
}
195+
196+
foreach (var @interface in systemType.FrameworkType.GetInterfaces())
197+
{
198+
if (@interface.IsGenericType)
199+
{
200+
var definition = @interface.GetGenericTypeDefinition();
201+
if (definition == typeof(IJsonModel<>) || definition == typeof(IPersistableModel<>))
202+
{
203+
return true;
204+
}
205+
}
206+
}
207+
208+
return false;
209+
}
210+
160211
protected override ConstructorProvider[] BuildConstructors()
161212
{
162213
List<ConstructorProvider> constructors = new();

packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator.ClientModel/test/Providers/MrwSerializationTypeDefinitions/SystemObjectModelSerializationTests.cs

Lines changed: 124 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,9 @@
22
// Licensed under the MIT License.
33

44
using System;
5+
using System.ClientModel.Primitives;
56
using System.Linq;
7+
using System.Text.Json;
68
using Microsoft.TypeSpec.Generator.ClientModel.Providers;
79
using Microsoft.TypeSpec.Generator.Input;
810
using Microsoft.TypeSpec.Generator.Primitives;
@@ -201,5 +203,127 @@ public void JsonModelCreateCore_IsOverride_WhenBaseIsRegularModel()
201203
Assert.IsTrue(method.Signature.Modifiers.HasFlag(MethodSignatureModifiers.Override),
202204
"JsonModelCreateCore should be 'override' with regular base model");
203205
}
206+
207+
/// <summary>
208+
/// Creates a derived model whose SystemObjectModelProvider base wraps a framework type that
209+
/// itself follows the generated MRW pattern (declares the protected virtual *Core methods).
210+
/// </summary>
211+
private static (ModelProvider Model, MrwSerializationTypeDefinition Serialization) CreateDerivedModelWithMrwSystemBase()
212+
{
213+
var baseProp = InputFactory.Property("Name", InputPrimitiveType.String);
214+
var baseInputModel = InputFactory.Model("Resource", properties: [baseProp]);
215+
var derivedProp = InputFactory.Property("Location", InputPrimitiveType.String);
216+
var derivedInputModel = InputFactory.Model("TrackedResource", properties: [derivedProp], baseModel: baseInputModel);
217+
218+
// The base wraps a framework type that declares the generated MRW *Core methods, so a
219+
// derived model must override them (mirrors deriving from an external MRW-generated type).
220+
var systemType = new CSharpType(typeof(FakeMrwBase));
221+
var systemBase = new SystemObjectModelProvider(systemType, baseInputModel);
222+
223+
var generator = MockHelpers.LoadMockGenerator(
224+
inputModels: () => [baseInputModel, derivedInputModel],
225+
createModelCore: (model) =>
226+
{
227+
if (model.Name == "Resource")
228+
return systemBase;
229+
return new ModelProvider(model);
230+
},
231+
createSerializationsCore: (inputType, typeProvider) =>
232+
inputType is InputModelType modelType && typeProvider is ModelProvider mp
233+
? [new MrwSerializationTypeDefinition(modelType, mp)]
234+
: []);
235+
generator.Object.TypeFactory.RootInputModels.Add(derivedInputModel);
236+
generator.Object.TypeFactory.RootOutputModels.Add(derivedInputModel);
237+
238+
var derived = ScmCodeModelGenerator.Instance.TypeFactory.CreateModel(derivedInputModel) as ModelProvider;
239+
Assert.IsNotNull(derived);
240+
Assert.IsInstanceOf<SystemObjectModelProvider>(derived!.BaseModelProvider);
241+
242+
var serializations = derived.SerializationProviders;
243+
Assert.AreEqual(1, serializations.Count);
244+
return (derived, (MrwSerializationTypeDefinition)serializations[0]);
245+
}
246+
247+
// -------------------------------------------------------------------
248+
// When the external/system base itself follows the generated MRW pattern (declares the
249+
// protected virtual *Core methods), the derived model must OVERRIDE them rather than
250+
// re-introduce them as virtual. Regression test for issue #11042 where deriving from an
251+
// external MRW-generated base (e.g. OpenAI's ResponseItem) produced CS0114.
252+
// -------------------------------------------------------------------
253+
254+
[Test]
255+
public void PersistableModelCreateCore_IsOverride_WhenSystemBaseDeclaresCoreMethods()
256+
{
257+
var (_, serialization) = CreateDerivedModelWithMrwSystemBase();
258+
var method = serialization.BuildPersistableModelCreateCoreMethod();
259+
260+
Assert.IsNotNull(method);
261+
Assert.IsTrue(method.Signature.Modifiers.HasFlag(MethodSignatureModifiers.Override),
262+
"PersistableModelCreateCore should be 'override' when the system base declares the *Core methods");
263+
Assert.IsFalse(method.Signature.Modifiers.HasFlag(MethodSignatureModifiers.Virtual),
264+
"PersistableModelCreateCore should NOT be 'virtual' when the system base declares the *Core methods");
265+
}
266+
267+
[Test]
268+
public void JsonModelCreateCore_IsOverride_WhenSystemBaseDeclaresCoreMethods()
269+
{
270+
var (_, serialization) = CreateDerivedModelWithMrwSystemBase();
271+
var method = serialization.BuildJsonModelCreateCoreMethod();
272+
273+
Assert.IsNotNull(method);
274+
Assert.IsTrue(method.Signature.Modifiers.HasFlag(MethodSignatureModifiers.Override),
275+
"JsonModelCreateCore should be 'override' when the system base declares the *Core methods");
276+
Assert.IsFalse(method.Signature.Modifiers.HasFlag(MethodSignatureModifiers.Virtual),
277+
"JsonModelCreateCore should NOT be 'virtual' when the system base declares the *Core methods");
278+
}
279+
280+
[Test]
281+
public void PersistableModelWriteCore_IsOverride_WhenSystemBaseDeclaresCoreMethods()
282+
{
283+
var (_, serialization) = CreateDerivedModelWithMrwSystemBase();
284+
var method = serialization.BuildPersistableModelWriteCoreMethod();
285+
286+
Assert.IsNotNull(method);
287+
Assert.IsTrue(method.Signature.Modifiers.HasFlag(MethodSignatureModifiers.Override),
288+
"PersistableModelWriteCore should be 'override' when the system base declares the *Core methods");
289+
Assert.IsFalse(method.Signature.Modifiers.HasFlag(MethodSignatureModifiers.Virtual),
290+
"PersistableModelWriteCore should NOT be 'virtual' when the system base declares the *Core methods");
291+
}
292+
293+
[Test]
294+
public void JsonModelWriteCore_IsOverride_WhenSystemBaseDeclaresCoreMethods()
295+
{
296+
var (_, serialization) = CreateDerivedModelWithMrwSystemBase();
297+
var method = serialization.BuildJsonModelWriteCoreMethod();
298+
299+
Assert.IsNotNull(method);
300+
Assert.IsTrue(method.Signature.Modifiers.HasFlag(MethodSignatureModifiers.Override),
301+
"JsonModelWriteCore should be 'override' when the system base declares the *Core methods");
302+
}
303+
304+
/// <summary>
305+
/// A stand-in for an external framework type generated by the MRW emitter. It implements
306+
/// <see cref="IJsonModel{T}"/> (and therefore <see cref="IPersistableModel{T}"/>) just as a
307+
/// generated model does, so the base is recognized as participating in the serialization
308+
/// override chain.
309+
/// </summary>
310+
private class FakeMrwBase : IJsonModel<FakeMrwBase>
311+
{
312+
void IJsonModel<FakeMrwBase>.Write(Utf8JsonWriter writer, ModelReaderWriterOptions options)
313+
{
314+
}
315+
316+
FakeMrwBase IJsonModel<FakeMrwBase>.Create(ref Utf8JsonReader reader, ModelReaderWriterOptions options)
317+
=> this;
318+
319+
BinaryData IPersistableModel<FakeMrwBase>.Write(ModelReaderWriterOptions options)
320+
=> BinaryData.FromString(string.Empty);
321+
322+
FakeMrwBase IPersistableModel<FakeMrwBase>.Create(BinaryData data, ModelReaderWriterOptions options)
323+
=> this;
324+
325+
string IPersistableModel<FakeMrwBase>.GetFormatFromOptions(ModelReaderWriterOptions options)
326+
=> "J";
327+
}
204328
}
205329
}

0 commit comments

Comments
 (0)