PR for 7.6 Beta by making Patchhandler and related classes internal#2606
PR for 7.6 Beta by making Patchhandler and related classes internal#2606Sreejithpin wants to merge 38 commits into
Conversation
* 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
* 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
| @@ -0,0 +1,115 @@ | |||
| // Copyright (c) Microsoft Corporation. All rights reserved. | |||
| // Licensed under the MIT License. See License.txt in the project root for license information. | |||
|
|
|||
There was a problem hiding this comment.
change all copyrights using existing one. #Resolved
| /// <param name="resourceWrapper">The resource wrapper.</param> | ||
| /// <param name="readContext">The read context.</param> | ||
| /// <returns>The resource wrapper.</returns> | ||
| private ODataResourceWrapper UpdateResourceWrapper(ODataResourceWrapper resourceWrapper, ODataDeserializerContext readContext) |
|
|
||
| foreach (string key in keyValues.Keys) | ||
| { | ||
| propertyInfos.Add(key, _clrType.GetProperty(key)); |
There was a problem hiding this comment.
Why do we need to populate this dictionary? In the for loop below, can't we just call _clrType.GetProperty(keyValue.Key).GetValue(item)? This would prevent use from having to enumerate keyValues twice, and we don't have to call GetProperty on keys that might come after we find the matching keyValue.Value
| /// <param name="keys">List of key names for the type</param> | ||
| public DeltaSet(IList<string> keys) | ||
| { | ||
| _keys = keys; |
There was a problem hiding this comment.
It seems that later on we assume that _keys is not null; since this is a public API, it's probably best to throw if keys is null.
| public DeltaSet(IList<string> keys) | ||
| { | ||
| _keys = keys; | ||
| _clrType = typeof(TStructuralType); |
There was a problem hiding this comment.
I'm not certain if there's any perf impact, but it might make more sense to have _clrType be static so that we don't have to call typeof as often. At the very least, it seems like good practice to follow DRY.
|
|
||
| private DeltaSet<TStructuralType> CreateDeltaSet() | ||
| { | ||
| Type type = typeof(DeltaSet<>).MakeGenericType(_clrType); |
There was a problem hiding this comment.
Is it not legal to just write typeof(DeltaSet<TStructuralType>)?
|
|
||
| if (hasnullKeys) | ||
| { | ||
| edmDeletedObject.Id = new Uri(string.Empty); |
There was a problem hiding this comment.
Small optimization, I think that Uri is immutable (or functionally immutable), so we can reduce object allocations if we have a singleton definition of new Uri(string.Empty)
| internal EdmChangedObjectCollection CopyChangedValues(EdmODataAPIHandler apiHandler, ODataEdmAPIHandlerFactory apiHandlerFactory = null) | ||
| { | ||
| EdmChangedObjectCollection changedObjectCollection = new EdmChangedObjectCollection(_entityType); | ||
| IEdmStructuralProperty[] keys = _entityType.Key().ToArray(); |
There was a problem hiding this comment.
I think there must be a better approach than this. I was initially suspicious because of the ToArray call, but then I saw it makes sense because it is possible for keys to be enumerated multiple times. However, at least for the failed case, we are doing the same .Name deference for the keys. Maybe here we should do _entityType.Key().Select(key => key.Name).ToArray() so that we don't have to deference the name so many times? #Resolved
| /// <param name="createdObject">The created object (Typeless)</param> | ||
| /// <param name="errorMessage">Any error message in case of an exception</param> | ||
| /// <returns>The status of the TryCreate Method, statuses are <see cref="ODataAPIResponseStatus"/></returns> | ||
| public abstract ODataAPIResponseStatus TryCreate(IEdmChangedObject changedObject, out IEdmStructuredObject createdObject, out string errorMessage); |
There was a problem hiding this comment.
I would prefer the return type be a structure that has both ODataAPIResponseStatus and errorMessage as properties. I think it makes the pattern of having both return values clearer. Do you have any thoughts on this?
| { | ||
|
|
||
| bool hasDefaultConstructor = (!clrType.IsClass) || clrType.GetConstructors(BindingFlags.Instance | BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Static). | ||
| Any(x => x.GetParameters().Count() == 0); |
There was a problem hiding this comment.
GetParameters returns an array, so calling Length instead of Count() should save us a cast
|
|
||
| if (keySegment != null) | ||
| { | ||
| foreach (KeyValuePair<string, object> key in keySegment.Keys) |
There was a problem hiding this comment.
I don't really see the value of doing this over keySegment.Keys.Select(key => new ODataProperty { Name = key.Key, Value = key.Value }).ToList(). If KeySegment has a count on it, then maybe it makes sense to initialize a new list with that capacity, but otherwise this just seems verbose.
| if (property == null && !readContext.DisableCaseInsensitiveRequestPropertyBinding) | ||
| { | ||
| //try case insensitive | ||
| List<ODataProperty> candidates = resourceWrapper.ResourceBase.Properties.Where(p => String.Equals(p.Name, keyName, StringComparison.InvariantCultureIgnoreCase)).ToList(); |
There was a problem hiding this comment.
I strongly suggest implementing an extension method on IEnumerable for this. I always thought it was weird that this wasn't the functionality for SingleOrDefault in the first place.
|
|
||
| if (deletedResource != null) | ||
| { | ||
| await writer.WriteStartAsync(deletedResource); |
There was a problem hiding this comment.
Do we need to continue execution on the same thread that awaited? I wonder if we want to call ConfigureAwait(false) here
| /// <summary> | ||
| /// Navigation Path of an OData ID | ||
| /// </summary> | ||
| public class NavigationPath : List<PathItem> |
There was a problem hiding this comment.
I don't realize understand the need for the NavigationPath type in the first place, but we always seems to be operating on the end of the sequence, so maybe a stack makes more sense than list here?
| /// Handler Class to handle users methods for create, delete and update. | ||
| /// This is the handler for data modification where there is a CLR type. | ||
| /// </summary> | ||
| internal abstract class ODataAPIHandler<TStructuralType>: IODataAPIHandler where TStructuralType : class |
There was a problem hiding this comment.
It seems weird that we are constraining the generic type parameter to just class. Why is that restriction necessary? #ByDesign
There was a problem hiding this comment.
It is used to handle delta request and it should be only for class types
| /// </summary> | ||
| /// <param name="navigationPath">Navigation path corresponding to an odataid</param> | ||
| /// <returns></returns> | ||
| public abstract EdmODataAPIHandler GetHandler(NavigationPath navigationPath); |
There was a problem hiding this comment.
Does it make sense for GetHandler to be able to add new PathItems to navigationPath?
| "\"Author@odata.bind\":\"Authors(1)\"}"; | ||
|
|
||
| string Uri = BaseAddress + "/odata/Books"; | ||
| HttpRequestMessage request = new HttpRequestMessage(HttpMethod.Post, Uri); |
There was a problem hiding this comment.
This implements IDisposable. Please wrap it in a using block
| request.Content.Headers.ContentType = MediaTypeWithQualityHeaderValue.Parse("application/json"); | ||
|
|
||
| // Act | ||
| HttpResponseMessage response = await Client.SendAsync(request); |
There was a problem hiding this comment.
This also implements IDisposable. Please check for other types in the tests as well
|
This PR has Quantification details
Why proper sizing of changes matters
Optimal pull request sizes drive a better predictable PR flow as they strike a
What can I do to optimize my changes
How to interpret the change counts in git diff output
Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email) |
14e5111 to
8104ae4
Compare
|
@Sreejithpin please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement ( “Agreement” ) is agreed to by the party signing below ( “You” ), 1. Definitions. “Code” means the computer software code, whether in human-readable or machine-executable form, “Project” means any of the projects owned or managed by .NET Foundation and offered under a license “Submit” is the act of uploading, submitting, transmitting, or distributing code or other content to any “Submission” means the Code and any other copyrightable material Submitted by You, including any 2. Your Submission. You must agree to the terms of this Agreement before making a Submission to any 3. Originality of Work. You represent that each of Your Submissions is entirely Your 4. Your Employer. References to “employer” in this Agreement include Your employer or anyone else 5. Licenses. a. Copyright License. You grant .NET Foundation, and those who receive the Submission directly b. Patent License. You grant .NET Foundation, and those who receive the Submission directly or c. Other Rights Reserved. Each party reserves all rights not expressly granted in this Agreement. 6. Representations and Warranties. You represent that You are legally entitled to grant the above 7. Notice to .NET Foundation. You agree to notify .NET Foundation in writing of any facts or 8. Information about Submissions. You agree that contributions to Projects and information about 9. Governing Law/Jurisdiction. This Agreement is governed by the laws of the State of Washington, and 10. Entire Agreement/Assignment. This Agreement is the entire agreement between the parties, and .NET Foundation dedicates this Contribution License Agreement to the public domain according to the Creative Commons CC0 1. |
Issues
This pull request fixes #xxx.
Description
PR for 7.6 Beta by making Patchhandler and related classes internal
Changes:
DeltaOfT
internal void Patch(
DeltaSetOfT
internal DeltaSet Patch(IC
internal DeltaSet Patch(
EdmChangedObjectcollection
internal EdmChangedObjectCollection Patch(OD
EdmODataAPIHandler
internal abstract class EdmODataAPIHandler
IODataAPIHandler
internal interface IODataAPIHandler
ODataAPIHandler
internal abstract class ODataAPIHandler: IO
ODataAPIHandlerFactory
internal abstract class ODataAPIHandlerFactory
ODataAPIResponseStatus
internal enum ODataAPIResponseStatus
ODataEdmAPIHandlerFactory
internal abstract class ODataEdmAPIHandlerFactory
ODataIDResolver
internal abstract class ODataIDResolver
Checklist (Uncheck if it is not completed)
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.