Fix Auto-Expand Regression #2585

ificator · 2021-10-11T02:42:28Z

Issues

Fixes #2584

Description

Make "GetAutoSelectPaths" and "GetAutoExpandPaths" behave like "GetAutoSelectProperties" when "baseEntityType" is null
Avoid some unnecessary allocations

Checklist (Uncheck if it is not completed)

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

…tAutoSelectProperties" when "baseEntityType" is null 2. Avoid some unnecessary allocations

src/Microsoft.AspNet.OData.Shared/Query/AutoSelectExpandHelper.cs

corranrogue9 · 2021-10-14T23:49:20Z

src/Microsoft.AspNet.OData.Shared/Query/AutoSelectExpandHelper.cs

+            {
+                switch (this.stage)
+                {
+                    case 0:


Can't this just be:

case 0: this.stage = 1; return true; default: return this.derivedEnumerator.MoveNext();

The compiler might be optimizing that out, but I think it will be faster and it's easier to read

src/Microsoft.AspNet.OData.Shared/Query/AutoSelectExpandHelper.cs

KenitoInc · 2021-11-01T08:51:02Z

@ificator Kindly fix conflicts

ificator · 2021-11-01T20:23:45Z

@KenitoInc it looks like #2588 fixed the original issue (null baseEntityType asserting when it previously didn't), so I guess the question is do we want my refactoring to avoid allocations? If so, I'll merge those changes in and convert this PR into a PR specifically for that.

KenitoInc · 2021-11-02T07:12:34Z

@KenitoInc it looks like #2588 fixed the original issue (null baseEntityType asserting when it previously didn't), so I guess the question is do we want my refactoring to avoid allocations? If so, I'll merge those changes in and convert this PR into a PR specifically for that.

If we are avoiding allocations for performance improvement, then that's fine.

marabooy · 2021-11-19T07:40:24Z

@ificator could you kindly rebase your changes to master so we can proceed with resolving this issue. Many thanks for your contribution.

pull-request-quantifier-deprecated · 2021-11-21T21:47:24Z

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

Quantification details

Label      : Small
Size       : +60 -24
Percentile : 33.6%

Total files changed: 3

Change summary by file extension:
.cs : +60 -24

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.

corranrogue9 · 2021-12-01T00:52:19Z

src/Microsoft.AspNet.OData.Shared/Query/AutoSelectExpandHelper.cs

@@ -175,6 +164,11 @@ private static bool HasAutoExpandProperty(this IEdmModel edmModel, IEdmStructure
                {
                    if (IsAutoSelect(property, pathProperty, edmStructuredType, edmModel, querySettings))
                    {
+                        if (autoSelectProperties == null)
+                        {
+                            autoSelectProperties = new List<SelectModelPath>(1);


I don't see the value in having to perform this null check every iteration. It probably makes more sense to just put line 160 back

corranrogue9 · 2021-12-01T00:53:49Z

src/Microsoft.AspNet.OData.Shared/Query/AutoSelectExpandHelper.cs

+
+            public SelfAndDerivedEnumerator(IEdmStructuredType structuredType, IEdmModel edmModel)
+            {
+                if (structuredType == null)


Given that this is all internal, these null checks probably make more sense as Debug.Asserts

Sreejithpin · 2021-12-07T18:49:31Z

Hi @ificator , will be great if you can look at comments and rebase

mikepizzo

KenitoInc · 2022-01-09T16:33:51Z

@ificator Thanks for this PR

1. Make "GetAutoSelectPaths" and "GetAutoExpandPaths" behave like "Ge…

944a843

…tAutoSelectProperties" when "baseEntityType" is null 2. Avoid some unnecessary allocations

pull-request-quantifier-deprecated bot added the Medium label Oct 11, 2021

xuzhg mentioned this pull request Oct 11, 2021

8.0.3 Broke Expand OData/AspNetCoreOData#330

Open

corranrogue9 reviewed Oct 14, 2021

View reviewed changes

src/Microsoft.AspNet.OData.Shared/Query/AutoSelectExpandHelper.cs Show resolved Hide resolved

corranrogue9 reviewed Oct 14, 2021

View reviewed changes

src/Microsoft.AspNet.OData.Shared/Query/AutoSelectExpandHelper.cs Show resolved Hide resolved

corranrogue9 reviewed Oct 14, 2021

View reviewed changes

src/Microsoft.AspNet.OData.Shared/Query/AutoSelectExpandHelper.cs Outdated Show resolved Hide resolved

corranrogue9 approved these changes Oct 18, 2021

View reviewed changes

corranrogue9 mentioned this pull request Oct 18, 2021

Auto-Expand code uses a loop when unnecessary #2589

Open

KenitoInc requested review from xuzhg, KenitoInc, chrisspre, ElizabethOkerio, gathogojr, habbes and marabooy October 29, 2021 08:10

KenitoInc approved these changes Nov 1, 2021

View reviewed changes

ificator added 3 commits November 21, 2021 12:26

Merge upstream/master

9ae0aaa

Merge in other nullref fix, keep optimization

55c3f50

Missed use of integer instead of enum

8b02339

pull-request-quantifier-deprecated bot added Small and removed Medium labels Nov 21, 2021

marabooy approved these changes Nov 22, 2021

View reviewed changes

corranrogue9 reviewed Dec 1, 2021

View reviewed changes

mikepizzo approved these changes Dec 14, 2021

View reviewed changes

KenitoInc merged commit 46a0d5a into OData:master Jan 9, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Auto-Expand Regression #2585

Fix Auto-Expand Regression #2585

ificator commented Oct 11, 2021

corranrogue9 Oct 14, 2021

KenitoInc commented Nov 1, 2021

ificator commented Nov 1, 2021

KenitoInc commented Nov 2, 2021

marabooy commented Nov 19, 2021

pull-request-quantifier-deprecated bot commented Nov 21, 2021

What can I do to optimize my changes

How to interpret the change counts in git diff output

corranrogue9 Dec 1, 2021

corranrogue9 Dec 1, 2021

Sreejithpin commented Dec 7, 2021

mikepizzo left a comment

KenitoInc commented Jan 9, 2022

Fix Auto-Expand Regression #2585

Fix Auto-Expand Regression #2585

Conversation

ificator commented Oct 11, 2021

Issues

Description

Checklist (Uncheck if it is not completed)

corranrogue9 Oct 14, 2021

Choose a reason for hiding this comment

KenitoInc commented Nov 1, 2021

ificator commented Nov 1, 2021

KenitoInc commented Nov 2, 2021

marabooy commented Nov 19, 2021

pull-request-quantifier-deprecated bot commented Nov 21, 2021

What can I do to optimize my changes

How to interpret the change counts in git diff output

corranrogue9 Dec 1, 2021

Choose a reason for hiding this comment

corranrogue9 Dec 1, 2021

Choose a reason for hiding this comment

Sreejithpin commented Dec 7, 2021

mikepizzo left a comment

Choose a reason for hiding this comment

KenitoInc commented Jan 9, 2022