FunctionCallBinder promotes arguments without checking return type #2257

JBBianchi · 2021-11-22T11:17:38Z

Issues

This pull request fixes #2241

Related to
OData/WebApi#1180
OData/WebApi#2173
OData/AspNetCoreOData#325

Description

In the FunctionCallBinder, the function signature's return type was checked before processing the arguments nodes. There is no apparent reason for this check which leads to some arguments being mistyped.
The check has been removed, allowing the arguments to be promoted as they should.

Checklist (Uncheck if it is not completed)

Test cases added
Build and test with one-click build and test script passed

Additional work necessary

Unfortunately, I'm not able to launch the tests. If anybody could double check this commit and maybe add some tests, it would be very much appreciated! ❤️

- Removed check for ReturnType in FunctionCallBinder before promoting function arguments

- unwrapped ConvertQueryNode for FunctionCallQueryNodes parameters

ghost · 2021-11-29T08:27:20Z

All CLA requirements met.

- unwrapped ConvertQueryNode for FunctionCallQueryNodes parameters

JBBianchi · 2021-11-29T09:46:45Z

I tried to blindly fix the tests as well as I could, I hope they are OK.

ElizabethOkerio · 2021-11-30T04:30:14Z

@JBBianchi Thanks for your contribution. Please rebase your branch.

gathogojr · 2021-11-30T04:50:13Z

...lTests/Microsoft.OData.Core.Tests/ScenarioTests/UriParser/FilterAndOrderByFunctionalTests.cs

@@ -723,7 +723,8 @@ public void NullValueInCanonicalFunction()
            var result = ParseFilter("day(null)", HardCodedTestModel.TestModel, HardCodedTestModel.GetPaintingType());

            var typeReference = result.Expression.ShouldBeSingleValueFunctionCallQueryNode("day")
-                .Parameters.Single().ShouldBeConstantQueryNode<object>(null).TypeReference;
+                .Parameters.Single().ShouldBeConvertQueryNode(EdmPrimitiveTypeKind.DateTimeOffset)


Not sure I understand how the change to FunctionCallBinder class results into the change to this test being required

I guess it's because day() is a function and the test was checking the parameter nodes type of that function. As the arguments are now "promoted", there is an extra hop via a convert node in the middle as day supposedly requires a Date or DateTime and null is considerated as Object. At least, that's my understanding but I'm far from knowning the OData inner workings, it's just an extrapolation based on the very little I saw.

- Removed check for ReturnType in FunctionCallBinder before promoting function arguments

- unwrapped ConvertQueryNode for FunctionCallQueryNodes parameters

pull-request-quantifier-deprecated · 2021-12-01T10:12:13Z

This PR has 19 quantified lines of changes. In general, a change size of upto 200 lines is ideal for the best PR experience!

Quantification details

Label      : Extra Small
Size       : +11 -8
Percentile : 7.6%

Total files changed: 4

Change summary by file extension:
.cs : +11 -8

Change counts above are quantified counts, based on the PullRequestQuantifier customizations.

Why proper sizing of changes matters

Optimal pull request sizes drive a better predictable PR flow as they strike a
balance between between PR complexity and PR review overhead. PRs within the
optimal size (typical small, or medium sized PRs) mean:

Fast and predictable releases to production:
- Optimal size changes are more likely to be reviewed faster with fewer
  iterations.
- Similarity in low PR complexity drives similar review times.
Review quality is likely higher as complexity is lower:
- Bugs are more likely to be detected.
- Code inconsistencies are more likely to be detetcted.
Knowledge sharing is improved within the participants:
- Small portions can be assimilated better.
Better engineering practices are exercised:
- Solving big problems by dividing them in well contained, smaller problems.
- Exercising separation of concerns within the code changes.

What can I do to optimize my changes

Use the PullRequestQuantifier to quantify your PR accurately
- Create a context profile for your repo using the context generator
- Exclude files that are not necessary to be reviewed or do not increase the review complexity. Example: Autogenerated code, docs, project IDE setting files, binaries, etc. Check out the Excluded section from your prquantifier.yaml context profile.
- Understand your typical change complexity, drive towards the desired complexity by adjusting the label mapping in your prquantifier.yaml context profile.
- Only use the labels that matter to you, see context specification to customize your prquantifier.yaml context profile.
Change your engineering behaviors
- For PRs that fall outside of the desired spectrum, review the details and check if:
  - Your PR could be split in smaller, self-contained PRs instead
  - Your PR only solves one particular issue. (For example, don't refactor and code new features in the same PR).

How to interpret the change counts in git diff output

One line was added: +1 -0
One line was deleted: +0 -1
One line was modified: +1 -1 (git diff doesn't know about modified, it will
interpret that line like one addition plus one deletion)
Change percentiles: Change characteristics (addition, deletion, modification)
of this PR in relation to all other PRs within the repository.

Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email)
Customize PullRequestQuantifier for this repository.

JBBianchi · 2021-12-01T10:13:13Z

@JBBianchi Thanks for your contribution. Please rebase your branch.

@ElizabethOkerio Did I do ok? I'm not so familiar with git tbh.

KenitoInc · 2021-12-01T11:10:20Z

@JBBianchi Thanks for your contribution. Please rebase your branch.

@ElizabethOkerio Did I do ok? I'm not so familiar with git tbh.

It looks fine.
Let's wait for the build to complete

gathogojr · 2021-12-01T11:36:03Z

@JBBianchi Thanks for your contribution

FunctionCallBinder promotes arguments without checking return type

8f7876f

- Removed check for ReturnType in FunctionCallBinder before promoting function arguments

pull-request-quantifier-deprecated bot added the Extra Small label Nov 22, 2021

xuzhg approved these changes Nov 23, 2021

View reviewed changes

JBBianchi force-pushed the master branch from 2dabae7 to 8f7876f Compare November 28, 2021 20:55

Unwrapped convert node in tests

10c4439

- unwrapped ConvertQueryNode for FunctionCallQueryNodes parameters

JBBianchi added 2 commits November 29, 2021 09:38

Unwrapped convert node in tests

fce9317

- unwrapped ConvertQueryNode for FunctionCallQueryNodes parameters

Merge branch 'master' of https://github.com/neuroglia-io/odata.net

b884e66

ElizabethOkerio requested review from habbes, gathogojr, KenitoInc and marabooy November 29, 2021 12:05

gathogojr approved these changes Nov 29, 2021

View reviewed changes

gathogojr reviewed Nov 30, 2021

View reviewed changes

JBBianchi and others added 4 commits November 30, 2021 09:14

Merge branch 'OData:master' into master

4fae397

FunctionCallBinder promotes arguments without checking return type

8311977

- Removed check for ReturnType in FunctionCallBinder before promoting function arguments

Unwrapped convert node in tests

4119c78

- unwrapped ConvertQueryNode for FunctionCallQueryNodes parameters

rebased?

ba73261

gathogojr merged commit 7bce571 into OData:master Dec 1, 2021

This was referenced Jan 25, 2024

issue while filtering on dynamic string property with tolower() function - Microsoft.AspNetCore.Odata -7.4.0 OData/WebApi#2173

Closed

filtering string type on dynamic properties not supported and causes misleading error message. OData/WebApi#1180

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FunctionCallBinder promotes arguments without checking return type #2257

FunctionCallBinder promotes arguments without checking return type #2257

JBBianchi commented Nov 22, 2021 •

edited

Loading

ghost commented Nov 29, 2021 •

edited by ghost

Loading

JBBianchi commented Nov 29, 2021 •

edited

Loading

ElizabethOkerio commented Nov 30, 2021

gathogojr Nov 30, 2021

JBBianchi Nov 30, 2021 •

edited

Loading

pull-request-quantifier-deprecated bot commented Dec 1, 2021

What can I do to optimize my changes

How to interpret the change counts in git diff output

JBBianchi commented Dec 1, 2021 •

edited

Loading

KenitoInc commented Dec 1, 2021

gathogojr commented Dec 1, 2021

FunctionCallBinder promotes arguments without checking return type #2257

FunctionCallBinder promotes arguments without checking return type #2257

Conversation

JBBianchi commented Nov 22, 2021 • edited Loading

Issues

Description

Checklist (Uncheck if it is not completed)

Additional work necessary

ghost commented Nov 29, 2021 • edited by ghost Loading

JBBianchi commented Nov 29, 2021 • edited Loading

ElizabethOkerio commented Nov 30, 2021

gathogojr Nov 30, 2021

Choose a reason for hiding this comment

JBBianchi Nov 30, 2021 • edited Loading

Choose a reason for hiding this comment

pull-request-quantifier-deprecated bot commented Dec 1, 2021

What can I do to optimize my changes

How to interpret the change counts in git diff output

JBBianchi commented Dec 1, 2021 • edited Loading

KenitoInc commented Dec 1, 2021

gathogojr commented Dec 1, 2021

JBBianchi commented Nov 22, 2021 •

edited

Loading

ghost commented Nov 29, 2021 •

edited by ghost

Loading

JBBianchi commented Nov 29, 2021 •

edited

Loading

JBBianchi Nov 30, 2021 •

edited

Loading

JBBianchi commented Dec 1, 2021 •

edited

Loading