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

DependsOn Ids for Multipart/Mixed Batch #1020

Merged
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -468,6 +468,9 @@
</Compile>
<Compile Include="$(ODataCrossTargettingSourcePath)\MultipartMixed\ODataMultipartMixedBatchWriterUtils.cs">
<Link>Microsoft\OData\Core\MultipartMixed\ODataMultipartMixedBatchWriterUtils.cs</Link>
</Compile>
<Compile Include="$(ODataCrossTargettingSourcePath)\MultipartMixed\DependsOnIdsTracker.cs">
<Link>Microsoft\OData\Core\MultipartMixed\DependsOnIdsTracker.cs</Link>
</Compile>
<Compile Include="$(ODataCrossTargettingSourcePath)\NonDisposingStream.cs">
<Link>Microsoft\OData\Core\NonDisposingStream.cs</Link>
Expand Down
Expand Up @@ -509,6 +509,9 @@
<Compile Include="..\MultipartMixed\ODataMultipartMixedBatchWriterUtils.cs">
<Link>MultipartMixed\ODataMultipartMixedBatchWriterUtils.cs</Link>
</Compile>
<Compile Include="..\MultipartMixed\DependsOnIdsTracker.cs">
<Link>MultipartMixed\DependsOnIdsTracker.cs</Link>
</Compile>
<Compile Include="..\NonDisposingStream.cs">
<Link>NonDisposingStream.cs</Link>
</Compile>
Expand Down Expand Up @@ -979,7 +982,7 @@
</Compile>
<Compile Include="..\TypeUtils.cs">
<Link>TypeUtils.cs</Link>
</Compile>
</Compile>
<Compile Include="..\UnknownEntitySet.cs">
<Link>UnknownEntitySet.cs</Link>
</Compile>
Expand Down
Expand Up @@ -181,7 +181,7 @@ protected override ODataBatchOperationRequestMessage CreateOperationRequestMessa
id,
atomicityGroupId,
dependsOnReqIds,
ODataFormat.Json);
/*dependsOnIdsValidationRequired*/true);

return requestMessage;
}
Expand Down
Expand Up @@ -245,8 +245,8 @@ protected override void WriteStartBatchImplementation()
}

/// <summary>
/// Given an enumerable of dependsOn ids containing request ids and group ids, convert the group ids
/// into associated request ids.
/// Given an enumerable of dependsOn ids containing request ids and group ids, return an enumeration of
/// equivalent request ids by converting the group ids into associated request ids.
/// </summary>
/// <param name="dependsOnIds">The dependsOn ids specifying current request's prerequisites.</param>
/// <returns>An enumerable consists of request ids.</returns>
Expand Down Expand Up @@ -342,10 +342,11 @@ protected override void WriteEndChangesetImplementation()

AddGroupIdLookup(contentId);

// create the new request operation
// Create the new request operation with a non-null dependsOnIds.
this.CurrentOperationRequestMessage = BuildOperationRequestMessage(
this.JsonLightOutputContext.GetOutputStream(), method, uri, contentId,
this.atomicityGroupId, dependsOnIds, ODataFormat.Json);
this.atomicityGroupId,
dependsOnIds ?? Enumerable.Empty<string>());

this.SetState(BatchWriterState.OperationCreated);

Expand Down
1 change: 1 addition & 0 deletions src/Microsoft.OData.Core/Microsoft.OData.Core.csproj
Expand Up @@ -53,6 +53,7 @@
<Compile Include="JsonLight\ODataJsonLightBatchReader.cs" />
<Compile Include="JsonLight\ODataJsonLightBatchReaderStream.cs" />
<Compile Include="JsonLight\ODataJsonLightBatchWriter.cs" />
<Compile Include="MultipartMixed\DependsOnIdsTracker.cs" />
<Compile Include="MultipartMixed\ODataMultipartMixedBatchFormat.cs" />
<Compile Include="MultipartMixed\ODataMultipartMixedBatchInputContext.cs" />
<Compile Include="MultipartMixed\ODataMultipartMixedBatchOutputContext.cs" />
Expand Down
90 changes: 90 additions & 0 deletions src/Microsoft.OData.Core/MultipartMixed/DependsOnIdsTracker.cs
@@ -0,0 +1,90 @@
//---------------------------------------------------------------------
// <copyright file="DependsOnIdsTracker.cs" company="Microsoft">
// Copyright (C) Microsoft Corporation. All rights reserved. See License.txt in the project root for license information.
// </copyright>
//---------------------------------------------------------------------

using System.Collections.Generic;
using System.Diagnostics;

namespace Microsoft.OData.MultipartMixed
{
/// <summary>
/// This class is to keep track of dependsOn ids and associated change set state,
/// and to return dependsOn ids pertaining to current state of the batch processing.
/// </summary>
internal sealed class DependsOnIdsTracker
{
/// <summary>
/// List of top-level dependsOn ids seen so far.
/// </summary>
private readonly IList<string> topLevelDependsOnIds;

/// <summary>
/// List of depeondsOn ids seen so far in current change set.
/// It should be empty if current processing is not within a change set.
/// </summary>
private readonly IList<string> changeSetDependsOnIds;

/// <summary>
/// Indicates whether current processing is inside a change set.
/// </summary>
private bool isInChangeSet;

/// <summary>
/// Constructor.
/// Initial state is batch start, and within change set.
/// </summary>
internal DependsOnIdsTracker()
{
this.topLevelDependsOnIds = new List<string>();
this.changeSetDependsOnIds = new List<string>();
this.isInChangeSet = false;
}

/// <summary>
/// Sets up the internal states corresponding to starting of change set.
/// </summary>
internal void ChangeSetStarted()
{
Debug.Assert(!isInChangeSet, "!isInChangeSet");
Debug.Assert(this.changeSetDependsOnIds.Count == 0);
this.isInChangeSet = true;
}

/// <summary>
/// Sets up the internal states corresponding to ending of change set.
/// </summary>
internal void ChangeSetEnded()
{
Debug.Assert(isInChangeSet, "isInChangeSet");
this.isInChangeSet = false;
this.changeSetDependsOnIds.Clear();
}

/// <summary>
/// Adds the id into the tracker.
/// </summary>
/// <param name="id">The operation request id to add.</param>
internal void AddDependsOnId(string id)
{
if (isInChangeSet)
{
this.changeSetDependsOnIds.Add(id);
}
else
{
this.topLevelDependsOnIds.Add(id);
}
}

/// <summary>
/// Get the list of dependsOn ids for the current state.
/// </summary>
/// <returns>The read-only list of dependsOn ids for the current batch operation request.</returns>
internal IEnumerable<string> GetDependsOnIds()
{
return isInChangeSet ? this.changeSetDependsOnIds : this.topLevelDependsOnIds;
}
}
}
Expand Up @@ -8,6 +8,7 @@ namespace Microsoft.OData.MultipartMixed
{
#region Namespaces
using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.Text;
Expand All @@ -22,18 +23,18 @@ internal sealed class ODataMultipartMixedBatchReader : ODataBatchReader
/// <summary>The batch stream used by the batch reader to divide a batch payload into parts.</summary>
private readonly ODataMultipartMixedBatchReaderStream batchStream;

/// <summary>
/// The dependsOnIds tracker for reader processing.
/// </summary>
private readonly DependsOnIdsTracker dependsOnIdsTracker;

/// <summary>
/// ContentId to apply to the next request. For legacy reasons, this might appear in the mime part headers
/// (which is why we have a property to remember it) but it should appear in the headers for the individual request (which will
/// be read when the individual request is created).
/// </summary>
private string currentContentId;

/// <summary>
/// The DependsOn-IDs for the current request.
/// </summary>
private string dependsOnIds;

/// <summary>
/// Constructor.
/// </summary>
Expand All @@ -48,6 +49,7 @@ internal ODataMultipartMixedBatchReader(ODataMultipartMixedBatchInputContext inp
Debug.Assert(!string.IsNullOrEmpty(batchBoundary), "!string.IsNullOrEmpty(batchBoundary)");

this.batchStream = new ODataMultipartMixedBatchReaderStream(this.MultipartMixedBatchInputContext, batchBoundary, batchEncoding);
this.dependsOnIdsTracker = new DependsOnIdsTracker();
}

/// <summary>
Expand Down Expand Up @@ -96,12 +98,16 @@ protected override ODataBatchOperationRequestMessage CreateOperationRequestMessa
requestUri,
headers,
this.currentContentId,
/*groupId*/ null,
this.dependsOnIds == null ? null : this.dependsOnIds.Split(','),
ODataFormat.Batch);
ODataMultipartMixedBatchWriterUtils.GetChangeSetIdFromBoundary(this.batchStream.ChangeSetBoundary),
this.dependsOnIdsTracker.GetDependsOnIds(), /*dependsOnIdsValidationRequired*/false);

if (this.currentContentId != null)
{
this.dependsOnIdsTracker.AddDependsOnId(this.currentContentId);
}

this.currentContentId = null;
this.dependsOnIds = null;

return requestMessage;
}

Expand Down Expand Up @@ -171,6 +177,7 @@ protected override ODataBatchReaderState ReadAtChangesetStartImplementation()
ThrowODataException(Strings.ODataBatchReader_ReaderStreamChangesetBoundaryCannotBeNull);
}

this.dependsOnIdsTracker.ChangeSetStarted();
return this.SkipToNextPartAndReadHeaders();
}

Expand All @@ -181,6 +188,7 @@ protected override ODataBatchReaderState ReadAtChangesetStartImplementation()
protected override ODataBatchReaderState ReadAtChangesetEndImplementation()
{
this.batchStream.ResetChangeSetBoundary();
this.dependsOnIdsTracker.ChangeSetEnded();
return this.SkipToNextPartAndReadHeaders();
}

Expand Down
Expand Up @@ -419,8 +419,12 @@ internal bool ProcessPartHeader(out string contentId)
// Verify that we only allow single byte encodings and UTF-8 for now.
ReaderValidationUtils.ValidateEncodingSupportedInBatch(this.changesetEncoding);
}
else if (this.ChangeSetBoundary != null)
else
{
// Read the contentId not only for request in changeset; but also
// top-level request (if available), so that top-level request dependsOn
// ids can be derived (even though top-level request ids are typically
// not showing up on the wire).
headers.TryGetValue(ODataConstants.ContentIdHeader, out contentId);
}

Expand Down
Expand Up @@ -21,6 +21,11 @@ internal sealed class ODataMultipartMixedBatchWriter : ODataBatchWriter
/// <summary>The boundary string for the batch structure itself.</summary>
private readonly string batchBoundary;

/// <summary>
/// The dependsOnIds tracker for writer processing.
/// </summary>
private readonly DependsOnIdsTracker dependsOnIdsTracker;

/// <summary>
/// The boundary string for the current changeset (only set when writing a changeset,
/// e.g., after WriteStartChangeSet has been called and before WriteEndChangeSet is called).
Expand Down Expand Up @@ -52,6 +57,7 @@ internal ODataMultipartMixedBatchWriter(ODataMultipartMixedBatchOutputContext ra
ExceptionUtils.CheckArgumentNotNull(batchBoundary, "batchBoundary is null");
this.batchBoundary = batchBoundary;
this.RawOutputContext.InitializeRawValueWriter();
this.dependsOnIdsTracker = new DependsOnIdsTracker();
}

/// <summary>
Expand Down Expand Up @@ -196,6 +202,21 @@ protected override void WriteStartChangesetImplementation(string changeSetId)
// write the change set headers
ODataMultipartMixedBatchWriterUtils.WriteChangeSetPreamble(this.RawOutputContext.TextWriter, this.changeSetBoundary);
this.changesetStartBoundaryWritten = false;

// Set state to track dependsOn Ids.
this.dependsOnIdsTracker.ChangeSetStarted();
}

/// <summary>
/// Given an enumerable of dependsOnIds, return an enumeration of equivalent request ids.
/// </summary>
/// <param name="dependsOnIds">The dependsOn ids specifying current request's prerequisites.</param>
/// <returns>If <code>dependsOnIds</code> is null, this is the implicit case therefore returns
/// an enumerable consists of request id from the <code>dependsOnIdsTracker</code>;
/// otherwise, this is explicit case therefore returns value passed in directly.</returns>
protected override IEnumerable<string> GetDependsOnRequestIds(IEnumerable<string> dependsOnIds)
{
return dependsOnIds ?? this.dependsOnIdsTracker.GetDependsOnIds();
}

/// <summary>
Expand All @@ -207,7 +228,9 @@ protected override void WriteStartChangesetImplementation(string changeSetId)
/// <param name="contentId">The Content-ID value to write in ChangeSet head.</param>
/// <param name="payloadUriOption">
/// The format of operation Request-URI, which could be AbsoluteUri, AbsoluteResourcePathAndHost, or RelativeResourcePath.</param>
/// <param name="dependsOnIds">The prerequisite request ids of this request.</param>
/// <param name="dependsOnIds">The prerequisite request ids of this request. By default its value should be null for Multipart/Mixed
/// format and the dependsOnIds implicitly derived per the protocol will be used; Otherwise, non-null will be used as override after
/// validation.</param>
/// <returns>The message that can be used to write the request operation.</returns>
protected override ODataBatchOperationRequestMessage CreateOperationRequestMessageImplementation(
string method, Uri uri, string contentId, BatchPayloadUriOption payloadUriOption,
Expand All @@ -217,15 +240,24 @@ protected override void WriteStartChangesetImplementation(string changeSetId)
this.WritePendingMessageData(true);

// create the new request operation
// For Multipart batch format, validate dependsOnIds if it is user explicit input, otherwise skip validation
// when it is implicitly derived per protocol.
ODataBatchOperationRequestMessage operationRequestMessage = BuildOperationRequestMessage(
this.RawOutputContext.OutputStream,
method, uri, contentId, /*groupId*/null, dependsOnIds, ODataFormat.Batch);
Copy link
Member

@mikepizzo mikepizzo Jan 11, 2018

Choose a reason for hiding this comment

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

/groupId/null [](start = 40, length = 15)

Why did we start passing null, rather than ODataMultipartMixedBatchWriterUtils.GetChangeSetIdFromBoundary(this.changeSetBoundary)? #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

groupId is introduced with JSON Batch. For MultipartMixed batch, we have changeset boundary instead and we originally considered groupId value is always null. And then we decided to use the value extracted from changeset boundary as group Id, likely from one of your comments.


In reply to: 160844104 [](ancestors = 160844104)

Copy link
Member

Choose a reason for hiding this comment

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

Sorry; I was reacting to an apparent change to pass null here, but that was an old iteration. Now we pass GetChangeSetIdFromBoundary, which is what I would expect.


In reply to: 160846771 [](ancestors = 160846771,160844104)

method, uri, contentId,
ODataMultipartMixedBatchWriterUtils.GetChangeSetIdFromBoundary(this.changeSetBoundary),
dependsOnIds);

this.SetState(BatchWriterState.OperationCreated);

// write the operation's start boundary string
this.WriteStartBoundaryForOperation();

if (contentId != null)
{
this.dependsOnIdsTracker.AddDependsOnId(contentId);
}

// write the headers and request line
ODataMultipartMixedBatchWriterUtils.WriteRequestPreamble(this.RawOutputContext.TextWriter, method, uri,
this.RawOutputContext.MessageWriterSettings.BaseUri, changeSetBoundary != null, contentId,
Expand Down Expand Up @@ -269,6 +301,8 @@ protected override void WriteEndChangesetImplementation()
// change the state first so we validate the change set boundary before attempting to write it.
this.SetState(BatchWriterState.ChangesetCompleted);

this.dependsOnIdsTracker.ChangeSetEnded();

Debug.Assert(this.changeSetBoundary != null, "this.changeSetBoundary != null");
this.changeSetBoundary = null;

Expand Down
8 changes: 8 additions & 0 deletions src/Microsoft.OData.Core/ODataBatchPayloadUriConverter.cs
Expand Up @@ -43,6 +43,14 @@ internal IODataPayloadUriConverter BatchMessagePayloadUriConverter
}
}

/// <summary>
/// A read-only enumeration of <code>contentIdCache</code>.
/// </summary>
internal IEnumerable<string> ContentIdCache
{
get { return this.contentIdCache; }
}

/// <summary>
/// Method to implement a custom URL conversion scheme.
/// This method returns null if not custom conversion is desired.
Expand Down