Fix MySQL PUT/PATCH regarding row-level update database policy#3734
Open
aaronburtle wants to merge 1 commit into
Open
Fix MySQL PUT/PATCH regarding row-level update database policy#3734aaronburtle wants to merge 1 commit into
aaronburtle wants to merge 1 commit into
Conversation
aaronburtle
requested review from
Alekhya-Polavarapu,
Aniruddh25,
JerryNixon,
RubenCerna2079,
anushakolan,
rusamant,
sourabh1007,
souvikghosh04,
stuartpa and
vadeveka
as code owners
July 21, 2026 22:06
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the MySQL REST upsert implementation (PUT/PATCH) to enforce the entity’s row-level update database policy, aligning MySQL behavior with other engines by restructuring the upsert into multiple statements/result sets and enabling previously skipped MySQL policy integration tests.
Changes:
- Restructured MySQL upsert SQL generation to: pre-count existing PK rows, attempt an
UPDATE ... WHERE <pk> AND <update-policy>, then conditionallyINSERTonly when the PK doesn’t exist. - Added MySQL executor logic to interpret multiple result sets and return the correct HTTP outcome (update vs insert vs policy failure vs not found).
- Enabled MySQL integration tests for update-policy scenarios and updated MySQL config generation + snapshot to include the new policy-testing role.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Core/Resolvers/MySqlQueryBuilder.cs | Reworked MySQL upsert SQL to enforce update policy and avoid ON DUPLICATE KEY UPDATE limitations. |
| src/Core/Resolvers/MySqlQueryExecutor.cs | Added multi-result-set interpretation for MySQL upsert outcomes and policy failures. |
| src/Service.Tests/SqlTests/RestApiTests/Put/MySqlPutApiTests.cs | Enabled PUT update-policy test and added MySQL validation query for it. |
| src/Service.Tests/SqlTests/RestApiTests/Patch/MySqlPatchApiTests.cs | Enabled PATCH update-policy tests and added MySQL validation query for it. |
| src/Service.Tests/Snapshots/ConfigurationTests.TestReadingRuntimeConfigForMySql.verified.txt | Updated snapshot to include database_policy_tester role with update DB policy. |
| config-generators/mysql-commands.txt | Added commands to grant database_policy_tester permissions and update DB policy for Stock. |
Comment on lines
+135
to
+150
| // Capture whether a record already exists for the given primary key BEFORE attempting the | ||
| // update/insert. This count is surfaced as the first result set and is used by the query | ||
| // executor to distinguish an update from an insert and to detect database policy failures. | ||
| string countExistingRows = | ||
| $"SET @cnt := (SELECT COUNT(*) FROM {tableName} WHERE {pkPredicates}); " + | ||
| $"SELECT @cnt AS {QuoteIdentifier(COUNT_ROWS_WITH_GIVEN_PK)};"; | ||
|
|
||
| // Update honoring the update database policy. When the policy is not satisfied, zero rows | ||
| // match and the subsequent select returns no rows. | ||
| string updateQuery = | ||
| $"UPDATE {tableName} " + | ||
| $"SET {Build(structure.UpdateOperations, ", ")} " + | ||
| ", " + updates + | ||
| $" WHERE {updatePredicates}; " + | ||
| $"SET @ROWCOUNT=ROW_COUNT(); " + | ||
| $"SELECT " + select + $" WHERE @ROWCOUNT > 0;"; |
Comment on lines
+159
to
+169
| // Insert honoring the create database policy, but only when no record already exists for | ||
| // the given primary key (@cnt = 0). Gating the insert on @cnt = 0 ensures the update | ||
| // policy enforced above cannot be bypassed by falling through to an insert on an | ||
| // existing record. | ||
| string createPredicates = JoinPredicateStrings(structure.GetDbPolicyForOperation(EntityActionOperation.Create)); | ||
| string insertQuery = | ||
| $"INSERT INTO {tableName} ({Build(structure.InsertColumns)}) " + | ||
| $"SELECT {string.Join(", ", structure.Values)} FROM DUAL " + | ||
| $"WHERE @cnt = 0 AND ({createPredicates}); " + | ||
| $"SET @ROWCOUNT=ROW_COUNT(); " + | ||
| $"SELECT {MakeUpsertSelections(structure)} WHERE @ROWCOUNT = 1;"; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why make this change?
On MySQL / Azure Database for MySQL, the REST
PUT(upsert) andPATCH(incremental upsert) paths did not enforce the row-level update database policy configured for an entity. We now respect that row level database policy.What is this change?
MySqlQueryBuilder.Build(SqlUpsertQueryStructure)previously filtered rows by primary key only and never retrieved the update policy (GetDbPolicyForOperation(EntityActionOperation.Update)), in both theIsFallbackToUpdateandON DUPLICATE KEY UPDATEbranches. Since MySQL'sON DUPLICATE KEY UPDATEsyntax cannot carry aWHEREclause, the upsert was restructured to mirror the MSSQL approach:MySqlQueryBuilder.csthe upsert now emits:UPDATE … SET … WHERE <pk> AND <update-policy>so a policy-violating update matches zero rows, andINSERT … SELECT … WHERE @cnt = 0gated on the pre-count, so an existing record can never fall through into an insert.MySqlQueryExecutor.csa newGetMultipleResultSetsIfAnyAsyncoverride interprets the result sets and returns the correct outcome:200(update),201(insert),403 DatabasePolicyFailure(record exists but the update policy blocked it), or404(auto-generated-PK record not found).Test coverage that was previously
[Ignore]d for MySQL is now enabled (this disabled coverage is why the gap went undetected):PutOneUpdateWithDatabasePolicy,PatchOneUpdateWithDatabasePolicy,PatchOneUpdateWithUnsatisfiedDatabasePolicy.database_policy_testerrole (update policy) to theStockentity inconfig-generators/mysql-commands.txt, and updated theTestReadingRuntimeConfigForMySqlsnapshot to match.How was this tested?
Validated end-to-end against a real MySQL 8.0.46 instance (
.NET 10, SDK10.0.301):PatchOneUpdateWithUnsatisfiedDatabasePolicy, which sends the attack request (modify a row the caller isn't authorized for) and asserts403 DatabasePolicyFailurewith the row left unchanged. On the vulnerable code this returned200and overwrote the row.MySqlPutApiTests+MySqlPatchApiTests: 68 passed / 0 failed / 12 skipped, confirming the upsert restructuring did not regress ordinary insert/update/idempotent behavior. (The 12 skips are create-policy and view tests unsupported on MySQL.)Sample Request(s)
Entity
commodities(tablestocks) with an update policy for roledatabase_policy_tester:@item.pieceid ne 1.Blocked (unauthorized) — attempt to update a row that violates the policy (
pieceid = 1):200 OKrow overwritten.403 Forbidden(DatabasePolicyFailure) row unchanged.Allowed (authorized) update a row that satisfies the policy (
pieceid = 99):200 OKrow updated.