-
Notifications
You must be signed in to change notification settings - Fork 349
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
Fix/1822 depends on atomicity group id v4 #1831
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -53,6 +53,11 @@ internal sealed class ODataJsonLightBatchReader : ODataBatchReader | |
/// </summary> | ||
private ODataJsonLightBatchPayloadItemPropertiesCache messagePropertiesCache = null; | ||
|
||
/// <summary> | ||
/// Collection for keeping track of unique atomic group ids and member request ids. | ||
/// </summary> | ||
private HashSet<string> requestIds = new HashSet<string>(); | ||
|
||
/// <summary> | ||
/// Constructor. | ||
/// </summary> | ||
|
@@ -124,6 +129,16 @@ protected override ODataBatchOperationRequestMessage CreateOperationRequestMessa | |
string atomicityGroupId = (string)this.messagePropertiesCache.GetPropertyValue( | ||
ODataJsonLightBatchPayloadItemPropertiesCache.PropertyNameAtomicityGroup); | ||
|
||
if (id != null) | ||
{ | ||
this.requestIds.Add(id); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Rather than do an explicit ContainsKey check before calling Add, why not just do: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed #Resolved There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should you use TryAdd(id) rather than Add(id) to avoid possible exceptions if the id already exists, or are we asserting that the id doesn't already exist? (same for below) In reply to: 454179343 [](ancestors = 454179343) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Never mind; I see that you moved to HashSet, which is perfect. In reply to: 467600534 [](ancestors = 467600534,454179343) |
||
|
||
if (atomicityGroupId != null) | ||
{ | ||
this.requestIds.Add(atomicityGroupId); | ||
} | ||
|
||
// dependsOn | ||
// Flatten the dependsOn list by converting every groupId into request Ids, so that the caller | ||
// can decide, at the earliest opportunity, whether the depending request can be invoked. | ||
|
@@ -332,6 +347,25 @@ protected override ODataBatchOperationResponseMessage CreateOperationResponseMes | |
return responseMessage; | ||
} | ||
|
||
/// <summary> | ||
/// Validate the dependsOnIds. | ||
/// </summary> | ||
/// <param name="dependsOnIds">The dependsOn ids specifying current request's prerequisites.</param> | ||
protected override void ValidateDependsOnIds(string contentId, IEnumerable<string> dependsOnIds) | ||
{ | ||
foreach (var id in dependsOnIds) | ||
{ | ||
// Content-ID cannot be part of dependsOnIds. This is to avoid self referencing. | ||
// The dependsOnId must be an existing request ID or atomicityGroup | ||
if (id == contentId || | ||
(!this.requestIds.Contains(id) && | ||
!this.requestIds.Contains(id))) | ||
{ | ||
throw new ODataException(Strings.ODataBatchReader_DependsOnIdNotFound(id, contentId)); | ||
} | ||
} | ||
} | ||
|
||
/// <summary> | ||
/// Validate that the property value is not null. | ||
/// </summary> | ||
|
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 need dictionaries, it looks like you just check to see if the dictionary contains the key, not the relationship between the id and the atomicity group. Can you just use a list? #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.
I have changed it to a HashSet. #Resolved