Fix bug on depends on atomicity group id v4

src/Microsoft.OData.Core/Batch/ODataBatchReader.cs

@@ -290,6 +290,13 @@ protected virtual string GetCurrentGroupIdImplementation()
         /// <returns>The batch reader state after the read.</returns>
         protected abstract ODataBatchReaderState ReadAtChangesetEndImplementation();
 
+        /// <summary>
+        /// Validate the dependsOnIds.
+        /// </summary>
+        /// <param name="contentId">The request Id</param>
+        /// <param name="dependsOnIds">The dependsOn ids specifying current request's prerequisites.</param>
+        protected abstract void ValidateDependsOnIds(string contentId, IEnumerable<string> dependsOnIds);
+
         /// <summary>
         /// Instantiate an <see cref="ODataBatchOperationRequestMessage"/> instance.
         /// </summary>
@@ -318,13 +325,7 @@ protected virtual string GetCurrentGroupIdImplementation()
         {
             if (dependsOnRequestIds != null && dependsOnIdsValidationRequired)
             {
-                foreach (string id in dependsOnRequestIds)
-                {
-                    if (!this.PayloadUriConverter.ContainsContentId(id))
-                    {
-                        throw new ODataException(Strings.ODataBatchReader_DependsOnIdNotFound(id, contentId));
-                    }
-                }
+                ValidateDependsOnIds(contentId, dependsOnRequestIds);
             }
 
             Uri uri = ODataBatchUtils.CreateOperationRequestUri(

src/Microsoft.OData.Core/Batch/ODataBatchWriter.cs

@@ -25,7 +25,7 @@ public abstract class ODataBatchWriter : IODataStreamListener, IODataOutputInStr
         private readonly ODataOutputContext outputContext;
 
         /// <summary>The batch-specific URL converter that stores the content IDs found in a changeset and supports resolving cross-referencing URLs.</summary>
-        private readonly ODataBatchPayloadUriConverter payloadUriConverter;
+        internal readonly ODataBatchPayloadUriConverter payloadUriConverter;
 
         /// <summary>The dependency injection container to get related services.</summary>
         private readonly IServiceProvider container;
@@ -498,6 +498,13 @@ protected void SetState(BatchWriterState newState)
         /// <returns>An enumerable consists of request ids.</returns>
         protected abstract IEnumerable<string> GetDependsOnRequestIds(IEnumerable<string> dependsOnIds);
 
+        /// <summary>
+        /// Validate the dependsOnIds.
+        /// </summary>
+        /// <param name="contentId">The request Id</param>
+        /// <param name="dependsOnIds">The dependsOn ids specifying current request's prerequisites.</param>
+        protected abstract void ValidateDependsOnIds(string contentId, IEnumerable<string> dependsOnIds);
+
         /// <summary>
         /// Wrapper method to create an operation request message that can be used to write the operation content to, utilizing
         /// private members <see cref="ODataBatchPayloadUriConverter"/> and <see cref="IServiceProvider"/>.
@@ -517,14 +524,7 @@ protected void SetState(BatchWriterState newState)
 
             if (dependsOnIds != null)
             {
-                // Validate explicit dependsOnIds cases.
-                foreach (string id in convertedDependsOnIds)
-                {
-                    if (!this.payloadUriConverter.ContainsContentId(id))
-                    {
-                        throw new ODataException(Strings.ODataBatchReader_DependsOnIdNotFound(id, contentId));
-                    }
-                }
+                ValidateDependsOnIds(contentId, convertedDependsOnIds);
             }
 
             // If dependsOnIds is not specified, use the <code>payloadUrlConverter</code>; otherwise use the dependOnIds converted

src/Microsoft.OData.Core/JsonLight/ODataJsonLightBatchReader.cs

@@ -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);
+            }
+
+            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>

src/Microsoft.OData.Core/JsonLight/ODataJsonLightBatchWriter.cs

@@ -261,6 +261,23 @@ protected override IEnumerable<string> GetDependsOnRequestIds(IEnumerable<string
             return dependsOnRequestIds;
         }
 
+        /// <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.
+                if (id == contentId || !this.requestIdToAtomicGroupId.ContainsKey(id))
+                {
+                    throw new ODataException(Strings.ODataBatchReader_DependsOnIdNotFound(id, contentId));
+                }
+            }
+        }
+
         /// <summary>
         /// Ends a batch - implementation of the actual functionality.
         /// </summary>

src/Microsoft.OData.Core/MultipartMixed/ODataMultipartMixedBatchReader.cs

@@ -196,6 +196,22 @@ protected override ODataBatchReaderState ReadAtChangesetEndImplementation()
             return this.SkipToNextPartAndReadHeaders();
         }
 
+        /// <summary>
+        /// Validate if the dependsOnIds are in the ContentIdCache.
+        /// </summary>
+        /// <param name="dependsOnIds">The dependsOn ids specifying current request's prerequisites.</param>
+        protected override void ValidateDependsOnIds(string contentId, IEnumerable<string> dependsOnIds)
+        {
+            // Validate explicit dependsOnIds cases.
+            foreach (string id in dependsOnIds)
+            {
+                if (!this.PayloadUriConverter.ContainsContentId(id))
+                {
+                    throw new ODataException(Strings.ODataBatchReader_DependsOnIdNotFound(id, contentId));
+                }
+            }
+        }
+
         /// <summary>
         /// Returns the next state of the batch reader after an end boundary has been found.
         /// </summary>

src/Microsoft.OData.Core/MultipartMixed/ODataMultipartMixedBatchWriter.cs

@@ -214,6 +214,22 @@ protected override IEnumerable<string> GetDependsOnRequestIds(IEnumerable<string
             return dependsOnIds ?? this.dependsOnIdsTracker.GetDependsOnIds();
         }
 
+        /// <summary>
+        /// Validate if the dependsOnIds are in the ContentIdCache.
+        /// </summary>
+        /// <param name="dependsOnIds">The dependsOn ids specifying current request's prerequisites.</param>
+        protected override void ValidateDependsOnIds(string contentId, IEnumerable<string> dependsOnIds)
+        {
+            // Validate explicit dependsOnIds cases.
+            foreach (string id in dependsOnIds)
+            {
+                if (!this.payloadUriConverter.ContainsContentId(id))
+                {
+                    throw new ODataException(Strings.ODataBatchReader_DependsOnIdNotFound(id, contentId));
+                }
+            }
+        }
+
         /// <summary>
         /// Creates an <see cref="ODataBatchOperationRequestMessage"/> for writing an operation of a batch request
         /// - implementation of the actual functionality.

test/FunctionalTests/Microsoft.OData.Core.Tests/ScenarioTests/Roundtrip/JsonLight/SingletonBatchRoundtripJsonLightTests.cs

@@ -1023,7 +1023,12 @@ public void BatchJsonLightSelfReferenceUriTest()
             Assert.True(expectedExceptionThrown, "Uri self-referencing with its Content-ID should not be allowed.");
         }
 
-        [Fact]
+        // This test isn't relevant in Json Batch v4.
+        // We use atomicGroup Id in dependsOn for 2 reasons:
+        // 1. To extract Request ids for use in reference Uris (in v4.01).
+        // 2. To sequence requests so that they are executed in a certain order (in both v4 and v4.01).
+        // This test made an assumption that we only use atomicityGroup for 1 above.
+        /*[Fact]
         public void BatchJsonLightReferenceUriV4TestShouldThrow()
         {
             bool exceptionThrown = true;
@@ -1040,6 +1045,16 @@ public void BatchJsonLightReferenceUriV4TestShouldThrow()
 
             Assert.True(exceptionThrown, "An exception should have been thrown when trying to refer to the " +
                 "content Id of the last request of a change set or atomic group in V4.");
+        }*/
+
+        [Fact]
+        public void BatchJsonLightDependsOnAtomicityGroupV4Test()
+        {
+            byte[] requestPayload = this.CreateReferenceUriBatchRequest(ODataVersion.V4);
+            VerifyPayload(requestPayload, ExpectedReferenceUriRequestPayload);
+
+            byte[] responsePayload = this.ServiceReadReferenceUriBatchRequestAndWriteResponse(requestPayload);
+            VerifyPayload(responsePayload, ExpectedReferenceUriResponsePayload);
         }
 
         [Fact]
@@ -1643,7 +1658,7 @@ private byte[] CreateCreateReferenceUriBatchRequestUseDifferentBaseUri(ODataVers
                 }
                 else if (useRequestIdOfGroupForDependsOnIds)
                 {
-                    dependsOnIds = new string[] {"1", "2"};
+                    dependsOnIds = new string[] { "1", "2" };
                 }
                 else
                 {