Skip to content

Commit 3c811b5

Browse files
aaronburtleCopilotAniruddh25
authored
Eliminate code duplication between Upsert and Insert operations (#3287)
## Why make this change? Closes #3209 ## What is this change? Allow Upserts coming from REST requests to fall through to the Insert logic when auto-generated primary keys are missing. This eliminates code duplication between these two ops in the `SqlMutationEngine` Add a test to cover the case when we have a PUT/PATCH with keys provided in the request body. ## How was this tested? New test was added to ensure complete coverage over the scenarios introduced with the keyless PUT/PATCH change. ## Sample Request(s) PUT that updates existing row (expects 200 OK): ``` PUT https://localhost:5001/api/magazine Content-Type: application/json { "id": 1, "title": "Updated Vogue", "issue_number": 9999 } ``` PUT that inserts new row (expects 201 Created): ``` PUT https://localhost:5001/api/magazine Content-Type: application/json { "id": 5001, "title": "Brand New Magazine", "issue_number": 42 } ``` PATCH that updates existing row (expects 200 OK): ``` PATCH https://localhost:5001/api/magazine Content-Type: application/json { "id": 1, "title": "Updated Vogue" } ``` PATCH that inserts new row (expects 201 Created): ``` PATCH https://localhost:5001/api/magazine Content-Type: application/json { "id": 5001, "title": "Brand New Magazine" } ``` --------- Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Co-authored-by: Aniruddh Munde <anmunde@microsoft.com>
1 parent 69b500d commit 3c811b5

10 files changed

Lines changed: 319 additions & 86 deletions

File tree

‎src/Core/Resolvers/SqlMutationEngine.cs‎

Lines changed: 69 additions & 77 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
// Copyright (c) Microsoft Corporation.
22
// Licensed under the MIT License.
33

4-
using System.Data;
54
using System.Data.Common;
65
using System.Net;
76
using System.Text.Json;
@@ -542,86 +541,59 @@ await queryExecutor.ExecuteQueryAsync(
542541

543542
try
544543
{
545-
if (context.OperationType is EntityActionOperation.Upsert || context.OperationType is EntityActionOperation.UpsertIncremental)
544+
// When the URL path has no primary key route but the request body contains
545+
// ALL PK columns, promote those values into PrimaryKeyValuePairs so the upsert
546+
// path can build a proper UPDATE ... WHERE pk = value (with INSERT fallback)
547+
// instead of blindly inserting and failing on a PK violation.
548+
// Every PK column must be present — including auto-generated ones — because
549+
// a partial composite key cannot uniquely identify a row for UPDATE.
550+
if ((context.OperationType is EntityActionOperation.Upsert || context.OperationType is EntityActionOperation.UpsertIncremental)
551+
&& context.PrimaryKeyValuePairs.Count == 0)
546552
{
547-
// When no primary key values are provided (empty PrimaryKeyValuePairs),
548-
// there is no row to look up for update. The upsert degenerates to a
549-
// pure INSERT - execute it via the insert path so the mutation engine
550-
// generates a correct INSERT statement instead of an UPDATE with an
551-
// empty WHERE clause (WHERE 1 = 1) that would match every row.
552-
if (context.PrimaryKeyValuePairs.Count == 0)
553-
{
554-
DbResultSetRow? insertResultRow = null;
553+
SourceDefinition sourceDefinition = sqlMetadataProvider.GetSourceDefinition(context.EntityName);
554+
bool allPKsInBody = true;
555+
List<string> pkExposedNames = new();
555556

556-
try
557+
foreach (string pk in sourceDefinition.PrimaryKey)
558+
{
559+
if (!sqlMetadataProvider.TryGetExposedColumnName(context.EntityName, pk, out string? exposedName))
557560
{
558-
using (TransactionScope transactionScope = ConstructTransactionScopeBasedOnDbType(sqlMetadataProvider))
559-
{
560-
insertResultRow =
561-
await PerformMutationOperation(
562-
entityName: context.EntityName,
563-
operationType: EntityActionOperation.Insert,
564-
parameters: parameters,
565-
sqlMetadataProvider: sqlMetadataProvider);
566-
567-
if (insertResultRow is null)
568-
{
569-
throw new DataApiBuilderException(
570-
message: "An unexpected error occurred while trying to execute the query.",
571-
statusCode: HttpStatusCode.InternalServerError,
572-
subStatusCode: DataApiBuilderException.SubStatusCodes.UnexpectedError);
573-
}
574-
575-
if (insertResultRow.Columns.Count == 0)
576-
{
577-
throw new DataApiBuilderException(
578-
message: "Could not insert row with given values.",
579-
statusCode: HttpStatusCode.Forbidden,
580-
subStatusCode: DataApiBuilderException.SubStatusCodes.DatabasePolicyFailure);
581-
}
582-
583-
if (isDatabasePolicyDefinedForReadAction)
584-
{
585-
FindRequestContext findRequestContext = ConstructFindRequestContext(context, insertResultRow, roleName, sqlMetadataProvider);
586-
IQueryEngine queryEngine = _queryEngineFactory.GetQueryEngine(sqlMetadataProvider.GetDatabaseType());
587-
selectOperationResponse = await queryEngine.ExecuteAsync(findRequestContext);
588-
}
589-
590-
transactionScope.Complete();
591-
}
561+
allPKsInBody = false;
562+
break;
592563
}
593-
catch (TransactionException)
564+
565+
if (!context.FieldValuePairsInBody.ContainsKey(exposedName))
594566
{
595-
throw _dabExceptionWithTransactionErrorMessage;
567+
allPKsInBody = false;
568+
break;
596569
}
597570

598-
if (isReadPermissionConfiguredForRole && !isDatabasePolicyDefinedForReadAction)
571+
pkExposedNames.Add(exposedName);
572+
}
573+
574+
if (allPKsInBody)
575+
{
576+
// Populate PrimaryKeyValuePairs from the body so the upsert path
577+
// generates an UPDATE with the correct WHERE clause.
578+
foreach (string exposedName in pkExposedNames)
599579
{
600-
IEnumerable<string> allowedExposedColumns = _authorizationResolver.GetAllowedExposedColumns(context.EntityName, roleName, EntityActionOperation.Read);
601-
foreach (string columnInResponse in insertResultRow.Columns.Keys)
580+
if (context.FieldValuePairsInBody.TryGetValue(exposedName, out object? value))
602581
{
603-
if (!allowedExposedColumns.Contains(columnInResponse))
604-
{
605-
insertResultRow.Columns.Remove(columnInResponse);
606-
}
582+
context.PrimaryKeyValuePairs[exposedName] = value!;
607583
}
608584
}
609-
610-
string pkRouteForLocationHeader = isReadPermissionConfiguredForRole
611-
? SqlResponseHelpers.ConstructPrimaryKeyRoute(context, insertResultRow.Columns, sqlMetadataProvider)
612-
: string.Empty;
613-
614-
return SqlResponseHelpers.ConstructCreatedResultResponse(
615-
insertResultRow.Columns,
616-
selectOperationResponse,
617-
pkRouteForLocationHeader,
618-
isReadPermissionConfiguredForRole,
619-
isDatabasePolicyDefinedForReadAction,
620-
context.OperationType,
621-
GetBaseRouteFromConfig(_runtimeConfigProvider.GetConfig()),
622-
GetHttpContext());
623585
}
586+
}
624587

588+
// When an upsert still has no primary key values after checking the body,
589+
// it degenerates to a pure INSERT. Fall through to the shared insert/update
590+
// handling so the mutation engine generates a correct INSERT statement instead
591+
// of an UPDATE with an empty WHERE clause (WHERE 1 = 1) that would match every row.
592+
bool isKeylessUpsert = (context.OperationType is EntityActionOperation.Upsert || context.OperationType is EntityActionOperation.UpsertIncremental)
593+
&& context.PrimaryKeyValuePairs.Count == 0;
594+
595+
if (!isKeylessUpsert && (context.OperationType is EntityActionOperation.Upsert || context.OperationType is EntityActionOperation.UpsertIncremental))
596+
{
625597
DbResultSet? upsertOperationResult;
626598
DbResultSetRow upsertOperationResultSetRow;
627599

@@ -723,7 +695,12 @@ await PerformMutationOperation(
723695
}
724696
else
725697
{
726-
// This code block gets executed when the operation type is one among Insert, Update or UpdateIncremental.
698+
// This code block handles Insert, Update, UpdateIncremental,
699+
// and keyless upsert (which degenerates to Insert).
700+
EntityActionOperation effectiveOperationType = isKeylessUpsert
701+
? EntityActionOperation.Insert
702+
: context.OperationType;
703+
727704
DbResultSetRow? mutationResultRow = null;
728705

729706
try
@@ -734,13 +711,13 @@ await PerformMutationOperation(
734711
mutationResultRow =
735712
await PerformMutationOperation(
736713
entityName: context.EntityName,
737-
operationType: context.OperationType,
714+
operationType: effectiveOperationType,
738715
parameters: parameters,
739716
sqlMetadataProvider: sqlMetadataProvider);
740717

741718
if (mutationResultRow is null || mutationResultRow.Columns.Count == 0)
742719
{
743-
if (context.OperationType is EntityActionOperation.Insert)
720+
if (effectiveOperationType is EntityActionOperation.Insert)
744721
{
745722
if (mutationResultRow is null)
746723
{
@@ -827,17 +804,32 @@ await PerformMutationOperation(
827804
string primaryKeyRouteForLocationHeader = isReadPermissionConfiguredForRole ? SqlResponseHelpers.ConstructPrimaryKeyRoute(context, mutationResultRow!.Columns, sqlMetadataProvider)
828805
: string.Empty;
829806

830-
if (context.OperationType is EntityActionOperation.Insert)
807+
if (effectiveOperationType is EntityActionOperation.Insert)
831808
{
832809
// Location Header is made up of the Base URL of the request and the primary key of the item created.
833-
// For POST requests, the primary key info would not be available in the URL and needs to be appended. So, the primary key of the newly created item
834-
// which is stored in the primaryKeyRoute is used to construct the Location Header.
835-
return SqlResponseHelpers.ConstructCreatedResultResponse(mutationResultRow!.Columns, selectOperationResponse, primaryKeyRouteForLocationHeader, isReadPermissionConfiguredForRole, isDatabasePolicyDefinedForReadAction, context.OperationType, GetBaseRouteFromConfig(_runtimeConfigProvider.GetConfig()), GetHttpContext());
810+
// For POST requests and keyless PUT/PATCH requests, the primary key info would not be available
811+
// in the URL and needs to be appended. So, the primary key of the newly created item which is
812+
// stored in the primaryKeyRoute is used to construct the Location Header.
813+
// effectiveOperationType (Insert) is passed so that ConstructCreatedResultResponse populates
814+
// the Location header for both true POST inserts and keyless upserts that result in an insert.
815+
return SqlResponseHelpers.ConstructCreatedResultResponse(
816+
mutationResultRow!.Columns,
817+
selectOperationResponse,
818+
primaryKeyRouteForLocationHeader,
819+
isReadPermissionConfiguredForRole,
820+
isDatabasePolicyDefinedForReadAction,
821+
effectiveOperationType,
822+
GetBaseRouteFromConfig(_runtimeConfigProvider.GetConfig()),
823+
GetHttpContext());
836824
}
837825

838-
if (context.OperationType is EntityActionOperation.Update || context.OperationType is EntityActionOperation.UpdateIncremental)
826+
if (effectiveOperationType is EntityActionOperation.Update || effectiveOperationType is EntityActionOperation.UpdateIncremental)
839827
{
840-
return SqlResponseHelpers.ConstructOkMutationResponse(mutationResultRow!.Columns, selectOperationResponse, isReadPermissionConfiguredForRole, isDatabasePolicyDefinedForReadAction);
828+
return SqlResponseHelpers.ConstructOkMutationResponse(
829+
mutationResultRow!.Columns,
830+
selectOperationResponse,
831+
isReadPermissionConfiguredForRole,
832+
isDatabasePolicyDefinedForReadAction);
841833
}
842834
}
843835

‎src/Core/Resolvers/SqlResponseHelpers.cs‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -369,16 +369,16 @@ HttpContext httpContext
369369
string locationHeaderURL = string.Empty;
370370
using JsonDocument emptyResponseJsonDocument = JsonDocument.Parse("[]");
371371

372-
// For PUT and PATCH API requests, the users are aware of the Pks as it is required to be passed in the request URL.
373-
// In case of tables with auto-gen PKs, PUT or PATCH will not result in an insert but error out. Seeing that Location Header does not provide users with
374-
// any additional information, it is set as an empty string always.
375-
// For POST API requests, the primary key route calculated will be an empty string in the following scenarions.
372+
// For PUT/PATCH requests where PKs are in the URL, the caller passes operationType as Upsert
373+
// and primaryKeyRoute as empty, so the Location header is not populated (the client already knows the URL).
374+
// For keyless PUT/PATCH requests that result in an insert, the caller passes operationType as Insert
375+
// with a non-empty primaryKeyRoute so the client can discover the newly created resource's location.
376+
// For POST requests, the primary key route will be empty in the following scenarios:
376377
// 1. When read action is not configured for the role.
377378
// 2. When the read action for the role does not have access to one or more PKs.
378-
// When the computed primaryKeyRoute is non-empty, the location header is calculated.
379-
// Location is made up of three parts, the first being constructed from the Host property found in the HttpContext.Request.
380-
// The second part being the base route configured in the config file.
381-
// The third part is the computed primary key route.
379+
// When the computed primaryKeyRoute is non-empty and operationType is Insert, the Location header is populated.
380+
// Location is made up of three parts: the scheme/host from the request, the base route from config,
381+
// and the computed primary key route.
382382
if (operationType is EntityActionOperation.Insert && !string.IsNullOrEmpty(primaryKeyRoute))
383383
{
384384
// Use scheme/host from X-Forwarded-* headers if present, else fallback to request values

‎src/Service.Tests/SqlTests/RestApiTests/Patch/MsSqlPatchApiTests.cs‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,19 @@ public class MsSqlPatchApiTests : PatchApiTestBase
2525
$"AND [publisher_id] = 1234 " +
2626
$"FOR JSON PATH, INCLUDE_NULL_VALUES, WITHOUT_ARRAY_WRAPPER"
2727
},
28+
{
29+
"PatchOne_Update_KeylessWithPKInBody_ExistingRow_Test",
30+
$"SELECT [id], [title], [issue_number] FROM [foo].{ _integration_NonAutoGenPK_TableName } " +
31+
$"WHERE [id] = 1 AND [title] = 'Updated Vogue' " +
32+
$"AND [issue_number] = 1234 " +
33+
$"FOR JSON PATH, INCLUDE_NULL_VALUES, WITHOUT_ARRAY_WRAPPER"
34+
},
35+
{
36+
"PatchOne_Insert_KeylessWithPKInBody_NewRow_Test",
37+
$"SELECT [id], [title], [issue_number] FROM [foo].{ _integration_NonAutoGenPK_TableName } " +
38+
$"WHERE [id] = { STARTING_ID_FOR_TEST_INSERTS } AND [title] = 'Brand New Magazine' " +
39+
$"FOR JSON PATH, INCLUDE_NULL_VALUES, WITHOUT_ARRAY_WRAPPER"
40+
},
2841
{
2942
"PatchOne_Insert_NonAutoGenPK_Test",
3043
$"SELECT [id], [title], [issue_number] FROM [foo].{ _integration_NonAutoGenPK_TableName } " +

‎src/Service.Tests/SqlTests/RestApiTests/Patch/MySqlPatchApiTests.cs‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,28 @@ public class MySqlPatchApiTests : PatchApiTestBase
2424
) AS subq
2525
"
2626
},
27+
{
28+
"PatchOne_Update_KeylessWithPKInBody_ExistingRow_Test",
29+
@"SELECT JSON_OBJECT('id', id, 'title', title, 'issue_number', issue_number) AS data
30+
FROM (
31+
SELECT id, title, issue_number
32+
FROM " + _integration_NonAutoGenPK_TableName + @"
33+
WHERE id = 1
34+
AND title = 'Updated Vogue' AND issue_number = 1234
35+
) AS subq
36+
"
37+
},
38+
{
39+
"PatchOne_Insert_KeylessWithPKInBody_NewRow_Test",
40+
@"SELECT JSON_OBJECT('id', id, 'title', title, 'issue_number', issue_number) AS data
41+
FROM (
42+
SELECT id, title, issue_number
43+
FROM " + _integration_NonAutoGenPK_TableName + @"
44+
WHERE id = " + STARTING_ID_FOR_TEST_INSERTS + @"
45+
AND title = 'Brand New Magazine'
46+
) AS subq
47+
"
48+
},
2749
{
2850
"PatchOne_Insert_NonAutoGenPK_Test",
2951
@"SELECT JSON_OBJECT('id', id, 'title', title, 'issue_number', issue_number ) AS data

0 commit comments

Comments
 (0)