Skip to content

Adds action/function suffix to tag names for action/function operations #642

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

Merged
merged 9 commits into from
Jan 20, 2025
Merged

Adds action/function suffix to tag names for action/function operations #642

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
8646818
Updates tag names for actions/functions operations (#586)
irvinesunday Oct 1, 2024
284ad67
Adds action/function suffix to tag names for action/function operations
Jan 17, 2025
b808232
Update src/Microsoft.OpenApi.OData.Reader/Operation/EdmOperationOpera…
timayabi2020 Jan 20, 2025
e6cc1f3
Fixed method name
Jan 20, 2025
5dd584b
Reverted back changes and added release notes
Jan 20, 2025
e74cc84
Updated expected test result
Jan 20, 2025
d5beb7c
Rectified expected test result
Jan 20, 2025
3b4b0d1
Rectified expected test result
Jan 20, 2025
494f302
Updated operation handler to make use of EDMOperation object instead …
Jan 20, 2025
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
6 changes: 3 additions & 3 deletions src/Microsoft.OpenApi.OData.Reader/Microsoft.OpenAPI.OData.Reader.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,13 @@
<TargetFrameworks>netstandard2.0</TargetFrameworks>
<PackageId>Microsoft.OpenApi.OData</PackageId>
<SignAssembly>true</SignAssembly>
<Version>1.7.1</Version>
<Version>1.7.2</Version>
<Description>This package contains the codes you need to convert OData CSDL to Open API Document of Model.</Description>
<Copyright>© Microsoft Corporation. All rights reserved.</Copyright>
<PackageTags>Microsoft OpenApi OData EDM</PackageTags>
<RepositoryUrl>https://github.com/Microsoft/OpenAPI.NET.OData</RepositoryUrl>
<PackageReleaseNotes>
- Further fix for generating unique operation ids for navigation property paths with composable overloaded functions #596
- Adds action/function suffix to tag names for actions/functions operations in #642
</PackageReleaseNotes>
<AssemblyName>Microsoft.OpenApi.OData.Reader</AssemblyName>
<AssemblyOriginatorKeyFile>..\..\tool\Microsoft.OpenApi.OData.snk</AssemblyOriginatorKeyFile>
Expand Down Expand Up @@ -66,4 +66,4 @@
</EmbeddedResource>
</ItemGroup>

</Project>
</Project>
50 changes: 26 additions & 24 deletions src/Microsoft.OpenApi.OData.Reader/Operation/EdmOperationOperationHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@
}

/// <inheritdoc/>
protected override void SetBasicInfo(OpenApiOperation operation)

Check warning on line 67 in src/Microsoft.OpenApi.OData.Reader/Operation/EdmOperationOperationHandler.cs

View workflow job for this annotation

GitHub Actions / Build 

Refactor this method to reduce its Cognitive Complexity from 25 to the 15 allowed. (https://rules.sonarsource.com/csharp/RSPEC-3776)
{
// Summary
operation.Summary = "Invoke " + (EdmOperation.IsAction() ? "action " : "function ") + EdmOperation.Name;
Expand All @@ -81,19 +81,18 @@
// duplicates in entity vs entityset functions/actions

List<string> identifiers = new();
string pathHash = string.Empty;
foreach (ODataSegment segment in Path.Segments)
{
if (segment is ODataKeySegment keySegment)
{
if (!keySegment.IsAlternateKey)
if (!keySegment.IsAlternateKey)
{
identifiers.Add(segment.EntityType.Name);
continue;
}

// We'll consider alternate keys in the operation id to eliminate potential duplicates with operation id of primary path
if (segment == Path.Segments.Last())

Check warning on line 95 in src/Microsoft.OpenApi.OData.Reader/Operation/EdmOperationOperationHandler.cs

View workflow job for this annotation

GitHub Actions / Build 

Indexing at Count-1 should be used instead of the "Enumerable" extension method "Last" (https://rules.sonarsource.com/csharp/RSPEC-6608)
{
identifiers.Add("By" + string.Join("", keySegment.Identifier.Split(',').Select(static x => Utils.UpperFirstChar(x))));
}
Expand All @@ -102,18 +101,6 @@
identifiers.Add(keySegment.Identifier);
}
}
else if (segment is ODataOperationSegment opSegment)
{
if (opSegment.Operation is IEdmFunction function && Context.Model.IsOperationOverload(function))
{
// Hash the segment to avoid duplicate operationIds
pathHash = string.IsNullOrEmpty(pathHash)
? opSegment.GetPathHash(Context.Settings)
: (pathHash + opSegment.GetPathHash(Context.Settings)).GetHashSHA256().Substring(0, 4);
}

identifiers.Add(segment.Identifier);
}
else
{
identifiers.Add(segment.Identifier);
Expand All @@ -122,13 +109,21 @@

string operationId = string.Join(".", identifiers);

if (!string.IsNullOrEmpty(pathHash))
if (EdmOperation.IsAction())
{
operation.OperationId = operationId + "-" + pathHash;
operation.OperationId = operationId;
}
else
{
operation.OperationId = operationId;
if (Path.LastSegment is ODataOperationSegment operationSegment &&
Context.Model.IsOperationOverload(operationSegment.Operation))
{
operation.OperationId = operationId + "-" + Path.LastSegment.GetPathHash(Context.Settings);
}
else
{
operation.OperationId = operationId;
}
}
}

Expand All @@ -152,29 +147,36 @@
}

/// <summary>
/// Genrates the tag name for the operation.
/// Genrates the tag name for the operation. Adds Action or Function name to the tag name if the operation is an action or function.
/// </summary>
/// <param name="tagName">The generated tag name.</param>
/// <param name="skip">The number of segments to skip.</param>
private void GenerateTagName(out string tagName, int skip = 1)
{
{
var targetSegment = Path.Segments.Reverse().Skip(skip).FirstOrDefault();

switch (targetSegment)
{
case ODataNavigationPropertySegment:
tagName = EdmModelHelper.GenerateNavigationPropertyPathTagName(Path, Context);
break;
case ODataOperationSegment:
case ODataOperationImportSegment:
// Previous segmment could be a navigation property or a navigation source segment
case ODataKeySegment:
skip += 1;
GenerateTagName(out tagName, skip);
break;
// ODataNavigationSourceSegment
default:
tagName = NavigationSource.Name + "." + NavigationSource.EntityType().Name;
if (EdmOperation.IsAction())
{
tagName += ".Actions";
}
else if (EdmOperation.IsFunction())
{
tagName += ".Functions";
}

break;
}
}
Expand All @@ -192,7 +194,7 @@
}

/// <inheritdoc/>
protected override void SetResponses(OpenApiOperation operation)
protected override void SetResponses(OpenApiOperation operation)
{
operation.Responses = Context.CreateResponses(EdmOperation);
base.SetResponses(operation);
Expand Down Expand Up @@ -228,7 +230,7 @@
}
}

private void AppendSystemQueryOptions(IEdmFunction function, OpenApiOperation operation)

Check warning on line 233 in src/Microsoft.OpenApi.OData.Reader/Operation/EdmOperationOperationHandler.cs

View workflow job for this annotation

GitHub Actions / Build 

Refactor this method to reduce its Cognitive Complexity from 22 to the 15 allowed. (https://rules.sonarsource.com/csharp/RSPEC-3776)
{
if (function.ReturnType.IsCollection())
{
Expand Down Expand Up @@ -292,10 +294,10 @@
{
LinkRelKey key = EdmOperation.IsAction() ? LinkRelKey.Action : LinkRelKey.Function;
Context.Settings.CustomHttpMethodLinkRelMapping.TryGetValue(key, out string linkRelValue);
CustomLinkRel = linkRelValue;
CustomLinkRel = linkRelValue;
}
}

/// <inheritdoc/>
protected override void SetExternalDocs(OpenApiOperation operation)
{
Expand Down
4 changes: 2 additions & 2 deletions test/Microsoft.OpenAPI.OData.Reader.Tests/Operation/EdmActionOperationHandlerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ public void CreateOperationForEdmActionReturnsCorrectOperation()
Assert.Equal("Details of the shared trip.", operation.Description);
Assert.NotNull(operation.Tags);
var tag = Assert.Single(operation.Tags);
Assert.Equal("People.Person", tag.Name);
Assert.Equal("People.Person.Actions", tag.Name);

Assert.NotNull(operation.Parameters);
Assert.Single(operation.Parameters);
Expand Down Expand Up @@ -79,7 +79,7 @@ public void CreateOperationForEdmActionReturnsCorrectOperationHierarchicalClass(
Assert.Equal($"Invoke action {actionName}", operation.Summary);
Assert.NotNull(operation.Tags);
var tag = Assert.Single(operation.Tags);
Assert.Equal($"{entitySetName}.AccountApiModel", tag.Name);
Assert.Equal($"{entitySetName}.AccountApiModel.Actions", tag.Name);

Assert.NotNull(operation.Parameters);
Assert.Single(operation.Parameters);
Expand Down
62 changes: 31 additions & 31 deletions test/Microsoft.OpenAPI.OData.Reader.Tests/Operation/EdmFunctionOperationHandlerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ public void CreateOperationForEdmFunctionReturnsCorrectOperation(bool useHTTPSta
Assert.Equal("Invoke function GetFavoriteAirline", operation.Summary);
Assert.NotNull(operation.Tags);
var tag = Assert.Single(operation.Tags);
Assert.Equal("People.Person", tag.Name);
Assert.Equal("People.Person.Functions", tag.Name);

Assert.NotNull(operation.Parameters);
Assert.Single(operation.Parameters);
Expand Down Expand Up @@ -138,7 +138,7 @@ public void CreateOperationForEdmFunctionReturnsCorrectOperationHierarchicalClas
Assert.Equal("Collection of contract attachments.", operation.Description);
Assert.NotNull(operation.Tags);
var tag = Assert.Single(operation.Tags);
Assert.Equal($"{entitySetName}.AccountApiModel", tag.Name);
Assert.Equal($"{entitySetName}.AccountApiModel.Functions", tag.Name);

Assert.NotNull(operation.Parameters);
Assert.Equal(6, operation.Parameters.Count); // id, top, skip, count, search, filter
Expand Down Expand Up @@ -378,10 +378,10 @@ public void CreateOperationForComposableOverloadEdmFunctionReturnsCorrectOperati

if (enableOperationId)
{
Assert.Equal("Customers.Customer.MyFunction1.MyFunction2-c53d", operation1.OperationId);
Assert.Equal("Customers.Customer.MyFunction1.MyFunction2-4d93", operation2.OperationId);
Assert.Equal("Customers.Customer.MyFunction1.MyFunction2-a2b2", operation3.OperationId);
Assert.Equal("Customers.Customer.MyFunction1.MyFunction2-7bea", operation4.OperationId);
Assert.Equal("Customers.Customer.MyFunction1.MyFunction2-6b6d", operation1.OperationId);
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this change is resulting in duplicate operation ids as captured in the validation at microsoftgraph/msgraph-metadata#752

Ideally, the logic here should results in all 4 of the operationIds in the tests being unique

Copy link
Member

Choose a reason for hiding this comment

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

@timayabi2020 can you prioritize a fix for this regression please?
Thank Andrew for catching this and sorry for not seeing it during the review

Copy link
Member

Choose a reason for hiding this comment

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

No worries. Created #645 to resolve this one.

Assert.Equal("Customers.Customer.MyFunction1.MyFunction2-2636", operation2.OperationId);
Assert.Equal("Customers.Customer.MyFunction1.MyFunction2-6b6d", operation3.OperationId);
Assert.Equal("Customers.Customer.MyFunction1.MyFunction2-2636", operation4.OperationId);
}
else
{
Expand Down Expand Up @@ -575,35 +575,35 @@ public void CreateOperationForFunctionWithDateTimeParametersReturnsCorrectPathIt
}

[Fact]
public void CreateFunctionOperationWithAlternateKeyReturnsCorrectOperationId()
{
// Arrange
public void CreateFunctionOperationWithAlternateKeyReturnsCorrectOperationId()
{
// Arrange
IEdmModel model = EdmModelHelper.GraphBetaModel;
ODataContext context = new(model, new OpenApiConvertSettings()
ODataContext context = new(model, new OpenApiConvertSettings()
{
EnableOperationId = true
EnableOperationId = true
});

IEdmSingleton singleton = model.EntityContainer.FindSingleton("communications");
IEdmEntityType entityType = model.SchemaElements.OfType<IEdmEntityType>().First(c => c.Name == "cloudCommunications");
IEdmNavigationProperty navProp = entityType.DeclaredNavigationProperties().First(c => c.Name == "onlineMeetings");
IEdmOperation action = model.SchemaElements.OfType<IEdmOperation>().First(f => f.Name == "sendVirtualAppointmentReminderSms");
IDictionary<string, string> keyMappings = new Dictionary<string, string> { { "joinWebUrl", "joinWebUrl" } };
ODataPath path = new(new ODataNavigationSourceSegment(singleton),
new ODataNavigationPropertySegment(navProp),
new ODataKeySegment(entityType, keyMappings)
{
IsAlternateKey = true
},
new ODataOperationSegment(action));
IEdmSingleton singleton = model.EntityContainer.FindSingleton("communications");
IEdmEntityType entityType = model.SchemaElements.OfType<IEdmEntityType>().First(c => c.Name == "cloudCommunications");
IEdmNavigationProperty navProp = entityType.DeclaredNavigationProperties().First(c => c.Name == "onlineMeetings");
IEdmOperation action = model.SchemaElements.OfType<IEdmOperation>().First(f => f.Name == "sendVirtualAppointmentReminderSms");
IDictionary<string, string> keyMappings = new Dictionary<string, string> { { "joinWebUrl", "joinWebUrl" } };

ODataPath path = new(new ODataNavigationSourceSegment(singleton),
new ODataNavigationPropertySegment(navProp),
new ODataKeySegment(entityType, keyMappings)
{
IsAlternateKey = true
},
new ODataOperationSegment(action));

// Act
var operation = _operationHandler.CreateOperation(context, path);
// Assert
Assert.NotNull(operation);
Assert.Equal("communications.onlineMeetings.joinWebUrl.sendVirtualAppointmentReminderSms", operation.OperationId);
var operation = _operationHandler.CreateOperation(context, path);

// Assert
Assert.NotNull(operation);
Assert.Equal("communications.onlineMeetings.joinWebUrl.sendVirtualAppointmentReminderSms", operation.OperationId);
}
}
}
}
12 changes: 10 additions & 2 deletions test/Microsoft.OpenAPI.OData.Reader.Tests/Resources/Multiple.Schema.OpenApi.V2.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -591,7 +591,7 @@
"/Documents({Id})/Default.Upload": {
"post": {
"tags": [
"Documents.DocumentDto"
"Documents.DocumentDto.Actions"
],
"summary": "Invoke action Upload",
"operationId": "Documents.DocumentDto.Upload",
Expand Down Expand Up @@ -3519,7 +3519,7 @@
"/Tasks({Id})/Default.Upload": {
"post": {
"tags": [
"Tasks.DocumentDto"
"Tasks.DocumentDto.Actions"
],
"summary": "Invoke action Upload",
"operationId": "Tasks.DocumentDto.Upload",
Expand Down Expand Up @@ -6275,6 +6275,10 @@
"name": "Documents.DocumentDto",
"x-ms-docs-toc-type": "page"
},
{
"name": "Documents.DocumentDto.Actions",
"x-ms-docs-toc-type": "container"
},
{
"name": "Documents.RevisionDto",
"x-ms-docs-toc-type": "page"
Expand Down Expand Up @@ -6315,6 +6319,10 @@
"name": "Tasks.DocumentDto",
"x-ms-docs-toc-type": "page"
},
{
"name": "Tasks.DocumentDto.Actions",
"x-ms-docs-toc-type": "container"
},
{
"name": "Tasks.RevisionDto",
"x-ms-docs-toc-type": "page"
Expand Down
8 changes: 6 additions & 2 deletions test/Microsoft.OpenAPI.OData.Reader.Tests/Resources/Multiple.Schema.OpenApi.V2.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -413,7 +413,7 @@ paths:
'/Documents({Id})/Default.Upload':
post:
tags:
- Documents.DocumentDto
- Documents.DocumentDto.Actions
summary: Invoke action Upload
operationId: Documents.DocumentDto.Upload
parameters:
Expand Down Expand Up @@ -2495,7 +2495,7 @@ paths:
'/Tasks({Id})/Default.Upload':
post:
tags:
- Tasks.DocumentDto
- Tasks.DocumentDto.Actions
summary: Invoke action Upload
operationId: Tasks.DocumentDto.Upload
parameters:
Expand Down Expand Up @@ -4545,6 +4545,8 @@ tags:
x-ms-docs-toc-type: page
- name: Documents.DocumentDto
x-ms-docs-toc-type: page
- name: Documents.DocumentDto.Actions
x-ms-docs-toc-type: container
- name: Documents.RevisionDto
x-ms-docs-toc-type: page
- name: Documents.DocumentTagRelDto
Expand All @@ -4565,6 +4567,8 @@ tags:
x-ms-docs-toc-type: page
- name: Tasks.DocumentDto
x-ms-docs-toc-type: page
- name: Tasks.DocumentDto.Actions
x-ms-docs-toc-type: container
- name: Tasks.RevisionDto
x-ms-docs-toc-type: page
- name: Tasks.DocumentTagRelDto
Expand Down
12 changes: 10 additions & 2 deletions test/Microsoft.OpenAPI.OData.Reader.Tests/Resources/Multiple.Schema.OpenApi.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -663,7 +663,7 @@
"description": "Provides operations to call the Upload method.",
"post": {
"tags": [
"Documents.DocumentDto"
"Documents.DocumentDto.Actions"
],
"summary": "Invoke action Upload",
"operationId": "Documents.DocumentDto.Upload",
Expand Down Expand Up @@ -3940,7 +3940,7 @@
"description": "Provides operations to call the Upload method.",
"post": {
"tags": [
"Tasks.DocumentDto"
"Tasks.DocumentDto.Actions"
],
"summary": "Invoke action Upload",
"operationId": "Tasks.DocumentDto.Upload",
Expand Down Expand Up @@ -7481,6 +7481,10 @@
"name": "Documents.DocumentDto",
"x-ms-docs-toc-type": "page"
},
{
"name": "Documents.DocumentDto.Actions",
"x-ms-docs-toc-type": "container"
},
{
"name": "Documents.RevisionDto",
"x-ms-docs-toc-type": "page"
Expand Down Expand Up @@ -7521,6 +7525,10 @@
"name": "Tasks.DocumentDto",
"x-ms-docs-toc-type": "page"
},
{
"name": "Tasks.DocumentDto.Actions",
"x-ms-docs-toc-type": "container"
},
{
"name": "Tasks.RevisionDto",
"x-ms-docs-toc-type": "page"
Expand Down
Loading
Loading