Support for direct value annotations when reading / writing CSDL model as JSON

akorygin commented 2 weeks ago

Direct value annotations are missing after parsing a CSDL model from JSON.

Assemblies affected

Which assemblies and versions are known to be affected e.g. OData .Net lib 8.1.0.0

Reproduce steps

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

The following code demonstrates the issue. Even though direct annotations are written into the output JSON document, they are ignored when reading it back and reported as "Unexpected element".

Note that a similar test that uses XML reader / writer correctly imports direct value annotations and sets them on the corresponding schema elements.

 var modelIn = new EdmModel();
 var type = new EdmComplexType("MyNamespace", "MyType");
 var property = type.AddStructuralProperty("Name", EdmPrimitiveTypeKind.String);

 modelIn.DirectValueAnnotationsManager.SetAnnotationValue(
     property,
     "MyNamespace",
     "ColumnSize",
     new EdmIntegerConstant(EdmCoreModel.Instance.GetInt16(false), 8));

 modelIn.AddElement(type);

 var memoryStream = new MemoryStream();
 using var writer = new Utf8JsonWriter(memoryStream);
 Assert.IsTrue(CsdlWriter.TryWriteCsdl(modelIn, writer, out IEnumerable<EdmError> errors));
 Assert.IsNotNull(errors);
 Assert.IsFalse(errors.Any());

 var json = Encoding.UTF8.GetString(memoryStream.ToArray());
 StringAssert.Contains(json, "ColumnSize");

 var reader = new Utf8JsonReader(memoryStream.ToArray());
 Assert.IsTrue(CsdlReader.TryParse(ref reader, new CsdlJsonReaderSettings(), out IEdmModel modelOut, out errors));

 Assert.IsNotNull(modelOut);
 Assert.AreEqual(1, modelOut.SchemaElements.Count());
 // This fails because of "Unexpected element 'ColumnSize' error"
 Assert.IsFalse(errors.Any());

Expected result

When reading CSDL model from JSON we would expect direct value annotations to be imported the same way they are when de-serializing from XML.

What would happen if there wasn't a bug.

Actual result

What is actually happening.

Additional detail

Optional, details of the root cause if known. Delete this section if you have no additional details to add.

xuzhg commented 2 weeks ago

Any reason why you don't use the vocabulary annotation? That's the OData v4 recommendation.

akorygin commented 1 week ago

Any reason why you don't use the vocabulary annotation? That's the OData v4 recommendation.

This is a legacy service one of our teams developed. The metadata generation is pretty heavy so they added metadata caching and noticed that client's functionality breaks after de-serializing the CSDL model from JSON. I guess they chose JSON because it's more compact than XML.

xuzhg commented 5 days ago

@akorygin Direct value annotation is the old technique which was actively used in old OData library. From OData spec v4.0, a new technique called vocabulary annotation is introduced to replace the direct value annotation. In the OData v4 library, direct value annotation is kept for back compability (@mikepizzo, I do think we should remove direct value annotation in next major ODL release), and the support for direct value annotation in current version is very limited. The end user should use the vocabulary annotation, it's a standard.

OData team try best to make it easy to use vocabulary annotation to replace direct value annotation:

Below is the corresponding codes based on your prototype, but using vocabulary annotation:

var modelIn = new EdmModel();
var type = new EdmComplexType("MyNamespace", "MyType");
var property = type.AddStructuralProperty("Name", EdmPrimitiveTypeKind.String);

EdmTerm term = new EdmTerm("MyNamespace", "ColumnSize", EdmPrimitiveTypeKind.Int16);
modelIn.AddElement(term);

EdmVocabularyAnnotation annotation = new EdmVocabularyAnnotation(property, term, new EdmIntegerConstant(8));
annotation.SetSerializationLocation(modelIn, EdmVocabularyAnnotationSerializationLocation.Inline);
modelIn.SetVocabularyAnnotation(annotation);

modelIn.AddElement(type);

Here's the CSDL-XML ouput:

<?xml version="1.0" encoding="UTF-16"?>
  <edmx:Edmx xmlns:edmx="http://docs.oasis-open.org/odata/ns/edmx" Version="4.0">
    <edmx:DataServices>
      <Schema xmlns="http://docs.oasis-open.org/odata/ns/edm" Namespace="MyNamespace">
        <Term Type="Edm.Int16" Name="ColumnSize"/>
       <ComplexType Name="MyType">
         <Property Type="Edm.String" Name="Name">
           <Annotation Int="8" Term="MyNamespace.ColumnSize"/>
         </Property>
      </ComplexType>
    </Schema>
  </edmx:DataServices>
</edmx:Edmx>

Here's the CSDL-JSON output:

{
  "$Version": "4.0",
  "MyNamespace": {
    "ColumnSize": {
      "$Kind": "Term",
      "$Type": "Edm.Int16",
      "$Nullable": true
    },
    "MyType": {
      "$Kind": "ComplexType",
      "Name": {
        "$Nullable": true,
        "@MyNamespace.ColumnSize": 8
      }
    }
  }
}

So, combine the test cases as follows (I use XUnit.Assert)

// omit ... the above model construct codes, 
var memoryStream = new MemoryStream();
using var writer = new Utf8JsonWriter(memoryStream);
Assert.True(CsdlWriter.TryWriteCsdl(modelIn, writer, out IEnumerable<EdmError> errors));
Assert.NotNull(errors);
Assert.False(errors.Any());

var json = Encoding.UTF8.GetString(memoryStream.ToArray());
Assert.Contains("ColumnSize", json);

var reader = new Utf8JsonReader(memoryStream.ToArray());
Assert.True(CsdlReader.TryParse(ref reader, new CsdlJsonReaderSettings(), out IEdmModel modelOut, out errors));

Assert.NotNull(modelOut);
IEdmTerm termOut = modelOut.SchemaElements.OfType<IEdmTerm>().FirstOrDefault();
Assert.NotNull(termOut);
Assert.Equal(term.FullName(), termOut.FullName());

IEdmComplexType complexType = Assert.Single(modelOut.SchemaElements.OfType<IEdmComplexType>());
IEdmProperty nameProperty = Assert.Single(complexType.Properties());

//  Should use the TermOut.
IEdmVocabularyAnnotation annotationOut = modelOut.FindVocabularyAnnotations<IEdmVocabularyAnnotation>(nameProperty, termOut).FirstOrDefault();
IEdmIntegerValue intergerValue = Assert.IsAssignableFrom<IEdmIntegerValue>(annotationOut.Value);
Assert.Equal(8, intergerValue.Value);
Assert.False(errors.Any());

Here's the test running result:

akorygin commented 5 days ago

Thanks, Sam. We are aware of vocabulary annotations and have been using them a lot. It's just in this specific case there is a legacy API that we would have to change and that change would also affect clients. So, this is not an easy change to make for us but we'll create a backlog to migrate.

Also, completely removing direct value annotations from ODL may be quite a disruptive change. There are situations when we want to annotate schema elements with additional metadata w/o defining vocabulary terms and / or exposing it in the model. I believe Microsoft's own convention OData model builder that comes with ASP.NET Core OData heavily uses this functionality to specify things like related CLR types, dynamic property dictionary refs, etc., is it not?

@akorygin Direct value annotation is the old technique which was actively used in old OData library. From OData spec v4.0, a new technique called vocabulary annotation is introduced to replace the direct value annotation. In the OData v4 library, direct value annotation is kept for back compability (@mikepizzo, I do think we should remove direct value annotation in next major ODL release), and the support for direct value annotation in current version is very limited. The end user should use the vocabulary annotation, it's a standard.

OData / odata.net