Fixes #1147: Enable skiptoken handler to handler advanced orderby clause #1164

xuzhg · 2024-01-30T23:48:48Z

Two parts:

1. How to generate the skip token when we have the advanced $orderby?

Based on the discussion and back-compatibility, the PR follows up:
a) If it's top level property orderby (declared or dynamic or computed), for example: $orderby=name
    the skiptoken will follow up the existing pattern:   `name-'abc'` 

b) If it's other orderby, for example: $orderby=tolower(substring(name,1,2))
    the skiptoken will only contains the value:   `tx`

2. How to consume the skip token when apply the $skiptoken?

Basically, the skiptoken is coverted to a 'Where(...)' clause (Linq Where function call). Behind the scene, it's a $filter clause.
The existing implementation cannot handle all and it contains errors.
This PR is to change it and based on the above truth, we should use our IFilterBinder to handle the $skiptoken.

The logic is that:
a) Convert the skiptoken and orderby to filter expression
b) Call IFilterBinder to bind the filter expression to 'Where' Linq expression
c) Apply the 'Where' to IQueryable.

xuzhg · 2024-01-30T23:50:04Z

src/Microsoft.AspNetCore.OData/Query/Query/DefaultSkipTokenHandler.cs

        {
            if (lastMember == null)
            {
                return string.Empty;
            }

-            IEnumerable<IEdmProperty> propertiesForSkipToken = GetPropertiesForSkipToken(lastMember, model, orderByNodes);
-            StringBuilder skipTokenBuilder = new StringBuilder(String.Empty);


Here's the part 1 refactor. #WontFix

xuzhg · 2024-01-30T23:50:21Z

src/Microsoft.AspNetCore.OData/Query/Query/DefaultSkipTokenHandler.cs

        {
-            Contract.Assert(query != null);


Here's the part 2 refactor #WontFix

habbes

Partial review. Will share more feedback later.

habbes · 2024-02-08T05:12:16Z

src/Microsoft.AspNetCore.OData/Edm/OrderByClauseHelpers.cs

+        /// </summary>
+        /// <param name="clause">The input orderby clause, the 'ThenBy' in each node does matter.</param>
+        /// <returns>The output orderby clauses, the 'ThenBy' in each node does NOT matter.</returns>
+        public static IList<OrderByClause> ToList(this OrderByClause clause)


Since this class is internal, I would suggest returning a List<T> instead of IList<T> since using List<T> directly is more efficient. For example when doing a foreach against an IList<T> it will box and allocate the enumerator to the heap, but when using List<T> directly you avoid boxing the enumerator. #Resolved

Keep eyes on List is more efficent than IList. Since it's internal, I am ok to replace it using List.

src/Microsoft.AspNetCore.OData/Query/Expressions/SelectExpandBinder.cs

habbes · 2024-02-08T05:39:39Z

src/Microsoft.AspNetCore.OData/Query/Expressions/SelectExpandBinder.cs

+            }
+
+            // Avoid duplicated binding.
+            ISet<string> usedPropertyNames = new HashSet<string>();


For internal, private or local variables, I suggest we use concrete types. Using interfaces lead to more boxing of enumerators and methods are less likely to get inlined by the JIT.

We should consider interfaces for public APIs or for scenarios where we actually want to support different implementations. But in this case, we have no reason to support other implementations than HashSet<string>.
#Resolved

Maybe you can give us a 'session' about this.

habbes · 2024-02-08T05:41:51Z

src/Microsoft.AspNetCore.OData/Query/Expressions/SelectExpandBinder.cs

+
+            Expression propertyName;
+            Expression propertyValue;
+            IList<string> names = new List<string>();


Suggested change

IList<string> names = new List<string>();

List<string> names = new List<string>();

``` #Resolved

Is it possible to estimate the capacity of the list from the input so that we avoid resizing?

habbes · 2024-02-08T05:47:57Z

src/Microsoft.AspNetCore.OData/Query/Expressions/SelectExpandBinder.cs

+            }
+        }
+
+        private static string GetOrderByName(ISet<string> usedPropertyNames, ref int start)


Suggested change

private static string GetOrderByName(ISet<string> usedPropertyNames, ref int start)

private static string GetOrderByName(HashSet<string> usedPropertyNames, ref int start)

``` #Resolved

src/Microsoft.AspNetCore.OData/Query/Expressions/SelectExpandBinder.cs

habbes · 2024-02-08T06:52:58Z

src/Microsoft.AspNetCore.OData/Query/Expressions/SelectExpandBinder.cs

+
+                name = GetOrderByName(usedPropertyNames, ref start);
+                names.Add(name);
+                usedPropertyNames.Add(name);


Given that each name is generated by adding an incrementing counter, each iteration will produce a unique name. In that case, do we still need to add to these auto-generated names to the usedPropertyNames set? #Resolved

Good Suggestion. Let me comment it out and leave a comment for it.

habbes · 2024-02-08T06:53:24Z

src/Microsoft.AspNetCore.OData/Query/Expressions/SelectExpandBinder.cs

+            Expression propertyValue;
+            IList<string> names = new List<string>();
+            string name;
+            int start = 1;


Suggested change

int start = 1;

int index = 1;

``` #Resolved

habbes · 2024-02-08T06:58:08Z

src/Microsoft.AspNetCore.OData/Query/ODataQueryOptions.cs

@@ -679,6 +680,9 @@ private static IEnumerable<IEdmStructuralProperty> GetAvailableOrderByProperties
                    : entityType
                        .StructuralProperties()
                        .Where(property => property.Type.IsPrimitive() && !property.Type.IsStream())
+
+                        // @discuss: Orderby the keys using the key name alphabet order doesn't make sense for me.
+                        // Since we don't have the 'Order' value, we should keep the order same as definition order in the schema?


Maybe the goal was to ensure the properties are always returned in a consistent order. Maybe any order is valid so long as it's consistent between calls. But I don't know the context.
But if there's something we can do to void sorting, I'd vouch for that. We saw some a lot of cycles from sorting and other LINQ operations related to ETag properties, and I think we should avoid that where possible. #Pending

I'd like to here input from @mikepizzo also.
It's just for discussion. I will leave this code unchanged since change it is a behavior breaking changes.

src/Microsoft.AspNetCore.OData/Query/Query/DefaultSkipTokenHandler.cs

habbes · 2024-02-09T07:01:54Z

src/Microsoft.AspNetCore.OData/Query/Query/DefaultSkipTokenHandler.cs

-            IEnumerable<IEdmProperty> propertiesForSkipToken = GetPropertiesForSkipToken(lastMember, model, orderByNodes);
-            StringBuilder skipTokenBuilder = new StringBuilder(String.Empty);
-            if (propertiesForSkipToken == null)
+            IList<OrderByClause> clauses = GetOrderByClauses(lastMember, model, clause);


Suggested change

IList<OrderByClause> clauses = GetOrderByClauses(lastMember, model, clause);

List<OrderByClause> clauses = GetOrderByClauses(lastMember, model, clause);

``` #Resolved

habbes · 2024-02-09T07:38:17Z

src/Microsoft.AspNetCore.OData/Query/Query/DefaultSkipTokenHandler.cs

            }

            return skipTokenBuilder.ToString();
        }

+        private static IList<KeyValuePair<string, object>> GetPropertyValues(object source, IEdmModel model, IList<OrderByClause> clauses,


Suggested change

private static IList<KeyValuePair<string, object>> GetPropertyValues(object source, IEdmModel model, IList<OrderByClause> clauses,

private static List<KeyValuePair<string, object>> GetPropertyValues(object source, IEdmModel model, List<OrderByClause> clauses,

``` #Resolved

habbes · 2024-02-09T07:38:35Z

src/Microsoft.AspNetCore.OData/Query/Query/DefaultSkipTokenHandler.cs

+            IList<OrderByClause> clauses = GetOrderByClauses(lastMember, model, clause);
+
+            TimeZoneInfo timeZoneInfo = context?.TimeZone;
+            IList<KeyValuePair<string, object>> values = GetPropertyValues(lastMember, model, clauses, context);


Suggested change

IList<KeyValuePair<string, object>> values = GetPropertyValues(lastMember, model, clauses, context);

IList<KeyValuePair<string, object>> values = GetPropertyValues(lastMember, model, clauses, context);

Suggested change

IList<KeyValuePair<string, object>> values = GetPropertyValues(lastMember, model, clauses, context);

List<KeyValuePair<string, object>> values = GetPropertyValues(lastMember, model, clauses, context);

``` #Resolved

habbes · 2024-02-09T07:48:45Z

src/Microsoft.AspNetCore.OData/Query/Query/DefaultSkipTokenHandler.cs

+
+            structuredObj.TryGetPropertyValue(OrderByClauseHelpers.OrderByGlobalNameKey, out object orderByNameObject);
+
+            IList<KeyValuePair<string, object>> values = new List<KeyValuePair<string, object>>();


Suggested change

IList<KeyValuePair<string, object>> values = new List<KeyValuePair<string, object>>();

List<KeyValuePair<string, object>> values = new List<KeyValuePair<string, object>>();

``` #Resolved

habbes · 2024-02-09T07:49:43Z

src/Microsoft.AspNetCore.OData/Query/Query/DefaultSkipTokenHandler.cs

            }

            return skipTokenBuilder.ToString();
        }

+        private static IList<KeyValuePair<string, object>> GetPropertyValues(object source, IEdmModel model, IList<OrderByClause> clauses,


Is this method called when consuming skip tokens? #WontFix

No, It's for 'Part-1' to generate the Skip Token string.

habbes · 2024-02-12T07:03:23Z

src/Microsoft.AspNetCore.OData/Query/Query/DefaultSkipTokenHandler.cs

@@ -464,28 +456,35 @@ internal static IEnumerable<IEdmProperty> GetPropertiesForSkipToken(object lastM
                return null;
            }

-            IEnumerable<IEdmProperty> key = entity.Key();
-            if (orderByNodes != null)
+            IList<OrderByClause> orderByClauses = new List<OrderByClause>();


Suggested change

IList<OrderByClause> orderByClauses = new List<OrderByClause>();

List<OrderByClause> orderByClauses = new List<OrderByClause>();

``` #Resolved

habbes · 2024-02-12T07:03:42Z

src/Microsoft.AspNetCore.OData/Query/Query/DefaultSkipTokenHandler.cs

-            IEnumerable<IEdmProperty> key = entity.Key();
-            if (orderByNodes != null)
+            IList<OrderByClause> orderByClauses = new List<OrderByClause>();
+            ISet<IEdmProperty> properties = new HashSet<IEdmProperty>();


Suggested change

ISet<IEdmProperty> properties = new HashSet<IEdmProperty>();

HashSet<IEdmProperty> properties = new HashSet<IEdmProperty>();

``` #Resolved

habbes · 2024-02-12T07:09:44Z

test/Microsoft.AspNetCore.OData.E2E.Tests/SkipToken/SkipTokenControllers.cs

+        #region Data
+        static SkipTokenControllers()
+        {
+            IList<StAddress> stAddresses = new List<StAddress>()


Do you have test cases with dates and negative numbers? #Resolved

habbes · 2024-02-22T15:39:11Z

src/Microsoft.AspNetCore.OData/Query/Query/DefaultSkipTokenHandler.cs

+            // Since we don't have such methods, let's simply split the request raw value.
+            IList<string> orderBys = orderByOption.RawValue.Split(',');
+            IList<OrderByClause> orderByClauses = orderByOption.OrderByClause.ToList();
+            IList<string> tokenValueParis = PopulatePropertyValuePairs(skipTokenRawValue);


Suggested change

IList<string> tokenValueParis = PopulatePropertyValuePairs(skipTokenRawValue);

IList<string> tokenValuePairs = PopulatePropertyValuePairs(skipTokenRawValue);

``` #Resolved

Actually, since this list only contains the values (without keys), why not just tokenValues?

Suggested change

IList<string> tokenValueParis = PopulatePropertyValuePairs(skipTokenRawValue);

List<string> tokenValues = PopulatePropertyValuePairs(skipTokenRawValue);

src/Microsoft.AspNetCore.OData/Query/Query/DefaultSkipTokenHandler.cs

habbes · 2024-02-23T04:11:26Z

src/Microsoft.AspNetCore.OData/Query/Query/DefaultSkipTokenHandler.cs

-
-            foreach (KeyValuePair<string, (object PropertyValue, Type PropertyType)> item in propertyValuePairs)
+            string where = string.Empty;
+            string lastEquality = null;


nit: The name lastEquality might give the impression that this refers to the last or final equality in the loop, but actually it refers to the equality generated in the previous iteration.

Suggested change

string lastEquality = null;

string previousEquality = null;

``` #Resolved

habbes · 2024-02-23T07:39:34Z

src/Microsoft.AspNetCore.OData/Query/Query/DefaultSkipTokenHandler.cs

-            IDictionary<string, (object PropertyValue, Type PropertyType)> propertyValuePairs = PopulatePropertyValuePairs(skipTokenRawValue, context);
+            // It's better to visit the nodes of OrderByClause reclusively to get the orderby raw value.
+            // Since we don't have such methods, let's simply split the request raw value.
+            IList<string> orderBys = orderByOption.RawValue.Split(',');


This will fail if for non-trivial orderby expressions. For example, if you have a function call with multiple parameters like $orderby=substring(Field, 1),Id, this will splitinto: ["substring(Field", "1)", "Id"] which is invalid and will lead to an argument out of range exception because orderBys list will have more items than orderByClauses list.

Did you intend to support these scenarios in this PR or in a future one? If you want to add support for that in the future then we should detect such cases and throw a meaningful exception that lets the user know that operation is not supported, and then we should create an issue to support that. Otherwise we should handle it better in this PR. Do we have an method that can generate a string representation of an orderByClause.Expression that is a valid OData query expression?

#Resolved

Fixed. Can you help to test it again?

src/Microsoft.AspNetCore.OData/Query/Query/DefaultSkipTokenHandler.cs

1) Can handle the 'asc' suffix 2) Can handle the ',' within expression

Fixes #1147: Enable skiptoken handler to handler advanced orderby clause

20004da

xuzhg requested review from KenitoInc, habbes, mikepizzo, ElizabethOkerio and gathogojr January 30, 2024 23:49

xuzhg commented Jan 30, 2024

View reviewed changes

Fix the failing test cases

2235416

xuzhg mentioned this pull request Feb 1, 2024

Fixes #1127: generate skip token using computed property #1143

Closed

habbes reviewed Feb 8, 2024

View reviewed changes

habbes reviewed Feb 9, 2024

View reviewed changes

src/Microsoft.AspNetCore.OData/Query/Query/DefaultSkipTokenHandler.cs Show resolved Hide resolved

habbes reviewed Feb 9, 2024

View reviewed changes

habbes reviewed Feb 12, 2024

View reviewed changes

Address the comments

ef156c7

habbes requested changes Feb 22, 2024

View reviewed changes

habbes reviewed Feb 23, 2024

View reviewed changes

src/Microsoft.AspNetCore.OData/Query/Query/DefaultSkipTokenHandler.cs Outdated Show resolved Hide resolved

Address the comments

f3d6d1d

1) Can handle the 'asc' suffix 2) Can handle the ',' within expression

habbes approved these changes Feb 29, 2024

View reviewed changes

xuzhg merged commit df8ca08 into main Feb 29, 2024
2 checks passed

justin-mellor mentioned this pull request Mar 7, 2024

$orderby no longer working after upgrading from Microsoft.AspNetCore.OData 8.2.4 to 8.2.5 #1190

Closed

habbes mentioned this pull request Oct 1, 2024

regresssion in 8.2.5 : ApplyTo method changes OrderBy property #1319

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #1147: Enable skiptoken handler to handler advanced orderby clause #1164

Fixes #1147: Enable skiptoken handler to handler advanced orderby clause #1164

xuzhg commented Jan 30, 2024

xuzhg Jan 30, 2024 •

edited

Loading

xuzhg Jan 30, 2024 •

edited

Loading

habbes left a comment

habbes Feb 8, 2024 •

edited by xuzhg

Loading

xuzhg Feb 12, 2024

habbes Feb 8, 2024 •

edited by xuzhg

Loading

xuzhg Feb 12, 2024

habbes Feb 8, 2024 •

edited by xuzhg

Loading

habbes Feb 8, 2024

habbes Feb 8, 2024 •

edited by xuzhg

Loading

habbes Feb 8, 2024 •

edited by xuzhg

Loading

xuzhg Feb 12, 2024

habbes Feb 8, 2024 •

edited by xuzhg

Loading

habbes Feb 8, 2024 •

edited by xuzhg

Loading

xuzhg Feb 12, 2024

habbes Feb 9, 2024 •

edited by xuzhg

Loading

habbes Feb 9, 2024 •

edited by xuzhg

Loading

habbes Feb 9, 2024 •

edited by xuzhg

Loading

habbes Feb 9, 2024 •

edited by xuzhg

Loading

habbes Feb 9, 2024 •

edited by xuzhg

Loading

xuzhg Feb 12, 2024

habbes Feb 12, 2024 •

edited by xuzhg

Loading

habbes Feb 12, 2024 •

edited by xuzhg

Loading

habbes Feb 12, 2024 •

edited by xuzhg

Loading

habbes Feb 22, 2024 •

edited by xuzhg

Loading

habbes Feb 23, 2024

habbes Feb 23, 2024 •

edited by xuzhg

Loading

habbes Feb 23, 2024 •

edited by xuzhg

Loading

xuzhg Feb 26, 2024

	IList<string> names = new List<string>();
	List<string> names = new List<string>();
	``` #Resolved

	private static string GetOrderByName(ISet<string> usedPropertyNames, ref int start)
	private static string GetOrderByName(HashSet<string> usedPropertyNames, ref int start)
	``` #Resolved

	IList<OrderByClause> clauses = GetOrderByClauses(lastMember, model, clause);
	List<OrderByClause> clauses = GetOrderByClauses(lastMember, model, clause);
	``` #Resolved

	private static IList<KeyValuePair<string, object>> GetPropertyValues(object source, IEdmModel model, IList<OrderByClause> clauses,
	private static List<KeyValuePair<string, object>> GetPropertyValues(object source, IEdmModel model, List<OrderByClause> clauses,
	``` #Resolved

	IList<KeyValuePair<string, object>> values = GetPropertyValues(lastMember, model, clauses, context);
	IList<KeyValuePair<string, object>> values = GetPropertyValues(lastMember, model, clauses, context);

	IList<KeyValuePair<string, object>> values = GetPropertyValues(lastMember, model, clauses, context);
	List<KeyValuePair<string, object>> values = GetPropertyValues(lastMember, model, clauses, context);
	``` #Resolved


		structuredObj.TryGetPropertyValue(OrderByClauseHelpers.OrderByGlobalNameKey, out object orderByNameObject);

		IList<KeyValuePair<string, object>> values = new List<KeyValuePair<string, object>>();

	IList<OrderByClause> orderByClauses = new List<OrderByClause>();
	List<OrderByClause> orderByClauses = new List<OrderByClause>();
	``` #Resolved

	ISet<IEdmProperty> properties = new HashSet<IEdmProperty>();
	HashSet<IEdmProperty> properties = new HashSet<IEdmProperty>();
	``` #Resolved

	IList<string> tokenValueParis = PopulatePropertyValuePairs(skipTokenRawValue);
	IList<string> tokenValuePairs = PopulatePropertyValuePairs(skipTokenRawValue);
	``` #Resolved

	IList<string> tokenValueParis = PopulatePropertyValuePairs(skipTokenRawValue);
	List<string> tokenValues = PopulatePropertyValuePairs(skipTokenRawValue);

	string lastEquality = null;
	string previousEquality = null;
	``` #Resolved

Fixes #1147: Enable skiptoken handler to handler advanced orderby clause #1164

Fixes #1147: Enable skiptoken handler to handler advanced orderby clause #1164

Conversation

xuzhg commented Jan 30, 2024

Two parts:

1. How to generate the skip token when we have the advanced $orderby?

2. How to consume the skip token when apply the $skiptoken?

xuzhg Jan 30, 2024 • edited Loading

Choose a reason for hiding this comment

xuzhg Jan 30, 2024 • edited Loading

Choose a reason for hiding this comment

habbes left a comment

Choose a reason for hiding this comment

habbes Feb 8, 2024 • edited by xuzhg Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

habbes Feb 8, 2024 • edited by xuzhg Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

habbes Feb 8, 2024 • edited by xuzhg Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

habbes Feb 8, 2024 • edited by xuzhg Loading

Choose a reason for hiding this comment

habbes Feb 8, 2024 • edited by xuzhg Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

habbes Feb 8, 2024 • edited by xuzhg Loading

Choose a reason for hiding this comment

habbes Feb 8, 2024 • edited by xuzhg Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

habbes Feb 9, 2024 • edited by xuzhg Loading

Choose a reason for hiding this comment

habbes Feb 9, 2024 • edited by xuzhg Loading

Choose a reason for hiding this comment

habbes Feb 9, 2024 • edited by xuzhg Loading

Choose a reason for hiding this comment

habbes Feb 9, 2024 • edited by xuzhg Loading

Choose a reason for hiding this comment

habbes Feb 9, 2024 • edited by xuzhg Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

habbes Feb 12, 2024 • edited by xuzhg Loading

Choose a reason for hiding this comment

habbes Feb 12, 2024 • edited by xuzhg Loading

Choose a reason for hiding this comment

habbes Feb 12, 2024 • edited by xuzhg Loading

Choose a reason for hiding this comment

habbes Feb 22, 2024 • edited by xuzhg Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

habbes Feb 23, 2024 • edited by xuzhg Loading

Choose a reason for hiding this comment

habbes Feb 23, 2024 • edited by xuzhg Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xuzhg Jan 30, 2024 •

edited

Loading

xuzhg Jan 30, 2024 •

edited

Loading

habbes Feb 8, 2024 •

edited by xuzhg

Loading

habbes Feb 8, 2024 •

edited by xuzhg

Loading

habbes Feb 8, 2024 •

edited by xuzhg

Loading

habbes Feb 8, 2024 •

edited by xuzhg

Loading

habbes Feb 8, 2024 •

edited by xuzhg

Loading

habbes Feb 8, 2024 •

edited by xuzhg

Loading

habbes Feb 8, 2024 •

edited by xuzhg

Loading

habbes Feb 9, 2024 •

edited by xuzhg

Loading

habbes Feb 9, 2024 •

edited by xuzhg

Loading

habbes Feb 9, 2024 •

edited by xuzhg

Loading

habbes Feb 9, 2024 •

edited by xuzhg

Loading

habbes Feb 9, 2024 •

edited by xuzhg

Loading

habbes Feb 12, 2024 •

edited by xuzhg

Loading

habbes Feb 12, 2024 •

edited by xuzhg

Loading

habbes Feb 12, 2024 •

edited by xuzhg

Loading

habbes Feb 22, 2024 •

edited by xuzhg

Loading

habbes Feb 23, 2024 •

edited by xuzhg

Loading

habbes Feb 23, 2024 •

edited by xuzhg

Loading