Fix cast in expand

[KenitoInc]

Issues

This pull request fixes OData/AspNetCoreOData#445

Description

In #2160 we added support for the TypeSegment after NavigationPropertySegment.
Example: ~/Orders?$expand=Customer/Model.VipCustomer
This means that we will expand only the Customers that are an instance of Model.VipCustomer.
Note: Model.VipCustomer is a derived type of the Customer.

Something that was not captured in that PR is scenarios where we have query options affecting properties that are only in the derived type but not in the base type.

Examples:
~/Orders?$expand=Customer/Model.VipCustomer($filter=VipCustomerID eq '12345')
~/Orders?$expand=Customer/Model.VipCustomer($expand=VipCustomerOrders')
~/Orders?$expand=Customer/Model.VipCustomer($orderby=VipEmail)

This PR fixes that by ensuring when we are doing semantic binding, we can bind the properties in the derived type.

Checklist (Uncheck if it is not completed)

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

Additional work necessary

If documentation update is needed, please add "Docs Needed" label to the issue and provide details about the required document change in the issue.

[habbes]

Looks good to me, but I don't know enough of the inner working to ensure that the tests are sufficient.

Could you add some tests that verify that the sample queries you made actually return the expected results? And also tests to verify the validation (e.g. if the nav prop's type and the type segment's type are not related, the expected exception is thrown?

[habbes]

Out of curiosity, why does the elementTypeReference default to null when there's no derived type segment as opposed to defaulting to the type of the property being expanded? #Resolved

[KenitoInc]

It's null since we can get it from the expanded property in these lines

odata.net/src/Microsoft.OData.Core/UriParser/Binders/SelectExpandBinder.cs

Lines 924 to 929 in 1452522

	BindingState state = new BindingState(config)
	{
	ImplicitRangeVariable =
	NodeFactory.CreateImplicitRangeVariable(elementType != null ? elementType :
	targetNavigationSource.EntityType().ToTypeReference(), targetNavigationSource)
	};

[mikepizzo]

I see that we are getting the type in SelectExpandBinder, but what is the contract with BindFilter, BindOrderby, etc? These private functions take an elementType which gets passed eventually to CreateBindingState where the logic is to fall back to the navigationSource, if it's not otherwise specified. But it's not clear in the various private methods that the elementType is nullable, and future code could make invalid assumptions about whether it may be null. If that private CreateBindingState method is only reached through these methods, would it be cleaner to compute the elementType from the targetNavigationSource here so it is never null in subsequent methods? Alternatively, maybe at least clarify in the description of the elementType parameter to the various methods (i.e., line 529) "The target element type if different from the type of the target navigation source." Logic is sound, I just want us to consider what is the cleaner/more intuitive contract for maintainability.

[KenitoInc]

@mikepizzo I have updated the description.

[habbes]

Does this check ensure that the relationship is one way? i.e. would it return false if the arguments were reversed:

UriEdmHelpers.CheckRelatedTo(this.edmType, this.state.ImplicitRangeVariable.TypeReference.Definition)

[KenitoInc]

The order of the arguments doesn't matter

[habbes]

Not a blocker, but is there a test that confirms that by any chance?

[KenitoInc]

I have refactored that logic. We don't need to run that validation here since it's validated in the previous level binding i.e
In this example
Orders?$expand=Customer/NS.VipCustomer($select=VipCustomerName)
We are binding the $select. However validation has already happened in the previous level $expand=Customer/NS.VipCustomer

odata.net/src/Microsoft.OData.Core/UriParser/Binders/SelectExpandBinder.cs

Line 392 in a510d92

UriEdmHelpers.CheckRelatedTo(currentNavProp.ToEntityType(), derivedType);

Test are here

odata.net/test/FunctionalTests/Microsoft.OData.Core.Tests/ScenarioTests/UriParser/SelectExpandFunctionalTests.cs

Line 1802 in a510d92

public void ExpandWithNavigationPropWithUndefinedTypeThrows(string query)

[gathogojr]

LGTM

[mikepizzo]

// instead of lastSegment.TargetEdmType which references the Base Type EdmType.

---

Shouldn't the TargetEdmType of a TypeSegment be the EdmType?

Do we have other places in code where we make the same assumption (that the targetEdmType of a Type Segment is the actual EdmType, not the base type? #Resolved

[KenitoInc]

Reverted this change since it was solving one use case $expand=Prop1($expand=Prop2).
Handling this in the GenerateExpandItem method in the SelectExpandBinder

[mikepizzo]

Code looks good for addressing the issue, but please consider whether the targetTypeEdmType of a TypeSegment should be the actual type or the base type. Do we have other places in code that expects it to be the base type? Do we have other places in code that (erroneously) expects it to be the actual type?

---

In reply to: 1030201271

[mikepizzo]

        IEdmType targetElementType = lastSegment.TargetEdmType;

Do we need similar logic here for taking a cast segment into account for $select? Anywhere else?

---

In reply to: 1030205204

---

In reply to: 1030205204

---

Refers to: src/Microsoft.OData.Core/UriParser/Binders/SelectExpandBinder.cs:287 in 6a4c552. [](commit_id = 6a4c552, deletion_comment = False)

[mikepizzo]

        ComputeClause computeOption = BindCompute(tokenIn.ComputeOption, this.ResourcePathNavigationSource, targetNavigationSource);

Should we be passing elementTypeReference to BindCompute?

---

In reply to: 1030205801

---

In reply to: 1030205801

---

Refers to: src/Microsoft.OData.Core/UriParser/Binders/SelectExpandBinder.cs:450 in 8d62b20. [](commit_id = 8d62b20, deletion_comment = False)

[mikepizzo]

🕐

[KenitoInc]

        ComputeClause computeOption = BindCompute(tokenIn.ComputeOption, this.ResourcePathNavigationSource, targetNavigationSource);

Should we be passing elementTypeReference to BindCompute?

Refers to: src/Microsoft.OData.Core/UriParser/Binders/SelectExpandBinder.cs:450 in aecf2af. [](commit_id = aecf2af, deletion_comment = False)

I have fixed this

[habbes]

The code looks good. I think adding some e2e tests that verify that the expected data is returned from a request would be great.

[mikepizzo]

        IEdmType targetElementType = lastSegment.TargetEdmType;

Is it as simple as changing this to lastSegment.EdmType?

---

In reply to: 1030205204

---

Refers to: src/Microsoft.OData.Core/UriParser/Binders/SelectExpandBinder.cs:287 in 8d62b20. [](commit_id = 8d62b20, deletion_comment = False)

[pull-request-quantifier-deprecated]

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

---

Quantification details

Label      : Medium
Size       : +138 -10
Percentile : 49.6%

Total files changed: 4

Change summary by file extension:
.cs : +138 -10

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.

[mikepizzo]