Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PR For Bulk Operation and ODataBind Review for Beta Release #2567

Open
wants to merge 28 commits into
base: master
Choose a base branch
from

Conversation

Sreejithpin
Copy link
Contributor

Issues

Used instead of PR #2524
PR For Bulk Operation and ODataBind Review for Beta Release

Description

PR For Bulk Operation and ODataBind Review for Beta Release

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.

/// Handler Class to handle users methods for create, delete and update.
/// This is the handler for data modification where there is no CLR type.
/// </summary>
public abstract class EdmODataAPIHandler
Copy link
Member

@mikepizzo mikepizzo Sep 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EdmODataAPIHandler

please rename file to match class name #Resolved

/// This is the default Patch Handler for non CLR type. This calss has default Get, Create and Update
/// and will do these actions. This will be used when the original collection to be Patched is provided.
/// </summary>
internal class DefaultEdmODataAPIHandler : EdmODataAPIHandler
Copy link
Member

@mikepizzo mikepizzo Sep 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DefaultEdmODataAPIHandler

Do we really have both a DefaultEdmPatchMethodHandler and a DefaultODataAPIHandler? #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No we dont have any DefaultEdmPatchMethodHandler
we have 2 defaults, 1 for typed another for typeles

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I was confused because the name of the file was still "DefaultEdmPatchMethodHandler"

/// <summary>
/// Navigation Path of an OData ID
/// </summary>
public class NavigationPath
Copy link
Member

@mikepizzo mikepizzo Sep 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NavigationPath

Maybe call it "ODataNavigationPath" for consistency? #Pending

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ODL has ODataExpandPath and ODataSelectPath, does it can meet your requirement?

private ConcurrentDictionary<string, PathItem[]> _pathItemCache = new ConcurrentDictionary<string, PathItem[]>();

/// <summary>
/// Constructor which takes and odataId and creates PathItems
Copy link
Member

@mikepizzo mikepizzo Sep 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

which takes and odataId and creates PathItems

please update description #Resolved

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check other class:

mainly use the sentence like: "Initializes a new instance of the class."

/// <summary>
/// If the item is a cast segment, Name of the Path Item (eg: derived entity name, entity set name)
/// </summary>
public string CastTypeName { internal set; get; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CastTypeName

why do we have a different property for CastTypeName? Why can't we use "Name" if it's a cast segment?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would have different values, one generic and another the cast type name think it could be used by developer for generic case and then specific cast case, eg: Name = NewFriends, CastTypeName = Microsoft.Test.E2E.AspNet.OData.BulkInsert.MyNewFriend

/// <summary>
/// Whether the PathItem is a cast segment
/// </summary>
public bool IsCastType { internal set; get; }
Copy link
Member

@mikepizzo mikepizzo Sep 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IsCastType

Maybe have a PathItemType that is "castSegmet", "pathSegment", "instanceSegment"

/// <summary>
/// List of Key properties of that entity
/// </summary>
public Dictionary<string, object> KeyProperties { internal set; get; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

KeyProperties

Should we define a derived type with keyProperties or just have null when not relevant?

{
navigationPathName = navigationPath;
_pathSegments = pathSegments;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we have a public constructor that takes both string and parsed form? when would a developer use this? When would we?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pathitems is already parsed in the deserializer by calling pathhandler.parse (for odata bind its already written by elizabeth) so wanteed to reuse it and not call it again , thats why used both in constructor

@@ -0,0 +1,98 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License. See License.txt in the project root for license information.

Copy link
Member

@xuzhg xuzhg Sep 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update the head since we moved to .NET Foundation #Resolved

public NavigationPath(string navigationPath, ReadOnlyCollection<ODataPathSegment> pathSegments)
{
navigationPathName = navigationPath;
_pathSegments = pathSegments;
Copy link
Member

@xuzhg xuzhg Sep 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need the paramater null check #Resolved

/// </summary>
public class NavigationPath
{
private string navigationPathName;
Copy link
Member

@xuzhg xuzhg Sep 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

navigationPathName

code style.

Other fields using '', why "navigationPathName" without ''. #Resolved

{

}

Copy link
Member

@xuzhg xuzhg Sep 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we have the default constructor?
And if we do have this, if customers use it, _pathSegments is not initializated? #Resolved

}

return pathItems;
}
Copy link
Member

@xuzhg xuzhg Sep 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method looks werid.

It looks to retrieve a PathItem[] using a path name, but, the path name is private field? It's not from "outside". Straigh?

In this case, why do we need a ConcurrencyDictionary?
#Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was introduced to handle multiple set of path items in the same path like customers(1)/Orders(1)/shipment(1) and customers(1)/Orders(1)/Address(1). so cache customes(1)/orders(1) not to call again . But since we are making seperate navigation paths for each object it wouldnt be ideal to keep a cache. Might be in later versions we can consider a centralizzed caching if we see such scenarios, even then we should think of heavy cache allocations . So removing it for now



/// <summary>
/// NavigationPath/ODataId in string
Copy link
Member

@xuzhg xuzhg Sep 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NavigationPath/ODataId in string

Code style.

Following other class comments. It's better:

///


/// Gets the navigation path name.
///
#Resolved

{
if (segment is EntitySetSegment || segment is NavigationPropertySegment)
{
pathItems.Add(new PathItem());
Copy link
Member

@xuzhg xuzhg Sep 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pathItems.Add(new PathItem());

why donot we consider "ComplexProperty" and "TypeCase"? #Resolved

Copy link
Member

@mikepizzo mikepizzo Sep 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for complex property. type cast is handled in first else if

<bindingRedirect oldVersion="0.0.0.0-4.0.3.0" newVersion="4.0.3.0" />
</dependentAssembly>
<dependentAssembly>
<assemblyIdentity name="Microsoft.Spatial" publicKeyToken="31bf3856ad364e35" culture="neutral" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it seems we don't need this redirect?

If no, why don't add OData.Edm, and OData.Core redirect?

@@ -15,9 +15,9 @@
<package id="Microsoft.Bcl.AsyncInterfaces" version="1.0.0" targetFramework="net461" />
<package id="Microsoft.CodeDom.Providers.DotNetCompilerPlatform" version="1.0.0" targetFramework="net461" />
<package id="Microsoft.Net.Compilers" version="1.0.0" targetFramework="net461" developmentDependency="true" />
<package id="Microsoft.OData.Core" version="7.9.0" targetFramework="net461" />
Copy link
Member

@xuzhg xuzhg Sep 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any reason to update the dependency version #ByDesign

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was a bug which is fixed in 7.9.1 with out which async would throw error in some scenarios

}

/// <summary>
/// Represents king of <see cref="DataModificationOperationKind"/> type of operation
Copy link
Member

@xuzhg xuzhg Sep 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

king

kind #Resolved

/// <summary>
/// Represents response code
/// </summary>
public Int16 ResponseCode { get; set; }
Copy link
Member

@xuzhg xuzhg Sep 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Int16

any reason to use Int16? #ByDesign

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was taken crom vocabs and its like that so used same.

List<PathItem> pathItems = new List<PathItem>();
PathItem currentPathItem = null;

foreach (ODataPathSegment segment in _pathSegments)
Copy link
Member

@mikepizzo mikepizzo Sep 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_pathSegments

what if _pathSegments is null? #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added check in constructor and in calling method

/// <summary>
/// Constructor which takes and odataId and creates PathItems
/// </summary>
public NavigationPath()
Copy link
Member

@mikepizzo mikepizzo Sep 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NavigationPath

I would have expected a single constructor that took the navigationPathName and parsed it into pathSegments in order construct the PathItem[]. If the navigationPathName could not be parsed it would return null for GetNavigationPathItems, and the consuming logic would have to fall back to the navigationPathName. #Resolved

Copy link
Contributor Author

@Sreejithpin Sreejithpin Sep 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pathitems is already parsed in the deserializer by calling pathhandler.parse (for odata bind its already written by elizabeth) so wanteed to reuse it and not call it again , thats why used both in constructor

Removed default constructor

{
private string navigationPathName;
private ReadOnlyCollection<ODataPathSegment> _pathSegments;
private ConcurrentDictionary<string, PathItem[]> _pathItemCache = new ConcurrentDictionary<string, PathItem[]>();
Copy link
Member

@mikepizzo mikepizzo Sep 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_pathItemCache

maybe something to take as a future work item, but if we do keep a cache of parsed paths, then we would have a much higher cache hit ratio if we cached the path without the key. #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was introduced to handle multiple set of path items in the same path like customers(1)/Orders(1)/shipment(1) and customers(1)/Orders(1)/Address(1). so cache customes(1)/orders(1) not to call again . But since we are making seperate navigation paths for each object it wouldnt be ideal to keep a cache. Might be in later versions we can consider a centralizzed caching if we see such scenarios, even then we should think of heavy cache allocations . So removing it for now

{
private string navigationPathName;
private ReadOnlyCollection<ODataPathSegment> _pathSegments;
private ConcurrentDictionary<string, PathItem[]> _pathItemCache = new ConcurrentDictionary<string, PathItem[]>();
Copy link
Member

@mikepizzo mikepizzo Sep 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_pathItemCache

The _pathItemCache is per instance of NavigationPath, which should only ever have one path, so we're not really saving anything by having a concurrent dictionary, are we? #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was introduced to handle multiple set of path items in the same path like customers(1)/Orders(1)/shipment(1) and customers(1)/Orders(1)/Address(1). so cache customes(1)/orders(1) not to call again . But since we are making seperate navigation paths for each object it wouldnt be ideal to keep a cache. Might be in later versions we can consider a centralizzed caching if we see such scenarios, even then we should think of heavy cache allocations . So removing it for now

Sreejithpin and others added 4 commits October 28, 2021 14:49
* Bulk operations6 (#2)

* BulkInsert1 draft

* Bulk insert changes

* deleted old test

* updates

* Update ODataResourceSetWrapper.cs

* updates

* Updated addressing  comments

* updates

* updates

updates

* Updates

* Updates

* Minor updates

* comments

* updates

* update publicapi for core

* Address comments

* Cleanup and additional tests

Cleanup and additional tests

* Updated code

* BulkInsert1 draft

* Bulk insert changes

* deleted old test

* updates

* updates

* Updated addressing  comments

* updates

* updates

updates

* Updates

* Updates

* Minor updates

* updates

* Address comments

* Cleanup and additional tests

Cleanup and additional tests

* DataAnnotationException updates

* comments

* small updates

* updates

* small update

* updates

* Updates

* Update DeltaSetOfT.cs

* Updates with Patch

* updates

* updates

* Update WebHostTestFixture.cs

* updates

* Update DeltaOfTStructuralType.cs

* Updates

* Updates for serializer etc

* Update WebHostTestFixture.cs

* updates

* updates

* updates

* Bulk Operations Updates

* Review comments addressed

* Updates

* updated for deltaentity

* Update Microsoft.AspNet.OData.Test.csproj

* Update Microsoft.AspNet.OData.PublicApi.bsl

* update public api

* test fix

* Update EdmStructuredObject.cs

* Update EdmStructuredObject.cs

* Update BulkInsertTest.cs

* update

* public api

* review comments

* Review comments updates

* smalll update

* updates

* updates

* updates
* support for odata.bind

* support for odata.bind
Sreejithpin and others added 14 commits October 28, 2021 14:49
* Bulk operations6 (#2)

* BulkInsert1 draft

* Bulk insert changes

* deleted old test

* updates

* Update ODataResourceSetWrapper.cs

* updates

* Updated addressing  comments

* updates

* updates

updates

* Updates

* Updates

* Minor updates

* comments

* updates

* update publicapi for core

* Address comments

* Cleanup and additional tests

Cleanup and additional tests

* Updated code

* BulkInsert1 draft

* Bulk insert changes

* deleted old test

* updates

* updates

* Updated addressing  comments

* updates

* updates

updates

* Updates

* Updates

* Minor updates

* updates

* Address comments

* Cleanup and additional tests

Cleanup and additional tests

* DataAnnotationException updates

* comments

* small updates

* updates

* small update

* updates

* Updates

* Update DeltaSetOfT.cs

* Updates with Patch

* updates

* updates

* Update WebHostTestFixture.cs

* updates

* Update DeltaOfTStructuralType.cs

* Updates

* Updates for serializer etc

* Update WebHostTestFixture.cs

* updates

* updates

* updates

* Bulk Operations Updates

* Review comments addressed

* Updates

* updated for deltaentity

* Update Microsoft.AspNet.OData.Test.csproj

* Update Microsoft.AspNet.OData.PublicApi.bsl

* update public api

* test fix

* Update EdmStructuredObject.cs

* Update EdmStructuredObject.cs

* Update BulkInsertTest.cs

* update

* public api

* review comments

* Review comments updates

* smalll update

* updates

* updates

* updates
* support for odata.bind

* support for odata.bind
Squashing Commits
Update DeltaOfTStructuralType.cs

test change

Update Microsoft.AspNetCore.OData.Test.csproj

Update Microsoft.Test.E2E.AspNetCore3x.OData.csproj

updates

Update WebStack.versions.settings.targets

Update WebStack.versions.settings.targets

Update GetNugetPackageMetadata.proj

Update WebStack.versions.settings.targets

For testing

update

Update BulkInsertController.cs

Updates

updates

updates

Update NuGet.Config

Update BulkOperationPatchHandlersEF.cs

updates
@pull-request-quantifier-deprecated

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


Quantification details

Label      : Extra Large
Size       : +5161 -460
Percentile : 100%

Total files changed: 109

Change summary by file extension:
.csproj : +114 -43
.config : +115 -100
.cs : +4508 -290
.resx : +18 -0
.projitems : +25 -0
.bsl : +375 -24
.proj : +3 -0
.targets : +3 -3

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants