Skip to content

Commit 3c7ef7d

Browse files
gavinbarronCopilot
gavinbarron
and
Copilot
authored
fix(odata): handle mixed annotation cache key collisions (#909)
Compare EdmTargetPath keys by ordinal path value without calling their unsafe cross-type Equals override. Preserve default nonpath equality and hashing, cache locking and model invalidation. Add deterministic mixed-key collision and cache semantics regressions. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 278c98ec-426d-4542-9536-6fa5bf6a821f
1 parent ec43631 commit 3c7ef7d

2 files changed

Lines changed: 161 additions & 2 deletions

File tree

‎src/Microsoft.OpenApi.OData.Reader/Edm/EdmAnnotationExtensions.cs‎

Lines changed: 27 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -390,7 +390,7 @@ internal static class EdmVocabularyAnnotationExtensions
390390

391391
if (_cachedAnnotations == null)
392392
{
393-
_cachedAnnotations = new Dictionary<IEdmVocabularyAnnotatable, IDictionary<string, object>>();
393+
_cachedAnnotations = new Dictionary<IEdmVocabularyAnnotatable, IDictionary<string, object>>(new AnnotationTargetComparer());
394394
}
395395

396396
object? restriction;
@@ -421,6 +421,32 @@ internal static class EdmVocabularyAnnotationExtensions
421421
}
422422
}
423423

424+
private sealed class AnnotationTargetComparer : IEqualityComparer<IEdmVocabularyAnnotatable>
425+
{
426+
public bool Equals(IEdmVocabularyAnnotatable? x, IEdmVocabularyAnnotatable? y)
427+
{
428+
// EdmTargetPath.Equals dereferences a failed cast when a hash collision involves another target kind.
429+
if (x is EdmTargetPath xPath)
430+
{
431+
return y is EdmTargetPath yPath && StringComparer.Ordinal.Equals(xPath.Path, yPath.Path);
432+
}
433+
434+
if (y is EdmTargetPath)
435+
{
436+
return false;
437+
}
438+
439+
return EqualityComparer<IEdmVocabularyAnnotatable>.Default.Equals(x, y);
440+
}
441+
442+
public int GetHashCode(IEdmVocabularyAnnotatable target)
443+
{
444+
return target is EdmTargetPath path
445+
? StringComparer.Ordinal.GetHashCode(path.Path)
446+
: EqualityComparer<IEdmVocabularyAnnotatable>.Default.GetHashCode(target);
447+
}
448+
}
449+
424450
private static bool? GetBoolean(this IEdmModel model, IEdmVocabularyAnnotatable target, IEdmTerm term)
425451
{
426452
Debug.Assert(model != null);

‎test/Microsoft.OpenAPI.OData.Reader.Tests/Edm/EdmVocabularyAnnotationExtensionsTests.cs‎

Lines changed: 134 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,20 +3,148 @@
33
// Licensed under the MIT License (MIT). See LICENSE in the repo root for license information.
44
// ------------------------------------------------------------
55

6+
using System;
67
using System.Collections.Generic;
78
using System.Linq;
8-
using System.Runtime.InteropServices;
99
using System.Xml.Linq;
1010
using Microsoft.OData.Edm;
1111
using Microsoft.OData.Edm.Csdl;
1212
using Microsoft.OData.Edm.Validation;
13+
using Microsoft.OData.Edm.Vocabularies;
1314
using Microsoft.OpenApi.OData.Vocabulary.Capabilities;
15+
using Moq;
1416
using Xunit;
1517

1618
namespace Microsoft.OpenApi.OData.Edm.Tests
1719
{
20+
[Collection("Vocabulary annotation cache")]
1821
public class EdmVocabularyAnnotationExtensionsTests
1922
{
23+
private static readonly EdmTerm CachedStringTerm = new EdmTerm("NS", "CachedString", EdmPrimitiveTypeKind.String);
24+
25+
[Theory]
26+
[InlineData(true)]
27+
[InlineData(false)]
28+
public void AnnotationCacheHandlesMixedTargetHashCollisions(bool pathFirst)
29+
{
30+
var path = CreateTargetPath();
31+
var other = new ValueEqualAnnotatable(1, path.GetHashCode());
32+
Assert.Equal(path.GetHashCode(), other.GetHashCode());
33+
var model = GetCacheModel(
34+
new EdmVocabularyAnnotation(path, CachedStringTerm, new EdmStringConstant("path")),
35+
new EdmVocabularyAnnotation(other, CachedStringTerm, new EdmStringConstant("other")));
36+
37+
IEdmVocabularyAnnotatable first = pathFirst ? path : other;
38+
IEdmVocabularyAnnotatable second = pathFirst ? other : path;
39+
Assert.Equal(pathFirst ? "path" : "other", model.Object.GetString(first, CachedStringTerm.FullName()));
40+
Assert.Equal(pathFirst ? "other" : "path", model.Object.GetString(second, CachedStringTerm.FullName()));
41+
// Revisit both keys so a stored path is compared with a nonpath in either insertion order.
42+
Assert.Equal("other", model.Object.GetString(other, CachedStringTerm.FullName()));
43+
Assert.Equal("path", model.Object.GetString(path, CachedStringTerm.FullName()));
44+
model.Verify(m => m.FindDeclaredVocabularyAnnotations(It.IsAny<IEdmVocabularyAnnotatable>()), Times.Exactly(2));
45+
}
46+
47+
[Fact]
48+
public void AnnotationCacheSharesEquivalentPathsUsingOrdinalEquality()
49+
{
50+
var path = CreateTargetPath();
51+
var equivalentPath = CreateTargetPath();
52+
var differentPath = CreateTargetPath("items");
53+
Assert.NotSame(path, equivalentPath);
54+
Assert.Equal(path.Path, equivalentPath.Path);
55+
var model = GetCacheModel(
56+
new EdmVocabularyAnnotation(path, CachedStringTerm, new EdmStringConstant("original")),
57+
new EdmVocabularyAnnotation(differentPath, CachedStringTerm, new EdmStringConstant("different")));
58+
59+
Assert.Equal("original", model.Object.GetString(path, CachedStringTerm.FullName()));
60+
Assert.Equal("original", model.Object.GetString(equivalentPath, CachedStringTerm.FullName()));
61+
Assert.Equal("different", model.Object.GetString(differentPath, CachedStringTerm.FullName()));
62+
model.Verify(m => m.FindDeclaredVocabularyAnnotations(It.IsAny<IEdmVocabularyAnnotatable>()), Times.Exactly(2));
63+
}
64+
65+
[Fact]
66+
public void AnnotationCachePreservesNonpathValueEqualityAndHashing()
67+
{
68+
var target = new ValueEqualAnnotatable(1, 42);
69+
var equivalentTarget = new ValueEqualAnnotatable(1, 42);
70+
var differentTarget = new ValueEqualAnnotatable(2, 42);
71+
var model = GetCacheModel(
72+
new EdmVocabularyAnnotation(target, CachedStringTerm, new EdmStringConstant("original")),
73+
new EdmVocabularyAnnotation(differentTarget, CachedStringTerm, new EdmStringConstant("different")));
74+
75+
Assert.Equal("original", model.Object.GetString(target, CachedStringTerm.FullName()));
76+
Assert.Equal("original", model.Object.GetString(equivalentTarget, CachedStringTerm.FullName()));
77+
Assert.Equal("different", model.Object.GetString(differentTarget, CachedStringTerm.FullName()));
78+
model.Verify(m => m.FindDeclaredVocabularyAnnotations(It.IsAny<IEdmVocabularyAnnotatable>()), Times.Exactly(2));
79+
}
80+
81+
[Fact]
82+
public void AnnotationCachePreservesNonpathReferenceEquality()
83+
{
84+
var target = new EdmEntityContainer("NS", "Default");
85+
var differentTarget = new EdmEntityContainer("NS", "Default");
86+
var model = GetCacheModel(
87+
new EdmVocabularyAnnotation(target, CachedStringTerm, new EdmStringConstant("original")),
88+
new EdmVocabularyAnnotation(differentTarget, CachedStringTerm, new EdmStringConstant("different")));
89+
90+
Assert.Equal("original", model.Object.GetString(target, CachedStringTerm.FullName()));
91+
Assert.Equal("different", model.Object.GetString(differentTarget, CachedStringTerm.FullName()));
92+
Assert.Equal("original", model.Object.GetString(target, CachedStringTerm.FullName()));
93+
model.Verify(m => m.FindDeclaredVocabularyAnnotations(It.IsAny<IEdmVocabularyAnnotatable>()), Times.Exactly(2));
94+
}
95+
96+
[Theory]
97+
[InlineData(true)]
98+
[InlineData(false)]
99+
public void AnnotationCacheInvalidatesBothTargetKindsWhenModelChanges(bool pathTarget)
100+
{
101+
IEdmVocabularyAnnotatable target = pathTarget
102+
? CreateTargetPath()
103+
: new ValueEqualAnnotatable(1, 42);
104+
var firstModel = GetCacheModel(new EdmVocabularyAnnotation(target, CachedStringTerm, new EdmStringConstant("first")));
105+
var secondModel = GetCacheModel(new EdmVocabularyAnnotation(target, CachedStringTerm, new EdmStringConstant("second")));
106+
107+
Assert.Equal("first", firstModel.Object.GetString(target, CachedStringTerm.FullName()));
108+
Assert.Equal("second", secondModel.Object.GetString(target, CachedStringTerm.FullName()));
109+
Assert.Equal("first", firstModel.Object.GetString(target, CachedStringTerm.FullName()));
110+
firstModel.Verify(m => m.FindDeclaredVocabularyAnnotations(It.IsAny<IEdmVocabularyAnnotatable>()), Times.Exactly(2));
111+
secondModel.Verify(m => m.FindDeclaredVocabularyAnnotations(It.IsAny<IEdmVocabularyAnnotatable>()), Times.Once());
112+
}
113+
114+
private static Mock<IEdmModel> GetCacheModel(params IEdmVocabularyAnnotation[] annotations)
115+
{
116+
var model = new Mock<IEdmModel>();
117+
model.Setup(m => m.DirectValueAnnotationsManager).Returns(new EdmModel().DirectValueAnnotationsManager);
118+
model.Setup(m => m.FindDeclaredTerm(CachedStringTerm.FullName())).Returns(CachedStringTerm);
119+
// Keep the model lookup reference-based to isolate the annotation cache's key comparison.
120+
model.Setup(m => m.FindDeclaredVocabularyAnnotations(It.IsAny<IEdmVocabularyAnnotatable>()))
121+
.Returns((IEdmVocabularyAnnotatable target) => annotations.Where(a => ReferenceEquals(a.Target, target)));
122+
return model;
123+
}
124+
125+
private static EdmTargetPath CreateTargetPath(string entitySetName = "Items")
126+
{
127+
var container = new EdmEntityContainer("NS", "Default");
128+
var entitySet = container.AddEntitySet(entitySetName, new EdmEntityType("NS", "Item"));
129+
return new EdmTargetPath(container, entitySet);
130+
}
131+
132+
private sealed class ValueEqualAnnotatable : IEdmVocabularyAnnotatable
133+
{
134+
private readonly int value;
135+
private readonly int hashCode;
136+
137+
public ValueEqualAnnotatable(int value, int hashCode)
138+
{
139+
this.value = value;
140+
this.hashCode = hashCode;
141+
}
142+
143+
public override bool Equals(object obj) => obj is ValueEqualAnnotatable other && value == other.value;
144+
145+
public override int GetHashCode() => hashCode;
146+
}
147+
20148
[Fact]
21149
public void GetStringWorksForString()
22150
{
@@ -255,4 +383,9 @@ private IEdmModel GetEdmModel(string annotation)
255383
return model;
256384
}
257385
}
386+
387+
[CollectionDefinition("Vocabulary annotation cache", DisableParallelization = true)]
388+
public class VocabularyAnnotationCacheCollection
389+
{
390+
}
258391
}

0 commit comments

Comments
 (0)