Skip to content

issue #568: Error when function/action name contains keyword "On" #576

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
xuzhg merged 4 commits into from
Apr 27, 2022
Merged

issue #568: Error when function/action name contains keyword "On" #576

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f78a4e2
issue #568: Error when function/action name contains keyword "On"
xuzhg Apr 22, 2022
2dd6f26
Add the comments
xuzhg Apr 25, 2022
a7e8ada
Address the comments.
xuzhg Apr 26, 2022
7803404
Address the comments.
xuzhg Apr 27, 2022
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
57 changes: 37 additions & 20 deletions src/Microsoft.AspNetCore.OData/Routing/Conventions/OperationRoutingConvention.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,31 +65,39 @@ protected void ProcessOperations(ODataControllerActionContext context, IEdmEntit
return;
}

// OperationNameOnCollectionOfEntityType
string operationName = SplitActionName(actionName, out string cast, out bool isOnCollection);

bool isOnCollection = false;
IEdmEntityType castTypeFromActionName = null;
if (cast != null)

// Let's find the operations using the action first,
// If not founding, let's split the action name, search again

@julealgon julealgon Apr 25, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

These comments imply to me we have 2 strategies, and a fallback/priotity between them.

It would be nice to model these as 2 distinct functions and then have an explicit fallback using ?? or even a small chain of responsibility implementation (though that might be a bit overkill here).

@xuzhg xuzhg Apr 25, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Refactor codes, please take a look. Thanks.

IEdmOperation[] candidates = FindCandidates(context, actionName);

if (candidates.Length == 0)
{
if (cast.Length == 0)
{
// Early return for the following cases:
// - {OperationName}On
// - {OperationName}OnCollectionOf
return;
}
// OperationNameOnCollectionOfEntityType
string operationName = SplitActionName(actionName, out string cast, out isOnCollection);

castTypeFromActionName = entityType.FindTypeInInheritance(context.Model, cast, context.Options?.RouteOptions?.EnableActionNameCaseInsensitive == true) as IEdmEntityType;
if (castTypeFromActionName == null)
castTypeFromActionName = null;
if (cast != null)

@corranrogue9 corranrogue9 Apr 22, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do I understand correctly that these early returns are an optimization and not actually involved with addressing the bug? In other words, wouldn't it make sense to factor the code this way:

string operationName = SplitActionName(actionName, out string cast, out isOnCollection);
castTypeFromActionName = null;
if (cast != null)
{
  if (cast.Length == 0)
  {
    return;
  }

  castTypeFromActionName = entityType.FindTypeInInheritance(context.Model, cast, context.Options?.RouteOptions?.EnableActionNameCaseInsensitive == true) as IEdmEntityType;
  if (castTypeFromActionName == null)
  {
    return;
  }
}

IEdmOperation[] candidates = FindCandidates(context, actionName);
if (candidates.Length == 0)
{
  candidates = FindCandidates(context, operationName);
}

@julealgon julealgon Apr 25, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do I understand correctly that these early returns are an optimization and not actually involved with addressing the bug?

If that's really the case, it would be nice to avoid making this change in the same PR IMHO. Or at the very least, the change should be done in a separate commit in the branch.

{
return;
if (cast.Length == 0)
{
// Early return for the following cases:
// - {OperationName}On
// - {OperationName}OnCollectionOf
return;
}

castTypeFromActionName = entityType.FindTypeInInheritance(context.Model, cast, context.Options?.RouteOptions?.EnableActionNameCaseInsensitive == true) as IEdmEntityType;
if (castTypeFromActionName == null)
{
return;
}
}

candidates = FindCandidates(context, operationName);
}

// TODO: refactor here
// If we have multiple same function defined, we should match the best one?
StringComparison actionNameComparison = context.Options?.RouteOptions?.EnableActionNameCaseInsensitive == true ? StringComparison.OrdinalIgnoreCase : StringComparison.Ordinal;
IEnumerable<IEdmOperation> candidates = context.Model.SchemaElements.OfType<IEdmOperation>().Where(f => f.IsBound && f.Name.Equals(operationName, actionNameComparison));
foreach (IEdmOperation edmOperation in candidates)
{
IEdmOperationParameter bindingParameter = edmOperation.Parameters.FirstOrDefault();
Expand Down Expand Up @@ -172,6 +180,15 @@ protected void ProcessOperations(ODataControllerActionContext context, IEdmEntit
}
}

private static IEdmOperation[] FindCandidates(ODataControllerActionContext context, string operationName)
{
// TODO: refactor here
// If we have multiple same function defined, we should match the best one?

StringComparison actionNameComparison = context.Options?.RouteOptions?.EnableActionNameCaseInsensitive == true ? StringComparison.OrdinalIgnoreCase : StringComparison.Ordinal;
return context.Model.SchemaElements.OfType<IEdmOperation>().Where(f => f.IsBound && f.Name.Equals(operationName, actionNameComparison)).ToArray();

@corranrogue9 corranrogue9 Apr 22, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why .ToArray()? The code was already running without it, and I don't see how adding it improves anything

@xuzhg xuzhg Apr 23, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

we will do use it:

  1. check the count
  2. iterator
    So, it could be an improvement to call toArray().

@xuzhg xuzhg Apr 25, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

#Resolved.

@julealgon julealgon Apr 25, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Long lines like this are really hard to read.

I'd suggest reformatting like:

Suggested change
return context.Model.SchemaElements.OfType<IEdmOperation>().Where(f => f.IsBound && f.Name.Equals(operationName, actionNameComparison)).ToArray();
return context.Model.SchemaElements
.OfType<IEdmOperation>()
.Where(f => f.IsBound && f.Name.Equals(operationName, actionNameComparison))
.ToArray();

@xuzhg xuzhg Apr 25, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

#Resolved.

}

/// <summary>
/// Split the action based on supporting pattern.
/// </summary>
Expand All @@ -190,7 +207,7 @@ internal static string SplitActionName(string actionName, out string cast, out b
cast = null;
isOnCollection = false;
string operation;
int index = actionName.IndexOf("OnCollectionOf", StringComparison.Ordinal);
int index = actionName.LastIndexOf("OnCollectionOf", StringComparison.Ordinal);

@corranrogue9 corranrogue9 Apr 22, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do these comparisons also need to be updated to honor context.Options?.RouteOptions?.EnableActionNameCaseInsensitive?

@xuzhg xuzhg Apr 27, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

#Resolved.

if (index > 0)
{
operation = actionName.Substring(0, index);
Expand All @@ -199,7 +216,7 @@ internal static string SplitActionName(string actionName, out string cast, out b
return operation;
}

index = actionName.IndexOf("On", StringComparison.Ordinal);
index = actionName.LastIndexOf("On", StringComparison.Ordinal);
if (index > 0)
{
operation = actionName.Substring(0, index);
Expand Down
62 changes: 60 additions & 2 deletions test/Microsoft.AspNetCore.OData.Tests/Routing/Conventions/FunctionRoutingConventionTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,11 +154,47 @@ public static TheoryDataSet<Type, string, string[]> FunctionRoutingConventionTes
"/Customers/NS.GetWholeSalary(minSalary={minSalary},maxSalary={maxSalary},aveSalary={aveSalary})",
"/Customers/GetWholeSalary(minSalary={minSalary},maxSalary={maxSalary},aveSalary={aveSalary})"
}
},
{
typeof(CustomersController),
"GetStatusOnLineOfflineUser",
new[]
{
"/Customers/NS.GetStatusOnLineOfflineUser()",
"/Customers/GetStatusOnLineOfflineUser()"
}
},
{
typeof(CustomersController),
"GetStatusOnLineOfflineUser",
new[]
{
"/Customers/NS.GetStatusOnLineOfflineUser()",
"/Customers/GetStatusOnLineOfflineUser()"
}
},
{
typeof(CustomersController),
"StatusLineOfflineUserOn",
new[]
{
"/Customers/NS.StatusLineOfflineUserOn()",
"/Customers/StatusLineOfflineUserOn()"
}
},
{
typeof(CustomersController),
"GetStatusOnLineOfflineUserOnVipCustomer",
new[]
{
"/Customers/NS.VipCustomer/NS.GetStatusOnLineOfflineUser(param={param})",
"/Customers/NS.VipCustomer/GetStatusOnLineOfflineUser(param={param})"
}
}
};
}
}
}

public static TheoryDataSet<Type, string, string[]> FunctionRoutingConventionCaseInsensitiveTestData
{
get
Expand Down Expand Up @@ -448,6 +484,19 @@ private static IEdmModel GetEdmModel()
getSalaray.AddOptionalParameter("aveSalary", intType, "129");
model.AddElement(getSalaray);

EdmFunction f = new EdmFunction("NS", "GetStatusOnLineOfflineUser", intType, isBound: true, entitySetPathExpression: null, isComposable: false);
f.AddParameter("entityset", new EdmCollectionTypeReference(new EdmCollectionType(new EdmEntityTypeReference(customer, false))));
model.AddElement(f);

EdmFunction f2 = new EdmFunction("NS", "StatusLineOfflineUserOn", intType, isBound: true, entitySetPathExpression: null, isComposable: false);
f2.AddParameter("entityset", new EdmCollectionTypeReference(new EdmCollectionType(new EdmEntityTypeReference(customer, false))));
model.AddElement(f2);

EdmFunction f3 = new EdmFunction("NS", "GetStatusOnLineOfflineUser", intType, isBound: true, entitySetPathExpression: null, isComposable: false);
f3.AddParameter("entityset", new EdmCollectionTypeReference(new EdmCollectionType(new EdmEntityTypeReference(vipCustomer, false))));
f3.AddParameter("param", intType);
model.AddElement(f3);

EdmEntityContainer container = new EdmEntityContainer("NS", "Default");
container.AddEntitySet("Customers", customer);
container.AddEntitySet("CustomersCaseInsensitive", customer);
Expand All @@ -461,6 +510,15 @@ private class CustomersController
public void Get()
{ }

[HttpGet]
public void GetStatusOnLineOfflineUser() { }

[HttpGet]
public void GetStatusOnLineOfflineUserOnVipCustomer(int param) { }

[HttpGet]
public void StatusLineOfflineUserOn() { }

[HttpGet]
public void IsBaseUpgraded(int key, CancellationToken cancellation)
{ }
Expand Down