TryGetPropertyValue on a nested ComplexType returns T, should be Delta<T>

[dxrdxr]

Issues

This pull request fixes #2500.

Description

As discussed in the issue, fix TrySetPropertyValue to return Delta<T> when a nested complex property.

Checklist (Uncheck if it is not completed)

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

Additional work necessary

Docs Needed

In addition to the functionality changes the following was also done:

• The Delta code in OData/WebAPI and OData/AspNetCoreOData are almost identical in functionality but have minor differences that show slow divergence.
• Minor code changes made to versions of DeltaOfT*.cs to bring back to being as close as possible.
• In addition to the tests added for this functionality change, ported all (DeltaTests) tests in OData/AspNetCoreOData missing from WebAPI.
• Minor code changes made to versions of DeltaTests.cs to bring back to being as close as possible.

[Sreejithpin]

Couple of things

1. This is done for TryGetPropertyValue and not TrySetPropertyvalue (as mentioned in PR)
2. This used to get value, and probably the purpose is same , IF we change it it would be a breaking change as well, right?
   @mikepizzo Please see

[dxrdxr]

@Sreejithpin

For 1) You are correct. Existing behavior is that the caller can "set" a nested property as a value T or as a Delta<T>. I have not changed that behavior. I can accept an argument that it setting a nested property as a value T should not be allowed because this is contrary to the Delta model of only updating the properties that change. Setting a nested property by value T results in all the properties within it to be updated. I decided not to change this existing behavior as I don't have any insights on how often any caller does a "set" by value (I think it is rare based on my investigation).

For 2) it is a change in behavior. Set by value T continues to Get by value T. However, Set by Delta<T> now returns Delta<T>. Previously it returned value T type. But the actual object returned by Get would be a combination of underlying properties that had been changed by the Delta<T> and the unchanged properties having default values. The caller of Get cannot distinguish between the two cases, thus the current behavior is at best broken and at worst unusable. Lastly, in the cast of deeply nested properties, it is impossible to determine if they have been changed or not.

From my limited investigation existing use in the OData repos, there are no caller to Set a nested property by value T. Additionally no callers to Get a nested property. For external code samples, the Get is used most often to implement a Patch() that only updates selective properties. This will "break" if that code is expecting a value T, but again, I don't know how that code could work correctly as doing an update by value T would be updating unchanged underlying properties to be default value.

As I said in the related issue, a root cause of the problem is that the documentation incorrectly states that the Delta<T> instance contains BOTH the changed values and the original values are returned by GetInstance()

Returns the instance that holds all the changes (and original values)

Likewise in the Delta documentation for TryGetPropertyValue states

Both modified and unmodified properties can be retrieved" which is not true, as unmodified values are not available and cannot be returned

---

In reply to: 926826747

[mikepizzo]

Yes, this is broken, and I want to take the fix, but we can't risk breaking existing applications.

We should definitely fix this in 8.x. For 7.x, can we do something like add a TryGetPropertyValue2 that does the right thing (and then use that in our serialization)?

---

In reply to: 926826747

[dxrdxr]

@mikepizzo
I will recode the PR to include the old and new interfaces. I will use TryGetPropertyValue2 or TryGetNestedPropertyValue?

Proposed backwards compatible fix:

• Implement TryGetNestedPropertyValue but with the restricted semantic that it will only return true if the property is a nested property (IDelta)
• Mark TryGetNestedPropertyValue as [Obsolete] indicating that it will go away in the next major version

Rationale: The existing API is only broken in one specific narrow scenario that is not likely used very often. If we create a new API that supports the full existing semantics plus the fix, developers may switch to the new API for all scenarios thinking that it is the expected future direction, even if they are not using it for nested properties. This could result in code unnecessarily migrating to the new API then having to migrate back in the next major version, this is inertia to keep the two APIs for longer than desired.

By creating a limited API, only developers who need the fixed semantic will opt-in and the Obsolete attribute will indicate that this is just a temporary fix and only these calls need to be updated in the future.

We also have to port this to AspNetCoreOData.

In the 8.x timeframe the Delta code needs to be refactored to remove duplication. In my branches I have greatly minimized the differences in DeltaOfT and DeltaTests, but they will get out of sync quickly.

Edit: Do not mark as obsolete

[corranrogue9]

• Mark TryGetNestedPropertyValue as [Obsolete] indicating that it will go away in the next major version

I think it would be ok to let this method continue to exist, but be a passthrough implementation in the next major version, reducing the friction for clients who need these changes now.

[dxrdxr]

• Mark TryGetNestedPropertyValue as [Obsolete] indicating that it will go away in the next major version

I think it would be ok to let this method continue to exist, but be a passthrough implementation in the next major version, reducing the friction for clients who need these changes now.

Did not add ObsoleteAttribute.

[pull-request-quantifier-deprecated]

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

---

Quantification details

Label      : Large
Size       : +296 -51
Percentile : 74.7%

Total files changed: 6

Change summary by file extension:
.cs : +293 -51
.bsl : +3 -0

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.

[dxrdxr]

@mikepizzo Please review

---

In reply to: 974882988

[mikepizzo]

Changes look good -- thanks @dxrdxr!

@xuzhg -- Since 8.x is still fairly new, I'm okay fixing TryGetPropertyValue in 8.x without adding the new TryGetNestedPropertyValue. What do you think? If we were really worried about breaking backward compat we could add a flag (off by default) in 8.x to retain the old behavior.