Enable custom query validato into SelectExpandQueryValidator by xuzhg · Pull Request #800 · OData/AspNetCoreOData

xuzhg · 2023-01-10T00:47:40Z

issue #767

Refactor SelectExpandQueryValidator by:

Remove the private field
Create/use the validator context
Separate each part validation into a protected virtual, so the customer can customize each part.
Remove "very bad" internal virtual method in FilterQueryValidator
Other changes.

gathogojr · 2023-01-11T11:32:09Z

        /// <param name="validationSettings">The validation settings.</param>
        void Validate(SelectExpandQueryOption selectExpandQueryOption, ODataValidationSettings validationSettings);
+
+       // Task ValidateAsync(SelectExpandClause selectExpandClause, ODataValidationSettings validationSettings);


What's the deal with the commented code? Shouldn't it be removed?

Oh, Yes. Forgot it. Will update it.

gathogojr · 2023-01-11T11:40:56Z

+                    remainDepth = maxDepth;
                }
+
+                //if (expandConfiguration.MaxDepth > 0 && currentDepth >= expandConfiguration.MaxDepth)


Should this commented out code be removed instead?

Yes. Thanks.

gathogojr · 2023-01-11T12:23:57Z

+        /// <summary>
+        /// Validates $select. For example, ~/Customers?$select=Prop($select=SubProp;$top=2)
+        /// </summary>
+        /// <param name="pathSelectItem"></param>


Supply the parameter description

gathogojr · 2023-01-11T12:30:06Z

+            ValidateSearch(pathSelectItem.SearchOption, subValidatorContext);
+
+            // Validate the nested $compute within $select
+            ValidateCompute(pathSelectItem.ComputeOption, subValidatorContext);


I assume it's not valid to have $levels and $apply nested in a $select expression?

No, They are not.

gathogojr · 2023-01-11T12:33:28Z

+        /// </summary>
+        /// <param name="filterClause">The nested $filter clause.</param>
+        /// <param name="validatorContext">The validator context.</param>
+        protected virtual void ValidateFilter(FilterClause filterClause, SelectExpandValidatorContext validatorContext)


For this method and the related ones, would it have been better to named them ValidateNestedXXX?

I original thought we don't need 'nested' since they are 'protected virtual' within SelectExpandValidator.

But, I am ok to rename them.

Given your explanation it would have been okay to retain them as they were...

#Resolved. Never mind, I updated them.

gathogojr · 2023-01-11T12:37:16Z

+                OrderByModelLimitationsValidator orderByQueryValidator =
+                   new OrderByModelLimitationsValidator(validatorContext.Context, validatorContext.Context.DefaultQuerySettings.EnableOrderBy);
+
+                orderByQueryValidator.TryValidate(validatorContext.Property, validatorContext.StructuredType, orderByClause, false);


Does TryValidate return a boolean value of true/false and should we be checking the return value?

That's the old codes/existing codes that I will refactor later (next PR).

gathogojr · 2023-01-11T12:40:29Z

+        protected virtual void ValidateOrderby(OrderByClause orderByClause, SelectExpandValidatorContext validatorContext)
        {
-            if (filterClause != null)
+            if (orderByClause != null)


Just to observe that in some methods, you're doing:

if (xxxOption == null) { return; } // Validation logic here

Here and in some other methods you're nesting the validation logic within the if block...

It depends on the "number of codes line". #won't fix.

…ryValidator.cs Co-authored-by: John Gathogo <john.gathogo@microsoft.com>

habbes · 2023-01-18T13:08:56Z

-                _structuredType = filterQueryOption.Context.TargetStructuredType;
-            }
-
+            _property = filterQueryOption.Context.TargetProperty;


I don't understand this change. Why was the if statement there in the first place?

I think it's safe to check.
Actually, in the ODataQueryContext, we have the Path null check already.
Now, I added an internal constructor to set the TargetStructuredType and property, so, we should remove the null check here.

habbes · 2023-01-18T13:10:41Z

+
+        /// <summary>
+        /// The remaining depth on property.
+        /// It's weird logic in current implementation. Need to improve it later.


What implementation does this comment refer to? Why is it weird? If it needs to be implemented late, consider creating an issue to track it.

It's weird because the 'RemainingDepth' is working with the query setting configuration on the property. I can't understand such logic.
Maybe it's correct, but I do not fully understand this.
Let me update the description.

habbes · 2023-01-18T13:27:08Z

+                ValidationSettings = this.ValidationSettings,
+                Property = this.Property,
+                StructuredType = this.StructuredType,
+                RemainingDepth = this.RemainingDepth,


Is RemainingDepth property value set by the customer or by the framework? At which point is it calculated?

In the SelectExpandQueryValidator.cs at line 192 at SelectExpandQueryValidator.cs

It's calculated at Line 170 - 176.
The codes read the query setting on property (Model query setting). Model query setting is something

public class Customer { [Expand(MaxDepth=3)] public Order MyOrder {get;set;} }

gathogojr

Enable custom query validato into SelectExpandQueryValidator

eb11c95

xuzhg requested review from ElizabethOkerio, KenitoInc, corranrogue9, gathogojr, lisicase and mikepizzo January 10, 2023 00:47

gathogojr reviewed Jan 11, 2023

View reviewed changes

Comment thread src/Microsoft.AspNetCore.OData/Query/Validator/SelectExpandQueryValidator.cs Outdated

gathogojr reviewed Jan 11, 2023

View reviewed changes

xuzhg and others added 3 commits January 11, 2023 10:50

Update src/Microsoft.AspNetCore.OData/Query/Validator/SelectExpandQue…

4113c50

…ryValidator.cs Co-authored-by: John Gathogo <john.gathogo@microsoft.com>

Address the comments

cd9bf86

Fix the build warnings

7084be0

habbes reviewed Jan 18, 2023

View reviewed changes

gathogojr approved these changes Jan 18, 2023

View reviewed changes

xuzhg merged commit b90269b into main Jan 18, 2023

xuzhg deleted the issue767_3 branch January 19, 2023 17:53

AndreaCuneo mentioned this pull request Mar 27, 2023

chore(deps): bump Microsoft.AspNetCore.OData from 8.0.12 to 8.1.0 ARKlab/Ark.Tools#57

Merged

AndreaCuneo mentioned this pull request Apr 4, 2023

chore(deps): bump Microsoft.AspNetCore.OData from 8.0.12 to 8.1.0 ARKlab/Ark.Tools#60

Closed

This was referenced Nov 7, 2023

The property '{propertyname}' cannot be used in the $expand query option OData/WebApi#2815

Open

Make the query validator thread-safe since the validator is singleton OData/WebApi#2817

Open

This was referenced Apr 29, 2026

Bump Microsoft.AspNetCore.OData from 7.5.1 to 8.3.1 pipeline-foundation/aspnet-api-versioning#8

Open

Bump Microsoft.AspNetCore.OData from 7.5.8 to 8.3.1 peteraritchie/aspnet-api-versioning#7

Open

Uh oh!

Conversation

xuzhg commented Jan 10, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gathogojr Jan 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gathogojr left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gathogojr Jan 11, 2023 •

edited

Loading