Handle custom external model bases #11183
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -74,10 +74,10 @@ public partial class MrwSerializationTypeDefinition : TypeProvider | |
| private ConstructorProvider? _serializationConstructor; | ||
|
Contributor
There was a problem hiding this comment. I'm wondering if we can avoid all of these changes by just changing the accessibility of public TypeProvider? BaseTypeProvider => BaseTypeProviderTypeProvider;
private protected virtual TypeProvider? BaseTypeProviderTypeProvider => null;Then in this file we can add a helper: private bool BaseDeclaresOverridable(string methodName)
{
for (var provider = _model.BaseTypeProvider; provider is not null; provider = provider.BaseTypeProvider)
{
// Prefer the custom view so hand-authored replacements/hooks win over generated members.
var source = provider.CustomCodeView ?? provider;
var method = source.Methods.FirstOrDefault(m => m.Signature.Name == methodName);
if (method is null)
{
continue; // not declared at this level; keep walking up
}
var mods = method.Signature.Modifiers;
return !mods.HasFlag(MethodSignatureModifiers.Sealed)
&& (mods.HasFlag(MethodSignatureModifiers.Virtual)
|| mods.HasFlag(MethodSignatureModifiers.Override)
|| mods.HasFlag(MethodSignatureModifiers.Abstract));
}
return false;
}Will this help simplify the proposed changes or am I missing something ?
Contributor
Author
There was a problem hiding this comment. Good suggestion. I exposed
Contributor
Author
There was a problem hiding this comment. Follow-up: after checking this again, the public API change did not remove the CSharpType fallback/return-type lookup, so I reverted that part in 8169c69 and kept the targeted correctness fixes.
Contributor
Author
There was a problem hiding this comment. Adding a bit more context: exposing
Contributor
There was a problem hiding this comment. For unresolved external CSharpType chains, how would we determine if that type has methods we should override? |
||
| // Flag to determine if the model should override the serialization methods | ||
| private bool? _shouldOverrideMethods; | ||
| private bool ShouldOverrideMethods => _shouldOverrideMethods ??= !_isStruct && | ||
| (_model.BaseModelProvider != null || HasCustomBaseMethod(JsonModelWriteCoreMethodName)); | ||
| private bool? _shouldSkipSerializationMethodOverrides; | ||
| private bool ShouldSkipSerializationMethodOverrides => _shouldSkipSerializationMethodOverrides ??= ShouldSkipDerivedSerializationMethodOverrides(_model); | ||
| private readonly Lazy<PropertyProvider[]> _additionalProperties; | ||
|
|
||
| // Unknown discriminator models use their base model as the serialization interface type. | ||
|
|
@@ -100,7 +100,6 @@ public MrwSerializationTypeDefinition(InputModelType inputModel, ModelProvider m | |
| _isStruct = _model.DeclarationModifiers.HasFlag(TypeSignatureModifiers.Struct); | ||
| _supportsXml = inputModel.Usage.HasFlag(InputModelTypeUsage.Xml); | ||
| _supportsJson = inputModel.Usage.HasFlag(InputModelTypeUsage.Json) || !_supportsXml; | ||
| _rawDataField = _model.Fields.FirstOrDefault(f => f.Name == AdditionalPropertiesHelper.AdditionalBinaryDataPropsFieldName); | ||
| _additionalBinaryDataProperty = new(GetAdditionalBinaryDataPropertiesProp); | ||
| _additionalProperties = new(() => [.. _model.Properties.Where(p => p.IsAdditionalProperties)]); | ||
|
|
@@ -152,7 +151,15 @@ private CSharpType GetRootModelType() | |
| { | ||
| // We need to explicitly use the BaseModelProvider when looking up the root type | ||
| // to account for any customizations that may have changed the base model. | ||
| var returnType = _model.BaseModelProvider?.Type ?? | ||
| GetCustomBaseRootType() ?? | ||
| GetCustomBaseMethodReturnType(new HashSet<string> | ||
| { | ||
| JsonModelCreateCoreMethodName, | ||
| PersistableModelCreateCoreMethodName | ||
| }) ?? | ||
| GetCustomSerializationBaseType() ?? | ||
| Type; | ||
| while (returnType.BaseType != null | ||
| && IsModelType(returnType.BaseType)) | ||
| { | ||
|
|
@@ -180,43 +187,155 @@ private static bool IsModelType(CSharpType type) | |
| /// When it does not (for example a hand-authored base such as <c>ResourceData</c>), derived | ||
| /// models re-introduce the methods as <c>virtual</c>. | ||
| /// </remarks> | ||
| private CSharpType? GetCustomSerializationBaseType() | ||
| => _model.BaseModelProvider is null && _model.CustomCodeView?.BaseType is not null | ||
| ? _model.BaseType | ||
| : null; | ||
|
|
||
| private CSharpType? GetCustomBaseRootType() | ||
| => GetCustomSerializationBaseType() is { } baseType ? GetRootBaseType(baseType) : null; | ||
|
|
||
| private bool HasCustomBaseMethod(string methodName) | ||
| => GetCustomSerializationBaseType() is { } baseType && | ||
| HasMethodInTypeHierarchy(baseType, methodName); | ||
|
|
||
| private CSharpType? GetCustomBaseMethodReturnType(IReadOnlySet<string> methodNames) | ||
| => GetCustomSerializationBaseType() is { } baseType | ||
| ? GetMethodReturnTypeInHierarchy(baseType, methodNames) | ||
| : null; | ||
|
|
||
| private static bool ShouldSkipDerivedSerializationMethodOverrides(ModelProvider model) | ||
| { | ||
| if (model.BaseModelProvider is null) | ||
| { | ||
| return model.CustomCodeView?.BaseType is null || | ||
| !HasMethodInTypeHierarchy(model.BaseType!, JsonModelCreateCoreMethodName) && | ||
| !HasMethodInTypeHierarchy(model.BaseType!, PersistableModelCreateCoreMethodName); | ||
| } | ||
|
|
||
| if (model.BaseModelProvider is SystemObjectModelProvider systemBase) | ||
| { | ||
| return !SystemTypeImplementsModelReaderWriter(systemBase.SystemType); | ||
| } | ||
|
|
||
| return model.BaseModelProvider.ShouldSkipDerivedSerializationMethodOverrides; | ||
| } | ||
|
|
||
| private static CSharpType? GetRootBaseType(CSharpType baseType) | ||
| { | ||
| var visited = new HashSet<string>(); | ||
| CSharpType? rootBaseType = null; | ||
| var current = baseType; | ||
|
|
||
| while (current is not null && visited.Add(current.FullyQualifiedName)) | ||
| { | ||
| if (!IsFrameworkRootType(current)) | ||
| { | ||
| rootBaseType = current; | ||
| } | ||
|
|
||
| current = current.BaseType; | ||
| } | ||
|
|
||
| return rootBaseType; | ||
| } | ||
|
|
||
| private static bool HasMethodInTypeHierarchy(CSharpType type, string methodName) | ||
| => GetMethodReturnTypeInHierarchy(type, new HashSet<string> { methodName }) is not null; | ||
|
|
||
| private static CSharpType? GetMethodReturnTypeInHierarchy(CSharpType type, IReadOnlySet<string> methodNames) | ||
| { | ||
| var visited = new HashSet<string>(); | ||
| return GetMethodReturnTypeInHierarchy(type, methodNames, visited); | ||
| } | ||
|
|
||
| private static CSharpType? GetMethodReturnTypeInHierarchy(CSharpType type, IReadOnlySet<string> methodNames, HashSet<string> visited) | ||
| { | ||
| if (!visited.Add(type.FullyQualifiedName)) | ||
| { | ||
| return null; | ||
| } | ||
|
|
||
| if (type.BaseType is not null && | ||
| GetMethodReturnTypeInHierarchy(type.BaseType, methodNames, visited) is { } baseReturnType) | ||
| { | ||
| return baseReturnType; | ||
| } | ||
|
|
||
| foreach (var methodName in methodNames) | ||
| { | ||
| if (TryGetMethodReturnType(type, methodName) is { } returnType) | ||
| { | ||
| return returnType; | ||
| } | ||
| } | ||
|
|
||
| return null; | ||
| } | ||
|
|
||
| private static CSharpType? TryGetMethodReturnType(CSharpType type, string methodName) | ||
| { | ||
| if (TryGetTypeProvider(type) is { } referencedType) | ||
| { | ||
| return referencedType.Methods.FirstOrDefault(method => method.Signature.Name == methodName)?.Signature.ReturnType; | ||
| } | ||
|
|
||
| return null; | ||
| } | ||
|
|
||
| private static TypeProvider? TryGetTypeProvider(CSharpType type) | ||
| { | ||
| if (TryGetMappedTypeProvider(type) is { } provider) | ||
| { | ||
| return provider; | ||
| } | ||
|
|
||
| return TryGetReferencedType(type); | ||
| } | ||
|
|
||
| private static TypeProvider? TryGetMappedTypeProvider(CSharpType type) | ||
| { | ||
| foreach (var (mappedType, provider) in ScmCodeModelGenerator.Instance.TypeFactory.CSharpTypeMap) | ||
| { | ||
| if (mappedType.FullyQualifiedName == type.FullyQualifiedName || | ||
| provider is SystemObjectModelProvider systemProvider && | ||
| systemProvider.SystemType.FullyQualifiedName == type.FullyQualifiedName) | ||
| { | ||
| return provider; | ||
| } | ||
| } | ||
|
|
||
| return null; | ||
| } | ||
|
|
||
| private static TypeProvider? TryGetReferencedType(CSharpType type) | ||
| => string.IsNullOrEmpty(type.Namespace) | ||
| ? null | ||
| : CodeModelGenerator.Instance.SourceInputModel.FindForTypeInCustomization( | ||
| type.Namespace, | ||
| type.Name, | ||
| declaringTypeName: type.DeclaringType?.Name, | ||
| includeReferencedAssemblies: true); | ||
|
|
||
| private static bool IsFrameworkRootType(CSharpType type) | ||
| => (type.IsFrameworkType && | ||
| (type.FrameworkType == typeof(object) || type.FrameworkType == typeof(ValueType))) || | ||
| (type.Namespace == "System" && (type.Name == "Object" || type.Name == "ValueType")); | ||
|
|
||
| private static bool SystemTypeImplementsModelReaderWriter(CSharpType systemType) | ||
| { | ||
| if (TryGetMappedTypeProvider(systemType) is not { } systemTypeProvider) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| return systemTypeProvider.Implements.Any(IsModelReaderWriterInterface); | ||
|
Contributor
There was a problem hiding this comment. Does |
||
| } | ||
|
|
||
| private static bool IsModelReaderWriterInterface(CSharpType type) | ||
|
Contributor
There was a problem hiding this comment. we already have a similar method in ModelReaderWriterContextDefinition. Should we consider consolidating this into a shared internal helper? |
||
| => type.Namespace == typeof(IJsonModel<>).Namespace && | ||
| (type.Name == nameof(IJsonModel<object>) || type.Name == nameof(IPersistableModel<object>)); | ||
|
|
||
| protected override ConstructorProvider[] BuildConstructors() | ||
| { | ||
| List<ConstructorProvider> constructors = new(); | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| #nullable disable | ||
|
|
||
| using System.ClientModel.Primitives; | ||
| using System.Xml; | ||
|
|
||
| namespace Sample.Models; | ||
|
|
||
| public partial class BaseModel | ||
| { | ||
| internal void XmlModelWriteCore(XmlWriter writer, ModelReaderWriterOptions options) | ||
| { | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| #nullable disable | ||
|
|
||
| using System.ClientModel.Primitives; | ||
| using System.Xml; | ||
|
|
||
| namespace Sample.Models; | ||
|
|
||
| public partial class BaseModel | ||
| { | ||
| protected virtual void XmlModelWriteCore(XmlWriter writer, ModelReaderWriterOptions options) | ||
| { | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| #nullable disable | ||
|
|
||
| using System.ClientModel.Primitives; | ||
| using System.Xml; | ||
|
|
||
| namespace Sample.Models; | ||
|
|
||
| public partial class BaseModel : BaseModelXmlHook | ||
| { | ||
| internal sealed override void XmlModelWriteCore(XmlWriter writer, ModelReaderWriterOptions options) | ||
| { | ||
| } | ||
| } | ||
|
|
||
| public class BaseModelXmlHook | ||
| { | ||
| internal virtual void XmlModelWriteCore(XmlWriter writer, ModelReaderWriterOptions options) | ||
| { | ||
| } | ||
| } |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like there is an opportunity to consolidate some of this logic with what's in the base implementation of mrw. Could we generalize some of these methods, possibly with additional params, so we don't have to maintain the logic in more than one place?