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

Issue 104, support dynamic properties in $filter, $orderby and $select #250

Closed
wants to merge 9 commits into from

Conversation

Projects
None yet
5 participants
@hvanbakel
Copy link

commented Mar 16, 2015

Added support for dynamic properties in $select, $filter and $orderby. When the dynamic property is not available, their value is set to 'null' which is then used in ordering for example. If unwanted, the filter option can be used to filter those results just as one would normally do.

connected-web-development added some commits Mar 13, 2015

@msftclas

This comment has been minimized.

Copy link

commented Mar 16, 2015

Hi @connected-web-development, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

var resultArray = result["value"] as JArray;
Assert.Equal(6, resultArray.Count);
Assert.NotNull(resultArray[2]["Token"]);//customer 2 has a token
Assert.NotNull(resultArray[4]["Token"]);//customer 3 has a token

This comment has been minimized.

Copy link
@VikingsFan

VikingsFan Mar 17, 2015

Contributor

customer 4?

@@ -0,0 +1,46 @@
using System.Web.Http;

This comment has been minimized.

Copy link
@VikingsFan

VikingsFan Mar 17, 2015

Contributor

please add Copyright like other files too

public IQueryable<SimpleOpenCustomer> Get(ODataQueryOptions<SimpleOpenCustomer> options)
{
var queryable = CreateCustomers().AsQueryable();
//if (options.Filter != null)

This comment has been minimized.

Copy link
@VikingsFan

VikingsFan Mar 17, 2015

Contributor

remove these lines if it's not comment

@hvanbakel

This comment has been minimized.

Copy link
Author

commented Mar 17, 2015

Comments processed.

@MrTomWhite

This comment has been minimized.

Copy link

commented Mar 18, 2015

@connected-web-development It looks like this produces a merge conflict. You should fix this so the PR can be merged. 📦

connected-web-development
Merge branch 'OData/master' into master
Conflicts:
	OData/src/System.Web.OData/OData/Formatter/Serialization/SelectExpandNode.cs
	OData/src/System.Web.OData/OData/Query/Expressions/SelectExpandBinder.cs
@hvanbakel

This comment has been minimized.

Copy link
Author

commented Mar 19, 2015

Merge done

@VikingsFan

This comment has been minimized.

Copy link
Contributor

commented Mar 19, 2015

@connected-web-development Thank you for responding so quick, the reason this change is not merge yet is we have two features have some relations with your change, for example the untyped scenario, so after we finish these two features (hope will done today), than we can merge your change or you can merge yourself, thanks :) .

public class OrderByOpenPropertyNode : OrderByNode
{
/// <summary>
/// Default constructor for a dynamic property order by

This comment has been minimized.

Copy link
@VikingsFan

VikingsFan Mar 23, 2015

Contributor

Initializes a new instance of the class.

This comment has been minimized.

Copy link
@hvanbakel

hvanbakel Mar 25, 2015

Author

I will change the comment though it's hardly informing that a constructor initializes a new instance

/// <summary>
/// Default constructor for a dynamic property order by
/// </summary>
/// <param name="orderByClause">The order by clause for this open property</param>

This comment has been minimized.

Copy link
@VikingsFan

VikingsFan Mar 23, 2015

Contributor

end with '.'

/// Default constructor for a dynamic property order by
/// </summary>
/// <param name="orderByClause">The order by clause for this open property</param>
public OrderByOpenPropertyNode(OrderByClause orderByClause)

This comment has been minimized.

Copy link
@VikingsFan

VikingsFan Mar 23, 2015

Contributor

why not call :base(...Direction)

var openPropertyExpression = orderByClause.Expression as SingleValueOpenPropertyAccessNode;
if (openPropertyExpression == null)
{
throw new ODataException(SRResources.OrderByClauseNotSupported);

This comment has been minimized.

Copy link
@VikingsFan

VikingsFan Mar 23, 2015

Contributor

add the the orderby node name?

/// <summary>
/// The order by clause
/// </summary>
public OrderByClause OrderByClause { get; set; }

This comment has been minimized.

Copy link
@VikingsFan

VikingsFan Mar 23, 2015

Contributor

private set?

}

var openPropertyExpression = clause.Expression as SingleValueOpenPropertyAccessNode;
if (openPropertyExpression != null)

This comment has been minimized.

Copy link
@VikingsFan

VikingsFan Mar 23, 2015

Contributor

change to " is SingleValueOpenPropertyAccessNode "

}
else
{
throw new NotImplementedException(string.Format("Error ordering the dynamic property {0} on type {1}", openPropertyNode.PropertyName, Context.ElementClrType.FullName));

This comment has been minimized.

Copy link
@VikingsFan

VikingsFan Mar 23, 2015

Contributor

Please add it into resource string, which will be translate into multiple language

@@ -32,6 +32,8 @@ internal class FilterBinder
{
private const string ODataItParameterName = "$it";

private static readonly string _dictionaryStringObjectIndexerName = typeof (Dictionary<string, object>).GetDefaultMembers()[0].Name;

This comment has been minimized.

Copy link
@VikingsFan

VikingsFan Mar 23, 2015

Contributor

remove white space after "typeof"

please run "build.cmd" to make sure build without any error

@@ -233,6 +237,57 @@ private Expression Bind(QueryNode node)
}
}

private Expression BindOpenPropertyAccessQueryNode(SingleValueOpenPropertyAccessNode openNode)

This comment has been minimized.

Copy link
@VikingsFan

VikingsFan Mar 23, 2015

Contributor

change to BindDynamicPropertyAccessQueryNode(..)

private Expression BindOpenPropertyAccessQueryNode(SingleValueOpenPropertyAccessNode openNode)
{
var prop = GetOpenTypeProperty(openNode);
var propertyAccessExpression = BindPropertyAccessExpression(openNode, prop);

This comment has been minimized.

Copy link
@VikingsFan

VikingsFan Mar 23, 2015

Contributor

there is a same function defined as MakeFunctionCall(..), please consider to re-use the existing function.

return propertyAccessExpression;
}

private PropertyInfo GetOpenTypeProperty(SingleValueOpenPropertyAccessNode openNode)

This comment has been minimized.

Copy link
@VikingsFan

VikingsFan Mar 23, 2015

Contributor

make change it as "GetDynamicPropertyDictionary" or "GetDynamicPropertyContainer()"
or make some comment for this function


private PropertyInfo GetOpenTypeProperty(SingleValueOpenPropertyAccessNode openNode)
{
IEdmStructuredType edmEntityType;

This comment has been minimized.

Copy link
@VikingsFan

VikingsFan Mar 23, 2015

Contributor

edmStructuredType?

}
else
{
throw Error.NotSupported(SRResources.QueryNodeBindingNotSupported, openNode.Kind, typeof (FilterBinder).Name);

This comment has been minimized.

Copy link
@VikingsFan

VikingsFan Mar 23, 2015

Contributor

remove the white space after "typeof"

}

[Fact]
public void Get_OpenEntityTypeWithOrderbyDescending()

This comment has been minimized.

Copy link
@VikingsFan

VikingsFan Mar 23, 2015

Contributor

Merge as one test case and use [Theory]

public void Get_OpenEntityTypeWithFilter()
{
// Arrange
const string RequestUri = "http://localhost/odata/SimpleOpenCustomers?$filter=Token ne null";

This comment has been minimized.

Copy link
@VikingsFan

VikingsFan Mar 23, 2015

Contributor

test in above, so use other condition

@hvanbakel

This comment has been minimized.

Copy link
Author

commented Mar 25, 2015

The only thing that's not fixed is the comment about the orderby ndoe name. However this would require changes in the orderbyproperty node as well as it works the same so the error should be the same. But that would also require the tests to be updated accordingly etc.

VikingsFan added a commit that referenced this pull request Mar 26, 2015

VikingsFan added a commit that referenced this pull request Mar 26, 2015

@VikingsFan

This comment has been minimized.

Copy link
Contributor

commented Mar 26, 2015

Checked in 3f43633

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.