Default to using entityType for incomplete serializationInfo. #2279

mikepizzo · 2021-12-16T05:20:26Z

Issues

This pull request fixes #2278

Description

ODL expects that, if you set SerializationInfo on the resource then you are taking responsibility for providing all of the information necessary for serialization.

When ODL tries to get key properties in order to append the key segment, it checks to see if the serializationInfo for the resource is null -- if so, it uses the model to determine key properties and all is good. On the other hand, if serializationInfo on the resource is non-null then it looks for properties whose serializationInfo identify them as key properties. Finding none, it errors.

This change makes ODL more robust by falling back to using the entityType to identify key properties if the resource has serializationInfo but none of the properties have serializationInfo that identifies them as key properties.

Checklist (Uncheck if it is not completed)

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

Additional work necessary

None

# Conflicts: # src/Microsoft.OData.Core/ODataWriterCore.cs

corranrogue9 · 2022-01-05T20:58:38Z

src/Microsoft.OData.Core/Evaluation/ODataResourceMetadataContext.cs

            KeyValuePair<string, object>[] keyProperties = null;
-            string actualEntityTypeName = null;
+            string actualEntityTypeName = resource.TypeName ?? actualEntityType?.FullName();


Do you know how important the old check for empty string is? We are missing that now, maybe this should be string.IsNullOrEmpty(resource.TypeName) ? actualEntityType?.FullName() : resource.TypeName #Resolved

It is unlikely this will be an empty string, but I like the robustness of your suggestion and will adopt it.

corranrogue9 · 2022-01-05T21:08:27Z

src/Microsoft.OData.Core/Evaluation/ODataResourceMetadataContext.cs

+
+            for (int keyProperty = 0; keyProperty < keyProperties.Length; keyProperty++)
+            {
+                if (keyProperties[keyProperty].Value == null || (keyProperties[keyProperty].Value is ODataValue && !(keyProperties[keyProperty].Value is ODataEnumValue)))


It appears that the compiler is not smart enough to optimize the multiple indexing operations (probably because nothing guarantees that there are no side effects):

Multiple indexes
Storing the indexed value

In the first example, the IL indicates that the indexing is performed a second time for the WriteLine call. The JITer might be able to optimize this, but there's really no way to know for sure. It would probably make the most sense to store keyProperties[keyProperty] in a variable scoped to the loop so that we only index the one time, instead of 4 times.
#Resolved

Good catch -- thanks!

corranrogue9 · 2022-01-05T21:13:14Z

src/Microsoft.OData.Core/Evaluation/ODataResourceMetadataContext.cs

                        {
-                            this.keyProperties = Enumerable.Empty<KeyValuePair<string, object>>().ToArray();
+                            return Enumerable.Empty<KeyValuePair<string, object>>().ToArray();


Probably not worth it as part of this change, but this will allocate a new array every time we return, even though all 0-length arrays are the same (we actually get warnings from the compiler for this). We could avoid unnecessary allocations and garbage collection with this small class:

public static class Array { public static T[] Empty<T>() { return Internal<T>.Empty; } private static class Internal<T> { public static T[] Empty = new T[0]; } }

New versions of .NET already have these singletons, but for back-compat we could write this one ourselves.

Yes; I tried to use Array.Empty(), but discovered it wasn't supported in .net45, so left it as it was. I agree we should go through and optimize everywhere we return empty arrays.

corranrogue9 · 2022-01-05T21:15:35Z

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

-                if (EdmExtensionMethods.HasKey(this.CurrentScope.NavigationSource, this.CurrentScope.ResourceType as IEdmStructuredType))
+                IEdmEntityType currentEntityType = this.CurrentScope.ResourceType as IEdmEntityType;
+                ODataResourceBase resource = this.CurrentScope.Item as ODataResourceBase;
+                KeyValuePair<string, object>[] keys = ODataResourceMetadataContext.GetKeyProperties(resource, null, currentEntityType, false);


GetKeyProperties seems to make the assumption that resource isn't null. I know that this isn't really related to your change, but not validating that currentEntityType is actually a IEdmEntityType means we could get a NullReferenceException here. #Resolved

This is our code and should never be called with a null resource. I added an assert to GetKeyProperties.

corranrogue9 · 2022-01-05T21:17:14Z

...a.Core.Tests/IntegrationTests/Evaluation/AutoComputePayloadMetadataInJsonIntegrationTests.cs

+            model.AddElements(new IEdmSchemaElement[] { entityType, nestedEntityType, container });
+
+            // setup writer
+            var stream = new MemoryStream();


MemoryStream implements IDisposable. Please wrap it in a using block #Resolved

pull-request-quantifier-deprecated · 2022-01-05T23:15:36Z

This PR has 186 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       : +146 -40
Percentile : 57.2%

Total files changed: 6

Change summary by file extension:
.cs : +146 -40

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.

Default to using entityType for incomplete serializationInfo.

47fd7e6

pull-request-quantifier-deprecated bot added the Medium label Dec 16, 2021

mikepizzo requested review from Sreejithpin and xuzhg December 16, 2021 21:01

xuzhg approved these changes Dec 16, 2021

View reviewed changes

pull-request-quantifier-deprecated bot added Large and removed Medium labels Dec 17, 2021

mikepizzo force-pushed the FixIncompleteSerializationInfo branch from a703224 to 3dd19f2 Compare December 17, 2021 00:50

Consolidate validation logic

a00e0a8

mikepizzo force-pushed the FixIncompleteSerializationInfo branch from 3dd19f2 to a00e0a8 Compare December 17, 2021 01:19

mikepizzo requested review from xuzhg December 17, 2021 02:26

pull-request-quantifier-deprecated bot added Medium and removed Large labels Jan 4, 2022

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

db2940e

# Conflicts: # src/Microsoft.OData.Core/ODataWriterCore.cs

mikepizzo force-pushed the FixIncompleteSerializationInfo branch from ddc5b50 to db2940e Compare January 4, 2022 20:37

mikepizzo added the Ready for review Use this label if a pull request is ready to be reviewed label Jan 4, 2022

corranrogue9 reviewed Jan 5, 2022

View reviewed changes

corranrogue9 approved these changes Jan 5, 2022

View reviewed changes

xuzhg approved these changes Jan 5, 2022

View reviewed changes

Address Code Review Comments.

1aa37b1

mikepizzo merged commit 88b4cf5 into master Jan 6, 2022

mikepizzo deleted the FixIncompleteSerializationInfo branch January 6, 2022 18:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Default to using entityType for incomplete serializationInfo. #2279

Default to using entityType for incomplete serializationInfo. #2279

mikepizzo commented Dec 16, 2021

corranrogue9 Jan 5, 2022 •

edited by mikepizzo

Loading

mikepizzo Jan 5, 2022

corranrogue9 Jan 5, 2022 •

edited by mikepizzo

Loading

mikepizzo Jan 5, 2022

corranrogue9 Jan 5, 2022

mikepizzo Jan 5, 2022

corranrogue9 Jan 5, 2022 •

edited by mikepizzo

Loading

mikepizzo Jan 5, 2022

corranrogue9 Jan 5, 2022 •

edited by mikepizzo

Loading

pull-request-quantifier-deprecated bot commented Jan 5, 2022

What can I do to optimize my changes

How to interpret the change counts in git diff output

Default to using entityType for incomplete serializationInfo. #2279

Default to using entityType for incomplete serializationInfo. #2279

Conversation

mikepizzo commented Dec 16, 2021

Issues

Description

Checklist (Uncheck if it is not completed)

Additional work necessary

corranrogue9 Jan 5, 2022 • edited by mikepizzo Loading

Choose a reason for hiding this comment

mikepizzo Jan 5, 2022

Choose a reason for hiding this comment

corranrogue9 Jan 5, 2022 • edited by mikepizzo Loading

Choose a reason for hiding this comment

mikepizzo Jan 5, 2022

Choose a reason for hiding this comment

corranrogue9 Jan 5, 2022

Choose a reason for hiding this comment

mikepizzo Jan 5, 2022

Choose a reason for hiding this comment

corranrogue9 Jan 5, 2022 • edited by mikepizzo Loading

Choose a reason for hiding this comment

mikepizzo Jan 5, 2022

Choose a reason for hiding this comment

corranrogue9 Jan 5, 2022 • edited by mikepizzo Loading

Choose a reason for hiding this comment

pull-request-quantifier-deprecated bot commented Jan 5, 2022

What can I do to optimize my changes

How to interpret the change counts in git diff output

corranrogue9 Jan 5, 2022 •

edited by mikepizzo

Loading

corranrogue9 Jan 5, 2022 •

edited by mikepizzo

Loading

corranrogue9 Jan 5, 2022 •

edited by mikepizzo

Loading

corranrogue9 Jan 5, 2022 •

edited by mikepizzo

Loading