Skip to content
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.

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

Add function support (part 1) #424

Merged
merged 2 commits into from Jun 15, 2016

Conversation

Projects
None yet
4 participants
@chinadragon0515
Copy link
Contributor

chinadragon0515 commented Jun 6, 2016

Issues

*This pull request fixes issue #378

Description

User does not need additional controller for operation route.

Checklist (Uncheck if it is not completed)

  • [ x ] Test cases added
  • [ x ] Build and test with one-click build and test script passed

Additional work necessary

Will need to update document

{
internal static object ConvertValue(object oDataValue, Type expectedReturnType, ref IEdmTypeReference propertyType, ApiBase api)
{
var model = api.GetModelAsync().Result;

This comment has been minimized.

Copy link
@rayao

rayao Jun 6, 2016

Contributor

This is dangerous, for a truly async model pipeline this could hang forever.

This comment has been minimized.

Copy link
@chinadragon0515

chinadragon0515 Jun 6, 2016

Author Contributor

I change these method to async and use await, this should avoid deadlock, let me know if you have other suggestions on this part. I am new to c# :)

@chinadragon0515 chinadragon0515 force-pushed the chinadragon0515:operation branch 2 times, most recently from 40c4f03 to 2e65485 Jun 6, 2016

@chinadragon0515 chinadragon0515 force-pushed the chinadragon0515:operation branch from 2e65485 to 347d081 Jun 8, 2016

MethodInfo method = Api.GetType().GetMethod(segment.Function.Name, BindingFlags.Public | BindingFlags.Instance | BindingFlags.FlattenHierarchy);
var parameterArray = method.GetParameters();

var model = Api.GetModelAsync(cancellationToken).Result;

This comment has been minimized.

Copy link
@rayao

rayao Jun 8, 2016

Contributor

await

@chinadragon0515 chinadragon0515 force-pushed the chinadragon0515:operation branch 4 times, most recently from bd04fe0 to 9a65c98 Jun 12, 2016

@@ -42,6 +42,7 @@ public RestierQueryBuilder(ApiBase api, ODataPath path)
this.handlers[ODataSegmentKinds.EntitySet] = this.HandleEntitySetPathSegment;
this.handlers[ODataSegmentKinds.Singleton] = this.HandleSingletonPathSegment;
this.handlers[ODataSegmentKinds.UnboundFunction] = this.HandleUnboundFunctionPathSegment;
this.handlers[ODataSegmentKinds.Function] = this.HandleBoundFunctionPathSegment;

This comment has been minimized.

Copy link
@lewischeng-ms

lewischeng-ms Jun 14, 2016

Contributor

Better add a static readonly action in this class probably called NoOpHandler whose body contains nothing and set it to both bound and unbound function handlers to make the code less and clear.

This comment has been minimized.

Copy link
@chinadragon0515

chinadragon0515 Jun 15, 2016

Author Contributor

Updated

@@ -76,16 +80,59 @@ private ApiBase Api
throw new InvalidOperationException(Resources.ControllerRequiresPath);
}

var result = default(IQueryable);

This comment has been minimized.

Copy link
@lewischeng-ms

lewischeng-ms Jun 14, 2016

Contributor

Just IQueryable result; here.

This comment has been minimized.

Copy link
@chinadragon0515

chinadragon0515 Jun 15, 2016

Author Contributor

Updated


// If it is a unbound function, call Api function directly
ODataPathSegment lastSegment = path.Segments.LastOrDefault();
if (lastSegment != null && lastSegment.SegmentKind != ODataSegmentKinds.UnboundFunction)

This comment has been minimized.

Copy link
@lewischeng-ms

lewischeng-ms Jun 14, 2016

Contributor

Refactor the code in this method (from line 90 to line 133):

  • Too many condition lastSegment != null. Actually if it is null, no result will be created (though I even doubt if this case would happen). Thus either throw exception (if necessary) or just remove this condition.
  • Three if(s) (line 90, line 118 and line 128) can be merged to make the code much clearer. The main logic here has three parts: unbound, bound and the others. Suggest we write a switch statement (on lastSegment.SegmentKind) with three cases; extract line 92 to line 114 to a private function. Each case just needs to call two to three methods.

This comment has been minimized.

Copy link
@chinadragon0515

chinadragon0515 Jun 15, 2016

Author Contributor

ODataSegmentKinds cannot be used as case condition, and error is "A constant value is expected", I continue use if but extract logic into different method.

if (this.shouldWriteRawValue)
{
// Query options don't apply to $value.
return queryable;
}

ODataPath path = this.GetPath();

This comment has been minimized.

Copy link
@lewischeng-ms

lewischeng-ms Jun 14, 2016

Contributor

this.GetPath is expensive. Consider pass it through parameter.

This comment has been minimized.

Copy link
@chinadragon0515

chinadragon0515 Jun 15, 2016

Author Contributor

Updated.

@@ -525,6 +578,127 @@ private ODataPath GetPath()
return originalValues;
}

private async Task<IQueryable> ExecuteUnboundFunction(UnboundFunctionPathSegment segment,

This comment has been minimized.

Copy link
@lewischeng-ms

lewischeng-ms Jun 14, 2016

Contributor

The major part of ExecuteUnboundFunction and ExecuteBoundFunction is the same. The difference is how we handle the first parameter (the binding parameter). Consider extract the common logic and make it clear, e.g., GetMethod, ReadBindingParameter (optional for unbound), ReadParameters, ExecuteFunction (suggest renaming it to more precise InvokeMethod).

parameters.Add(bindingParameterValue.SingleOrDefault());
}

for (int i=1;i< parameterArray.Length;i++)

This comment has been minimized.

Copy link
@lewischeng-ms

lewischeng-ms Jun 14, 2016

Contributor

If we still keep this code, please fix the style.

This comment has been minimized.

Copy link
@chinadragon0515

chinadragon0515 Jun 15, 2016

Author Contributor

Fixed, and moved to a separate method.

var model = await Api.GetModelAsync(cancellationToken);

// Parameters of method and model is exactly mapped or there is parsing error
var parameters = new Collection<object>();

This comment has been minimized.

Copy link
@lewischeng-ms

lewischeng-ms Jun 14, 2016

Contributor

Consider using array directly to avoid future ToArray().

This comment has been minimized.

Copy link
@chinadragon0515

chinadragon0515 Jun 15, 2016

Author Contributor

Updated

private async Task<IQueryable> ExecuteFunction(MethodInfo method, Collection<object> parameters, CancellationToken cancellationToken)
{
object result = method.Invoke(Api, parameters.ToArray());
var model = await Api.GetModelAsync(cancellationToken);

This comment has been minimized.

Copy link
@lewischeng-ms

lewischeng-ms Jun 14, 2016

Contributor

Api.GetModelAsync is called multiple times during executing a function. Consider cache it to private member or pass through parameter.

Also for the returnType. Resolve it from the model takes time. Consider cache it as well.

This comment has been minimized.

Copy link
@chinadragon0515

chinadragon0515 Jun 15, 2016

Author Contributor

Updated.

object result = method.Invoke(Api, parameters.ToArray());
var model = await Api.GetModelAsync(cancellationToken);
IEdmTypeReference returnType = method.ReturnType.GetReturnTypeReference(model);
var asQueryableMethod = typeof(Queryable).GetMethod(

This comment has been minimized.

Copy link
@lewischeng-ms

lewischeng-ms Jun 14, 2016

Contributor

For these "constant" instances, we should only compute them once and put them into a static class just like Where, Select, etc.

This comment has been minimized.

Copy link
@chinadragon0515

chinadragon0515 Jun 15, 2016

Author Contributor

Updated

// For entity set, user can write as ICollection<> or IEnumerable<> or array as method parameters
if (parameterArray[0].ParameterType.IsArray)
{
var toListMethodInfo = typeof(Enumerable).GetMethod("ToArray").MakeGenericMethod(elementClrType);

This comment has been minimized.

Copy link
@lewischeng-ms

lewischeng-ms Jun 14, 2016

Contributor

Also for ToArray.

This comment has been minimized.

Copy link
@chinadragon0515

chinadragon0515 Jun 15, 2016

Author Contributor

Updated, also in DeserializerHelper method

}
else if (parameterArray[0].ParameterType.FindGenericType(typeof (ICollection<>)) != null)
{
var toListMethodInfo = typeof (Enumerable).GetMethod("ToList").MakeGenericMethod(elementClrType);

This comment has been minimized.

Copy link
@lewischeng-ms

lewischeng-ms Jun 14, 2016

Contributor

Also for ToList.

This comment has been minimized.

Copy link
@chinadragon0515

chinadragon0515 Jun 15, 2016

Author Contributor

Updated, also in DeserializerHelper method

if (returnType.IsCollection())
{
if (result == null)
{

This comment has been minimized.

Copy link
@lewischeng-ms

lewischeng-ms Jun 14, 2016

Contributor

Consider extracting the body into a static method CreateEmptyQueryable(elementType) (please note the spell).

This comment has been minimized.

Copy link
@chinadragon0515

chinadragon0515 Jun 15, 2016

Author Contributor

Added

return emptyQuerable;
}

var enumerableType = result.GetType().FindGenericType(typeof(IEnumerable<>));

This comment has been minimized.

Copy link
@lewischeng-ms

lewischeng-ms Jun 14, 2016

Contributor

If the result is a IQueryable, what's the consequence of calling AsQueryable.

This comment has been minimized.

Copy link
@chinadragon0515

chinadragon0515 Jun 15, 2016

Author Contributor

Per discussion, no issues.


// cannot return new[] { result }.AsQueryable(); as need to return in its own type but not object type
var objectQueryable = new[] { result }.AsQueryable();
var castMethodInfo = typeof(Enumerable).GetMethod("Cast").MakeGenericMethod(method.ReturnType);

This comment has been minimized.

Copy link
@lewischeng-ms

lewischeng-ms Jun 14, 2016

Contributor

Also for Cast.

This comment has been minimized.

Copy link
@chinadragon0515

chinadragon0515 Jun 15, 2016

Author Contributor

Updated, also in DeserializerHelper method

}

// cannot return new[] { result }.AsQueryable(); as need to return in its own type but not object type
var objectQueryable = new[] { result }.AsQueryable();

This comment has been minimized.

Copy link
@lewischeng-ms

lewischeng-ms Jun 14, 2016

Contributor

Hasn't quite got the point here...if control goes here, what does result look like? These code looks to me equivalent to new[] {result}.AsQueryable().Cast<object>().AsQueryable()...

This comment has been minimized.

Copy link
@chinadragon0515

chinadragon0515 Jun 15, 2016

Author Contributor

It is new[] {result}.AsQueryable().Cast< Type >().AsQueryable(), we need such convert or apply query will can not find element for object from Model.

/// </summary>
internal static class DeserializationHelpers
{
internal static async Task<object> ConvertValueAsync(object oDataValue, Type expectedReturnType, IEdmTypeReference propertyType, ApiBase api)

This comment has been minimized.

Copy link
@lewischeng-ms

lewischeng-ms Jun 14, 2016

Contributor

Do we have a public API to use from Web API OData instead of writing it by ourselves?

This comment has been minimized.

Copy link
@chinadragon0515

chinadragon0515 Jun 15, 2016

Author Contributor

No, I talked with Sam before I started this part.

@chinadragon0515 chinadragon0515 force-pushed the chinadragon0515:operation branch from 9a65c98 to 6c8124c Jun 15, 2016

@chinadragon0515 chinadragon0515 merged commit 6c8124c into OData:master Jun 15, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.