Database policy support for PUT/PATCH operations - PostgreSQL - #3694
Conversation
…ate error/success message is returned
…te/update policies
There was a problem hiding this comment.
Pull request overview
This PR extends PostgreSQL PUT/PATCH (upsert) behavior to correctly apply database policies for both the update and insert branches, aligning Postgres behavior with the expected policy semantics for REST upserts.
Changes:
- Updated PostgreSQL upsert SQL generation to evaluate update-policy vs create-policy depending on which branch executes, while preserving “try update then insert” semantics.
- Added a PostgreSQL-specific
GetMultipleResultSetsIfAnyAsyncimplementation to interpret multi-result-set upsert outcomes and return 403/404 appropriately. - Updated PostgreSQL REST integration tests and test config to exercise create-policy behavior for PUT/PATCH insert cases.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Service.Tests/SqlTests/RestApiTests/Put/PostgreSqlPutApiTests.cs | Adds expected SQL for PUT insert-with-policy and removes ignored overrides so tests run. |
| src/Service.Tests/SqlTests/RestApiTests/Patch/PostgreSqlPatchApiTests.cs | Adds expected SQL for PATCH insert-with-policy and removes ignored overrides so tests run. |
| src/Service.Tests/dab-config.PostgreSql.json | Adds a PostgreSQL create-action database policy used by the new PUT/PATCH scenarios. |
| src/Core/Resolvers/SqlMutationEngine.cs | Passes additional context into the result-set handler for upsert result interpretation. |
| src/Core/Resolvers/PostgresQueryBuilder.cs | Reworks PostgreSQL upsert SQL to support separate create/update policies and emit a PK-existence count result set. |
| src/Core/Resolvers/PostgreSqlExecutor.cs | Implements PostgreSQL-specific multi-result-set handling to map policy failures vs not-found. |
| src/Core/Configurations/RuntimeConfigValidator.cs | Allows PostgreSQL to define create-action database policies in config validation. |
| config-generators/postgresql-commands.txt | Updates generator commands to set separate create/read permissions and the new create policy. |
…er a PostgreSQL upsert operation performed an update or not. Reliance on this flag could potentially lead to incorrect HTTP response code being sent in certain race scenarios.
… whether an upsert operation performed an update or not because it is required by other database providers and does not apply for PostgreSQL
|
So it looks like all the automated tests in Azure pipelines are passing now but at one point one of the Cosmos tests was failing. Specifically, one of the tests in the |
|
/azp run |
|
Azure Pipelines: Successfully started running 6 pipeline(s). |
RubenCerna2079
left a comment
There was a problem hiding this comment.
I don't think the use of the isFallbackToUpdate is correct. Please remove it if possible from the PostgreSqlExecutor.cs file.
…via the query result instead
…or insert, update, and update incremental operations
Souvik Ghosh (souvikghosh04)
left a comment
There was a problem hiding this comment.
LGTM
|
/azp run |
|
Azure Pipelines: Successfully started running 6 pipeline(s). |
## Why make this change? As per the behaviour expected from PUT/PATCH operations with database policies discussed in #1430, implement db policy support for PostgreSQL to fix #1372. ## What is this change? 1. Prior to this change, there was only one database policy for each operation. Since now database policies will be supported for both insert (or create)/update actions via PUT/PATCH operations, these 2 operations can have 2 database policies defined for them, one for each action. 2. The query generated by `PostgresQueryBuilder.Build(SqlUpsertQueryStructure structure)` is modified to accommodate create/update policies while also keeping intact the normal upsert behavior expected (try update, then insert). 3. The method `IQueryExecutor.GetMultipleResultSetsIfAnyAsync` has been provided another implementation specific to PostgreSql in `PostgreSqlExecutor`. The `DbDataReader` instance for the query being executed for the PUT/PATCH operation will always contain two result sets. 4. Different scenarios are added to the method `PostgreSqlExecutor.GetMultipleResultSetsIfAnyAsync` to throw appropriate exceptions (Forbidden/Authorization failure - 403 and NotFound - 404). Appropriate comments are added within the code to demonstrate each case. ## How was this tested? Integration Tests - Done --------- Co-authored-by: RubenCerna2079 <32799214+RubenCerna2079@users.noreply.github.com> (cherry picked from commit 57a195c)
…ease/2.0 (#3767) Ports the PostgreSQL GraphQL groupby/aggregation and related feature commits from `main` (released in v2.1.0-rc) to `release/2.0` via cherry-pick. ## Commits ported (chronological) | PR | Title | |----|-------| | #3450 | Fix GraphQL aggregation features disabled when runtime.graphql config section is absent | | #3694 | Database policy support for PUT/PATCH operations - PostgreSQL | | #3728 | Add support for DateTime filters in PostgreSQL | | #3750 | Fix column mapping in GroupBy and aggregation queries | | #3741 | Add groupby/aggregation support for PostgreSQL in GraphQL | | #3753 | Enhance test coverage for GraphQL queries by adding orderBy clause | ## Notes - All six cherry-picks applied cleanly. - One manual adjustment in `SqlMutationEngine.cs` (part of #3694 port): the original referenced `effectiveOperationType` (a local introduced by the unrelated refactor #3287, which is not in `release/2.0`). Substituted `context.OperationType`, which is functionally equivalent in that non-upsert branch and matches the `release/2.0` convention. Folded into the #3694 commit. - Solution builds clean (0 warnings, 0 errors). - Integration tests (PostgreSql/MsSql) require live databases and were not run locally. --------- Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com> Co-authored-by: Aniruddh25 <3513779+Aniruddh25@users.noreply.github.com> Co-authored-by: Aniruddh Munde <anmunde@microsoft.com> Co-authored-by: Souvik Ghosh <souvikofficial04@gmail.com> Co-authored-by: Arjun Narendra <arjunnarendra1@gmail.com> Co-authored-by: RubenCerna2079 <32799214+RubenCerna2079@users.noreply.github.com> Co-authored-by: Arpit Gupta <106474712+ar-guptaar@users.noreply.github.com> Co-authored-by: ARPIT GUPTA <guptaar@microsoft.com> Co-authored-by: Anusha Kolan <anushakolan10@gmail.com>
Why make this change?
As per the behaviour expected from PUT/PATCH operations with database policies discussed in #1430, implement db policy support for PostgreSQL to fix #1372.
What is this change?
Prior to this change, there was only one database policy for each operation. Since now database policies will be supported for both insert (or create)/update actions via PUT/PATCH operations, these 2 operations can have 2 database policies defined for them, one for each action.
The query generated by
PostgresQueryBuilder.Build(SqlUpsertQueryStructure structure)is modified to accommodate create/update policies while also keeping intact the normal upsert behavior expected (try update, then insert).The method
IQueryExecutor.GetMultipleResultSetsIfAnyAsynchas been provided another implementation specific to PostgreSql inPostgreSqlExecutor. TheDbDataReaderinstance for the query being executed for the PUT/PATCH operation will always contain two result sets.Different scenarios are added to the method
PostgreSqlExecutor.GetMultipleResultSetsIfAnyAsyncto throw appropriate exceptions (Forbidden/Authorization failure - 403 and NotFound - 404). Appropriate comments are added within the code to demonstrate each case.How was this tested?
Integration Tests - Done