Skip to content

fix: preserve cached annotations when another term is absent #955

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.

Sign up for GitHub

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

Open
Gavin Barron (gavinbarron) wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: preserve cached annotations when another term is absent #955

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 6 additions & 8 deletions src/Microsoft.OpenApi.OData.Reader/Edm/EdmAnnotationExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
{
private static IDictionary<IEdmVocabularyAnnotatable, IDictionary<string, object>>? _cachedAnnotations;
private static IEdmModel? _savedModel = null; // if different model, the cache will be cleaned.
private static readonly object _objectLock = new object();

Check warning on line 26 in src/Microsoft.OpenApi.OData.Reader/Edm/EdmAnnotationExtensions.cs

View workflow job for this annotation

GitHub Actions / Build 

Use 'System.Threading.Lock' for field '_objectLock'.

Check warning on line 26 in src/Microsoft.OpenApi.OData.Reader/Edm/EdmAnnotationExtensions.cs

View workflow job for this annotation

GitHub Actions / Build 

Use 'System.Threading.Lock' for field '_objectLock'.

/// <summary>
/// Gets the boolean term value for the given <see cref="IEdmVocabularyAnnotatable"/>.
Expand Down Expand Up @@ -402,16 +402,14 @@
T ret = (T)restriction;
return ret;
}
else if (createFunc() is T ret)
{
value[qualifiedName] = ret;
return ret;
}
}
else
{
// Only new targets need a dictionary; absent annotations must not evict cached records.
value = new Dictionary<string, object>();
_cachedAnnotations[target] = value;
}

// It's first time to query this target, create new dictionary and restriction.
value = new Dictionary<string, object>();
_cachedAnnotations[target] = value;
if (createFunc() is T newAnnotation)
{
value[qualifiedName] = newAnnotation;
Expand Down Expand Up @@ -482,7 +480,7 @@
return null;
}

private static IEnumerable<string>? GetCollection(this IEdmModel model, IEdmVocabularyAnnotatable target, IEdmTerm term)

Check warning on line 483 in src/Microsoft.OpenApi.OData.Reader/Edm/EdmAnnotationExtensions.cs

View workflow job for this annotation

GitHub Actions / Build 

All 'GetCollection' method overloads should be adjacent.

Check warning on line 483 in src/Microsoft.OpenApi.OData.Reader/Edm/EdmAnnotationExtensions.cs

View workflow job for this annotation

GitHub Actions / Build 

All 'GetCollection' method overloads should be adjacent.
{
Debug.Assert(model != null);
Debug.Assert(target != null);
Expand Down
57 changes: 57 additions & 0 deletions test/Microsoft.OpenAPI.OData.Reader.Tests/Edm/EdmVocabularyAnnotationExtensionsTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,63 @@ public class EdmVocabularyAnnotationExtensionsTests
{
private static readonly EdmTerm CachedStringTerm = new EdmTerm("NS", "CachedString", EdmPrimitiveTypeKind.String);

[Theory]
[InlineData(true)]
[InlineData(false)]
public void AnnotationCachePreservesMergedRecordsAfterAbsentTermLookups(bool queryAbsentTermFirst)
{
var model = GetEdmModel(@"<Annotation Term=""Org.OData.Capabilities.V1.ReadRestrictions"">
<Record>
<PropertyValue Property=""Description"" String=""List items"" />
</Record>
</Annotation>
<Annotation Term=""Org.OData.Core.V1.Description"" String=""Items"" />");
var target = model.EntityContainer;
if (queryAbsentTermFirst)
{
Assert.Null(model.GetRecord<CountRestrictionsType>(target));
}

var readRestrictions = model.GetRecord<ReadRestrictionsType>(target);
Assert.NotNull(readRestrictions);
var inheritedRestrictions = new ReadRestrictionsType();
inheritedRestrictions.Initialize(new EdmRecordExpression(
new EdmPropertyConstructor("CustomQueryOptions", new EdmCollectionExpression(
new EdmRecordExpression(new EdmPropertyConstructor("Name", new EdmStringConstant("includeHiddenFolders")))))));
readRestrictions.MergePropertiesIfNull(inheritedRestrictions);
Assert.Equal("Items", model.GetString(target, "Org.OData.Core.V1.Description"));

Assert.Null(model.GetRecord<CountRestrictionsType>(target));
Assert.Null(model.GetRecord<CountRestrictionsType>(target));
Assert.Null(model.GetString(target, "NS.UndefinedTerm"));

var cachedRestrictions = model.GetRecord<ReadRestrictionsType>(target);
Assert.Same(readRestrictions, cachedRestrictions);
Assert.Equal("List items", cachedRestrictions.Description);
Assert.Equal("includeHiddenFolders", Assert.Single(cachedRestrictions.CustomQueryOptions).Name);
Assert.Equal("Items", model.GetString(target, "Org.OData.Core.V1.Description"));

var secondModel = GetEdmModel("");
Assert.Null(secondModel.GetRecord<ReadRestrictionsType>(secondModel.EntityContainer));

var reloadedRestrictions = model.GetRecord<ReadRestrictionsType>(target);
Assert.NotSame(readRestrictions, reloadedRestrictions);
Assert.Equal("List items", reloadedRestrictions.Description);
Assert.Null(reloadedRestrictions.CustomQueryOptions);
}

[Fact]
public void AnnotationCacheLooksUpAbsentTermOncePerCallForNewAndExistingTargets()
{
var target = new ValueEqualAnnotatable(1, 42);
var model = GetCacheModel();

Assert.Null(model.GetString(target, CachedStringTerm.FullName()));
Assert.Equal(1, model.FindDeclaredVocabularyAnnotationsCallCount);
Assert.Null(model.GetString(target, CachedStringTerm.FullName()));
Assert.Equal(2, model.FindDeclaredVocabularyAnnotationsCallCount);
}

[Theory]
[InlineData(true)]
[InlineData(false)]
Expand Down
73 changes: 73 additions & 0 deletions test/Microsoft.OpenAPI.OData.Reader.Tests/Operation/DollarCountGetOperationHandlerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,17 @@
// ------------------------------------------------------------

using Microsoft.OData.Edm;
using Microsoft.OData.Edm.Csdl;
using System.Linq;
using System.Xml.Linq;
using Microsoft.OpenApi.OData.Edm;
using Microsoft.OpenApi.OData.Tests;
using Microsoft.OpenApi.OData.Vocabulary.Capabilities;
using Xunit;

namespace Microsoft.OpenApi.OData.Operation.Tests
{
[Collection("Vocabulary annotation cache")]
public class DollarCountGetOperationHandlerTests
{
private readonly OpenApiDocument openApiDocument = new();
Expand All @@ -22,6 +26,75 @@ public DollarCountGetOperationHandlerTests()
openApiDocument.AddComponent("ConsistencyLevel", new OpenApiParameter {Name = "ConsistencyLevel", In = ParameterLocation.Query, Schema = new OpenApiSchema {Type = JsonSchemaType.String}});
}

[Fact]
public void CreateDollarCountGetOperationPreservesCustomOptionsMergedByNavigationGet()
{
const string csdl = """
<edmx:Edmx Version="4.0" xmlns:edmx="http://docs.oasis-open.org/odata/ns/edmx">
<edmx:DataServices>
<Schema Namespace="NS" xmlns="http://docs.oasis-open.org/odata/ns/edm">
<EntityType Name="User">
<Key><PropertyRef Name="id" /></Key>
<Property Name="id" Type="Edm.String" Nullable="false" />
<NavigationProperty Name="mailFolders" Type="Collection(NS.MailFolder)" ContainsTarget="true" />
</EntityType>
<EntityType Name="MailFolder">
<Key><PropertyRef Name="id" /></Key>
<Property Name="id" Type="Edm.String" Nullable="false" />
</EntityType>
<EntityContainer Name="Default">
<Singleton Name="me" Type="NS.User">
<Annotation Term="Org.OData.Capabilities.V1.NavigationRestrictions">
<Record>
<PropertyValue Property="RestrictedProperties">
<Collection>
<Record>
<PropertyValue Property="NavigationProperty" NavigationPropertyPath="mailFolders" />
<PropertyValue Property="ReadRestrictions">
<Record>
<PropertyValue Property="CustomQueryOptions">
<Collection>
<Record>
<PropertyValue Property="Name" String="includeHiddenFolders" />
</Record>
</Collection>
</PropertyValue>
</Record>
</PropertyValue>
</Record>
</Collection>
</PropertyValue>
</Record>
</Annotation>
</Singleton>
</EntityContainer>
<Annotations Target="NS.Default/me/mailFolders">
<Annotation Term="Org.OData.Capabilities.V1.ReadRestrictions">
<Record>
<PropertyValue Property="Description" String="List mail folders" />
</Record>
</Annotation>
</Annotations>
</Schema>
</edmx:DataServices>
</edmx:Edmx>
""";
var model = CsdlReader.Parse(XElement.Parse(csdl).CreateReader());
var context = new ODataContext(model, new OpenApiConvertSettings());
var me = model.EntityContainer.FindSingleton("me");
var mailFolders = me.EntityType.DeclaredNavigationProperties().Single();
var path = new ODataPath(new ODataNavigationSourceSegment(me), new ODataNavigationPropertySegment(mailFolders));
var collectionOperation = new NavigationPropertyGetOperationHandler(openApiDocument).CreateOperation(context, path);
Assert.Contains(collectionOperation.Parameters, p => p.Name == "includeHiddenFolders" && p.In == ParameterLocation.Query);

Assert.Null(model.GetRecord<CountRestrictionsType>("NS.Default/me/mailFolders", CapabilitiesConstants.CountRestrictions));
var countPath = new ODataPath(new ODataNavigationSourceSegment(me),
new ODataNavigationPropertySegment(mailFolders), new ODataDollarCountSegment());
var countOperation = _operationHandler.CreateOperation(context, countPath);

Assert.Contains(countOperation.Parameters, p => p.Name == "includeHiddenFolders" && p.In == ParameterLocation.Query);
}

[Theory]
[InlineData(true, true)]
[InlineData(false, true)]
Expand Down
Loading