Conversation
| isOnCollection = false; | ||
| string operation; | ||
| int index = actionName.IndexOf("OnCollectionOf", StringComparison.Ordinal); | ||
| int index = actionName.LastIndexOf("OnCollectionOf", StringComparison.Ordinal); |
There was a problem hiding this comment.
Do these comparisons also need to be updated to honor context.Options?.RouteOptions?.EnableActionNameCaseInsensitive?
| castTypeFromActionName = entityType.FindTypeInInheritance(context.Model, cast, context.Options?.RouteOptions?.EnableActionNameCaseInsensitive == true) as IEdmEntityType; | ||
| if (castTypeFromActionName == null) | ||
| castTypeFromActionName = null; | ||
| if (cast != null) |
There was a problem hiding this comment.
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);
}
There was a problem hiding this comment.
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.
| // 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(); |
There was a problem hiding this comment.
Why .ToArray()? The code was already running without it, and I don't see how adding it improves anything
There was a problem hiding this comment.
we will do use it:
- check the count
- iterator
So, it could be an improvement to call toArray().
| // Let's find the operations using the action first, | ||
| // If not founding, let's split the action name, search again |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Refactor codes, please take a look. Thanks.
| // 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(); |
There was a problem hiding this comment.
Long lines like this are really hard to read.
I'd suggest reformatting like:
| 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(); |
| { | ||
| return; | ||
| } | ||
| if (candidates == null || candidates.Length == 0) |
There was a problem hiding this comment.
Can we make it so that none of the FindCandidates overloads ever return null, and instead make it return an empty array (for instance, using Array.Empty<T>)?
Handling null collections like this is usually ill-advised unless there is a very clear semantic value difference between null and empty (which I don't think is the case here).
We could then simplify this back to:
| if (candidates == null || candidates.Length == 0) | |
| if (candidates.Length == 0) |
3 participants
issue #568
Describe the bug
when function/action name contains keyword "On", the operation convention can't correct parse the action name.
/odata/SEChat/SE.GetStatusOnLineOfflineUser => error
/odata/SEChat/SE.StatusLineOfflineUserOn => error
What is in the PR: