Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OData reader fails to read payloads having lowercase EDM types #2884

Open
budt12 opened this issue Mar 4, 2024 · 2 comments
Open

OData reader fails to read payloads having lowercase EDM types #2884

budt12 opened this issue Mar 4, 2024 · 2 comments
Assignees

Comments

@budt12
Copy link

budt12 commented Mar 4, 2024

Short summary (3-5 sentences) describing the issue.
Running into issues when odata would not parse payloads having lowercase edm types and throws up ODataException

Assemblies affected

Which assemblies and versions are known to be affected e.g. OData .Net lib 7.x
Microsoft.OData.Edm, all versions.

Reproduce steps

Any payload with lowercase types like 'string', 'int32', 'binary' etc would break payload reader, ODataMessageReader while reading the payload.

Here's the exception stack for following example payload -

Callsite Method Name: [ReadEntryAsync], Callsite line number: [73]; Exception=Microsoft.OData.ODataException: Incompatible type kinds were found. The type 'string' was found to be of kind 'Complex' instead of the expected kind 'Primitive'.
  at Microsoft.OData.ReaderValidationUtils.ResolvePayloadTypeNameAndComputeTargetType(EdmTypeKind expectedTypeKind, Nullable`1 expectStructuredType, IEdmType defaultPrimitivePayloadType, IEdmTypeReference expectedTypeReference, String payloadTypeName, IEdmModel model, Func`3 clientCustomTypeResolver, Boolean throwIfTypeConflictsWithMetadata, Boolean enablePrimitiveTypeConversion, Func`1 typeKindFromPayloadFunc, EdmTypeKind& targetTypeKind, ODataTypeAnnotation& typeAnnotation)
  at Microsoft.OData.JsonLight.ODataJsonLightPropertyAndValueDeserializer.ReadNonEntityValueImplementationAsync(String payloadTypeName, IEdmTypeReference expectedTypeReference, PropertyAndAnnotationCollector propertyAndAnnotationCollector, CollectionWithoutExpectedTypeValidator collectionValidator, Boolean validateNullValue, Boolean isTopLevelPropertyValue, Boolean insideResourceValue, String propertyName, Nullable`1 isDynamicProperty)
  at Microsoft.OData.JsonLight.ODataJsonLightPropertyAndValueDeserializer.ReadNonEntityValueAsync(String payloadTypeName, IEdmTypeReference expectedValueTypeReference, PropertyAndAnnotationCollector propertyAndAnnotationCollector, CollectionWithoutExpectedTypeValidator collectionValidator, Boolean validateNullValue, Boolean isTopLevelPropertyValue, Boolean insideResourceValue, String propertyName, Nullable`1 isDynamicProperty)
[trunc…]


*The simplest set of steps to reproduce the issue. If possible, reference a commit that demonstrates the issue.*
N/A

### Expected result

*What would happen if there wasn't a bug.*
Lowercase types like 'string' should match with 'String' or 'int32' should match with 'Int32' types.

### Actual result

*What is actually happening.*
ODataException is thrown while reading the payload.

### Additional detail

*Optional, details of the root cause if known. Delete this section if you have no additional details to add.*
Here are some additional details - 

We are running into an issue with a dictionary in EdmCoreModel not being able to identify lower case types like “string”, “int32” or “binary” which causes an exception like below - 
at Microsoft.OData.ValidationUtils.ValidateTypeKind(EdmTypeKind actualTypeKind, EdmTypeKind expectedTypeKind, Nullable`1 expectStructuredType, String typeName)
 Callsite file path: [sources\dev\Sds\src\Protocol\OData\Reader\ODataRequestReader.cs] Callsite Method Name: [ReadEntryAsync], Callsite line number: [73]; Exception=Microsoft.OData.ODataException: Incompatible type kinds were found. The type 'string' was found to be of kind 'Complex' instead of the expected kind 'Primitive'.
  at Microsoft.OData.ReaderValidationUtils.ResolvePayloadTypeNameAndComputeTargetType(EdmTypeKind expectedTypeKind, Nullable`1 expectStructuredType, IEdmType defaultPrimitivePayloadType, IEdmTypeReference expectedTypeReference, String payloadTypeName, IEdmModel model, Func`3 clientCustomTypeResolver, Boolean throwIfTypeConflictsWithMetadata, Boolean enablePrimitiveTypeConversion, Func`1 typeKindFromPayloadFunc, EdmTypeKind& targetTypeKind, ODataTypeAnnotation& typeAnnotation)
  at Microsoft.OData.JsonLight.ODataJsonLightPropertyAndValueDeserializer.ReadNonEntityValueImplementationAsync(String payloadTypeName, IEdmTypeReference expectedTypeReference, PropertyAndAnnotationCollector propertyAndAnnotationCollector, CollectionWithoutExpectedTypeValidator collectionValidator, Boolean validateNullValue, Boolean isTopLevelPropertyValue, Boolean insideResourceValue, String propertyName, Nullable`1 isDynamicProperty)
  at Microsoft.OData.JsonLight.ODataJsonLightPropertyAndValueDeserializer.ReadNonEntityValueAsync(String payloadTypeName, IEdmTypeReference expectedValueTypeReference, PropertyAndAnnotationCollector propertyAndAnnotationCollector, CollectionWithoutExpectedTypeValidator collectionValidator, Boolean validateNullValue, Boolean isTopLevelPropertyValue, Boolean insideResourceValue, String propertyName, Nullable`1 isDynamicProperty)
[trunc…]

To give you a little background on this issue -
Some of our partners are sending payloads like –

Payload Success/Failure
{val@odata.type:"string","val":”abc”} Failure
{val@odata.type:"String","val":”abc”} Success

Failure would cause above exception. Odata Type string will go unmapped while going through the odata stack. In EdmCoreModel.cs, you have a dictionary declaration – Which does not have StringComparer.OrdinalIgnoreCase comparer.
EdmCoreModel-coreSchemaTypes

Below code fails to map passed in type “string” to the keys in the dictionary, which then results in the exception further down the stack.
EdmCoreModel.cs-FindDeclaredType()

       /// <summary>
       /// Searches for a type with the given name in this model only and returns null if no such type exists.
       /// </summary>
       /// <param name="qualifiedName">The qualified name of the type being found.</param>
       /// <returns>The requested type, or null if no such type exists.</returns>
       public IEdmSchemaType FindDeclaredType(string qualifiedName)
       {
           IEdmSchemaType element;
           if (coreSchemaTypes.TryGetValue(qualifiedName, out element))
           {
               return element;
           } 
           return null;
       }

I tried this on my local environment and it works fine for both the payloads above. This change would help us continue to use ReadUntypedAsString = false.

Would it be possible to change the dictionary declaration (here - EdmCoreModel-coreSchemaTypes) from

       private readonly IDictionary<string, IEdmSchemaType> coreSchemaTypes = new Dictionary<string, IEdmSchemaType>();

To

       private readonly IDictionary<string, IEdmSchemaType> coreSchemaTypes = new Dictionary<string, IEdmSchemaType>(StringComparer.OrdinalIgnoreCase);  
@corranrogue9
Copy link
Contributor

Let's see if we can parameterize the comparer and run benchmarks at a later time. We will need to benchmark if we can't parameterize the comparer for some reason.

@habbes
Copy link
Contributor

habbes commented Mar 14, 2024

@budt12 Since some concerns have been raised about the currently open PR, and discussions are ongoing, I want to suggest a workaround in the meantime. The workaround consists of providing a custom type resolver delegate to the ODataMessageWriterSettings.ClientCustomTypeResolver property. The custom resolver can then implement custom logic to handle types that are not found in your model using the default (case-sensitive) IEdmModel.FindType extension method.

Here's a sample implementation of such a custom resolver. It builds a static case-insensitive copy of the core schema types in the EdmCoreModel. When a type is not found, it looks it up in this cache. Building the cache is a one-time cost. I opted to copy only the core model types in order to keep the cache small and because your use-case seems to be concerned only with EDM primitve types.

private static Dictionary<string, IEdmType> caseInsensitiveCoreTypesCache = BuildCaseInsensitiveCoreTypesCache();

public static IEdmType ResolveType(IEdmType expectedType, string typeName)
{
      IEdmType resolvedType = MyModel.FindType(typeName);
      if (resolvedType == null)
      {
            var qualifiedTypeName = typeName.Contains(".") ? typeName : $"Edm.{typeName}";
            caseInsensitiveCoreTypeCache.TryGetValue(qualifiedTypeName, out resolvedType);
      }

      return resolvedType;
}

private static Dictionary<string, IEdmType> BuildCaseInsensitiveCoreTypesCache()
{
    var cache = new Dictionary<string, IEdmType>(StringComparer.OrdinalIgnoreCase);
    foreach (var type in EdmCoreModel.Instance.SchemaElements)
    {
        cache.Add(type.FullName(), type as IEdmType);
    }

    return cache;
}

And here's how you would pass it to the reader settings:

messageReaderSettings.ClientCustomTypeResolver = ResolveType

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants