fixes #2785: support property instance annotation without property value #2786

xuzhg · 2023-11-06T17:31:33Z

Issues

This pull request fixes #2785.

Description

Can write property instance annotations without property value.
Can read property instance annotations without property value.

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.

xuzhg · 2023-11-13T04:09:35Z

@mikepizzo @gathogojr @habbes @ElizabethOkerio @KenitoInc

gathogojr · 2023-11-21T05:02:17Z

test/FunctionalTests/Tests/DataOData/Tests/OData.Reader.Tests/Reader/PropertyReaderTests.cs

-                                ExpectedException = tc.Format == ODataFormat.Json
-                                        ? ODataExpectedExceptions.ODataException("ODataJsonLightResourceDeserializer_PropertyWithoutValueWithWrongType", "NonStreamProperty", "Edm.Boolean")
-                                        : ODataExpectedExceptions.ODataException("JsonReaderExtensions_UnexpectedNodeDetected", "PrimitiveValue", "StartObject")
+                                //ExpectedException = tc.Format == ODataFormat.Json


Remove commented code #Resolved

gathogojr · 2023-11-21T05:02:33Z

...unctionalTests/Tests/DataOData/Tests/OData.Reader.Tests/Reader/AssociationLinkReaderTests.cs

-                                        (tc.Format == ODataFormat.Json)
-                                        ? ODataExpectedExceptions.ODataException("ODataJsonLightResourceDeserializer_PropertyWithoutValueWithWrongType", "Name", "Edm.String")
-                                        : ODataExpectedExceptions.ODataException("ValidationUtils_NavigationPropertyExpected", "Name", "TestModel.CityType", "Structural"),
+                                    //ExpectedException = 


Remove commented code #Resolved

gathogojr · 2023-11-21T05:02:46Z

...sts/DataOData/Tests/OData.Reader.Tests/JsonLight/StreamReferenceValueReaderJsonLightTests.cs

                    Json = 
                        "\"" + JsonLightUtils.GetPropertyAnnotationName("Name", JsonLightConstants.ODataMediaEditLinkAnnotationName) + "\":\"http://odata.org/streamproperty/editlink\"",
-                    ExpectedException = ODataExpectedExceptions.ODataException("ODataJsonLightResourceDeserializer_PropertyWithoutValueWithWrongType", "Name", "Edm.String"),
+                   // ExpectedException = ODataExpectedExceptions.ODataException("ODataJsonLightResourceDeserializer_PropertyWithoutValueWithWrongType", "Name", "Edm.String"),


Remove commented code #Resolved

gathogojr · 2023-11-21T05:28:35Z

src/Microsoft.OData.Core/JsonLight/ODataJsonLightResourceDeserializer.cs

+
+            IEdmTypeReference propertyType = property?.Type;
+            IDictionary<string, object> odataAnnotations = resourceState.PropertyAndAnnotationCollector.GetODataPropertyAnnotations(propertyName);
+            IList<KeyValuePair<string, object>> propertyAnnotations = resourceState.PropertyAndAnnotationCollector.GetCustomPropertyAnnotations(propertyName).ToList();


Suggested change

IList<KeyValuePair<string, object>> propertyAnnotations = resourceState.PropertyAndAnnotationCollector.GetCustomPropertyAnnotations(propertyName).ToList();

IList<KeyValuePair<string, object>> customAnnotations = resourceState.PropertyAndAnnotationCollector.GetCustomPropertyAnnotations(propertyName).ToList();

``` #Resolved

gathogojr · 2023-11-21T05:30:18Z

src/Microsoft.OData.Core/JsonLight/ODataJsonLightResourceDeserializer.cs

+            IEdmTypeReference propertyType = property?.Type;
+            IDictionary<string, object> odataAnnotations = resourceState.PropertyAndAnnotationCollector.GetODataPropertyAnnotations(propertyName);
+            IList<KeyValuePair<string, object>> propertyAnnotations = resourceState.PropertyAndAnnotationCollector.GetCustomPropertyAnnotations(propertyName).ToList();
+            if ((odataAnnotations.Count + propertyAnnotations.Count) == 0)


Why do you need to do the addition while you can use:

if (odataAnnotations.Count == 0 && propertyAnnotations.Count == 0) ``` #Resolved

gathogojr · 2023-11-21T05:36:51Z

src/Microsoft.OData.Core/JsonLight/ODataJsonLightResourceDeserializer.cs

@@ -1706,6 +1756,7 @@ private ODataJsonLightReaderNestedInfo ReadUndeclaredProperty(IODataJsonLightRea
            // Property without a value can't be ignored if we don't know what it is.
            if (!propertyWithValue)
            {
+                // TODO: Shall we return ODataJsonLightReaderNestedPropertyInfo for a dynamic property without value?


Question or gap? Do you need to create an issue to track this? #Resolved

No, That's handled in Line 1752 and I should remove this comment.

gathogojr · 2023-11-21T05:46:20Z

...Tests/Microsoft.OData.Core.Tests/ScenarioTests/Streaming/ODataJsonLightStreamWritingTests.cs

+            },
+            expectedPayload);
+        }
+


Add tests for the following code block in the ReadNestedPropertyInfoWithoutValue method:

if ((odataAnnotations.Count + propertyAnnotations.Count) == 0) { // If we don't have any annotations for the property, it could contain errors. if (property != null) { throw new ODataException(ODataErrorStrings.ODataJsonLightResourceDeserializer_PropertyWithoutValueWithWrongType(propertyName, propertyType.FullName())); } else { // it's a dynamic property throw new ODataException(ODataErrorStrings.ODataJsonLightResourceDeserializer_OpenPropertyWithoutValue(propertyName)); } }

Part of me thinks it's dead code. I don't see how we'd get here if the reader doesn't come across an OData or custom property annotation.

@xuzhg What's your reaction to the above comment?

I'd like to keep those codes; I do remember It throws exception for dynamic property.

@xuzhg If that is the case, can we add a test for that scenario?

It's baffling though since property without value would be in the form of "property@custom.annotation":"#ANNOTATION_VALUE#" or property@odata.annotation="#ANNOTATION_VALUE#".
I believe even in the case of dynamic property we'd have something like "dynamicproperty@custom.annotation":"#ANNOTATION_VALUE#" or dynamicproperty@odata.annotation="#ANNOTATION_VALUE#".

How we'd get here only to find that odataAnnotations.Count == 0 && propertyAnnotations.Count == 0 I'm still not sure.

I am curious about the scenario: a property:

without value

without odata instance annotation

without custom instance annotation

Let me remove it and see what will happen

gathogojr · 2023-11-21T05:49:35Z

...Tests/Microsoft.OData.Core.Tests/ScenarioTests/Streaming/ODataJsonLightStreamWritingTests.cs

+                writer.WriteStart(resource);
+
+                writer.WriteStart(propertyInfo);
+                    writer.WritePrimitive(new ODataPrimitiveValue(37));


Fix indentation #WontFix

Indent intentionally.

@xuzhg I don't think this approach is sustainable. The indentation will most probably be undone the next time VS auto-formats the contents of this file

auto-format automatically runs? We have lots of such intentionally indent in our test codes, so far, I haven't seen any un-done.

@xuzhg I meant that the next contributor who "touches" that file and triggers the formatter (which often happens), the indentation will be undone. Though I understand what you're trying to visually communicate by applying the indentation.

We should review the chances and let the contributor to revert it.

gathogojr · 2023-11-21T05:52:31Z

...Tests/Microsoft.OData.Core.Tests/ScenarioTests/Streaming/ODataJsonLightStreamWritingTests.cs

+            },
+            expectedPayload);
+        }
+


@xuzhg There at least 3 places where the new ReadNestedPropertyInfoWithoutValue is called. Do the two tests cover all 3 calls? #Pending

Do you mean the 'readingTests.cs'?

So far, the reader tests added covers normal property and dynamic property scenarios.
But, I didn't add the test cases using Async. I tried to find a place to add 'async' test cases, but I can't find it. Do you know where or which file to add async is better?

@xuzhg The async tests for ODataJsonLightReader are here https://github.com/OData/odata.net/blob/master/test/FunctionalTests/Microsoft.OData.Core.Tests/JsonLight/ODataJsonLightReaderTests.cs.
Other async tests for ODataJsonLightResourceDeserializer are here https://github.com/OData/odata.net/blob/master/test/FunctionalTests/Microsoft.OData.Core.Tests/JsonLight/ODataJsonLightResourceDeserializerTests.cs

@gathogojr I added async test cases to verify the async scenarios. Please take a look again.

gathogojr · 2023-11-21T06:14:10Z

src/Microsoft.OData.Core/JsonLight/ODataJsonLightResourceDeserializer.cs

+            foreach (KeyValuePair<string, object> annotation in propertyAnnotations)
+            {
+                 // annotation.Value == null indicates that this annotation should be skipped?
+                 propertyInfo.InstanceAnnotations.Add(new ODataInstanceAnnotation(annotation.Key, annotation.Value.ToODataValue()));
+            }


@xuzhg This code for adding the custom property annotations is repeated in 3 places in the base class and now here. Wouldn't it make more sense to add a protected static method for that - just like AttachODataAnnotations - to avoid the code duplication? #WontFix

So far, I think the codes are only couple lines and it's easy to maintain. But, I agree if we have a helper method to maintain them together, it could be be. I'd like to leave it to a new PR to improve this.

1) there's not value 2) there's no odata instance annotation 3) There's no custom instance annotation

pull-request-quantifier-deprecated · 2023-11-30T18:31:14Z

This PR has 378 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       : +298 -80
Percentile : 77.8%

Total files changed: 13

Change summary by file extension:
.cs : +298 -80

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

fixes #2785: support property instance annotation without property value

d2a6f17

pull-request-quantifier-deprecated bot added the Large label Nov 6, 2023

xuzhg requested review from KenitoInc, habbes, mikepizzo, ElizabethOkerio and gathogojr November 6, 2023 17:31

xuzhg added 2 commits November 10, 2023 12:23

Enable dynamic property without value

0e164bc

Fixes the failing test cases

a2d6f8d

gathogojr reviewed Nov 21, 2023

View reviewed changes

xuzhg added 3 commits November 27, 2023 11:46

Add the comments

af842ec

Add async test cases

02ca9f6

What scenario could be like a property in payload meets:

bab4ea6

1) there's not value 2) there's no odata instance annotation 3) There's no custom instance annotation

gathogojr approved these changes Dec 1, 2023

View reviewed changes

xuzhg merged commit 0209fb9 into master Dec 1, 2023
4 checks passed

xuzhg deleted the issue2785 branch December 19, 2023 18:54

gathogojr mentioned this pull request Apr 3, 2024

Change ODataResource.Properties property type to IEnumerable<ODataPropertyInfo> #2916

Merged

2 tasks

gathogojr mentioned this pull request Sep 11, 2024

Fix subtle bug when deserializing property without value OData/AspNetCoreOData#1308

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixes #2785: support property instance annotation without property value #2786

fixes #2785: support property instance annotation without property value #2786

xuzhg commented Nov 6, 2023

xuzhg commented Nov 13, 2023

gathogojr Nov 21, 2023 •

edited by xuzhg

Loading

gathogojr Nov 21, 2023 •

edited by xuzhg

Loading

gathogojr Nov 21, 2023 •

edited by xuzhg

Loading

gathogojr Nov 21, 2023 •

edited by xuzhg

Loading

gathogojr Nov 21, 2023 •

edited by xuzhg

Loading

gathogojr Nov 21, 2023 •

edited by xuzhg

Loading

xuzhg Nov 27, 2023

gathogojr Nov 21, 2023

gathogojr Nov 28, 2023

xuzhg Nov 30, 2023

gathogojr Nov 30, 2023

gathogojr Nov 30, 2023 •

edited

Loading

xuzhg Nov 30, 2023

gathogojr Nov 21, 2023 •

edited by xuzhg

Loading

xuzhg Nov 27, 2023

gathogojr Nov 28, 2023

xuzhg Nov 29, 2023

gathogojr Nov 30, 2023 •

edited

Loading

xuzhg Nov 30, 2023

gathogojr Nov 21, 2023 •

edited by xuzhg

Loading

xuzhg Nov 27, 2023

xuzhg Nov 27, 2023

gathogojr Nov 28, 2023 •

edited

Loading

xuzhg Nov 29, 2023

gathogojr Nov 21, 2023 •

edited by xuzhg

Loading

xuzhg Nov 27, 2023

pull-request-quantifier-deprecated bot commented Nov 30, 2023

What can I do to optimize my changes

How to interpret the change counts in git diff output

gathogojr left a comment

	IList<KeyValuePair<string, object>> propertyAnnotations = resourceState.PropertyAndAnnotationCollector.GetCustomPropertyAnnotations(propertyName).ToList();
	IList<KeyValuePair<string, object>> customAnnotations = resourceState.PropertyAndAnnotationCollector.GetCustomPropertyAnnotations(propertyName).ToList();
	``` #Resolved

fixes #2785: support property instance annotation without property value #2786

fixes #2785: support property instance annotation without property value #2786

Conversation

xuzhg commented Nov 6, 2023

Issues

Description

Checklist (Uncheck if it is not completed)

Additional work necessary

xuzhg commented Nov 13, 2023

gathogojr Nov 21, 2023 • edited by xuzhg Loading

Choose a reason for hiding this comment

gathogojr Nov 21, 2023 • edited by xuzhg Loading

Choose a reason for hiding this comment

gathogojr Nov 21, 2023 • edited by xuzhg Loading

Choose a reason for hiding this comment

gathogojr Nov 21, 2023 • edited by xuzhg Loading

Choose a reason for hiding this comment

gathogojr Nov 21, 2023 • edited by xuzhg Loading

Choose a reason for hiding this comment

gathogojr Nov 21, 2023 • edited by xuzhg Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gathogojr Nov 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gathogojr Nov 21, 2023 • edited by xuzhg Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gathogojr Nov 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gathogojr Nov 21, 2023 • edited by xuzhg Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gathogojr Nov 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gathogojr Nov 21, 2023 • edited by xuzhg Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pull-request-quantifier-deprecated bot commented Nov 30, 2023

What can I do to optimize my changes

How to interpret the change counts in git diff output

gathogojr left a comment

Choose a reason for hiding this comment

gathogojr Nov 21, 2023 •

edited by xuzhg

Loading

gathogojr Nov 21, 2023 •

edited by xuzhg

Loading

gathogojr Nov 21, 2023 •

edited by xuzhg

Loading

gathogojr Nov 21, 2023 •

edited by xuzhg

Loading

gathogojr Nov 21, 2023 •

edited by xuzhg

Loading

gathogojr Nov 21, 2023 •

edited by xuzhg

Loading

gathogojr Nov 30, 2023 •

edited

Loading

gathogojr Nov 21, 2023 •

edited by xuzhg

Loading

gathogojr Nov 30, 2023 •

edited

Loading

gathogojr Nov 21, 2023 •

edited by xuzhg

Loading

gathogojr Nov 28, 2023 •

edited

Loading

gathogojr Nov 21, 2023 •

edited by xuzhg

Loading