-
Notifications
You must be signed in to change notification settings - Fork 473
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
Implement Bulk Operations in WebAPI #2455
base: master
Are you sure you want to change the base?
Conversation
<Reference Include="System.Buffers, Version=4.0.3.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51, processorArchitecture=MSIL"> | ||
<HintPath>..\..\sln\packages\System.Buffers.4.5.0\lib\netstandard2.0\System.Buffers.dll</HintPath> | ||
</Reference> | ||
<Reference Include="Microsoft.OData.Edm, Version=7.7.3.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35, processorArchitecture=MSIL"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Microsoft.OData.Edm [](start = 24, length = 19)
We have Microsoft.OData package 7.8.2 installed at line 91~-97 , why do you install the 7.7.3 again? #Resolved
<dependentAssembly> | ||
<assemblyIdentity name="System.Collections.Concurrent" publicKeyToken="B03F5F7F11D50A3A" culture="neutral" /> | ||
<bindingRedirect oldVersion="0.0.0.0-4.0.11.0" newVersion="4.0.11.0" /> | ||
</dependentAssembly> | ||
<dependentAssembly> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do you update this web.config? #Resolved
@@ -39,6 +39,15 @@ | |||
<Reference Include="Microsoft.Spatial, Version=7.8.2.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35, processorArchitecture=MSIL"> | |||
<HintPath>..\..\sln\packages\Microsoft.Spatial.7.8.2\lib\net45\Microsoft.Spatial.dll</HintPath> | |||
</Reference> | |||
<Reference Include="Microsoft.OData.Core, Version=7.8.1.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35, processorArchitecture=MSIL"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it was accidental during some rebase or so !!
ODataConventionModelBuilder builder = new ODataConventionModelBuilder(); | ||
builder.EntityType<DataModificationExceptionType>(); | ||
|
||
builder.Namespace = typeof(DataModificationExceptionType).Namespace; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have a model builder here .....
What's it for? what's the usage of buider?
#Resolved
/// Represents king of <see cref="DataModificationOperationKind"/> type of operation | ||
/// </summary> | ||
[System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Naming", "CA1709:IdentifiersShouldBeCasedCorrectly", MessageId = "failed")] | ||
public DataModificationOperationKind failedOperation { get; set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
failedOperation [](start = 45, length = 15)
Are you sure use the camelcase for the public property name?
It's same as for "responseCode"?
#Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The casing of the term definition reflects the casing in a json error payload, which is lower camel-case (unlike other term properties).
However, the public API for ODataError still uses upper-camel case for consistency.
We should do the same thing here -- the public properties should be upper camel case, even though we serialize in lower camel case.
In reply to: 604691364 [](ancestors = 604691364,604610652)
/// <summary> | ||
/// Initializes a new instance of the <see cref="DataModificationExceptionType"/> class. | ||
/// </summary> | ||
public DataModificationExceptionType(DataModificationOperationKind failedOperation) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DataModificationOperationKind failedOperation [](start = 45, length = 45)
If you have a construct to set the "FailedOperation",
- remove the "set" for the property, it means it's read only.
Or
- remove the constructor, #Resolved
using System; | ||
using Microsoft.AspNet.OData.Builder; | ||
|
||
namespace Org.OData.Core.V1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The namespace is by design here? #ByDesign
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we need to have the same namespace name in order to consider it as vocabs in the core lib @mikepizzo
In reply to: 604611580 [](ancestors = 604611580)
{ | ||
_id = value; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use Auto-property :
public Uri Id {get;set;} #Resolved
{ | ||
_reason = (DeltaDeletedEntryReason)value; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use Auto-property :
public DeltaDeletedEntryReason Reason {get;set;} #Resolved
} | ||
|
||
/// <inheritdoc /> | ||
public DeltaDeletedEntryReason Reason |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DeltaDeletedEntryReason [](start = 15, length = 23)
Is it a nullable?
public DeltaDeletedEntryReason? Reason {get;set;} #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reason is optional -- we should be able to omit it from the response. See http://docs.oasis-open.org/odata/odata-json-format/v4.01/odata-json-format-v4.01.html#sec_DeletedEntity
In reply to: 604692550 [](ancestors = 604692550,604612233)
} | ||
|
||
/// <inheritdoc /> | ||
public IEdmNavigationSource NavigationSource |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public IEdmNavigationSource NavigationSource [](start = 8, length = 44)
auto property #Resolved
/// | ||
/// </summary> | ||
/// <param name="keys"></param> | ||
/// <returns></returns> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
finish it #Resolved
/// Used to hold the Entry object in the Delta Feed Payload. | ||
/// </summary> | ||
[NonValidatingParameterBinding] | ||
public class EdmDeltaEntityObject<TStructuralType> : EdmDeltaEntityObject, IEdmChangedObject<TStructuralType> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EdmDeltaEntityObject [](start = 17, length = 37)
why do you still have this class?
Edm* for no-clr-type scenario, we don't have the , how can we use it?
If we have thte TStructuralType, we are in typed scenario. #Resolved
namespace Microsoft.AspNet.OData | ||
{ | ||
/// <summary> | ||
/// Basic interface to represented a typed deleted entity object |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basic => Base? #Resolved
/// Base interface to be implemented by any Delta object required to be part of the DeltaFeed Payload. | ||
/// </summary> | ||
/// <typeparam name="TStructuralType">Generic Type for changed object</typeparam> | ||
public interface IEdmChangedObject<TStructuralType> : IEdmChangedObject |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IEdmChangedObject : IEdmChangedObject [](start = 21, length = 54)
we don't need this interface anymore, right? #Resolved
/// Represents an instance of an <see cref="IEdmDeltaDeletedEntityObject"/>. | ||
/// Holds the properties necessary to create the ODataDeltaDeletedEntry. | ||
/// </summary> | ||
public interface IEdmDeltaDeletedEntityObject<TStructuralType> : IEdmChangedObject<TStructuralType>, IEdmDeltaDeletedEntityObject |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't need this interface any more? #Resolved
/// <summary> | ||
/// Handler Class to handle users methods for create, delete and update | ||
/// </summary> | ||
public abstract class TypelessPatchMethodHandler |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just wanted to call it out seperately for typeless, to distinguish
In reply to: 604618607 [](ancestors = 604618607)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe IEdmPathMethodHandler, to follow the pattern for non clr-backed classes?
In reply to: 604693905 [](ancestors = 604693905,604618607)
@@ -203,6 +203,7 @@ internal static bool TryGetCharSet(MediaTypeHeaderValue mediaType, IEnumerable<s | |||
writeContext.Path = path; | |||
writeContext.MetadataLevel = metadataLevel; | |||
writeContext.QueryOptions = internalRequest.Context.QueryOptions; | |||
writeContext.IsUntyped = typeof(IEdmObject).IsAssignableFrom(type); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
writeContext.IsUntyped = typeof(IEdmObject).IsAssignableFrom(type); [](start = 16, length = 67)
why do you have such line code?
IsUntyped is calculated in the ODataSerializerContext #Resolved
@@ -88,6 +88,7 @@ internal ODataDeserializer GetODataDeserializerImpl(Type type, Func<IEdmModel> m | |||
return _rootContainer.GetRequiredService<ODataActionPayloadDeserializer>(); | |||
} | |||
|
|||
|
|||
// Get the model. Using a Func<IEdmModel> to delay evaluation of the model | |||
// until after the above checks have passed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
revert #Resolved
{ | ||
return Item as ODataResourceSetBase; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public ODataResourceSetBase ResourceSetBase {get;}
in constructor, just do:
ResourceSetBase = item; #Resolved
/// <summary> | ||
/// Gets the wrapped <see cref="ResourceSetBase"/>. | ||
/// </summary> | ||
public ODataResourceSetBase ResourceSetBase |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public ODataResourceSetBase ResourceSetBase [](start = 8, length = 43)
ODataDeltaResourceSet #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{ | ||
Type clrType = EdmLibHelpers.GetClrType(elementType, readContext.Model); | ||
|
||
foreach (ODataItemBase odataItemBase in resourceSet.Resources) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ODataItemBase [](start = 25, length = 13)
the type of resoureSet.Resources is ODataResourceWrapper, why don't you use ODataResourceWrapper in foreach.
Then we can get rid of line 211 #Resolved
|
||
if (readContext.IsUntyped) | ||
{ | ||
readContext.ResourceType = resourceWrapper.ResourceBase is ODataDeletedResource ? typeof(EdmDeltaDeletedEntityObject) : typeof(EdmDeltaEntityObject); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should just use EdmEntityObject, not use EdmDeltaEntityObject? #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EdmDeltaEntityObject inherits from IEdmChangedObject (edmchangedobjcoll is coll if idmchangedobject) , but EdmEntity doesnt , @mikepizzo - pls look
In reply to: 604621960 [](ancestors = 604621960)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (readContext.IsDeltaEntity) | ||
{ | ||
return new EdmDeltaEntityObject(structuredType.AsEntity()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should not distiguish DeltaEntity and Normal Entity.
DeltaEntity is a normal entity. @mikepizzo
#Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EdmDeltaEntityObject inherits from IEdmChangedObject (edmchangedobjcoll is coll if idmchangedobject) , dut EdmEntity doesnt
In reply to: 604622500 [](ancestors = 604622500)
Delta entity derives from iedmchangedobject, but normal entity doesn’t
On Mar 30, 2021, at 11:17 PM, Sam Xu ***@***.***> wrote:
@xuzhg commented on this pull request.
In src/Microsoft.AspNet.OData.Shared/Formatter/Deserialization/ODataResourceSetDeserializer.cs:
> + foreach (ODataResourceWrapper resourceWrapper in resourceSet.Resources)
+ {
+ yield return deserializer.ReadInline(resourceWrapper, elementType, readContext);
+ }
+ }
+ else
+ {
+ Type clrType = EdmLibHelpers.GetClrType(elementType, readContext.Model);
+
+ foreach (ODataItemBase odataItemBase in resourceSet.Resources)
+ {
+ ODataResourceWrapper resourceWrapper = odataItemBase as ODataResourceWrapper;
+
+ if (readContext.IsUntyped)
+ {
+ readContext.ResourceType = resourceWrapper.ResourceBase is ODataDeletedResource ? typeof(EdmDeltaDeletedEntityObject) : typeof(EdmDeltaEntityObject);
should just use EdmEntityObject, not use EdmDeltaEntityObject?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or unsubscribe.
#Resolved
|
using System.Collections.Generic; | ||
using System.Linq; | ||
using System.Linq.Expressions; | ||
using System.Threading.Tasks; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
copyright, sorting
#Resolved
@@ -44,6 +44,15 @@ | |||
<Reference Include="Microsoft.OData.Edm, Version=7.8.2.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35, processorArchitecture=MSIL"> | |||
<HintPath>..\..\..\..\sln\packages\Microsoft.OData.Edm.7.8.2\lib\net45\Microsoft.OData.Edm.dll</HintPath> | |||
</Reference> | |||
<Reference Include="Microsoft.OData.Client, Version=7.8.1.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35, processorArchitecture=MSIL"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Microsoft.OData.Client, Version=7.8.1.0, [](start = 24, length = 41)
why have 7.8.1? We alredy have 7.8.2 #Resolved
/// <summary> | ||
/// Enum to determine the type of Resource Set | ||
/// </summary> | ||
internal enum ResourceSetType |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{ | ||
IEdmStructuredType structuredType = resourceContext.StructuredType; | ||
IEdmStructuredObject structuredObject = resourceContext.EdmObject; | ||
|
||
//For appending transient and persistent instance annotations for both enity object and normal resources | ||
//var entityObject = structuredObject; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks like leftover code - cleanup? #Resolved
return; | ||
} | ||
EdmEntityObject edmEntityObject = null; | ||
object value = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Design", "CA1053:StaticHolderTypesShouldNotHaveConstructors")] | ||
public class ODataSerializerHelper | ||
{ | ||
internal static void AppendInstanceAnnotations(ODataResourceBase resource, ResourceContext resourceContext, object value, ODataSerializerProvider SerializerProvider) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can save a cast within the method for by making this an IODataInstanceAnnotationContainer, and requiring that the caller pass as an IODataInstanceAnnotationContainer (since, in many cases, the caller already knows it is an IODataInstanceAnnotationContainer). Keep the null check on line 22, though, so if the caller does pass in their object cast to an IODataInstanceAnnotationContainer, and the cast fails, it will be a no-op.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was kept so because from the calling method it sends an object(got from trygetvalue outs object) . So a cast would be inevitable, may be can move that cast to the calling method
[System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Design", "CA1053:StaticHolderTypesShouldNotHaveConstructors")] | ||
public class ODataSerializerHelper | ||
{ | ||
internal static void AppendInstanceAnnotations(ODataResourceBase resource, ResourceContext resourceContext, object value, ODataSerializerProvider SerializerProvider) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was kept so because from the calling method it sends an object(got from trygetvalue outs object) . So a cast would be inevitable, may be can move that cast to the calling method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🕐
We can throw an exception if its null, which would change the current behaviour, but i guess thats fine right In reply to: 828778014 Refers to: src/Microsoft.AspNet.OData.Shared/Formatter/Serialization/ODataSerializerContext.cs:290 in ab656df. [](commit_id = ab656df, deletion_comment = False) |
* 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
f9f672d
to
a7a67fc
Compare
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) |
Issues
Implements Bulk Operation in webAPI
Description
Briefly describe the changes of this pull request.
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.