From eb15593ee76155b2cb4c144e838ce13833a402cc Mon Sep 17 00:00:00 2001 From: Jake Willey Date: Mon, 2 Dec 2019 07:41:23 -0800 Subject: [PATCH 1/5] Fixed bug where if a user provided the wrong partition key value the request would be sent twice. The retry logic for the partition key extraction was always being used if it was a user provided partition key value or an extracted partition key value. The retry logic should only be used for SDK extracted partition key value in scenarios where the cache is stale. --- .../Resource/Container/ContainerCore.Items.cs | 58 ++++++++++--------- .../CosmosHandlersTests.cs | 15 ----- .../NameRoutingTests.cs | 46 +++++++++++++++ .../Utils/CustomHandler.cs | 22 +++++++ 4 files changed, 99 insertions(+), 42 deletions(-) create mode 100644 Microsoft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.EmulatorTests/Utils/CustomHandler.cs diff --git a/Microsoft.Azure.Cosmos/src/Resource/Container/ContainerCore.Items.cs b/Microsoft.Azure.Cosmos/src/Resource/Container/ContainerCore.Items.cs index e957611d32..7ac6a6aa82 100644 --- a/Microsoft.Azure.Cosmos/src/Resource/Container/ContainerCore.Items.cs +++ b/Microsoft.Azure.Cosmos/src/Resource/Container/ContainerCore.Items.cs @@ -48,7 +48,6 @@ public override Task CreateItemStreamAsync( streamPayload, OperationType.Create, requestOptions, - extractPartitionKeyIfNeeded: false, cancellationToken: cancellationToken); } @@ -66,7 +65,7 @@ public override Task> CreateItemAsync( Task response = this.ExtractPartitionKeyAndProcessItemStreamAsync( partitionKey: partitionKey, itemId: null, - streamPayload: this.ClientContext.CosmosSerializer.ToStream(item), + item: item, operationType: OperationType.Create, requestOptions: requestOptions, cancellationToken: cancellationToken); @@ -86,7 +85,6 @@ public override Task ReadItemStreamAsync( null, OperationType.Read, requestOptions, - extractPartitionKeyIfNeeded: false, cancellationToken: cancellationToken); } @@ -117,7 +115,6 @@ public override Task UpsertItemStreamAsync( streamPayload, OperationType.Upsert, requestOptions, - extractPartitionKeyIfNeeded: false, cancellationToken: cancellationToken); } @@ -135,7 +132,7 @@ public override Task> UpsertItemAsync( Task response = this.ExtractPartitionKeyAndProcessItemStreamAsync( partitionKey: partitionKey, itemId: null, - streamPayload: this.ClientContext.CosmosSerializer.ToStream(item), + item: item, operationType: OperationType.Upsert, requestOptions: requestOptions, cancellationToken: cancellationToken); @@ -156,7 +153,6 @@ public override Task ReplaceItemStreamAsync( streamPayload, OperationType.Replace, requestOptions, - extractPartitionKeyIfNeeded: false, cancellationToken: cancellationToken); } @@ -180,7 +176,7 @@ public override Task> ReplaceItemAsync( Task response = this.ExtractPartitionKeyAndProcessItemStreamAsync( partitionKey: partitionKey, itemId: id, - streamPayload: this.ClientContext.CosmosSerializer.ToStream(item), + item: item, operationType: OperationType.Replace, requestOptions: requestOptions, cancellationToken: cancellationToken); @@ -200,7 +196,6 @@ public override Task DeleteItemStreamAsync( null, OperationType.Delete, requestOptions, - extractPartitionKeyIfNeeded: false, cancellationToken: cancellationToken); } @@ -511,24 +506,39 @@ internal FeedIterator GetItemQueryStreamIteratorInternal( // Extracted partition key might be invalid as CollectionCache might be stale. // Stale collection cache is refreshed through PartitionKeyMismatchRetryPolicy // and partition-key is extracted again. - internal async Task ExtractPartitionKeyAndProcessItemStreamAsync( + internal async Task ExtractPartitionKeyAndProcessItemStreamAsync( PartitionKey? partitionKey, string itemId, - Stream streamPayload, + T item, OperationType operationType, RequestOptions requestOptions, CancellationToken cancellationToken) { + Stream streamPayload = this.ClientContext.CosmosSerializer.ToStream(item); + + // User specified PK value, no need to extract it + if (partitionKey.HasValue) + { + return await this.ProcessItemStreamAsync( + partitionKey, + itemId, + streamPayload, + operationType, + requestOptions, + cancellationToken: cancellationToken); + } + PartitionKeyMismatchRetryPolicy requestRetryPolicy = null; while (true) { + partitionKey = await this.GetPartitionKeyValueFromStreamAsync(streamPayload, cancellationToken); + ResponseMessage responseMessage = await this.ProcessItemStreamAsync( partitionKey, itemId, streamPayload, operationType, requestOptions, - extractPartitionKeyIfNeeded: true, cancellationToken: cancellationToken); if (responseMessage.IsSuccessStatusCode) @@ -555,7 +565,6 @@ internal async Task ProcessItemStreamAsync( Stream streamPayload, OperationType operationType, RequestOptions requestOptions, - bool extractPartitionKeyIfNeeded, CancellationToken cancellationToken = default(CancellationToken)) { if (requestOptions != null && requestOptions.IsEffectivePartitionKeyRouting) @@ -563,25 +572,20 @@ internal async Task ProcessItemStreamAsync( partitionKey = null; } - if (extractPartitionKeyIfNeeded && partitionKey == null) - { - partitionKey = await this.GetPartitionKeyValueFromStreamAsync(streamPayload, cancellationToken); - } - ContainerCore.ValidatePartitionKey(partitionKey, requestOptions); Uri resourceUri = this.GetResourceUri(requestOptions, operationType, itemId); return await this.ClientContext.ProcessResourceOperationStreamAsync( - resourceUri, - ResourceType.Document, - operationType, - requestOptions, - this, - partitionKey, - itemId, - streamPayload, - null, - cancellationToken); + resourceUri: resourceUri, + resourceType: ResourceType.Document, + operationType: operationType, + requestOptions: requestOptions, + cosmosContainerCore: this, + partitionKey: partitionKey, + itemId: itemId, + streamPayload: streamPayload, + requestEnricher: null, + cancellationToken: cancellationToken); } internal async Task GetPartitionKeyValueFromStreamAsync( diff --git a/Microsoft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.EmulatorTests/CosmosHandlersTests.cs b/Microsoft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.EmulatorTests/CosmosHandlersTests.cs index 3d31f18112..7647525113 100644 --- a/Microsoft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.EmulatorTests/CosmosHandlersTests.cs +++ b/Microsoft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.EmulatorTests/CosmosHandlersTests.cs @@ -111,20 +111,5 @@ public class ToDoActivity public string description { get; set; } public string status { get; set; } } - - public class CustomHandler : RequestHandler - { - public Action UpdateRequestMessage = null; - - public override Task SendAsync(RequestMessage request, CancellationToken cancellationToken) - { - if (this.UpdateRequestMessage != null) - { - this.UpdateRequestMessage(request); - } - - return base.SendAsync(request, cancellationToken); - } - } } } diff --git a/Microsoft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.EmulatorTests/NameRoutingTests.cs b/Microsoft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.EmulatorTests/NameRoutingTests.cs index 05277fdcf1..90bc6e7323 100644 --- a/Microsoft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.EmulatorTests/NameRoutingTests.cs +++ b/Microsoft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.EmulatorTests/NameRoutingTests.cs @@ -1363,6 +1363,52 @@ internal async Task TestPartitionKeyDefinitionOnContainerRecreateFromDifferentPa } } + [TestMethod] + public async Task TestInvalidPartitionKeyException() + { + CustomHandler testHandler = new CustomHandler(); + int createItemCount = 0; + string partitionKey = null; + testHandler.UpdateRequestMessage = (requestMessage) => + { + if (requestMessage.ResourceType == ResourceType.Document) + { + createItemCount++; + string pk = requestMessage.Headers.PartitionKey; + Assert.IsFalse(string.Equals(partitionKey, pk), $"Same PK value should not be sent again. PK value:{pk}"); + partitionKey = pk; + } + }; + + using (CosmosClient client = TestCommon.CreateCosmosClient((builder) => builder.AddCustomHandlers(testHandler))) + { + Cosmos.Database database = null; + try + { + database = await client.CreateDatabaseAsync("TestInvalidPartitionKey" + Guid.NewGuid().ToString()); + Container container = await database.CreateContainerAsync(id: "coll1", partitionKeyPath: "/doesnotexist"); + ToDoActivity toDoActivity = ToDoActivity.CreateRandomToDoActivity(); + + ItemResponse response = await container.CreateItemAsync(toDoActivity, partitionKey: new Cosmos.PartitionKey(toDoActivity.status)); + Assert.Fail("Create item should fail with wrong partition key value"); + } + catch (CosmosException ce) + { + Assert.AreEqual(HttpStatusCode.BadRequest, ce.StatusCode); + Assert.AreEqual(SubStatusCodes.PartitionKeyMismatch, (SubStatusCodes)ce.SubStatusCode); + } + finally + { + if (database != null) + { + await database.DeleteAsync(); + } + } + + Assert.AreEqual(1, createItemCount, $"Request should use the custom handler, and it should only be used once. Count {createItemCount}"); + } + } + /// /// Tests that collection cache is refreshed when collection is recreated. /// The test just ensures that client retries and completes successfully for query - request which doesn't target single partition key. diff --git a/Microsoft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.EmulatorTests/Utils/CustomHandler.cs b/Microsoft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.EmulatorTests/Utils/CustomHandler.cs new file mode 100644 index 0000000000..7aaa6f00b6 --- /dev/null +++ b/Microsoft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.EmulatorTests/Utils/CustomHandler.cs @@ -0,0 +1,22 @@ +//------------------------------------------------------------ +// Copyright (c) Microsoft Corporation. All rights reserved. +//------------------------------------------------------------ + +namespace Microsoft.Azure.Cosmos.SDK.EmulatorTests +{ + using System; + using System.Threading; + using System.Threading.Tasks; + + public class CustomHandler : RequestHandler + { + public Action UpdateRequestMessage = null; + + public override Task SendAsync(RequestMessage request, CancellationToken cancellationToken) + { + this.UpdateRequestMessage?.Invoke(request); + + return base.SendAsync(request, cancellationToken); + } + } +} From c616f76f2593e463e5f2e3f44460772bed7d59c5 Mon Sep 17 00:00:00 2001 From: Jake Willey Date: Mon, 2 Dec 2019 07:47:07 -0800 Subject: [PATCH 2/5] Updated changelog --- changelog.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/changelog.md b/changelog.md index eb3e55eeef..913c5d3f6a 100644 --- a/changelog.md +++ b/changelog.md @@ -30,7 +30,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - [#1036](https://github.com/Azure/azure-cosmos-dotnet-v3/pull/1036) Fixed query responses to return null Content if it is a failure - [#1045](https://github.com/Azure/azure-cosmos-dotnet-v3/pull/1045) Added stack trace and innner exception to CosmosException - [#1050](https://github.com/Azure/azure-cosmos-dotnet-v3/pull/1050) Add mocking constructors to TransactionalBatchOperationResult - +- [#1070](https://github.com/Azure/azure-cosmos-dotnet-v3/pull/1070) Fixed bug where wrong partition key value might cause two requests + ## [3.4.1](https://www.nuget.org/packages/Microsoft.Azure.Cosmos/3.4.1) - 2019-11-06 ### Fixed From 56e23e1b510aabcaa58181f2ad6e9984dd3f1758 Mon Sep 17 00:00:00 2001 From: j82w Date: Mon, 2 Dec 2019 08:11:21 -0800 Subject: [PATCH 3/5] Update Microsoft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.EmulatorTests/NameRoutingTests.cs Co-Authored-By: Matias Quaranta --- .../Microsoft.Azure.Cosmos.EmulatorTests/NameRoutingTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Microsoft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.EmulatorTests/NameRoutingTests.cs b/Microsoft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.EmulatorTests/NameRoutingTests.cs index 90bc6e7323..769facd171 100644 --- a/Microsoft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.EmulatorTests/NameRoutingTests.cs +++ b/Microsoft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.EmulatorTests/NameRoutingTests.cs @@ -1375,7 +1375,7 @@ public async Task TestInvalidPartitionKeyException() { createItemCount++; string pk = requestMessage.Headers.PartitionKey; - Assert.IsFalse(string.Equals(partitionKey, pk), $"Same PK value should not be sent again. PK value:{pk}"); + Assert.AreEqual(partitionKey, pk, $"Same PK value should not be sent again. PK value:{pk}"); partitionKey = pk; } }; From 85382e3918aa18c5a69f6fa5a8241b95fc8b8eba Mon Sep 17 00:00:00 2001 From: Jake Willey Date: Mon, 2 Dec 2019 09:18:09 -0800 Subject: [PATCH 4/5] Fixed test, updated test handler name --- .../src/Resource/Container/ContainerCore.Items.cs | 1 - .../CosmosHandlersTests.cs | 2 +- .../Microsoft.Azure.Cosmos.EmulatorTests/NameRoutingTests.cs | 4 ++-- .../Utils/{CustomHandler.cs => RequestHandlerHelper.cs} | 2 +- 4 files changed, 4 insertions(+), 5 deletions(-) rename Microsoft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.EmulatorTests/Utils/{CustomHandler.cs => RequestHandlerHelper.cs} (92%) diff --git a/Microsoft.Azure.Cosmos/src/Resource/Container/ContainerCore.Items.cs b/Microsoft.Azure.Cosmos/src/Resource/Container/ContainerCore.Items.cs index 7ac6a6aa82..39589a32b3 100644 --- a/Microsoft.Azure.Cosmos/src/Resource/Container/ContainerCore.Items.cs +++ b/Microsoft.Azure.Cosmos/src/Resource/Container/ContainerCore.Items.cs @@ -6,7 +6,6 @@ namespace Microsoft.Azure.Cosmos { using System; using System.Collections.Generic; - using System.Diagnostics.Contracts; using System.Globalization; using System.IO; using System.Linq; diff --git a/Microsoft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.EmulatorTests/CosmosHandlersTests.cs b/Microsoft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.EmulatorTests/CosmosHandlersTests.cs index 7647525113..88ec42dc76 100644 --- a/Microsoft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.EmulatorTests/CosmosHandlersTests.cs +++ b/Microsoft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.EmulatorTests/CosmosHandlersTests.cs @@ -38,7 +38,7 @@ public async Task Cleanup() [TestMethod] public async Task TestCustomPropertyWithHandler() { - CustomHandler testHandler = new CustomHandler(); + RequestHandlerHelper testHandler = new RequestHandlerHelper(); // Add the random guid to the property Guid randomGuid = Guid.NewGuid(); diff --git a/Microsoft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.EmulatorTests/NameRoutingTests.cs b/Microsoft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.EmulatorTests/NameRoutingTests.cs index 769facd171..f63d978725 100644 --- a/Microsoft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.EmulatorTests/NameRoutingTests.cs +++ b/Microsoft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.EmulatorTests/NameRoutingTests.cs @@ -1366,7 +1366,7 @@ internal async Task TestPartitionKeyDefinitionOnContainerRecreateFromDifferentPa [TestMethod] public async Task TestInvalidPartitionKeyException() { - CustomHandler testHandler = new CustomHandler(); + RequestHandlerHelper testHandler = new RequestHandlerHelper(); int createItemCount = 0; string partitionKey = null; testHandler.UpdateRequestMessage = (requestMessage) => @@ -1375,7 +1375,7 @@ public async Task TestInvalidPartitionKeyException() { createItemCount++; string pk = requestMessage.Headers.PartitionKey; - Assert.AreEqual(partitionKey, pk, $"Same PK value should not be sent again. PK value:{pk}"); + Assert.AreNotEqual(partitionKey, pk, $"Same PK value should not be sent again. PK value:{pk}"); partitionKey = pk; } }; diff --git a/Microsoft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.EmulatorTests/Utils/CustomHandler.cs b/Microsoft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.EmulatorTests/Utils/RequestHandlerHelper.cs similarity index 92% rename from Microsoft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.EmulatorTests/Utils/CustomHandler.cs rename to Microsoft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.EmulatorTests/Utils/RequestHandlerHelper.cs index 7aaa6f00b6..7ebd8e10b7 100644 --- a/Microsoft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.EmulatorTests/Utils/CustomHandler.cs +++ b/Microsoft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.EmulatorTests/Utils/RequestHandlerHelper.cs @@ -8,7 +8,7 @@ namespace Microsoft.Azure.Cosmos.SDK.EmulatorTests using System.Threading; using System.Threading.Tasks; - public class CustomHandler : RequestHandler + public class RequestHandlerHelper : RequestHandler { public Action UpdateRequestMessage = null; From fef0cf75bccd3b042aaeb28dac8f8e511289fb79 Mon Sep 17 00:00:00 2001 From: j82w Date: Mon, 2 Dec 2019 12:05:19 -0800 Subject: [PATCH 5/5] Update changelog.md --- changelog.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.md b/changelog.md index 913c5d3f6a..71bbba5b40 100644 --- a/changelog.md +++ b/changelog.md @@ -30,7 +30,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - [#1036](https://github.com/Azure/azure-cosmos-dotnet-v3/pull/1036) Fixed query responses to return null Content if it is a failure - [#1045](https://github.com/Azure/azure-cosmos-dotnet-v3/pull/1045) Added stack trace and innner exception to CosmosException - [#1050](https://github.com/Azure/azure-cosmos-dotnet-v3/pull/1050) Add mocking constructors to TransactionalBatchOperationResult -- [#1070](https://github.com/Azure/azure-cosmos-dotnet-v3/pull/1070) Fixed bug where wrong partition key value might cause two requests +- [#1070](https://github.com/Azure/azure-cosmos-dotnet-v3/pull/1070) CreateItem will only retry for auto-extracted partition key in-case of collection re-creation ## [3.4.1](https://www.nuget.org/packages/Microsoft.Azure.Cosmos/3.4.1) - 2019-11-06