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 5 commits into from Jan 12, 2018

Conversation

Projects
None yet
2 participants
@biaol-odata
Member

biaol-odata commented Dec 20, 2017

  • Reader extracts contentId header value for both changeset and top-level requests;
  • Uses DependsOnIdsTracker to track request ids in Multipart/Mixed batch;
  • DependsOnIds validation is performed only for JsonBatch since it is client provided; not for Multipart/Mixed since it is not provided by client.

Issues

This pull request is to introduce DependsOn ids to Multipart/Mixed batch format. This makes DependsOn Ids available for query when service processes the batch request.

Description

Introducing DependsOn IDs to Multipart/Mixed batch so that both formats are symmetrical in semantics. The dependsOn ids for multipart/mixed format is derived from protocol and is not explicitly provided by client.

Checklist (Uncheck if it is not completed)

  • Test cases added
  • Build and test with one-click build and test script passed

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.

dependsOn Ids for Multipart/Mixed Batch
- Reader extracts contentId header value for both changeset and top-level requests;
- Uses DependsOnIdsTracker to track request ids in Multipart/Mixed batch;
- DependsOnIds validation is performed only for JsonBatch since it is client provided; not for Multipart/Mixed since it is not provided by client.
{
// Validate dependsOn ids in Json batch.

This comment has been minimized.

@mikepizzo

mikepizzo Dec 21, 2017

Member

Base class should not refer to derived classes, even in comments. We validate dependsOnIds if dependsOnIdsValidationRequired is true. Why it's true is outside of the scope of this class. #Closed

This comment has been minimized.

@mikepizzo

mikepizzo Jan 9, 2018

Member

Please remove references to JSON and Multipart from comments in the base class. Comment should read something like "Validate dependsOnIds if specified".


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

This comment has been minimized.

@biaol-odata

biaol-odata Jan 10, 2018

Member

done for base classes reader and writer.


In reply to: 160554679 [](ancestors = 160554679,158349752)

throw new ODataException(
"For Multipart/Mixed batch format, request dependency is implicit by the protocol and should be null.");
}
// write pending message data (headers, response line) for a previously unclosed message/request

This comment has been minimized.

@mikepizzo

mikepizzo Dec 21, 2017

Member

Do not enforce null. We should validate that any ids specified are in the ids read by the tracker for this scope (changeset or toplevel). #Closed

This comment has been minimized.

@biaol-odata

biaol-odata Jan 9, 2018

Member

removed null check / exception thrown


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

method, uri, contentId,
ODataMultipartMixedBatchWriterUtils.GetChangeSetIdFromBoundary(this.changeSetBoundary),
this.dependsOnIdsTracker.GetDependsOnIds(),
/*dependsOnIdsValidationRequired*/ false);

This comment has been minimized.

@mikepizzo

mikepizzo Dec 21, 2017

Member

/dependsOnIdsValidationRequired/ false [](start = 16, length = 40)

Not sure why we wouldn't pass true in order to validate against the ids already read? Is this just for backward compatibility? Seems like the request is destined to fail if it doesn't reference a valid id. #Closed

This comment has been minimized.

@biaol-odata

biaol-odata Jan 9, 2018

Member

Updated to use only dependsOnIds which is only null for the implicit dependsOnIds Multipart/Mixed case; and the GetDependsOnIds overrides in derived classes getting the convertedDependsOnIds in the BuildOperationRequestMessage method.


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

/// <summary>
/// List of top-level dependsOn ids seen so far.
/// </summary>
private readonly IList<string> topLevelDepeondsOnIds;

This comment has been minimized.

@mikepizzo

mikepizzo Jan 2, 2018

Member

topLevelDepeondsOnIds [](start = 39, length = 21)

Spelling; should be topLevelDependsOnIds #Closed

This comment has been minimized.

@biaol-odata

biaol-odata Jan 9, 2018

Member

oops! fixed.


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

@mikepizzo

This comment has been minimized.

Member

mikepizzo commented Jan 3, 2018

            if (HttpUtils.IsQueryMethod(method))

This restriction (inability to have query methods in a changeset) is no longer true. Please add "&& this.outputContext.MessageWriterSettings.Version <= ODataVersion.V4" #ByDesign


Refers to: src/Microsoft.OData.Core/ODataBatchWriter.cs:811 in 2ce1695. [](commit_id = 2ce1695, deletion_comment = False)

biaol-odata added some commits Jan 5, 2018

Addressing CR comment: using null dependsOnIds of MultipartMixed batc…
…h as a flag to uniquely indicate implicit dependsOnIds case
@biaol-odata

This comment has been minimized.

Member

biaol-odata commented Jan 9, 2018

            if (HttpUtils.IsQueryMethod(method))

I think this is aligned to the protocol and we keep this restriction at least for now per offline discussion with the group. Right?


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


Refers to: src/Microsoft.OData.Core/ODataBatchWriter.cs:811 in 2ce1695. [](commit_id = 2ce1695, deletion_comment = False)

//---------------------------------------------------------------------
namespace Microsoft.OData
{

This comment has been minimized.

@mikepizzo

mikepizzo Jan 9, 2018

Member

If this is only used by multipart/mixed, should it go in the multipart/mixed namespace? #Closed

This comment has been minimized.

@biaol-odata

biaol-odata Jan 10, 2018

Member

yes, moved.


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

@@ -554,8 +549,7 @@ protected virtual IEnumerable<string> GetDependsOnRequestIds(IEnumerable<string>
}
}
ODataBatchUtils.ValidateReferenceUri(uri, flattenDependsOnIds,
this.outputContext.MessageWriterSettings.BaseUri, batchFormat);
ODataBatchUtils.ValidateReferenceUri(uri, convertedDependsOnIds, this.outputContext.MessageWriterSettings.BaseUri);

This comment has been minimized.

@mikepizzo

mikepizzo Jan 10, 2018

Member

convertedDependsOnIds [](start = 54, length = 21)

I don't think this is the right logic. I thought that, if the derived class passed in null for dependsOnIds, we were going to use the ids from the payloadUriConverter (which would be be correctly scoped for the odata version). The ids from the DependsOnIdsTracker will be return only the ids in the changeset (if currently in changeset) otherwise top level ids, which is correct for 4.0, but wrong for 4.01. I actually don't think we need the dependsOnIdsTracker at all for the ODataMultipartMixedBatchWriter, only for the ODataMultipartMixedBatchReader. #Closed

This comment has been minimized.

@biaol-odata

biaol-odata Jan 10, 2018

Member

There are pros and cons of using payloadUrlConverter. I thought about using it but ended up not doing it because it is undesirable to couple with legacy issue of difference between changeset scope in V4.0 implementation and batch scope in V4.0 protocol. In other words, current payloadUrlConverter is actually implemented incorrectly against the V4.0 protocol.
Furthermore, Json Batch Format 19.1: (for referencing url...)"The id of this request MUST be specified in the dependsOn name/value pair", it is natural extension to validate the referenced Id in url using the dependsOnIds?

I understand doing this will allow, on the writer side, the referencing of preceding top level request, e.g. request_2's url uses "$request_1", which should not be allowed in V4.0 implementation due to scope issue above but should not be an problem in V4.01.

All these said, I just don't want to let the scope issue above limit the validation on the writer side, and the referencing urls in both cases above will still be validated / processed correctly on the reader side.


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

This comment has been minimized.

@mikepizzo

mikepizzo Jan 10, 2018

Member

For the case of the non-null dependsOnIds (JSON batch and multipartmixed with explicit dependsOnIds) yes, absolutely we should use the (flattened) list of dependsOnIds explicitly passed by the user. However, for the dependsOnIds==null (multipartmixed implicit) case, we should default to the set of Ids in the payloadUrlConverter.


In reply to: 160567393 [](ancestors = 160567393,160558905)

This comment has been minimized.

@biaol-odata

biaol-odata Jan 10, 2018

Member

updated with tests added.


In reply to: 160573714 [](ancestors = 160573714,160567393,160558905)

Addressed addtional CR comments, primarily:
- Changing to use payloadUrlConverter to validate referencing uri for the case of Multipart batch with implicit dependsOnIds.
- Tests added.
ODataBatchOperationRequestMessage operationRequestMessage = BuildOperationRequestMessage(
this.RawOutputContext.OutputStream,
method, uri, contentId, /*groupId*/null, dependsOnIds, ODataFormat.Batch);

This comment has been minimized.

@mikepizzo

mikepizzo Jan 11, 2018

Member

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

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

This comment has been minimized.

@biaol-odata

biaol-odata Jan 11, 2018

Member

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)

This comment has been minimized.

@mikepizzo

mikepizzo Jan 11, 2018

Member

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)

@mikepizzo

This comment has been minimized.

Member

mikepizzo commented Jan 11, 2018

            if (HttpUtils.IsQueryMethod(method))

Actually, I don't think this restriction is in the json specification. We should probably make it either 4.0 specific (here), move it to multipart/mixed, or remove it all together. In general, if a service can support GET requests in a changeset I'd rather not get in their way, so we should probably just make this a 4.0 restriction.


In reply to: 356447377 [](ancestors = 356447377,355098382)


Refers to: src/Microsoft.OData.Core/ODataBatchWriter.cs:811 in 2ce1695. [](commit_id = 2ce1695, deletion_comment = False)

@@ -333,9 +327,9 @@ protected virtual string GetCurrentGroupIdImplementation()
string contentId,
string groupId,
IEnumerable<string> dependsOnRequestIds,
ODataFormat batchFormat)
bool dependsOnIdsValidationRequired)

This comment has been minimized.

@mikepizzo

mikepizzo Jan 11, 2018

Member

bool dependsOnIdsValidationRequired [](start = 12, length = 35)

Can we do the same thing that we did in the writer, and pass null versus an empty list of requestIds instead of a boolean flag here, and validate the Url against the payloadUriConverter in the null dependsOnIds case? #Closed

This comment has been minimized.

@biaol-odata

biaol-odata Jan 11, 2018

Member

I thought about that but we are out of luck / by design here. The dependsOnRequestIds is always not null, and it is always derived (implicit) for Multipart/Mixed batch, and always explicit for Json batch.


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

This comment has been minimized.

@mikepizzo

mikepizzo Jan 11, 2018

Member

What would happen if we removed the dependsOnIdsValidationRequired flag and always validated the dependsOnRequestIds? Shouldn't the implicit dependsOnRequestIds accumulated in the multipart/mixed case always be valid?


In reply to: 160847347 [](ancestors = 160847347,160846029)

This comment has been minimized.

@biaol-odata

biaol-odata Jan 11, 2018

Member

In V4, since the payloadUriConverter is reset after changset, the validation (L336) will throw because implicit dependsOnIds could contain Content-IDs from preceding top-level requests --> error.


In reply to: 160848457 [](ancestors = 160848457,160847347,160846029)

This comment has been minimized.

@mikepizzo

mikepizzo Jan 11, 2018

Member

Right; in 4.0, requests can only reference other requests within the same changeset, so payloadUriConverter only has the set of ids in the changeset, which is a subset of the requests that this request depends on coming before this request. Makes sense.


In reply to: 160849419 [](ancestors = 160849419,160848457,160847347,160846029)

Restricting BatchOperationRequestMessage.dependsOnIds from List<strin…
…g> to IEnumerable<string>, not allowing element updates.

@biaol-odata biaol-odata merged commit ff6102b into OData:master Jan 12, 2018

2 checks passed

continuous-integration/vsts The build trigger has fired.
Details
license/cla All CLA requirements met.
Details

@biaol-odata biaol-odata deleted the biaol-odata:master_dependsOnIdsForMultipartBatch branch Jan 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment