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

Referenced external permissions annotations don't reflect in main EDM model #1936

Closed
irvinesunday opened this issue Dec 1, 2020 · 3 comments · Fixed by #1987
Closed

Referenced external permissions annotations don't reflect in main EDM model #1936

irvinesunday opened this issue Dec 1, 2020 · 3 comments · Fixed by #1987
Assignees
Labels

Comments

@irvinesunday
Copy link

Using this notation in the main XML document to reference an external permissions annotation XML document does not seem to surface the referenced permissions annotations in the main EdmModel object.

<edmx:Reference  Uri="https://raw.githubusercontent.com/irvinesunday/msgraph-metadata/master/scopes_metadata/apiPermissionsAndScopes-v1.0.xml">
    <edmx:IncludeAnnotations  TermNamespace="Org.OData.Authorization.V1"/>
    <edmx:IncludeAnnotations  TermNamespace="Org.OData.Capabilities.V1"/>
  </edmx:Reference >

Main XML file: https://raw.githubusercontent.com/irvinesunday/msgraph-metadata/master/clean_v10_metadata/cleanMetadataWithDescriptionsv1.0.xml
Referenced XML file (with permissions annotations): https://raw.githubusercontent.com/irvinesunday/msgraph-metadata/master/scopes_metadata/apiPermissionsAndScopes-v1.0.xml

Assemblies affected

Microsoft.OData.Edm, Version=7.6.1.0

Reproduce steps

Below is the .NET code for parsing an EdmModel from a CSDL (including any referenced XML in the referencing XML document)

// Function to load referenced model xml
XmlReader getReferencedModelReaderFunc(Uri uri)
{
  if (uri != null)
  {
    var httpClient = new HttpClient();
    var csdl = httpClient.GetStringAsync(uri.OriginalString);
    var xmlReader =  XElement.Parse(csdl).CreateReader();
    return xmlReader;
  }
  return null;
}

var edmModel = CsdlReader.Parse(XElement.Load(csdl).CreateReader(), getReferencedModelReaderFunc);

Expected result

The EdmModel.VocabularyAnnotations property should contain the collection of vocabulary annotations that are contained in both the referencing and referenced models.

Actual result

The EdmModel.VocabularyAnnotations property does not contain the collection of vocabulary annotations that are contained in the referenced model.

@habbes habbes self-assigned this Dec 8, 2020
@habbes
Copy link
Contributor

habbes commented Dec 18, 2020

@irvinesunday
Looking at the two CSDL documents, the permissions' document has annotations that refer to targets that are defined in the main document, e.g. (Target="microsoft.graph.GraphService"). The parser would not be able to resolve such types because they are not defined in the permissions document. Since the permissions document has a dependency on the main document, I think it's the permissions document that should have an edm reference to the main document, and not the other way around, i.e.:

<edmx:Reference Uri=".../cleanMetadataWithDescriptionsv1.0.xml">
  <edmx:Include Namespace="microsoft.graph" />
</edmx:Reference>

In this case your edmModel object will load the permissions xml and reference the metadata xml. And edmModel.VocabularyAnnotations will contain the annotations from the permissions model.

Now on to your main request. While working on making the annotations in the referenced model available in the main model's VocabularyAnnotations, I realized that this would be inconsistent with the behaviour of other objects. Currently edmModel.SchemaElements does not include the schema elements of the referenced models, and edmModel.EntityContainer does not include the entity container from the referenced models. Similarly, methods like edmModel.FindDeclaredEntitySet() only check the current model and not its referenced models.

If you want to access elements from a referenced model you'd need to get the referenced model directly from edmModel.ReferencedModels. So I don't know if it's a good idea to merge elements from referenced models just in the case of VocabularyAnnotations and not other types of elements, and whether this is the intended behaviour. Or whether we should consider introducing a new method to merges elements from referenced models.

In the case of vocabulary annotations however there still exists a feature gap/bug. The referenced model does not include its own out-of-line annotations. E.g. edmModel.ReferencedModels.First().VocabularyAnnotations would not include the out-of-line annotations defined in the referenced model, even if the edmx:Reference has edmx:IncludeAnnotations elements.

What do you think @mikepizzo @g2mula ?

@mikepizzo
Copy link
Member

Actually, the microsoft.graph model needs to reference the external document that contains the annotations, specifying the namespaces from which to IncludeAnnotations, as per @irvinesunday's example. And, for those references to be valid, the referenced schema needs to include a reference to the microsoft.graph namespace as per @habbes's example.

To @habbes comment on searching across models, the pattern is that the FindDeclaredXYZ() methods search only the current model, while he FindXYZ() methods search across models (and use the FindDeclaredXYZ() for each model). So, myModel.FindDeclaredVocabularyAnnotations(...) should only find annotations on myModel, while myModel.FindVocabularyAnnotations(...) should find annotations in myModel and within the namespaces of referenced models as specified by IncludeAnnotations().

@habbes
Copy link
Contributor

habbes commented Jan 26, 2021

@mikepizzo thanks for the clarifications. I have a made a PR that makes the annotations in referenced models accessible via myModel.FindVocabularyAnnotations(...).

However, it seems the issue with the target type of the annotation not being properly resolved still persists even when you have a reference to the main document from inside the referenced permission document. And out of curiosity, is this kind of cyclic references supported by OData?

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

Successfully merging a pull request may close this issue.

3 participants