Add flag to skip property verification in ODataResource

[habbes]

Issues

This pull request fixes #2803

Description

This PR adds a SkipPropertyVerification property to ODataResource which skips property verification. By default, ODataResource will verify the property values when the Properties property is set. The aim of the of the verification is to ensure that properties ODataResourceValues are not included in the properties. I.e. the property values should either primitive types or collections of primitive types (ODataResourceValue is intended for use in annotations).

However, in scenarios like AspNetCoreOData, where the library already creates ODataResource that already match this expectation, it's an unnecessary cost to still go ahead and perform this verification. This flag can be used in AspNetCoreOData to avoid paying this cost for every resource that is created.

Checklist (Uncheck if it is not completed)

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

[pull-request-quantifier-deprecated]

This PR has 32 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       : +31 -1
Percentile : 12.8%

Total files changed: 9

Change summary by file extension:
.cs : +20 -1
.txt : +8 -0
.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 detected.
• 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.

[gathogojr]

Wouldn't this be better as a global setting (e.g. ODataMessageWriterSettings/ODataMessageReaderSettings) instead of a property that has to be set on each ODataResource instance?

[habbes]

Problem is ODataResource doesn't have access to the message writer settings.

[habbes]

Maybe the serializer that creates ODataResources could use the ODataMessageWritter settings to decide whether to set the skip verification flag, but in that case it would still have to set it for every resource.

[gathogojr]

"it would still have to set it for every resource" - Why would it need to be set for every resource? Why can't the serializer control whether the property verification methods should be called? I don't think it's any of the ODataResource methods that calls methods for verifying or writing the properties. It's the serializer that does so... If it has access to the settings, it can control whether to skip verification or not.

A different question, by which means did you envision that the SkipPropertyVerification would be set?

[KenitoInc]

I agree with @gathogojr
Can we use the same setup as the IDuplicatePropertyNameChecker where the serializer have access to the ODataMessageWriteSettings and this set in the WriterValidator

[habbes]

@gathogojr

I don't think it's any of the ODataResource methods that calls methods for verifying or writing the properties. It's the serializer that does so... If

In the existing code, it is actually the ODataResource that calls the method for verifying properties, it happens inside the ODataResourceBase.Properties setter. That's why I applied the flag to the ODataResource.

Each ODataResource instance is independent, and the ODataMessageWriter operates on a resource-per-resource basis. The ODataMessageWriter does not know about the serializer.

@KenitoInc does have access to the the ODataMessageWriterSettings but the ODataResource does not. Currently the verification happens within the ODataResource itself, not the serializer nor the message writer.

We could change things up such that the verification is moved out of the ODataResource and moved into the message writer. Or we could remove the verification altogether and let the serializer handle that manually if it deems fit. I'm not sure if this delayed verification would have unintended consequences in those where verification is desired.

[habbes]

To make a correction, the verification ensures that the property values are not ODataResourceValue instances. Apparently ODataResourceValue is different from ODataResource. An ODataResourceValue is an ODataValue that has nested properties, so basically still a nested resource. It's intended for use in instance annotations that have complex values.

I tried to create an ODataResource which nested ODataResourceValues in its properties after disabling verification, and it got serialized without error. This means that the writer does not validate against this. So, if we remove the validation from within ODataResource and want to retain the existing behaviour, then we'd need to perform this verification in the writer or ask the caller to handle it on their own.