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

Import vocabulary annotations declared in external referenced models #1987

Merged
merged 11 commits into from
Feb 23, 2021
193 changes: 167 additions & 26 deletions src/Microsoft.OData.Edm/Csdl/Semantics/CsdlSemanticsModel.cs
Original file line number Diff line number Diff line change
Expand Up @@ -113,14 +113,7 @@ 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)
{
this.AddSchema(schema, false /*addAnnotations*/);
}

// TODO: REF add annotations
this.AddSchemaIfReferenced(schema, referencedCsdlModel.ParentModelReferences);
}
}

Expand Down Expand Up @@ -531,6 +524,119 @@ private void AddSchema(CsdlSchema schema, bool addAnnotations)
{
CsdlSemanticsSchema schemaWrapper = new CsdlSemanticsSchema(this, schema);
this.schemata.Add(schemaWrapper);

AddSchemaElements(schema, schemaWrapper);

if (!string.IsNullOrEmpty(schema.Alias))
{
this.SetNamespaceAlias(schema.Namespace, schema.Alias);
}

if (addAnnotations)
{
AddOutOfLineAnnotationsFromSchema(schema, schemaWrapper, includeAnnotationsIndex: null);
}

var edmVersion = this.GetEdmVersion();
if (edmVersion == null || edmVersion < schema.Version)
habbes marked this conversation as resolved.
Show resolved Hide resolved
{
this.SetEdmVersion(schema.Version);
}
}

private void AddSchemaIfReferenced(CsdlSchema schema, IEnumerable<IEdmReference> parentReferences)
habbes marked this conversation as resolved.
Show resolved Hide resolved
{
CsdlSemanticsSchema schemaWrapper = new CsdlSemanticsSchema(this, schema);

bool shouldAddSchemaElements;
Dictionary<string, List<IEdmIncludeAnnotations>> includeAnnotationsIndex;
ProcessSchemaParentReferences(schema, schemaWrapper, parentReferences, out shouldAddSchemaElements, out includeAnnotationsIndex);

if (!string.IsNullOrEmpty(schema.Alias))
{
this.SetNamespaceAlias(schema.Namespace, schema.Alias);
}

if (shouldAddSchemaElements)
{
AddSchemaElements(schema, schemaWrapper);
}

if (includeAnnotationsIndex.Count > 0)
{
AddOutOfLineAnnotationsFromSchema(schema, schemaWrapper, includeAnnotationsIndex);
}

if (shouldAddSchemaElements || includeAnnotationsIndex.Count > 0)
{
this.schemata.Add(schemaWrapper);
habbes marked this conversation as resolved.
Show resolved Hide resolved

var edmVersion = this.GetEdmVersion();
habbes marked this conversation as resolved.
Show resolved Hide resolved
if (edmVersion == null || edmVersion < schema.Version)
{
this.SetEdmVersion(schema.Version);
}
}
}

/// <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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

/// be imported
/// </summary>
/// <param name="schema"></param>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Document this param and the following 2

/// <param name="schemaWrapper"></param>
/// <param name="parentReferences"></param>
/// <param name="shouldAddSchemaElements">Whether schema types such entity types, operations, enums, etc. should be important</param>
Copy link
Contributor

@gathogojr gathogojr Feb 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// <param name="shouldAddSchemaElements">Whether schema types such entity types, operations, enums, etc. should be important</param>
/// <param name="shouldAddSchemaElements">Whether schema types such entity types, operations, enums, etc. should be added</param>

/// <param name="includeAnnotationsIndex">Cache of the schema's out-of-line <see cref="IEdmIncludeAnnotations"/>s indexed by the term namespace</param>
private static void ProcessSchemaParentReferences(
CsdlSchema schema,
CsdlSemanticsSchema schemaWrapper,
IEnumerable<IEdmReference> parentReferences,
out bool shouldAddSchemaElements,
out Dictionary<string, List<IEdmIncludeAnnotations>> includeAnnotationsIndex)
{
string schemaNamespace = schema.Namespace;

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.



Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

foreach (IEdmReference reference in parentReferences)
{
if (!shouldAddSchemaElements)
{
// we should add types from this schema if there's at least
// one edmx:Include that references this schema's namespace
foreach (IEdmInclude include in reference.Includes)
{
if (include.Namespace == schemaNamespace)
{
shouldAddSchemaElements = true;
break;
}
}
}

foreach(IEdmIncludeAnnotations includeAnnotations in reference.IncludeAnnotations)
{
gathogojr marked this conversation as resolved.
Show resolved Hide resolved
// index the edmx:IncludeAnnotations by namespace to make it
// easier to filter which annotations to import with the schema
string termNamespace = includeAnnotations.TermNamespace;
List<IEdmIncludeAnnotations> cachedAnnotations;
if (!includeAnnotationsIndex.TryGetValue(termNamespace, out cachedAnnotations))
{
cachedAnnotations = new List<IEdmIncludeAnnotations>();
includeAnnotationsIndex[termNamespace] = cachedAnnotations;
}

cachedAnnotations.Add(includeAnnotations);
gathogojr marked this conversation as resolved.
Show resolved Hide resolved
}
}
}

private void AddSchemaElements(CsdlSchema schema, CsdlSemanticsSchema schemaWrapper)
{
foreach (IEdmSchemaType type in schemaWrapper.Types)
{
CsdlSemanticsStructuredTypeDefinition structuredType = type as CsdlSemanticsStructuredTypeDefinition;
Expand Down Expand Up @@ -574,34 +680,69 @@ private void AddSchema(CsdlSchema schema, bool addAnnotations)
{
RegisterElement(container);
}
}

if (!string.IsNullOrEmpty(schema.Alias))
private void AddOutOfLineAnnotationsFromSchema(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

CsdlSchema schema,
CsdlSemanticsSchema schemaWrapper,
Dictionary<string, List<IEdmIncludeAnnotations>> includeAnnotationsIndex)
{
foreach (CsdlAnnotations schemaOutOfLineAnnotations in schema.OutOfLineAnnotations)
{
this.SetNamespaceAlias(schema.Namespace, schema.Alias);
string target = this.ReplaceAlias(schemaOutOfLineAnnotations.Target);

List<CsdlSemanticsAnnotations> annotations;
if (!this.outOfLineAnnotations.TryGetValue(target, out annotations))
{
annotations = new List<CsdlSemanticsAnnotations>();
this.outOfLineAnnotations[target] = annotations;
}

CsdlAnnotations filteredAnnotations = FilterIncludedAnnotations(schemaOutOfLineAnnotations, target, includeAnnotationsIndex);
annotations.Add(new CsdlSemanticsAnnotations(schemaWrapper, filteredAnnotations));
}
}

if (addAnnotations)
private CsdlAnnotations FilterIncludedAnnotations(
CsdlAnnotations csdlAnnotations,
string target,
Dictionary<string, List<IEdmIncludeAnnotations>> includeAnnotationsIndex)
{
if (includeAnnotationsIndex == null)
Copy link
Contributor

@gathogojr gathogojr Feb 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
foreach (CsdlAnnotations schemaOutOfLineAnnotations in schema.OutOfLineAnnotations)
{
string target = this.ReplaceAlias(schemaOutOfLineAnnotations.Target);
return csdlAnnotations;
}

List<CsdlSemanticsAnnotations> annotations;
if (!this.outOfLineAnnotations.TryGetValue(target, out annotations))
{
annotations = new List<CsdlSemanticsAnnotations>();
this.outOfLineAnnotations[target] = annotations;
}
var filteredAnnotations = csdlAnnotations.Annotations.Where(annotation =>
ShouldIncludeAnnotation(annotation, target, includeAnnotationsIndex));
return new CsdlAnnotations(filteredAnnotations, csdlAnnotations.Target, csdlAnnotations.Qualifier);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

private bool ShouldIncludeAnnotation(
CsdlAnnotation annotation,
string target,
Dictionary<string, List<IEdmIncludeAnnotations>> includeAnnotationsIndex)
{
// include annotation if it matches one of the edmx:IncludeAnnotation references
// 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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
if (includeAnnotationsIndex.TryGetValue(termNamespace, out List<IEdmIncludeAnnotations> includeAnnotations))
{

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)));

?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe they are case sensitive

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

(string.IsNullOrEmpty(include.Qualifier) || annotation.Qualifier == include.Qualifier)
&& (string.IsNullOrEmpty(include.TargetNamespace) || targetNamespace == include.TargetNamespace));
}

}

var edmVersion = this.GetEdmVersion();
if (edmVersion == null || edmVersion < schema.Version)
{
this.SetEdmVersion(schema.Version);
}
return false;
}
}
}
Expand Down
Loading