Skip to content

Commit

Permalink
Addressed addtional CR comments, primarily:
Browse files Browse the repository at this point in the history
- Changing to use payloadUrlConverter to validate referencing uri for the case of Multipart batch with implicit dependsOnIds.
- Tests added.
  • Loading branch information
biaol-odata committed Jan 12, 2018
1 parent c1ed88d commit 79f9de0
Show file tree
Hide file tree
Showing 10 changed files with 157 additions and 33 deletions.
Original file line number Diff line number Diff line change
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 @@ -880,9 +883,6 @@
<Compile Include="$(ODataCrossTargettingSourcePath)\TypeUtils.cs">
<Link>Microsoft\OData\Core\TypeUtils.cs</Link>
</Compile>
<Compile Include="$(ODataCrossTargettingSourcePath)\DependsOnIdsTracker.cs">
<Link>Microsoft\OData\Core\DependsOnIdsTracker.cs</Link>
</Compile>
<Compile Include="$(ODataCrossTargettingSourcePath)\UriParser\Binders\BinaryOperatorBinder.cs">
<Link>Microsoft\OData\Core\UriParser\Binders\BinaryOperatorBinder.cs</Link>
</Compile>
Expand Down
Original file line number Diff line number Diff line change
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,10 +982,7 @@
</Compile>
<Compile Include="..\TypeUtils.cs">
<Link>TypeUtils.cs</Link>
</Compile>
<Compile Include="..\DependsOnIdsTracker.cs">
<Link>DependsOnIdsTracker.cs</Link>
</Compile>
</Compile>
<Compile Include="..\UnknownEntitySet.cs">
<Link>UnknownEntitySet.cs</Link>
</Compile>
Expand Down
2 changes: 1 addition & 1 deletion src/Microsoft.OData.Core/Microsoft.OData.Core.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,6 @@
<Compile Include="AnnotationFilterPattern.cs" />
<Compile Include="AsyncBufferedStream.cs" />
<Compile Include="ContainerBuilderExtensions.cs" />
<Compile Include="DependsOnIdsTracker.cs" />
<Compile Include="EdmExtensionMethods.cs" />
<Compile Include="Evaluation\ODataConventionalResourceMetadataBuilder.cs" />
<Compile Include="JsonLight\ODataJsonLightBatchAtomicGroupCache.cs" />
Expand All @@ -54,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
Original file line number Diff line number Diff line change
@@ -1,14 +1,14 @@
//---------------------------------------------------------------------
//---------------------------------------------------------------------
// <copyright file="DependsOnIdsTracker.cs" company="Microsoft">
// Copyright (C) Microsoft Corporation. All rights reserved. See License.txt in the project root for license information.
// </copyright>
//---------------------------------------------------------------------

namespace Microsoft.OData
{
using System.Collections.Generic;
using System.Diagnostics;
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.
Expand Down
8 changes: 8 additions & 0 deletions src/Microsoft.OData.Core/ODataBatchPayloadUriConverter.cs
Original file line number Diff line number Diff line change
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
18 changes: 6 additions & 12 deletions src/Microsoft.OData.Core/ODataBatchReader.cs
Original file line number Diff line number Diff line change
Expand Up @@ -107,10 +107,8 @@ private set

/// <summary>
/// Public property for the current group id the reader is processing.
/// The primary usage of this property is, for Json batch, to correlate atomic group id
/// in request and response operation messages.
/// For multipart batch, since there are no correlations between the changeset boundaries in
/// request and response, this value is ignored / not used, is always null.
/// The primary usage of this to correlate atomic group id in request and
/// response operation messages as needed.
/// </summary>
public string CurrentGroupId
{
Expand Down Expand Up @@ -258,9 +256,7 @@ protected void ThrowODataException(string errorMessage)

/// <summary>
/// Gets the group id for the current request.
/// This is for Json batch format only, which requires atomic group ids from corresponding request and
/// response should be the same.
/// Default implementation here is provided for multipart batch.
/// Default implementation here is provided returning null.
/// </summary>
/// <returns>The group id for the current request.</returns>
[SuppressMessage("Microsoft.Design", "CA1024:UsePropertiesWhereAppropriate", Justification = "A method for consistency with the rest of the API.")]
Expand Down Expand Up @@ -317,9 +313,8 @@ protected virtual string GetCurrentGroupIdImplementation()
/// <param name="contentId">The contentId of this request message.</param>
/// <param name="groupId">The group id that this request belongs to. Can be null.</param>
/// <param name="dependsOnRequestIds">
/// The prerequisite request Ids of this request message. For batch in Json format,
/// some of these request Ids are resolved from prerequisite atomic groups that are
/// specified in the dependsOn attribute of the request.
/// The prerequisite request Ids of this request message that could be specified by caller
/// explicitly.
/// </param>
/// <param name="dependsOnIdsValidationRequired">
/// Whether the <code>dependsOnIds</code> value needs to be validated.</param>
Expand Down Expand Up @@ -362,8 +357,7 @@ protected ODataBatchOperationRequestMessage BuildOperationRequestMessage(
/// <param name="statusCode">The status code for the response.</param>
/// <param name="headers">The headers for this response message.</param>
/// <param name="contentId">The contentId of this request message.</param>
/// <param name="groupId">The groupId of this request message.
/// Value is null for multipart batch operation response message.</param>
/// <param name="groupId">The groupId of this request message.</param>
/// <returns>The <see cref="ODataBatchOperationResponseMessage"/> instance.</returns>
protected ODataBatchOperationResponseMessage BuildOperationResponseMessage(
Func<Stream> streamCreatorFunc,
Expand Down
3 changes: 2 additions & 1 deletion src/Microsoft.OData.Core/ODataBatchUtils.cs
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,8 @@ internal static void ValidateReferenceUri(Uri uri, IEnumerable<string> dependsOn
if (dependsOnRequestIds == null || !dependsOnRequestIds.Contains(referenceId))
{
throw new ODataException(Strings.ODataBatchReader_ReferenceIdNotIncludedInDependsOn(
referenceId, UriUtils.UriToString(uri), string.Join(",", dependsOnRequestIds.ToArray())));
referenceId, UriUtils.UriToString(uri),
dependsOnRequestIds != null ? string.Join(",", dependsOnRequestIds.ToArray()) : "null"));
}
}
}
Expand Down
12 changes: 8 additions & 4 deletions src/Microsoft.OData.Core/ODataBatchWriter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -460,7 +460,7 @@ public Task FlushAsync()
/// Starts a new changeset.
/// </summary>
/// <param name="groupOrChangesetId">
/// The atomic group id (for Json batch) / changeset GUID (for Multipart/Mixed batch) of the batch request.
/// The atomic group id, aka changeset GUID of the batch request.
/// </param>
protected abstract void WriteStartChangesetImplementation(string groupOrChangesetId);

Expand Down Expand Up @@ -538,8 +538,7 @@ protected ODataBatchOperationRequestMessage BuildOperationRequestMessage(Stream

if (dependsOnIds != null)
{
// Validate explicit dependsOnIds cases, which include all cases for JSON batch
// plus explicit dependsOnIds case for Multipart batch.
// Validate explicit dependsOnIds cases.
foreach (string id in convertedDependsOnIds)
{
if (!this.payloadUriConverter.ContainsContentId(id))
Expand All @@ -549,7 +548,12 @@ protected ODataBatchOperationRequestMessage BuildOperationRequestMessage(Stream
}
}

ODataBatchUtils.ValidateReferenceUri(uri, convertedDependsOnIds, this.outputContext.MessageWriterSettings.BaseUri);
// If dependsOnIds is not specified, use the <code>payloadUrlConverter</code>; otherwise use the dependOnIds converted
// from specified value.
IEnumerable<string> requestIdsForUrlReferenceValidation =
dependsOnIds == null ? this.payloadUriConverter.ContentIdCache : convertedDependsOnIds;

ODataBatchUtils.ValidateReferenceUri(uri, requestIdsForUrlReferenceValidation, this.outputContext.MessageWriterSettings.BaseUri);

Func<Stream> streamCreatorFunc = () => ODataBatchUtils.CreateBatchOperationWriteStream(outputStream, this);
ODataBatchOperationRequestMessage requestMessage =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ public MultipartMixedBatchDependsOnIdsTests()
}

[Fact]
public void MultipartBatchTestWithTopLevelDependsOnIds()
public void MultipartBatchTestWithTopLevelDependsOnIdsV401()
{
string customizedValidRequest = Regex.Replace(RequestPayloadVerifyDependsOnIdsTemplate,
"__REF_URI_1__",
Expand All @@ -169,6 +169,27 @@ public void MultipartBatchTestWithTopLevelDependsOnIds()
ClientReadSingletonBatchResponse(responsePayload, batchContentTypeMultipartMime);
}

[Fact]
public void MultipartBatchTestWithTopLevelDependsOnIdsV4()
{
string customizedValidRequest = Regex.Replace(RequestPayloadVerifyDependsOnIdsTemplate,
"__REF_URI_1__",
"$2B",
RegexOptions.Multiline);

customizedValidRequest = Regex.Replace(customizedValidRequest,
"__REF_URI_2__",
"$1",
RegexOptions.Multiline);

// For V4 MultipartMixed batch, top-level request "3"'s url "/$1"(referencing a preceding top-level request "1")
// should trigger an exception because request reference scope is limited to change set in V4 implementation.
ODataException ode = Assert.Throws<ODataException>(
() => this.ServiceReadRequestAndWriterResponseForMultipartBatchVerifyDependsOnIds(customizedValidRequest, ODataVersion.V4));
Assert.True(ode.Message.Contains(
"When the relative URI is a reference to a content ID, the content ID does not exist in the current change set."));
}

private void VerifyPayloadForMultipartBatch(byte[] payloadBytes, string expectedPayload)
{
using (MemoryStream stream = new MemoryStream(payloadBytes))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -824,6 +824,31 @@ public void MultipartBatchImplicitChangeSetInvalidDependsOnIdShouldThrow()
}
}

[Fact]
public void MultipartBatchVerifyDependsOnIdsForTopLevelRequestV4()
{
// In V4, referencing preceding top-level request Id from another request
// after change set should throw.
string topLevelContentId = "1";

ODataException ode = Assert.Throws<ODataException>(
() => ClientWriteRequestForMultipartBatchVerifyDependsOnIdsForTopLevelRequest(
topLevelContentId, ODataVersion.V4));
Assert.True(ode.Message.Contains(referenceIdNotIncludedInDependsOn));
}

[Fact]
public void MultipartBatchVerifyDependsOnIdsForTopLevelRequestV401()
{
string topLevelContentId = "1";

byte[] requestBytes = ClientWriteRequestForMultipartBatchVerifyDependsOnIdsForTopLevelRequest(
topLevelContentId, ODataVersion.V401);

Assert.NotNull(requestBytes);
Assert.True(requestBytes.Length > 0);
}

[Fact]
public void MultipartBatchImplicitTopLevelDependsOnIdsTest()
{
Expand Down Expand Up @@ -1017,6 +1042,22 @@ public void BatchJsonLightContentIdUniquenessScopeForJsonBatchV401ShouldThrow()
Assert.Contains(expectedPartialErrorMesage, ode.Message);
}

private static void PopulateWebResourceId(ODataBatchOperationRequestMessage dataModificationRequestMessage, int webId)
{
// Use a new message writer to write the body of this operation.
using (ODataMessageWriter operationMessageWriter = new ODataMessageWriter(dataModificationRequestMessage))
{
ODataWriter entryWriter = operationMessageWriter.CreateODataResourceWriter();
ODataResource entry = new ODataResource
{
TypeName = "NS.Web",
Properties = new[] { new ODataProperty { Name = "WebId", Value = webId } }
};
entryWriter.WriteStart(entry);
entryWriter.WriteEnd();
}
}

private void BatchJsonLightTestUsingBatchFormat(BatchFormat batchFormat, int idx)
{
byte[] requestPayload = null;
Expand Down Expand Up @@ -1745,7 +1786,7 @@ private byte[] ClientWriteMuitipartChangesetsWithSameContentId(ODataMessageWrite
}
}

private byte[] ClientWriteRequestForMultipartBatchVerifyDependsOnIds(string contendIdRef, ODataVersion version)
private byte[] ClientWriteRequestForMultipartBatchVerifyDependsOnIds(string contentIdRef, ODataVersion version)
{
MemoryStream stream = new MemoryStream();

Expand Down Expand Up @@ -1808,7 +1849,7 @@ private byte[] ClientWriteRequestForMultipartBatchVerifyDependsOnIds(string cont
entryWriter.WriteEnd();
}

Uri referenceUri = new Uri("$" + contendIdRef, UriKind.Relative);
Uri referenceUri = new Uri("$" + contentIdRef, UriKind.Relative);
updateOperationMessage = batchWriter.CreateOperationRequestMessage("PATCH", referenceUri, "2C",
BatchPayloadUriOption.RelativeUri);

Expand Down Expand Up @@ -1863,6 +1904,61 @@ private byte[] ClientWriteRequestForMultipartBatchVerifyDependsOnIds(string cont
}
}

private byte[] ClientWriteRequestForMultipartBatchVerifyDependsOnIdsForTopLevelRequest(string contentIdRef, ODataVersion version)
{
// Batch consists of one top-level request, one change set, and one more top-level request referencing the first top-level request.
MemoryStream stream = new MemoryStream();

IODataRequestMessage requestMessage = new InMemoryMessage { Stream = stream };
requestMessage.SetHeader("Content-Type", batchContentTypeMultipartMime);

using (ODataMessageWriter messageWriter = new ODataMessageWriter(requestMessage,
new ODataMessageWriterSettings
{
Version = version,
BaseUri = new Uri(serviceDocumentUri)
}))
{
ODataBatchWriter batchWriter = messageWriter.CreateODataBatchWriter();

batchWriter.WriteStartBatch();

// A create operation.
ODataBatchOperationRequestMessage createOperationMessage =
batchWriter.CreateOperationRequestMessage("POST", new Uri(serviceDocumentUri + "MySingleton"), contentIdRef);
PopulateWebResourceId(createOperationMessage, 8);

// Header modification on inner payload.
// createOperationMessage.SetHeader("Accept", "application/json;odata.metadata=full");
Assert.NotNull(createOperationMessage.ContentId);

// A change set with multi update operation.
batchWriter.WriteStartChangeset();
{
// Create a update operation in the change set.
ODataBatchOperationRequestMessage updateOperationMessage =
batchWriter.CreateOperationRequestMessage("PATCH", new Uri(serviceDocumentUri + "/MySingleton"), "2A" /*written*/);
PopulateWebResourceId(updateOperationMessage, 9);
}
batchWriter.WriteEndChangeset();

// An update operation referencing the preceding create operation.
Uri referenceUri = new Uri("$" + contentIdRef, UriKind.Relative);
ODataBatchOperationRequestMessage topLevelOperationMessage =
batchWriter.CreateOperationRequestMessage("PATCH", referenceUri, "3", BatchPayloadUriOption.AbsoluteUri );
PopulateWebResourceId(topLevelOperationMessage, 10);

// Header modification on inner payload.
// topLevelOperationMessage.SetHeader("Accept", "application/json;odata.metadata=full");
Assert.NotNull(topLevelOperationMessage.ContentId);

batchWriter.WriteEndBatch();

stream.Position = 0;
return stream.ToArray();
}
}

private byte[] ServiceReadRequestAndWriterResponseForMultipartBatchVerifyDependsOnIds(byte[] requestPayload, ODataVersion odataVersion)
{
IODataRequestMessage requestMessage = new InMemoryMessage() { Stream = new MemoryStream(requestPayload) };
Expand Down

0 comments on commit 79f9de0

Please sign in to comment.