Skip to content

Commit e9393c2

Browse files
severussundarseverussundar
authored
Executing GraphQL and REST Mutations in a transaction (#1451)
## Why make this change? Closes #814 When two update mutations act on the same item, the responses returned back to these mutations are misleading. Update Mutation that was run: ```graphql mutation updateNotebook($id: Int!, $item: UpdateNotebookInput!) { updateNotebook(id: $id, item: $item) { id color } } ``` When two graphQL mutations were executed in parallel with the following variables, ```json { "id": 3, "item": { "color": "cyan" } } ``` ```json { "id": 3, "item": { "color": "magenta" } } ``` the responses for both the mutations were either ```json {"data":{"updateNotebook":{"id":3,"color":"magenta"}}} ``` or ```json {"data":{"updateNotebook":{"id":3,"color":"cyan"}}} ``` Within the same mutation, the data read back from the database should be the same as that of the updated value. ## What is this change? - For a single graphQL mutation, there are 2 database queries performed. Both these DB queries are run within a single transaction to ensure consistency during concurrent updates. - [Implicit Transactions docs](https://learn.microsoft.com/en-us/dotnet/framework/data/transactions/implementing-an-implicit-transaction-using-transaction-scope). - The transaction isolation level is chosen based on the database type and the value chosen is the default isolation level for each database type. - In the database query constructed for REST upsert scenario, the explicit creation of a transaction using T-SQL statements such as `SET TRANSACTION ISOLATION LEVEL SERIALIZABLE;BEGIN TRANSACTION;..` is removed. Since, the entire `PerformMutation()` block executes within a transactionscope, creation of another transaction with a different isolation level should not be done. - Integration tests to validate that responses are consistent when there are concurrent requests are added ## How was this tested? - [x] Existing Unit Tests - [x] Integration Tests - k6 test suite: > The concurrent test suite written using k6 - [link](https://github.com/Azure/data-api-builder/tree/main/src/Service.Tests/ConcurrentTests/SqlTests) was run in local against each of the database types. > > Scenarios covered by the Concurrent Test suite: > > > Parallel Read Operations on different items > > Parallel CRUD Operations on different items > > Parallel Update and Read Operations on the same item using GraphQL > > Parallel Update and Read Operations on the same item using REST > > Parallel Create Operations when the PK is auto-generated > > Parallel Create Operations when the PK is not auto-generated > > Parallel Delete Operations on the same item > - Jmeter: > Tested concurrent updates with two mutations trying to update the same row > Concurrent graphQL mutation and query updating/reading the same row > Concurrent REST PATCH requests updating the same row > Concurrent REST PATCH and REST GET requests updating/reading the same row > Concurrent graphQL mutation and REST GET acting on the same row. Performed all the scenarios with 100 threads (50 threads of each request type) to see if there were any occurrences of errors related to transactions when the volume of requests trying to update the same row is high.
1 parent bcc1e04 commit e9393c2

File tree

3 files changed

+373
-77
lines changed

3 files changed

+373
-77
lines changed

‎src/Service.Tests/SqlTests/GraphQLMutationTests/GraphQLMutationTestBase.cs‎

Lines changed: 179 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -677,6 +677,185 @@ public async Task DeleteMutationWithVariablesAndMappings(string dbQuery, string
677677
Assert.AreEqual(result.RootElement.GetProperty("count").GetInt64(), 0);
678678
}
679679

680+
/// <summary>
681+
/// Performs concurrent update mutations on the same item and validates that the responses
682+
/// returned are consistent
683+
/// gQLMutation1 : Updates the title column of Book table to New Title
684+
/// gQLMutation2 : Updates the title column of Book table to Updated Title
685+
/// The title field in the responses returned for each of the mutations should be
686+
/// the same value it had written to the table.
687+
/// </summary>
688+
[TestMethod]
689+
public async Task TestParallelUpdateMutations()
690+
{
691+
string graphQLMutationName = "updatebook";
692+
string gQLMutation1 = @"
693+
mutation {
694+
updatebook(id : 1, item: { title: ""New Title"" })
695+
{
696+
title
697+
}
698+
}";
699+
700+
string gQLMutation2 = @"
701+
mutation {
702+
updatebook(id : 1, item: { title: ""Updated Title"" })
703+
{
704+
title
705+
}
706+
}";
707+
708+
Task<JsonElement> responeTask1 = ExecuteGraphQLRequestAsync(gQLMutation1, graphQLMutationName, isAuthenticated: true);
709+
Task<JsonElement> responseTask2 = ExecuteGraphQLRequestAsync(gQLMutation2, graphQLMutationName, isAuthenticated: true);
710+
711+
JsonElement response1 = await responeTask1;
712+
JsonElement response2 = await responseTask2;
713+
714+
Assert.AreEqual("{\"title\":\"New Title\"}", response1.ToString());
715+
Assert.AreEqual("{\"title\":\"Updated Title\"}", response2.ToString());
716+
}
717+
718+
/// <summary>
719+
/// Performs concurrent insert mutation on a table where the PK is auto-generated.
720+
/// Since, PK is auto-generated, essentially both the mutations are operating on
721+
/// different items. Therefore, both the mutations should succeed.
722+
/// </summary>
723+
[TestMethod]
724+
public async Task TestParallelInsertMutationPKAutoGenerated()
725+
{
726+
string graphQLMutationName = "createbook";
727+
string graphQLMutation1 = @"
728+
mutation {
729+
createbook(item: { title: ""Awesome Book"", publisher_id: 1234 }) {
730+
title
731+
}
732+
}
733+
";
734+
735+
string graphQLMutation2 = @"
736+
mutation {
737+
createbook(item: { title: ""Another Awesome Book"", publisher_id: 1234 }) {
738+
title
739+
}
740+
}
741+
";
742+
743+
Task<JsonElement> responeTask1 = ExecuteGraphQLRequestAsync(graphQLMutation1, graphQLMutationName, isAuthenticated: true);
744+
Task<JsonElement> responseTask2 = ExecuteGraphQLRequestAsync(graphQLMutation2, graphQLMutationName, isAuthenticated: true);
745+
746+
JsonElement response1 = await responeTask1;
747+
JsonElement response2 = await responseTask2;
748+
749+
Assert.AreEqual("{\"title\":\"Awesome Book\"}", response1.ToString());
750+
Assert.AreEqual("{\"title\":\"Another Awesome Book\"}", response2.ToString());
751+
752+
}
753+
754+
/// <summary>
755+
/// Performs concurrent insert mutation on a table where the PK is not auto-generated and
756+
/// validates that only one of the mutations is successful.
757+
/// Both the mutations attempt to create an item with the same primary key. The mutation request
758+
/// that runs first at the database layer should succeed and the other request should fail with
759+
/// primary key violation constraint.
760+
/// </summary>
761+
[TestMethod]
762+
public async Task TestParallelInsertMutationPKNonAutoGenerated()
763+
{
764+
string graphQLMutationName = "createComic";
765+
766+
string graphQLMutation1 = @"
767+
mutation {
768+
createComic (item: { id : 5001, categoryName: ""Fantasy"", title: ""Harry Potter""}){
769+
id
770+
title
771+
}
772+
}
773+
";
774+
775+
string graphQLMutation2 = @"
776+
mutation {
777+
createComic (item: { id : 5001, categoryName: ""Fantasy"", title: ""Lord of the Rings""}){
778+
id
779+
title
780+
}
781+
}
782+
";
783+
784+
Task<JsonElement> responeTask1 = ExecuteGraphQLRequestAsync(graphQLMutation1, graphQLMutationName, isAuthenticated: true);
785+
Task<JsonElement> responseTask2 = ExecuteGraphQLRequestAsync(graphQLMutation2, graphQLMutationName, isAuthenticated: true);
786+
787+
JsonElement response1 = await responeTask1;
788+
JsonElement response2 = await responseTask2;
789+
790+
string responseString1 = response1.ToString();
791+
string responseString2 = response2.ToString();
792+
string expectedStatusCode = $"{DataApiBuilderException.SubStatusCodes.DatabaseOperationFailed}";
793+
794+
// It is not possible to know beforehand which mutation created the new item. So, validations
795+
// are performed for the cases where either mutation could have succeeded. In each case,
796+
// one of the mutation's reponse will contain a valid repsonse and the other mutation's
797+
// response would contain DatabaseOperationFailed sub-status code as it would've failed at
798+
// the database layer due to primary key violation constraint.
799+
if (responseString1.Contains($"\"code\":\"{expectedStatusCode}\""))
800+
{
801+
Assert.AreEqual("{\"id\":5001,\"title\":\"Lord of the Rings\"}", responseString2);
802+
}
803+
else if (responseString2.Contains($"\"code\":\"{expectedStatusCode}\""))
804+
{
805+
Assert.AreEqual("{\"id\":5001,\"title\":\"Harry Potter\"}", responseString1);
806+
}
807+
else
808+
{
809+
Assert.Fail("Unexpected error. Atleast one of the mutations should've succeeded");
810+
}
811+
}
812+
813+
/// <summary>
814+
/// Performs concurrent delete mutations on the same item and validates that only one of the
815+
/// requests is successful.
816+
/// </summary>
817+
[TestMethod]
818+
public async Task TestParallelDeleteMutations()
819+
{
820+
string graphQLMutationName = "deletebook";
821+
822+
string graphQLMutation1 = @"
823+
mutation {
824+
deletebook (id: 1){
825+
id
826+
title
827+
}
828+
}
829+
";
830+
831+
Task<JsonElement> responeTask1 = ExecuteGraphQLRequestAsync(graphQLMutation1, graphQLMutationName, isAuthenticated: true);
832+
Task<JsonElement> responseTask2 = ExecuteGraphQLRequestAsync(graphQLMutation1, graphQLMutationName, isAuthenticated: true);
833+
834+
JsonElement response1 = await responeTask1;
835+
JsonElement response2 = await responseTask2;
836+
837+
string responseString1 = response1.ToString();
838+
string responseString2 = response2.ToString();
839+
string expectedResponse = "{\"id\":1,\"title\":\"Awesome book\"}";
840+
841+
// The mutation request that deletes the item is expected to have a valid response
842+
// and the other mutation is expected to receive an empty response as it
843+
// won't see the item in the table.
844+
if (responseString1.Length == 0)
845+
{
846+
Assert.AreEqual(expectedResponse, responseString2);
847+
}
848+
else if (responseString2.Length == 0)
849+
{
850+
Assert.AreEqual(expectedResponse, responseString1);
851+
}
852+
else
853+
{
854+
Assert.Fail("Unexpected failure. Atleast one of the delete mutations should've succeeded");
855+
}
856+
857+
}
858+
680859
#endregion
681860

682861
#region Negative Tests

‎src/Service/Resolvers/MsSqlQueryBuilder.cs‎

Lines changed: 5 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -105,12 +105,7 @@ public string Build(SqlExecuteStructure structure)
105105
$"{BuildProcedureParameterList(structure.ProcedureParameters)}";
106106
}
107107

108-
/// <summary>
109-
/// Avoid redundant check, wrap the sequence in a transaction,
110-
/// and protect the first table access with appropriate locking.
111-
/// </summary>
112-
/// <param name="structure"></param>
113-
/// <returns>Query generated for the PUT(upsert)/PATCH(upsertIncremental) operation.</returns>
108+
/// <inheritdoc />
114109
public string Build(SqlUpsertQueryStructure structure)
115110
{
116111
string tableName = $"{QuoteIdentifier(structure.DatabaseObject.SchemaName)}.{QuoteIdentifier(structure.DatabaseObject.Name)}";
@@ -125,10 +120,8 @@ public string Build(SqlUpsertQueryStructure structure)
125120
string outputColumns = MakeOutputColumns(structure.OutputColumns, OutputQualifier.Inserted);
126121
string queryToGetCountOfRecordWithPK = $"SELECT COUNT(*) as {COUNT_ROWS_WITH_GIVEN_PK} FROM {tableName} WHERE {pkPredicates}";
127122

128-
// Query to initiate transaction and get number of records with given PK.
129-
string prefixQuery = $"SET TRANSACTION ISOLATION LEVEL SERIALIZABLE;" +
130-
$"BEGIN TRANSACTION;" +
131-
$"DECLARE @ROWS_TO_UPDATE int;" +
123+
// Query to get the number of records with a given PK.
124+
string prefixQuery = $"DECLARE @ROWS_TO_UPDATE int;" +
132125
$"SET @ROWS_TO_UPDATE = ({queryToGetCountOfRecordWithPK}); " +
133126
$"{queryToGetCountOfRecordWithPK};";
134127

@@ -137,8 +130,8 @@ public string Build(SqlUpsertQueryStructure structure)
137130

138131
// Query to update record (if there exists one for given PK).
139132
StringBuilder updateQuery = new(
140-
$"IF @ROWS_TO_UPDATE = 1" +
141-
$"UPDATE {tableName} WITH(UPDLOCK) " +
133+
$"IF @ROWS_TO_UPDATE = 1 " +
134+
$"UPDATE {tableName} " +
142135
$"SET {updateOperations} " +
143136
$"OUTPUT {outputColumns} " +
144137
$"WHERE {updatePredicates};");
@@ -172,9 +165,6 @@ public string Build(SqlUpsertQueryStructure structure)
172165
upsertQuery.Append(insertQuery.ToString());
173166
}
174167

175-
// Commit the transaction.
176-
upsertQuery.Append("COMMIT TRANSACTION");
177-
178168
return upsertQuery.ToString();
179169
}
180170

0 commit comments

Comments
 (0)