diff --git a/src/Service.Tests/SqlTests/GraphQLMutationTests/GraphQLMutationTestBase.cs b/src/Service.Tests/SqlTests/GraphQLMutationTests/GraphQLMutationTestBase.cs index 98e1194240..c3857d331c 100644 --- a/src/Service.Tests/SqlTests/GraphQLMutationTests/GraphQLMutationTestBase.cs +++ b/src/Service.Tests/SqlTests/GraphQLMutationTests/GraphQLMutationTestBase.cs @@ -677,6 +677,185 @@ public async Task DeleteMutationWithVariablesAndMappings(string dbQuery, string Assert.AreEqual(result.RootElement.GetProperty("count").GetInt64(), 0); } + /// + /// Performs concurrent update mutations on the same item and validates that the responses + /// returned are consistent + /// gQLMutation1 : Updates the title column of Book table to New Title + /// gQLMutation2 : Updates the title column of Book table to Updated Title + /// The title field in the responses returned for each of the mutations should be + /// the same value it had written to the table. + /// + [TestMethod] + public async Task TestParallelUpdateMutations() + { + string graphQLMutationName = "updatebook"; + string gQLMutation1 = @" + mutation { + updatebook(id : 1, item: { title: ""New Title"" }) + { + title + } + }"; + + string gQLMutation2 = @" + mutation { + updatebook(id : 1, item: { title: ""Updated Title"" }) + { + title + } + }"; + + Task responeTask1 = ExecuteGraphQLRequestAsync(gQLMutation1, graphQLMutationName, isAuthenticated: true); + Task responseTask2 = ExecuteGraphQLRequestAsync(gQLMutation2, graphQLMutationName, isAuthenticated: true); + + JsonElement response1 = await responeTask1; + JsonElement response2 = await responseTask2; + + Assert.AreEqual("{\"title\":\"New Title\"}", response1.ToString()); + Assert.AreEqual("{\"title\":\"Updated Title\"}", response2.ToString()); + } + + /// + /// Performs concurrent insert mutation on a table where the PK is auto-generated. + /// Since, PK is auto-generated, essentially both the mutations are operating on + /// different items. Therefore, both the mutations should succeed. + /// + [TestMethod] + public async Task TestParallelInsertMutationPKAutoGenerated() + { + string graphQLMutationName = "createbook"; + string graphQLMutation1 = @" + mutation { + createbook(item: { title: ""Awesome Book"", publisher_id: 1234 }) { + title + } + } + "; + + string graphQLMutation2 = @" + mutation { + createbook(item: { title: ""Another Awesome Book"", publisher_id: 1234 }) { + title + } + } + "; + + Task responeTask1 = ExecuteGraphQLRequestAsync(graphQLMutation1, graphQLMutationName, isAuthenticated: true); + Task responseTask2 = ExecuteGraphQLRequestAsync(graphQLMutation2, graphQLMutationName, isAuthenticated: true); + + JsonElement response1 = await responeTask1; + JsonElement response2 = await responseTask2; + + Assert.AreEqual("{\"title\":\"Awesome Book\"}", response1.ToString()); + Assert.AreEqual("{\"title\":\"Another Awesome Book\"}", response2.ToString()); + + } + + /// + /// Performs concurrent insert mutation on a table where the PK is not auto-generated and + /// validates that only one of the mutations is successful. + /// Both the mutations attempt to create an item with the same primary key. The mutation request + /// that runs first at the database layer should succeed and the other request should fail with + /// primary key violation constraint. + /// + [TestMethod] + public async Task TestParallelInsertMutationPKNonAutoGenerated() + { + string graphQLMutationName = "createComic"; + + string graphQLMutation1 = @" + mutation { + createComic (item: { id : 5001, categoryName: ""Fantasy"", title: ""Harry Potter""}){ + id + title + } + } + "; + + string graphQLMutation2 = @" + mutation { + createComic (item: { id : 5001, categoryName: ""Fantasy"", title: ""Lord of the Rings""}){ + id + title + } + } + "; + + Task responeTask1 = ExecuteGraphQLRequestAsync(graphQLMutation1, graphQLMutationName, isAuthenticated: true); + Task responseTask2 = ExecuteGraphQLRequestAsync(graphQLMutation2, graphQLMutationName, isAuthenticated: true); + + JsonElement response1 = await responeTask1; + JsonElement response2 = await responseTask2; + + string responseString1 = response1.ToString(); + string responseString2 = response2.ToString(); + string expectedStatusCode = $"{DataApiBuilderException.SubStatusCodes.DatabaseOperationFailed}"; + + // It is not possible to know beforehand which mutation created the new item. So, validations + // are performed for the cases where either mutation could have succeeded. In each case, + // one of the mutation's reponse will contain a valid repsonse and the other mutation's + // response would contain DatabaseOperationFailed sub-status code as it would've failed at + // the database layer due to primary key violation constraint. + if (responseString1.Contains($"\"code\":\"{expectedStatusCode}\"")) + { + Assert.AreEqual("{\"id\":5001,\"title\":\"Lord of the Rings\"}", responseString2); + } + else if (responseString2.Contains($"\"code\":\"{expectedStatusCode}\"")) + { + Assert.AreEqual("{\"id\":5001,\"title\":\"Harry Potter\"}", responseString1); + } + else + { + Assert.Fail("Unexpected error. Atleast one of the mutations should've succeeded"); + } + } + + /// + /// Performs concurrent delete mutations on the same item and validates that only one of the + /// requests is successful. + /// + [TestMethod] + public async Task TestParallelDeleteMutations() + { + string graphQLMutationName = "deletebook"; + + string graphQLMutation1 = @" + mutation { + deletebook (id: 1){ + id + title + } + } + "; + + Task responeTask1 = ExecuteGraphQLRequestAsync(graphQLMutation1, graphQLMutationName, isAuthenticated: true); + Task responseTask2 = ExecuteGraphQLRequestAsync(graphQLMutation1, graphQLMutationName, isAuthenticated: true); + + JsonElement response1 = await responeTask1; + JsonElement response2 = await responseTask2; + + string responseString1 = response1.ToString(); + string responseString2 = response2.ToString(); + string expectedResponse = "{\"id\":1,\"title\":\"Awesome book\"}"; + + // The mutation request that deletes the item is expected to have a valid response + // and the other mutation is expected to receive an empty response as it + // won't see the item in the table. + if (responseString1.Length == 0) + { + Assert.AreEqual(expectedResponse, responseString2); + } + else if (responseString2.Length == 0) + { + Assert.AreEqual(expectedResponse, responseString1); + } + else + { + Assert.Fail("Unexpected failure. Atleast one of the delete mutations should've succeeded"); + } + + } + #endregion #region Negative Tests diff --git a/src/Service/Resolvers/MsSqlQueryBuilder.cs b/src/Service/Resolvers/MsSqlQueryBuilder.cs index 79594f6df4..380ee0fbc2 100644 --- a/src/Service/Resolvers/MsSqlQueryBuilder.cs +++ b/src/Service/Resolvers/MsSqlQueryBuilder.cs @@ -105,12 +105,7 @@ public string Build(SqlExecuteStructure structure) $"{BuildProcedureParameterList(structure.ProcedureParameters)}"; } - /// - /// Avoid redundant check, wrap the sequence in a transaction, - /// and protect the first table access with appropriate locking. - /// - /// - /// Query generated for the PUT(upsert)/PATCH(upsertIncremental) operation. + /// public string Build(SqlUpsertQueryStructure structure) { string tableName = $"{QuoteIdentifier(structure.DatabaseObject.SchemaName)}.{QuoteIdentifier(structure.DatabaseObject.Name)}"; @@ -125,10 +120,8 @@ public string Build(SqlUpsertQueryStructure structure) string outputColumns = MakeOutputColumns(structure.OutputColumns, OutputQualifier.Inserted); string queryToGetCountOfRecordWithPK = $"SELECT COUNT(*) as {COUNT_ROWS_WITH_GIVEN_PK} FROM {tableName} WHERE {pkPredicates}"; - // Query to initiate transaction and get number of records with given PK. - string prefixQuery = $"SET TRANSACTION ISOLATION LEVEL SERIALIZABLE;" + - $"BEGIN TRANSACTION;" + - $"DECLARE @ROWS_TO_UPDATE int;" + + // Query to get the number of records with a given PK. + string prefixQuery = $"DECLARE @ROWS_TO_UPDATE int;" + $"SET @ROWS_TO_UPDATE = ({queryToGetCountOfRecordWithPK}); " + $"{queryToGetCountOfRecordWithPK};"; @@ -137,8 +130,8 @@ public string Build(SqlUpsertQueryStructure structure) // Query to update record (if there exists one for given PK). StringBuilder updateQuery = new( - $"IF @ROWS_TO_UPDATE = 1" + - $"UPDATE {tableName} WITH(UPDLOCK) " + + $"IF @ROWS_TO_UPDATE = 1 " + + $"UPDATE {tableName} " + $"SET {updateOperations} " + $"OUTPUT {outputColumns} " + $"WHERE {updatePredicates};"); @@ -172,9 +165,6 @@ public string Build(SqlUpsertQueryStructure structure) upsertQuery.Append(insertQuery.ToString()); } - // Commit the transaction. - upsertQuery.Append("COMMIT TRANSACTION"); - return upsertQuery.ToString(); } diff --git a/src/Service/Resolvers/SqlMutationEngine.cs b/src/Service/Resolvers/SqlMutationEngine.cs index 90fe23acd5..f3ead0daf4 100644 --- a/src/Service/Resolvers/SqlMutationEngine.cs +++ b/src/Service/Resolvers/SqlMutationEngine.cs @@ -11,6 +11,7 @@ using System.Text.Json; using System.Text.Json.Nodes; using System.Threading.Tasks; +using System.Transactions; using Azure.DataApiBuilder.Auth; using Azure.DataApiBuilder.Config; using Azure.DataApiBuilder.Service.Authorization; @@ -40,6 +41,11 @@ public class SqlMutationEngine : IMutationEngine private readonly IHttpContextAccessor _httpContextAccessor; private readonly GQLFilterParser _gQLFilterParser; public const string IS_UPDATE_RESULT_SET = "IsUpdateResultSet"; + private const string TRANSACTION_EXCEPTION_ERROR_MSG = "An unexpected error occurred during the transaction execution"; + + private static DataApiBuilderException _dabExceptionWithTransactionErrorMessage = new(message: TRANSACTION_EXCEPTION_ERROR_MSG, + statusCode: HttpStatusCode.InternalServerError, + subStatusCode: DataApiBuilderException.SubStatusCodes.UnexpectedError); /// /// Constructor @@ -91,58 +97,74 @@ public SqlMutationEngine( // If authorization fails, an exception will be thrown and request execution halts. AuthorizeMutationFields(context, parameters, entityName, mutationOperation); - if (mutationOperation is Config.Operation.Delete) + try { - // compute the mutation result before removing the element, - // since typical GraphQL delete mutations return the metadata of the deleted item. - result = await _queryEngine.ExecuteAsync(context, GetBackingColumnsFromCollection(entityName, parameters)); - - Dictionary? resultProperties = - await PerformDeleteOperation( - entityName, - parameters); - - // If the number of records affected by DELETE were zero, - // and yet the result was not null previously, it indicates this DELETE lost - // a concurrent request race. Hence, empty the non-null result. - if (resultProperties is not null - && resultProperties.TryGetValue(nameof(DbDataReader.RecordsAffected), out object? value) - && Convert.ToInt32(value) == 0 - && result is not null && result.Item1 is not null) + // Creating an implicit transaction + using (TransactionScope transactionScope = ConstructTransactionScopeBasedOnDbType()) { - result = new Tuple( - default(JsonDocument), - PaginationMetadata.MakeEmptyPaginationMetadata()); + if (mutationOperation is Config.Operation.Delete) + { + // compute the mutation result before removing the element, + // since typical GraphQL delete mutations return the metadata of the deleted item. + result = await _queryEngine.ExecuteAsync(context, GetBackingColumnsFromCollection(entityName, parameters)); + + Dictionary? resultProperties = + await PerformDeleteOperation( + entityName, + parameters); + + // If the number of records affected by DELETE were zero, + // and yet the result was not null previously, it indicates this DELETE lost + // a concurrent request race. Hence, empty the non-null result. + if (resultProperties is not null + && resultProperties.TryGetValue(nameof(DbDataReader.RecordsAffected), out object? value) + && Convert.ToInt32(value) == 0 + && result is not null && result.Item1 is not null) + { + result = new Tuple( + default(JsonDocument), + PaginationMetadata.MakeEmptyPaginationMetadata()); + } + } + else + { + DbResultSetRow? mutationResultRow = + await PerformMutationOperation( + entityName, + mutationOperation, + parameters, + context); + + if (mutationResultRow is not null && mutationResultRow.Columns.Count > 0 + && !context.Selection.Type.IsScalarType()) + { + // Because the GraphQL mutation result set columns were exposed (mapped) column names, + // the column names must be converted to backing (source) column names so the + // PrimaryKeyPredicates created in the SqlQueryStructure created by the query engine + // represent database column names. + result = await _queryEngine.ExecuteAsync( + context, + GetBackingColumnsFromCollection(entityName, mutationResultRow.Columns)); + } + } + + transactionScope.Complete(); } } - else + // All the exceptions that can be thrown by .Complete() and .Dispose() methods of transactionScope + // derive from TransactionException. Hence, TransactionException acts as a catch-all. + // When an exception related to Transactions is encountered, the mutation is deemed unsuccessful and + // a DataApiBuilderException is thrown + catch (TransactionException) { - DbResultSetRow? mutationResultRow = - await PerformMutationOperation( - entityName, - mutationOperation, - parameters, - context); - - if (mutationResultRow is not null && mutationResultRow.Columns.Count > 0 - && !context.Selection.Type.IsScalarType()) - { - // Because the GraphQL mutation result set columns were exposed (mapped) column names, - // the column names must be converted to backing (source) column names so the - // PrimaryKeyPredicates created in the SqlQueryStructure created by the query engine - // represent database column names. - result = await _queryEngine.ExecuteAsync( - context, - GetBackingColumnsFromCollection(entityName, mutationResultRow.Columns)); - } + throw _dabExceptionWithTransactionErrorMessage; } if (result is null) { - throw new DataApiBuilderException( - message: "Failed to resolve any query based on the current configuration.", - statusCode: HttpStatusCode.BadRequest, - subStatusCode: DataApiBuilderException.SubStatusCodes.UnexpectedError); + throw new DataApiBuilderException(message: "Failed to resolve any query based on the current configuration.", + statusCode: HttpStatusCode.BadRequest, + subStatusCode: DataApiBuilderException.SubStatusCodes.UnexpectedError); } return result; @@ -191,12 +213,32 @@ await PerformMutationOperation( context.ResolvedParameters); string queryText = _queryBuilder.Build(executeQueryStructure); - JsonArray? resultArray = - await _queryExecutor.ExecuteQueryAsync( - queryText, - executeQueryStructure.Parameters, - _queryExecutor.GetJsonArrayAsync, - GetHttpContext()); + JsonArray? resultArray = null; + + try + { + // Creating an implicit transaction + using (TransactionScope transactionScope = ConstructTransactionScopeBasedOnDbType()) + { + resultArray = + await _queryExecutor.ExecuteQueryAsync( + queryText, + executeQueryStructure.Parameters, + _queryExecutor.GetJsonArrayAsync, + GetHttpContext()); + + transactionScope.Complete(); + } + } + + // All the exceptions that can be thrown by .Complete() and .Dispose() methods of transactionScope + // derive from TransactionException. Hence, TransactionException acts as a catch-all. + // When an exception related to Transactions is encountered, the mutation is deemed unsuccessful and + // a DataApiBuilderException is thrown + catch (TransactionException) + { + throw _dabExceptionWithTransactionErrorMessage; + } // A note on returning stored procedure results: // We can't infer what the stored procedure actually did beyond the HasRows and RecordsAffected attributes @@ -271,10 +313,28 @@ await _queryExecutor.ExecuteQueryAsync( if (context.OperationType is Config.Operation.Delete) { - Dictionary? resultProperties = - await PerformDeleteOperation( - context.EntityName, - parameters); + Dictionary? resultProperties = null; + + try + { + // Creating an implicit transaction + using (TransactionScope transactionScope = ConstructTransactionScopeBasedOnDbType()) + { + resultProperties = await PerformDeleteOperation( + context.EntityName, + parameters); + transactionScope.Complete(); + } + } + + // All the exceptions that can be thrown by .Complete() and .Dispose() methods of transactionScope + // derive from TransactionException. Hence, TransactionException acts as a catch-all. + // When an exception related to Transactions is encountered, the mutation is deemed unsuccessful and + // a DataApiBuilderException is thrown + catch (TransactionException) + { + throw _dabExceptionWithTransactionErrorMessage; + } // Records affected tells us that item was successfully deleted. // No records affected happens for a DELETE request on nonexistent object @@ -287,10 +347,28 @@ await PerformDeleteOperation( } else if (context.OperationType is Config.Operation.Upsert || context.OperationType is Config.Operation.UpsertIncremental) { - DbResultSet? upsertOperationResult = - await PerformUpsertOperation( - parameters, - context); + DbResultSet? upsertOperationResult = null; + + try + { + // Creating an implicit transaction + using (TransactionScope transactionScope = ConstructTransactionScopeBasedOnDbType()) + { + upsertOperationResult = await PerformUpsertOperation( + parameters, + context); + transactionScope.Complete(); + } + } + + // All the exceptions that can be thrown by .Complete() and .Dispose() methods of transactionScope + // derive from TransactionException. Hence, TransactionException acts as a catch-all. + // When an exception related to Transactions is encountered, the mutation is deemed unsuccessful and + // a DataApiBuilderException is thrown + catch (TransactionException) + { + throw _dabExceptionWithTransactionErrorMessage; + } DbResultSetRow? dbResultSetRow = upsertOperationResult is not null ? (upsertOperationResult.Rows.FirstOrDefault() ?? new()) : null; @@ -323,11 +401,30 @@ await PerformUpsertOperation( } else { - DbResultSetRow? mutationResultRow = - await PerformMutationOperation( - context.EntityName, - context.OperationType, - parameters); + DbResultSetRow? mutationResultRow = null; + + try + { + // Creating an implicit transaction + using (TransactionScope transactionScope = ConstructTransactionScopeBasedOnDbType()) + { + mutationResultRow = + await PerformMutationOperation( + context.EntityName, + context.OperationType, + parameters); + transactionScope.Complete(); + } + } + + // All the exceptions that can be thrown by .Complete() and .Dispose() methods of transactionScope + // derive from TransactionException. Hence, TransactionException acts as a catch-all. + // When an exception related to Transactions is encountered, the mutation is deemed unsuccessful and + // a DataApiBuilderException is thrown + catch (TransactionException) + { + throw _dabExceptionWithTransactionErrorMessage; + } if (context.OperationType is Config.Operation.Insert) { @@ -825,5 +922,35 @@ private HttpContext GetHttpContext() { return _httpContextAccessor.HttpContext!; } + + /// + /// For MySql database type, the isolation level is set at Repeatable Read as it is the default isolation level. Likeweise, for MsSql and PostgreSql + /// database types, the isolation level is set at Read Committed as it is the default. + /// + /// TransactionScope object with the appropriate isolation level based on the database type + private TransactionScope ConstructTransactionScopeBasedOnDbType() + { + return _sqlMetadataProvider.GetDatabaseType() is DatabaseType.mysql ? ConstructTransactionScopeWithSpecifiedIsolationLevel(isolationLevel: System.Transactions.IsolationLevel.RepeatableRead) + : ConstructTransactionScopeWithSpecifiedIsolationLevel(isolationLevel: System.Transactions.IsolationLevel.ReadCommitted); + } + + /// + /// Helper method to construct a TransactionScope object with the specified isolation level and + /// with the TransactionScopeAsyncFlowOption option enabled. + /// + /// Transaction isolation level + /// + /// + /// + /// TransactionScope object set at the specified isolation level + private static TransactionScope ConstructTransactionScopeWithSpecifiedIsolationLevel(System.Transactions.IsolationLevel isolationLevel) + { + return new(TransactionScopeOption.Required, + new TransactionOptions + { + IsolationLevel = isolationLevel + }, + TransactionScopeAsyncFlowOption.Enabled); + } } }