Skip to content

Commit a464bed

Browse files
irvinesundaybaywetandrueastman
authored
Fixes potential duplicate operation ids of navigation property paths with overloaded composable functions (#597)
* Fixes generation of navigation property op ids with overloaded composable fxns * Adds validation unit test * Updates release note * chore: adds string comparison Co-authored-by: Andrew Omondi <[email protected]> --------- Co-authored-by: Vincent Biret <[email protected]> Co-authored-by: Andrew Omondi <[email protected]>
1 parent 59a8d35 commit a464bed

9 files changed

+122
-25
lines changed

src/Microsoft.OpenApi.OData.Reader/Common/EdmModelHelper.cs

Lines changed: 46 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -93,38 +93,40 @@ internal static bool NavigationRestrictionsAllowsNavigability(
9393
/// Generates the operation id from a navigation property path.
9494
/// </summary>
9595
/// <param name="path">The target <see cref="ODataPath"/>.</param>
96+
/// <param name="context">The OData context.</param>
9697
/// <param name="prefix">Optional: Identifier indicating whether it is a collection-valued non-indexed or single-valued navigation property.</param>
9798
/// <returns>The operation id generated from the given navigation property path.</returns>
98-
internal static string GenerateNavigationPropertyPathOperationId(ODataPath path, string prefix = null)
99+
internal static string GenerateNavigationPropertyPathOperationId(ODataPath path, ODataContext context, string prefix = null)
99100
{
100-
IList<string> items = RetrieveNavigationPropertyPathsOperationIdSegments(path);
101+
IList<string> items = RetrieveNavigationPropertyPathsOperationIdSegments(path, context);
101102

102103
if (!items.Any())
103104
return null;
104105

105-
int lastItemIndex = items.Count - 1;
106+
int lastItemIndex = items[^1].StartsWith('-') ? items.Count - 2 : items.Count - 1;
106107

107108
if (!string.IsNullOrEmpty(prefix))
108109
{
109-
items[lastItemIndex] = prefix + Utils.UpperFirstChar(items.Last());
110+
items[lastItemIndex] = prefix + Utils.UpperFirstChar(items[lastItemIndex]);
110111
}
111112
else
112113
{
113-
items[lastItemIndex] = Utils.UpperFirstChar(items.Last());
114+
items[lastItemIndex] = Utils.UpperFirstChar(items[lastItemIndex]);
114115
}
115116

116-
return string.Join(".", items);
117+
return GenerateNavigationPropertyPathOperationId(items);
117118
}
118119

119120
/// <summary>
120121
/// Generates the operation id from a complex property path.
121122
/// </summary>
122123
/// <param name="path">The target <see cref="ODataPath"/>.</param>
124+
/// <param name="context">The OData context.</param>
123125
/// <param name="prefix">Optional: Identifier indicating whether it is a collection-valued or single-valued complex property.</param>
124126
/// <returns>The operation id generated from the given complex property path.</returns>
125-
internal static string GenerateComplexPropertyPathOperationId(ODataPath path, string prefix = null)
127+
internal static string GenerateComplexPropertyPathOperationId(ODataPath path, ODataContext context, string prefix = null)
126128
{
127-
IList<string> items = RetrieveNavigationPropertyPathsOperationIdSegments(path);
129+
IList<string> items = RetrieveNavigationPropertyPathsOperationIdSegments(path, context);
128130

129131
if (!items.Any())
130132
return null;
@@ -141,15 +143,29 @@ internal static string GenerateComplexPropertyPathOperationId(ODataPath path, st
141143
items.Add(Utils.UpperFirstChar(lastSegment?.Identifier));
142144
}
143145

144-
return string.Join(".", items);
146+
return GenerateNavigationPropertyPathOperationId(items);
147+
}
148+
149+
/// <summary>
150+
/// Generates a navigation property operation id from a list of string values.
151+
/// </summary>
152+
/// <param name="items">The list of string values.</param>
153+
/// <returns>The generated navigation property operation id.</returns>
154+
private static string GenerateNavigationPropertyPathOperationId(IList<string> items)
155+
{
156+
if (!items.Any())
157+
return null;
158+
159+
return string.Join(".", items).Replace(".-", "-", StringComparison.OrdinalIgnoreCase); // Format any hashed value appropriately (this will be the last value)
145160
}
146161

147162
/// <summary>
148163
/// Retrieves the segments of an operation id generated from a navigation property path.
149164
/// </summary>
150165
/// <param name="path">The target <see cref="ODataPath"/>.</param>
166+
/// <param name="context">The OData context.</param>
151167
/// <returns>The segments of an operation id generated from the given navigation property path.</returns>
152-
internal static IList<string> RetrieveNavigationPropertyPathsOperationIdSegments(ODataPath path)
168+
internal static IList<string> RetrieveNavigationPropertyPathsOperationIdSegments(ODataPath path, ODataContext context)
153169
{
154170
Utils.CheckArgumentNull(path, nameof(path));
155171

@@ -173,6 +189,8 @@ s is ODataOperationSegment ||
173189
Utils.CheckArgumentNull(segments, nameof(segments));
174190

175191
string previousTypeCastSegmentId = null;
192+
string pathHash = string.Empty;
193+
176194
foreach (var segment in segments)
177195
{
178196
if (segment is ODataNavigationPropertySegment navPropSegment)
@@ -192,6 +210,14 @@ s is ODataOperationSegment ||
192210
else if (segment is ODataOperationSegment operationSegment)
193211
{
194212
// Navigation property generated via composable function
213+
if (operationSegment.Operation is IEdmFunction function && context.Model.IsOperationOverload(function))
214+
{
215+
// Hash the segment to avoid duplicate operationIds
216+
pathHash = string.IsNullOrEmpty(pathHash)
217+
? operationSegment.GetPathHash(context.Settings)
218+
: (pathHash + operationSegment.GetPathHash(context.Settings)).GetHashSHA256()[..4];
219+
}
220+
195221
items.Add(operationSegment.Identifier);
196222
}
197223
else if (segment is ODataKeySegment keySegment && keySegment.IsAlternateKey)
@@ -208,6 +234,11 @@ s is ODataOperationSegment ||
208234
}
209235
}
210236

237+
if (!string.IsNullOrEmpty(pathHash))
238+
{
239+
items.Add("-" + pathHash);
240+
}
241+
211242
return items;
212243
}
213244

@@ -320,9 +351,10 @@ internal static string GenerateComplexPropertyPathTagName(ODataPath path, ODataC
320351
/// Generates the operation id prefix from an OData type cast path.
321352
/// </summary>
322353
/// <param name="path">The target <see cref="ODataPath"/>.</param>
354+
/// <param name="context">The OData context.</param>
323355
/// <param name="includeListOrGetPrefix">Optional: Whether to include the List or Get prefix to the generated operation id.</param>
324356
/// <returns>The operation id prefix generated from the OData type cast path.</returns>
325-
internal static string GenerateODataTypeCastPathOperationIdPrefix(ODataPath path, bool includeListOrGetPrefix = true)
357+
internal static string GenerateODataTypeCastPathOperationIdPrefix(ODataPath path, ODataContext context, bool includeListOrGetPrefix = true)
326358
{
327359
// Get the segment before the last OData type cast segment
328360
ODataTypeCastSegment typeCastSegment = path.Segments.OfType<ODataTypeCastSegment>()?.Last();
@@ -352,7 +384,7 @@ internal static string GenerateODataTypeCastPathOperationIdPrefix(ODataPath path
352384
if (secondLastSegment is ODataComplexPropertySegment complexSegment)
353385
{
354386
string listOrGet = includeListOrGetPrefix ? (complexSegment.Property.Type.IsCollection() ? "List" : "Get") : null;
355-
operationId = GenerateComplexPropertyPathOperationId(path, listOrGet);
387+
operationId = GenerateComplexPropertyPathOperationId(path, context, listOrGet);
356388
}
357389
else if (secondLastSegment is ODataNavigationPropertySegment navPropSegment)
358390
{
@@ -362,13 +394,13 @@ internal static string GenerateODataTypeCastPathOperationIdPrefix(ODataPath path
362394
prefix = navPropSegment?.NavigationProperty.TargetMultiplicity() == EdmMultiplicity.Many ? "List" : "Get";
363395
}
364396

365-
operationId = GenerateNavigationPropertyPathOperationId(path, prefix);
397+
operationId = GenerateNavigationPropertyPathOperationId(path, context, prefix);
366398
}
367399
else if (secondLastSegment is ODataKeySegment keySegment)
368400
{
369401
if (isIndexedCollValuedNavProp)
370402
{
371-
operationId = GenerateNavigationPropertyPathOperationId(path, "Get");
403+
operationId = GenerateNavigationPropertyPathOperationId(path, context, "Get");
372404
}
373405
else
374406
{

src/Microsoft.OpenApi.OData.Reader/Microsoft.OpenAPI.OData.Reader.csproj

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@
1515
<TargetFrameworks>net8.0</TargetFrameworks>
1616
<PackageId>Microsoft.OpenApi.OData</PackageId>
1717
<SignAssembly>true</SignAssembly>
18-
<Version>2.0.0-preview.5</Version>
18+
<Version>2.0.0-preview.6</Version>
1919
<Description>This package contains the codes you need to convert OData CSDL to Open API Document of Model.</Description>
2020
<Copyright>© Microsoft Corporation. All rights reserved.</Copyright>
2121
<PackageTags>Microsoft OpenApi OData EDM</PackageTags>
@@ -28,9 +28,10 @@
2828
- Adds nullable to double schema conversions #581
2929
- Updates tag names for actions/functions operations #585
3030
- Creates unique operation ids for paths with composable overloaded functions #580
31-
- Further fixes for double/decimal/float schema conversions #581
32-
- Replaced integer types by number types
31+
- Further fixes for double/decimal/float schema conversions #581
32+
- Replaced integer types by number types
3333
- Further fix for generating unique operation ids for paths with composable overloaded functions where all functions in path are overloaded #594
34+
- Further fix for generating unique operation ids for navigation property paths with composable overloaded functions #596
3435
</PackageReleaseNotes>
3536
<AssemblyName>Microsoft.OpenApi.OData.Reader</AssemblyName>
3637
<AssemblyOriginatorKeyFile>..\..\tool\Microsoft.OpenApi.OData.snk</AssemblyOriginatorKeyFile>

src/Microsoft.OpenApi.OData.Reader/Operation/ComplexPropertyGetOperationHandler.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ protected override void SetBasicInfo(OpenApiOperation operation)
3939
if (Context.Settings.EnableOperationId)
4040
{
4141
string prefix = ComplexPropertySegment.Property.Type.IsCollection() ? "List" : "Get";
42-
operation.OperationId = EdmModelHelper.GenerateComplexPropertyPathOperationId(Path, prefix);
42+
operation.OperationId = EdmModelHelper.GenerateComplexPropertyPathOperationId(Path, Context, prefix);
4343
}
4444

4545
// Summary and Description

src/Microsoft.OpenApi.OData.Reader/Operation/ComplexPropertyPostOperationHandler.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ protected override void SetBasicInfo(OpenApiOperation operation)
4242
// OperationId
4343
if (Context.Settings.EnableOperationId)
4444
{
45-
operation.OperationId = EdmModelHelper.GenerateComplexPropertyPathOperationId(Path, "Set");
45+
operation.OperationId = EdmModelHelper.GenerateComplexPropertyPathOperationId(Path, Context, "Set");
4646
}
4747

4848
// Summary and Description

src/Microsoft.OpenApi.OData.Reader/Operation/ComplexPropertyUpdateOperationHandler.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,7 @@ protected override void SetBasicInfo(OpenApiOperation operation)
4040
// OperationId
4141
if (Context.Settings.EnableOperationId)
4242
{
43-
operation.OperationId = EdmModelHelper.GenerateComplexPropertyPathOperationId(Path, "Update");
43+
operation.OperationId = EdmModelHelper.GenerateComplexPropertyPathOperationId(Path, Context, "Update");
4444
}
4545
}
4646

src/Microsoft.OpenApi.OData.Reader/Operation/DollarCountGetOperationHandler.cs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -128,17 +128,17 @@ protected override void SetBasicInfo(OpenApiOperation operation)
128128
}
129129
else if (SecondLastSegment is ODataNavigationPropertySegment)
130130
{
131-
var navPropOpId = string.Join(".", EdmModelHelper.RetrieveNavigationPropertyPathsOperationIdSegments(Path));
131+
var navPropOpId = string.Join(".", EdmModelHelper.RetrieveNavigationPropertyPathsOperationIdSegments(Path, Context));
132132
operation.OperationId = $"{navPropOpId}.GetCount-{Path.GetPathHash(Context.Settings)}";
133133
}
134134
else if (SecondLastSegment is ODataTypeCastSegment odataTypeCastSegment)
135135
{
136136
IEdmNamedElement targetStructuredType = odataTypeCastSegment.StructuredType as IEdmNamedElement;
137-
operation.OperationId = $"{EdmModelHelper.GenerateODataTypeCastPathOperationIdPrefix(Path, false)}.GetCount.As{Utils.UpperFirstChar(targetStructuredType.Name)}-{Path.GetPathHash(Context.Settings)}";
137+
operation.OperationId = $"{EdmModelHelper.GenerateODataTypeCastPathOperationIdPrefix(Path, Context, false)}.GetCount.As{Utils.UpperFirstChar(targetStructuredType.Name)}-{Path.GetPathHash(Context.Settings)}";
138138
}
139139
else if (SecondLastSegment is ODataComplexPropertySegment)
140140
{
141-
operation.OperationId = $"{EdmModelHelper.GenerateComplexPropertyPathOperationId(Path)}.GetCount-{Path.GetPathHash(Context.Settings)}";
141+
operation.OperationId = $"{EdmModelHelper.GenerateComplexPropertyPathOperationId(Path, Context)}.GetCount-{Path.GetPathHash(Context.Settings)}";
142142
}
143143
}
144144

src/Microsoft.OpenApi.OData.Reader/Operation/NavigationPropertyOperationHandler.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -107,7 +107,7 @@ protected override void SetExtensions(OpenApiOperation operation)
107107

108108
internal string GetOperationId(string prefix = null)
109109
{
110-
return EdmModelHelper.GenerateNavigationPropertyPathOperationId(Path, prefix);
110+
return EdmModelHelper.GenerateNavigationPropertyPathOperationId(Path, Context, prefix);
111111
}
112112

113113
/// <inheritdoc/>

src/Microsoft.OpenApi.OData.Reader/Operation/ODataTypeCastGetOperationHandler.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -188,7 +188,7 @@ protected override void SetBasicInfo(OpenApiOperation operation)
188188

189189
// OperationId
190190
if (Context.Settings.EnableOperationId)
191-
operation.OperationId = EdmModelHelper.GenerateODataTypeCastPathOperationIdPrefix(Path) + $".As{Utils.UpperFirstChar(TargetSchemaElement.Name)}";
191+
operation.OperationId = EdmModelHelper.GenerateODataTypeCastPathOperationIdPrefix(Path, Context) + $".As{Utils.UpperFirstChar(TargetSchemaElement.Name)}";
192192

193193
base.SetBasicInfo(operation);
194194
}

test/Microsoft.OpenAPI.OData.Reader.Tests/Operation/NavigationPropertyGetOperationHandlerTests.cs

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -131,6 +131,70 @@ public void CreateNavigationGetOperationViaComposableFunctionReturnsCorrectOpera
131131
Assert.Contains(operation.Parameters, x => x.Name == "path");
132132
}
133133

134+
[Fact]
135+
public void CreateNavigationGetOperationViaOverloadedComposableFunctionReturnsCorrectOperation()
136+
{
137+
// Arrange
138+
IEdmModel model = EdmModelHelper.GraphBetaModel;
139+
ODataContext context = new(model, new OpenApiConvertSettings()
140+
{
141+
EnableOperationId = true
142+
});
143+
144+
IEdmEntitySet drives = model.EntityContainer.FindEntitySet("drives");
145+
IEdmEntityType drive = model.SchemaElements.OfType<IEdmEntityType>().First(c => c.Name == "drive");
146+
IEdmNavigationProperty items = drive.DeclaredNavigationProperties().First(c => c.Name == "items");
147+
IEdmEntityType driveItem = model.SchemaElements.OfType<IEdmEntityType>().First(c => c.Name == "driveItem");
148+
IEdmNavigationProperty workbook = driveItem.DeclaredNavigationProperties().First(c => c.Name == "workbook");
149+
IEdmEntityType workbookEntity = model.SchemaElements.OfType<IEdmEntityType>().First(c => c.Name == "workbook");
150+
IEdmNavigationProperty worksheets = workbookEntity.DeclaredNavigationProperties().First(c => c.Name == "worksheets");
151+
IEdmEntityType workbookWorksheet = model.SchemaElements.OfType<IEdmEntityType>().First(c => c.Name == "workbookWorksheet");
152+
IEdmOperation usedRangeWithParams = model.SchemaElements.OfType<IEdmOperation>().First(f => f.Name == "usedRange" && f.Parameters.Any(x => x.Name.Equals("valuesOnly")));
153+
IEdmOperation usedRange = model.SchemaElements.OfType<IEdmOperation>().First(f => f.Name == "usedRange" && f.Parameters.Count() == 1);
154+
IEdmEntityType workbookRange = model.SchemaElements.OfType<IEdmEntityType>().First(c => c.Name == "workbookRange");
155+
IEdmNavigationProperty format = workbookRange.DeclaredNavigationProperties().First(c => c.Name == "format");
156+
157+
158+
ODataPath path1 = new(new ODataNavigationSourceSegment(drives),
159+
new ODataKeySegment(drive),
160+
new ODataNavigationPropertySegment(items),
161+
new ODataKeySegment(driveItem),
162+
new ODataNavigationPropertySegment(workbook),
163+
new ODataNavigationPropertySegment(worksheets),
164+
new ODataKeySegment(workbookWorksheet),
165+
new ODataOperationSegment(usedRangeWithParams),
166+
new ODataNavigationPropertySegment(format));
167+
168+
ODataPath path2 = new(new ODataNavigationSourceSegment(drives),
169+
new ODataKeySegment(drive),
170+
new ODataNavigationPropertySegment(items),
171+
new ODataKeySegment(driveItem),
172+
new ODataNavigationPropertySegment(workbook),
173+
new ODataNavigationPropertySegment(worksheets),
174+
new ODataKeySegment(workbookWorksheet),
175+
new ODataOperationSegment(usedRange),
176+
new ODataNavigationPropertySegment(format));
177+
178+
// Act
179+
var operation1 = _operationHandler.CreateOperation(context, path1);
180+
var operation2 = _operationHandler.CreateOperation(context, path2);
181+
182+
// Assert
183+
Assert.NotNull(operation1);
184+
Assert.NotNull(operation2);
185+
186+
Assert.Equal("drives.items.workbook.worksheets.usedRange.GetFormat-206d", operation1.OperationId);
187+
Assert.Equal("drives.items.workbook.worksheets.usedRange.GetFormat-ec2c", operation2.OperationId);
188+
189+
Assert.NotNull(operation1.Parameters);
190+
Assert.Equal(6, operation1.Parameters.Count);
191+
Assert.Contains(operation1.Parameters, x => x.Name == "valuesOnly");
192+
193+
Assert.NotNull(operation2.Parameters);
194+
Assert.Equal(5, operation2.Parameters.Count);
195+
Assert.DoesNotContain(operation2.Parameters, x => x.Name == "valuesOnly");
196+
}
197+
134198
[Theory]
135199
[InlineData(true)]
136200
[InlineData(false)]

0 commit comments

Comments
 (0)