Import vocabulary annotations declared in external referenced models #1987

habbes · 2021-01-25T09:21:38Z

Issues

Fix #1936

Description

Includes annotations referenced in the edmx:IncludeAnnotations tag of the edmx:Reference to be included when loading external referenced models.

This makes the vocabulary annotations imported from referenced models available through:

IEnumerable<IEdmVocabularyAnnotation> annotations = model.FindVocabularyAnnotations(element);

or

IEnumerable<IEdmVocabularyAnnotation> annotations = element.VocabularyAnnotations(model);

Note, these external vocabulary annotations will not appear in model.FindDeclaredVocabularyAnnotations(element) or model.VocabularyAnnotations since they only return the annotations declared directly in the current model.

This PR attempts to restrict the imported annotations based on the TermNamespace, TargetNamespace and Qualifier
as specified in the specification: http://docs.oasis-open.org/odata/odata-csdl-xml/v4.01/odata-csdl-xml-v4.01.html#sec_IncludedAnnotations

Assuming the main model has the following reference:

<edmx:Reference  Uri=""SimplePermissions.xml"">
    <edmx:IncludeAnnotations  TermNamespace=""Org.OData.Capabilities.V1"" Qualifier=""Insert""/>
    <edmx:IncludeAnnotations  TermNamespace=""OtherIncludedNS"" TargetNamespace=""Example.Types""/>
</edmx:Reference>

then this feature will import vocabulary annotations from the SimplePermissions.xml csdl which meet either of the following criteria:

The annotation's term is in the namespace Org.OData.Capabilities.V1 namespace and it has the qualifier Insert
The annotation's term is in the namespace OtherIncludedNS and it's target element is in the namespace Example.Types

Other vocabulary annotations in the referenced csdl will be skipped.

Note: This restriction only applies to "out of line" annotations, i.e. annotations with are declared inside of a top-level <Annotations> element rather than being declared directly inside ("in-line") the target element.

Note: If the reference only has edmx:IncludeAnnotations declarations, but not edmx:Include declaration (http://docs.oasis-open.org/odata/odata-csdl-xml/v4.01/odata-csdl-xml-v4.01.html#sec_IncludedSchema), then this feature will only vocabulary annotations from the referenced models and exclude other schema elements like entity types, operations, etc.

Checklist (Uncheck if it is not completed)

Test cases added
Build and test with one-click build and test script passed

Additional work necessary

If documentation update is needed, please add "Docs Needed" label to the issue and provide details about the required document change in the issue.

…t and qualifier

…nced

gathogojr · 2021-01-27T12:41:29Z

src/Microsoft.OData.Edm/Csdl/Semantics/CsdlSemanticsModel.cs

@@ -114,13 +114,14 @@ private CsdlSemanticsModel(CsdlModel referencedCsdlModel, IEdmDirectValueAnnotat
            foreach (var schema in referencedCsdlModel.Schemata)
            {
                string schemaNamespace = schema.Namespace;
-                IEdmInclude edmInclude = referencedCsdlModel.ParentModelReferences.SelectMany(s => s.Includes).FirstOrDefault(s => s.Namespace == schemaNamespace);
-                if (edmInclude != null)
+                bool shouldAddTypes = referencedCsdlModel.ParentModelReferences.SelectMany(s => s.Includes).Any(s => s.Namespace == schemaNamespace);


Suggested change

bool shouldAddTypes = referencedCsdlModel.ParentModelReferences.SelectMany(s => s.Includes).Any(s => s.Namespace == schemaNamespace);

bool addSchemaElements = referencedCsdlModel.ParentModelReferences.SelectMany(s => s.Includes).Any(s => s.Namespace == schemaNamespace);

test/FunctionalTests/Microsoft.OData.Edm.Tests/Csdl/CsdlReaderTests.cs

src/Microsoft.OData.Edm/Csdl/Semantics/CsdlSemanticsModel.cs

xuzhg

gathogojr · 2021-01-28T08:30:59Z

src/Microsoft.OData.Edm/Csdl/Semantics/CsdlSemanticsModel.cs

        {
-            this.AddSchema(schema, true);
+            this.AddSchema(schema, addSchemaElements, true, null);


Suggested change

this.AddSchema(schema, addSchemaElements, true, null);

this.AddSchema(schema, addSchemaElements, true /*addAnnotations*/, null /*includeAnnotations*/);

Maybe I could use named parameters instead, like:

this.AddSchema(schema, addSchemaElements, addAnnotations: true, includeAnnotations: null);

@habbes Use John's suggestion. Named parameters aren't supported in some c# versions.

src/Microsoft.OData.Edm/Csdl/Semantics/CsdlSemanticsModel.cs

mikepizzo · 2021-01-28T21:06:41Z

src/Microsoft.OData.Edm/Csdl/Semantics/CsdlSemanticsModel.cs

-                IEdmInclude edmInclude = referencedCsdlModel.ParentModelReferences.SelectMany(s => s.Includes).FirstOrDefault(s => s.Namespace == schemaNamespace);
-                if (edmInclude != null)
+                bool shouldAddTypes = referencedCsdlModel.ParentModelReferences.SelectMany(s => s.Includes).Any(s => s.Namespace == schemaNamespace);
+                IEnumerable<IEdmIncludeAnnotations> includeAnnotations = referencedCsdlModel.ParentModelReferences.SelectMany(s => s.IncludeAnnotations);


includeAnnotations [](start = 52, length = 18)

I'm concerned about the performance of executing this (somewhat expensive) SelectMany LINQ expression multiple times -- we execute it once on line 119, again on line 625, and then for each term on line 641. Probably be better, especially since we are iterating multiple times, to make this a list. removing the linq expression all together and simply iterating through building the collection would be even better, but probably not a significant savings here.

mikepizzo · 2021-01-28T21:08:50Z

src/Microsoft.OData.Edm/Csdl/Semantics/CsdlSemanticsModel.cs

+            // we check if the namespace matches, and (if applicable) also the target namespace and the qualifier
+            // see: http://docs.oasis-open.org/odata/odata-csdl-xml/v4.01/odata-csdl-xml-v4.01.html#sec_IncludedAnnotations
+
+            return includeAnnotations.Any(include =>


Any [](start = 38, length = 3)

LINQ is expensive -- AGS has gone through a LINQ hunt and removed all use of LINQ from their code. We should avoid it where we can.

Instead, it would be more efficient to iterate through the collection and evaluate the options.

mikepizzo · 2021-01-28T21:22:26Z

test/FunctionalTests/Microsoft.OData.Edm.Tests/Csdl/CsdlReaderTests.cs

+<edmx:Edmx Version=""4.0"" xmlns:edmx=""http://docs.oasis-open.org/odata/ns/edmx"">
+  <edmx:Reference  Uri=""SimpleModel.xml"">
+    <edmx:Include Namespace=""Default""/>
+    <edmx:Include Namespace=""Example.Types"" />


Maybe add a test where the annotation schema defines an alias for the referenced namespace that is used locally in specifying the target, and make sure that annotations restricted to that target are still (exclusively) included.

i.e., define an alias for the "Example.Types" namespace in permissions.Csdl (say "examples") and use it in the Annotations Target, and then in SimpleModel.xml specify a TargetNamespace of "Example.Types" in your include annotations and make sure it still returns the right set of annotations (I think your code handles that, just want to make sure it's in the tests...)

I've added the test

mikepizzo · 2021-01-28T21:25:42Z

src/Microsoft.OData.Edm/Csdl/Semantics/CsdlSemanticsModel.cs

+
+            var filteredAnnotations = csdlAnnotations.Annotations.Where(annotation =>
+                ShouldIncludeAnnotation(annotation, target, includeAnnotations));
+            return new CsdlAnnotations(filteredAnnotations, csdlAnnotations.Target, csdlAnnotations.Qualifier);


csdlAnnotations.Target [](start = 60, length = 22)

should this use csdlAnnotations.Target, or "target" (which has the alias already resolved)?

The aim here is to return a copy of the CsdlAnnotations instance but with the actual annotations filtered. So every other property should be identical to the original CsdlAnnotations instance, hence csdlAnnotations.Target. The target with the alias already resolved is used when comparing against the include.TargetNamespace.

test/FunctionalTests/Microsoft.OData.Edm.Tests/Csdl/CsdlReaderTests.cs

src/Microsoft.OData.Edm/Csdl/Semantics/CsdlSemanticsModel.cs

gathogojr · 2021-02-05T05:58:17Z

src/Microsoft.OData.Edm/Csdl/Semantics/CsdlSemanticsModel.cs

+        /// figure out whether the schema's types (entiy types, enums, containers, etc.) and annotations should
+        /// be imported
+        /// </summary>
+        /// <param name="schema"></param>


Document this param and the following 2

gathogojr · 2021-02-05T06:00:22Z

src/Microsoft.OData.Edm/Csdl/Semantics/CsdlSemanticsModel.cs

+
+        /// <summary>
+        /// Process the <see cref="IEdmReference"/>s of the parent model to
+        /// figure out whether the schema's types (entiy types, enums, containers, etc.) and annotations should


Suggested change

/// figure out whether the schema's types (entiy types, enums, containers, etc.) and annotations should

/// figure out whether the schema's types (entity types, enums, containers, etc.) and annotations should

gathogojr · 2021-02-05T06:01:44Z

src/Microsoft.OData.Edm/Csdl/Semantics/CsdlSemanticsModel.cs

+            shouldAddSchemaElements = false;
+            includeAnnotationsIndex = new Dictionary<string, List<IEdmIncludeAnnotations>>();
+
+


Suggested change

gathogojr · 2021-02-05T06:14:55Z

src/Microsoft.OData.Edm/Csdl/Semantics/CsdlSemanticsModel.cs

+            string schemaNamespace = schema.Namespace;
+
+            shouldAddSchemaElements = false;
+            includeAnnotationsIndex = new Dictionary<string, List<IEdmIncludeAnnotations>>();


Maybe apply the constructor that takes an IEqualityComparer<T> argument if TermNamespace="Org.OData.Capabilities.V1" is considered to be equivalent to TermNamespace="org.OData.Capabilities.V1" (i.e. case-insensitive)

I believe namespaces are case-sensitive. At least in many cases, the spec mentions that identifiers are case-sensitive. I assume it applies to namespaces as well.

src/Microsoft.OData.Edm/Csdl/Semantics/CsdlSemanticsModel.cs

gathogojr · 2021-02-05T06:41:59Z

src/Microsoft.OData.Edm/Csdl/Semantics/CsdlSemanticsModel.cs

+            string target,
+            Dictionary<string, List<IEdmIncludeAnnotations>> includeAnnotationsIndex)
+        {
+            if (includeAnnotationsIndex == null)


Is this ever likely to evaluate to true given the initialization of includeAnnotationsIndex here? Maybe check for null and empty (or just empty if null is unlikely)?

It's actually set to null when called from the original AddSchema(schema, addAnnotations) method here.

In that method, when the addAnnotations was set true, all the annotations in the schema were added without any filtering. In that case I call the AddSchemaOutOfLineAnnotations with includeAnnotationsIndex set to null to indicate that all annotations should be added without any filtering. If, on the other hand, the includeAnnotationsIndex is empty, then all annotations will be filtered out and not get added, since the ShouldIncludeAnnotation method will return false.

I think I should mention in the doc comments what the intended behaviour is when includeAnnotationsIndex is null.

I think I should make an optimization to avoid filtering the annotations if the includeAnnotationsIndex is empty, since in that case none of the csdl annotations will be imported.

gathogojr · 2021-02-05T06:53:31Z

src/Microsoft.OData.Edm/Csdl/Semantics/CsdlSemanticsModel.cs

+
+            string qualifiedTerm = this.ReplaceAlias(annotation.Term);
+            string targetNamespace;
+            EdmUtil.TryGetNamespaceNameFromQualifiedName(target, out targetNamespace, out string baseTarget);


Suggested change

EdmUtil.TryGetNamespaceNameFromQualifiedName(target, out targetNamespace, out string baseTarget);

EdmUtil.TryGetNamespaceNameFromQualifiedName(target, out targetNamespace, out _);

gathogojr · 2021-02-05T06:59:49Z

src/Microsoft.OData.Edm/Csdl/Semantics/CsdlSemanticsModel.cs

+            string qualifiedTerm = this.ReplaceAlias(annotation.Term);
+            string targetNamespace;
+            EdmUtil.TryGetNamespaceNameFromQualifiedName(target, out targetNamespace, out string baseTarget);
+            if (EdmUtil.TryGetNamespaceNameFromQualifiedName(qualifiedTerm, out string termNamespace, out string baseTerm))


Suggested change

if (EdmUtil.TryGetNamespaceNameFromQualifiedName(qualifiedTerm, out string termNamespace, out string baseTerm))

if (EdmUtil.TryGetNamespaceNameFromQualifiedName(qualifiedTerm, out string termNamespace, out _))

gathogojr · 2021-02-05T06:59:54Z

src/Microsoft.OData.Edm/Csdl/Semantics/CsdlSemanticsModel.cs


-                    annotations.Add(new CsdlSemanticsAnnotations(schemaWrapper, schemaOutOfLineAnnotations));
+                    return includeAnnotations.Any(include =>


Just to confirm annotation.Qualifier == include.Qualifier and targetNamespace == include.TargetNamespace will be a case-sensitive comparison...

Maybe

(string.IsNullOrEmpty(include.Qualifier) || include.Qualifier.Equals(annotation.Qualifier, StringComparison.OrdinalIgnoreCase)) && (string.IsNullOrEmpty(include.TargetNamespace) || include.TargetNamespace.Equals(targetNamespace, StringComparison.OrdinalIgnoreCase)));

?

I believe they are case sensitive

Indeed: Names are case-sensitive, but service authors SHOULD NOT choose names that differ only in case.

src/Microsoft.OData.Edm/Csdl/Semantics/CsdlSemanticsModel.cs

gathogojr · 2021-02-09T09:15:21Z

src/Microsoft.OData.Edm/Csdl/Semantics/CsdlSemanticsModel.cs

+        /// If this dictionary is non-empty, then only annotations that match
+        /// the references will be added. If it's empty, no annotations will be added. If it's null, all annotations
+        /// will be added unconditionally.</param>
+        private void AddOutOfLineAnnotationsFromSchema(


@habbes RE: If it's null, all annotations will be added unconditionally
Does this happen from the calling method or this (AddOutOfLineAnnotationsFromSchema) method?

That actually takes place in the FilterIncludedAnnotations method, which is called by this method (AddOutOfLineAnnotationsFromSchema). The FilterIncludedAnnotations method has a similar note in its doc comments.

gathogojr

LGTM

KenitoInc

LGTM

KenitoInc · 2021-02-10T18:38:09Z

I see the build is failing.
You have defined AddSchemaElements accepting 1 parameter, but calling it with 2 parameters.

mikepizzo · 2021-02-12T04:18:10Z

😀

In reply to: 772912424 [](ancestors = 772912424)

marabooy · 2021-02-12T06:24:44Z

src/Microsoft.OData.Edm/Csdl/Semantics/CsdlSemanticsModel.cs

+            CsdlSemanticsSchema schemaWrapper,
+            Dictionary<string, List<IEdmIncludeAnnotations>> includeAnnotationsIndex)
+        {
+            if (includeAnnotationsIndex?.Count == 0)


Suggested change

if (includeAnnotationsIndex?.Count == 0)

if (!includeAnnotationsIndex?.Count > 0)

or handle the null in a better way as Any can cause some issues with performance here due. I believe null == 0 wil give you false but I could be wrong.

I didn't understand this comment very well. But I made the null check more explicit includeAnnotationsIndex != null && includeAnnotationsIndex.Count == 0

mikepizzo · 2021-02-12T16:32:25Z

test/FunctionalTests/Microsoft.OData.Edm.Tests/Csdl/CsdlReaderTests.cs

@@ -1230,5 +1231,323 @@ public void ValidateEdmxVersions(string odataVersion)
            Assert.Empty(edmErrors);
            Assert.Equal(edmModel.GetEdmVersion(), odataVersion == "4.0" ? EdmConstants.EdmVersion4 : EdmConstants.EdmVersion401);
        }
+
+        [Fact]


Fact [](start = 9, length = 4)

Do you have any tests that validate referenced types (not annotations) from referenced namespace.

For example, in your tests, you could test loading permissionsCsdl and validating that it imported types from "Default" and "Exmaple.Types" from main.csdl, but didn't import types from main.csdl defined in 3rd namespace.

I've added the test.

mikepizzo

mikepizzo · 2021-02-12T16:45:37Z

nice.

habbes added 3 commits January 26, 2021 09:41

Import referenced schemas that include annotations

36af6c3

Only import annotations with match IncludeAnnotation namespace, targe…

256e3b1

…t and qualifier

Add tests for IncludeAnnotations with Qualifier and TargetNamespace

26a681e

habbes force-pushed the fix/1936-get-external-referenced-vocab branch from f998660 to 26a681e Compare January 26, 2021 06:43

Fix issues in FilterIncludedAnnotations method

d6087fc

habbes requested review from mikepizzo, gathogojr, KenitoInc, marabooy, ElizabethOkerio and xuzhg January 26, 2021 07:01

Don't import schema types when only vocabulary annotations are refere…

9613de6

…nced

gathogojr reviewed Jan 27, 2021

View reviewed changes

test/FunctionalTests/Microsoft.OData.Edm.Tests/Csdl/CsdlReaderTests.cs Outdated Show resolved Hide resolved

xuzhg reviewed Jan 27, 2021

View reviewed changes

src/Microsoft.OData.Edm/Csdl/Semantics/CsdlSemanticsModel.cs Show resolved Hide resolved

xuzhg approved these changes Jan 27, 2021

View reviewed changes

gathogojr reviewed Jan 28, 2021

View reviewed changes

src/Microsoft.OData.Edm/Csdl/Semantics/CsdlSemanticsModel.cs Outdated Show resolved Hide resolved

gathogojr reviewed Jan 28, 2021

View reviewed changes

src/Microsoft.OData.Edm/Csdl/Semantics/CsdlSemanticsModel.cs Outdated Show resolved Hide resolved

mikepizzo reviewed Jan 28, 2021

View reviewed changes

KenitoInc requested changes Feb 1, 2021

View reviewed changes

test/FunctionalTests/Microsoft.OData.Edm.Tests/Csdl/CsdlReaderTests.cs Outdated Show resolved Hide resolved

marabooy reviewed Feb 1, 2021

View reviewed changes

src/Microsoft.OData.Edm/Csdl/Semantics/CsdlSemanticsModel.cs Outdated Show resolved Hide resolved

Refactor adding external schema if referenced

a3a8877

habbes requested review from KenitoInc, gathogojr, marabooy and mikepizzo February 3, 2021 23:57

gathogojr reviewed Feb 5, 2021

View reviewed changes

src/Microsoft.OData.Edm/Csdl/Semantics/CsdlSemanticsModel.cs Show resolved Hide resolved

gathogojr reviewed Feb 5, 2021

View reviewed changes

src/Microsoft.OData.Edm/Csdl/Semantics/CsdlSemanticsModel.cs Show resolved Hide resolved

gathogojr reviewed Feb 5, 2021

View reviewed changes

src/Microsoft.OData.Edm/Csdl/Semantics/CsdlSemanticsModel.cs Show resolved Hide resolved

gathogojr reviewed Feb 5, 2021

View reviewed changes

src/Microsoft.OData.Edm/Csdl/Semantics/CsdlSemanticsModel.cs Show resolved Hide resolved

habbes added the Ready for review Use this label if a pull request is ready to be reviewed label Feb 5, 2021

KenitoInc requested changes Feb 9, 2021

View reviewed changes

src/Microsoft.OData.Edm/Csdl/Semantics/CsdlSemanticsModel.cs Show resolved Hide resolved

habbes added 2 commits February 9, 2021 11:45

Minor optimization when adding annotations to model

b838022

Add more doc comments

717346e

gathogojr reviewed Feb 9, 2021

View reviewed changes

gathogojr approved these changes Feb 9, 2021

View reviewed changes

KenitoInc approved these changes Feb 10, 2021

View reviewed changes

marabooy reviewed Feb 12, 2021

View reviewed changes

mikepizzo reviewed Feb 12, 2021

View reviewed changes

mikepizzo approved these changes Feb 12, 2021

View reviewed changes

habbes added 3 commits February 23, 2021 19:04

Fix compiler error in CsdlSemanticsModel

6ed8841

Test import of referenced schema elements

747511b

Fix bugs in tests

e4279cf

habbes merged commit 8444960 into OData:master Feb 23, 2021

habbes deleted the fix/1936-get-external-referenced-vocab branch February 23, 2021 18:11

	bool shouldAddTypes = referencedCsdlModel.ParentModelReferences.SelectMany(s => s.Includes).Any(s => s.Namespace == schemaNamespace);
	bool addSchemaElements = referencedCsdlModel.ParentModelReferences.SelectMany(s => s.Includes).Any(s => s.Namespace == schemaNamespace);

	this.AddSchema(schema, addSchemaElements, true, null);
	this.AddSchema(schema, addSchemaElements, true /addAnnotations/, null /includeAnnotations/);

	/// figure out whether the schema's types (entiy types, enums, containers, etc.) and annotations should
	/// figure out whether the schema's types (entity types, enums, containers, etc.) and annotations should

		shouldAddSchemaElements = false;
		includeAnnotationsIndex = new Dictionary<string, List<IEdmIncludeAnnotations>>();

	EdmUtil.TryGetNamespaceNameFromQualifiedName(target, out targetNamespace, out string baseTarget);
	EdmUtil.TryGetNamespaceNameFromQualifiedName(target, out targetNamespace, out _);

	if (EdmUtil.TryGetNamespaceNameFromQualifiedName(qualifiedTerm, out string termNamespace, out string baseTerm))
	if (EdmUtil.TryGetNamespaceNameFromQualifiedName(qualifiedTerm, out string termNamespace, out _))


		annotations.Add(new CsdlSemanticsAnnotations(schemaWrapper, schemaOutOfLineAnnotations));
		return includeAnnotations.Any(include =>

	if (includeAnnotationsIndex?.Count == 0)
	if (!includeAnnotationsIndex?.Count > 0)

Import vocabulary annotations declared in external referenced models #1987

Import vocabulary annotations declared in external referenced models #1987

Conversation

habbes commented Jan 25, 2021 • edited Loading

Issues

Description

Checklist (Uncheck if it is not completed)

Additional work necessary

Choose a reason for hiding this comment

xuzhg left a comment

Choose a reason for hiding this comment

gathogojr Jan 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gathogojr Feb 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gathogojr left a comment

Choose a reason for hiding this comment

KenitoInc left a comment

Choose a reason for hiding this comment

KenitoInc commented Feb 10, 2021

mikepizzo commented Feb 12, 2021

marabooy Feb 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikepizzo left a comment

Choose a reason for hiding this comment

mikepizzo commented Feb 12, 2021

habbes commented Jan 25, 2021 •

edited

Loading

gathogojr Jan 28, 2021 •

edited

Loading

gathogojr Feb 5, 2021 •

edited

Loading

marabooy Feb 12, 2021 •

edited

Loading