add: Pgrst Patch for partial document update #4227

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Draft

taimoorzaeem wants to merge 1 commit into PostgREST:main

from taimoorzaeem:feat/json-patch

Draft

add: Pgrst Patch for partial document update #4227

taimoorzaeem wants to merge 1 commit into PostgREST:main from taimoorzaeem:feat/json-patch

Conversation

taimoorzaeem

Copy link

Collaborator

@taimoorzaeem taimoorzaeem commented Jul 28, 2025 •

edited

Loading

This should close #3166.

Implementation
Tests (coverage)
Documentation

@taimoorzaeem taimoorzaeem marked this pull request as draft

July 28, 2025 15:04

@taimoorzaeem taimoorzaeem force-pushed the feat/json-patch branch from 296e67d to fed1e3f Compare

July 28, 2025 15:10

@taimoorzaeem taimoorzaeem changed the title ~~(削除) feat: partial document update using JSON Patch (削除ここまで)~~ (追記) add: partial document update using JSON Patch (追記ここまで)

Jul 28, 2025

@taimoorzaeem taimoorzaeem force-pushed the feat/json-patch branch 2 times, most recently from 7b0250d to 47102a7 Compare

July 29, 2025 18:10

steve-chavez

steve-chavez reviewed

Jul 29, 2025

View reviewed changes

src/PostgREST/ApiRequest/Payload.hs Outdated

Comment on lines 157 to 160

"incr" -> Incr path <$> parseString o "value"

"replace" -> Replace path <$> parseString o "value"

"interval_pl" -> Interval path <$> parseString o "value"

_ -> fail $ "Unknown JSON Patch operation " ++ show op

Copy link

Member

@steve-chavez steve-chavez Jul 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👀 I would suggest just focusing on incr for this PR, easier implementation and review.

steve-chavez

steve-chavez reviewed

Jul 29, 2025

View reviewed changes

src/PostgREST/MediaType.hs Outdated

toMime MTOpenAPI = "application/openapi+json"

toMime (MTVndSingularJSON True) = "application/vnd.pgrst.object+json;nulls=stripped"

toMime (MTVndSingularJSON False) = "application/vnd.pgrst.object+json"

toMime MTVndJSONPatch = "application/vnd.pgrst.json-patch"

Copy link

Member

@steve-chavez steve-chavez Jul 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be clearer to name this as application/vnd.pgrst.patch+json.

Azure cosmosdb uses application/json_patch+json
https://learn.microsoft.com/en-us/azure/cosmos-db/partial-document-update#rest-api-reference-for-partial-document-update

steve-chavez

steve-chavez reviewed

Jul 29, 2025

View reviewed changes

src/PostgREST/Plan/MutatePlan.hs Outdated

, where_ :: [CoercibleLogicTree]

, returning :: [FieldName]

}

| JSONPatch -- JSON Patch for partial document update (not fully RFC compliant for now)

Copy link

Member

@steve-chavez steve-chavez Jul 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we'll ever be RFC compliant here, we could call this JSONPatchesque or just PgrstPatch.

@taimoorzaeem taimoorzaeem force-pushed the feat/json-patch branch from 47102a7 to 741908d Compare

July 30, 2025 18:03

@taimoorzaeem taimoorzaeem changed the title ~~(削除) add: partial document update using JSON Patch (削除ここまで)~~ (追記) add: partial document update using Pgrst Patch (追記ここまで)

Jul 30, 2025

@taimoorzaeem taimoorzaeem force-pushed the feat/json-patch branch from 741908d to 99d476a Compare

August 1, 2025 19:00

wolfgangwalther

wolfgangwalther reviewed

Aug 4, 2025

View reviewed changes

src/PostgREST/Query/QueryBuilder.hs Outdated

getValAndApplyOp :: PgrstPatchOp -> SQL.Snippet

getValAndApplyOp (Incr field val) =

pgFmtIdent field <> " + CAST(" <> intToSnip val <> " AS INTEGER)"

Copy link

Member

@wolfgangwalther wolfgangwalther Aug 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should not put the val directly into the query string like this. We expect all values to be passed as parameters by hasql.

src/PostgREST/Plan/MutatePlan.hs Outdated

Comment on lines 41 to 47

| PgrstPatch -- A modified version of JSON Patch

{ in_ :: QualifiedIdentifier

, patchBody :: Maybe [PgrstPatchOp]

, where_ :: [CoercibleLogicTree]

, returning :: [FieldName]

}

Copy link

Member

@wolfgangwalther wolfgangwalther Aug 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should not add a new MutatePlan here, but instead generalize the Update plan to contain "operations".

The default "update" that we currently support, should then be a special case "operation". This operation just uses the value from the json body.

Thus, I think these steps would make sense:

First refactor/generalize the Update method
Then implement the pgrst patch mimetype with a basic "set" operation (which is the same as the default update, just via the custom mimetype structure)
Last implement the new "incr" operation.

Copy link

Collaborator Author

@taimoorzaeem taimoorzaeem Aug 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting.

I went for a separate MutatePlan because I didn't see any negatives on this. Generalizing the current update method seems extra work and can likely cause regressions too.

TBH, I see that the Update and Pgrst Patch are quite similar and the extra code looks like it could be deduplicated. I am not sure either what's the best way to go here. But I would push for the current implementation because it's easier to review and maintain against the cost of some redundancy.

Copy link

Member

@steve-chavez steve-chavez Aug 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then implement the pgrst patch mimetype with a basic "set" operation (which is the same as the default update, just via the custom mimetype structure)

This makes a lot of sense. I agree.

Generalizing the current update method seems extra work and can likely cause regressions too.

@taimoorzaeem Have about doing set on a different PR? It would not cause regressions as it would essentially be an "alias" for the current PATCH operation.

Copy link

Collaborator Author

@taimoorzaeem taimoorzaeem Aug 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@taimoorzaeem Have about doing set on a different PR? It would not cause regressions as it would essentially be an "alias" for the current PATCH operation.

Yes, sure. I'll do in the current PR, we don't have much to lose and I too would like to focus on the set op first. We can do incr later in a separate PR.

src/PostgREST/ApiRequest/Payload.hs Outdated

{-# LANGUAGE LambdaCase #-}

{-# LANGUAGE NamedFieldPuns #-}

{-# OPTIONS_GHC -fno-warn-orphans #-}

Copy link

Member

@wolfgangwalther wolfgangwalther Aug 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need it and can we avoid it? We really should.

Copy link

Collaborator Author

@taimoorzaeem taimoorzaeem Aug 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's because of GHC doesn't allow defining instances if the type class or the type itself is also not in the same module. We use this in other places as well. However, yes we really should avoid it and a simple solution is to just move the type from Types module to Payload module.

CHANGELOG.md Outdated

+ The exposed schemas are now listed in the `hint` instead of the `message` field.

- Improve error details of `PGRST301` error by @taimoorzaeem in #4051

- Bounded JWT cache using the SIEVE algorithm by @mkleczek in #4084

- Add partial document update using PGRST Patch by @taimoorzaeem in #3166

Copy link

Member

@wolfgangwalther wolfgangwalther Aug 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's entirely unclear what "PGRST Patch" is in this note, when not familiar with the PR.

@taimoorzaeem taimoorzaeem force-pushed the feat/json-patch branch from 99d476a to 5a82150 Compare

August 8, 2025 09:04

@taimoorzaeem

Copy link

Collaborator Author

taimoorzaeem commented Aug 13, 2025

If we want to do this without creating a separate PgrstPatch mutation, then we would have to break abstraction with Update mutation. Would it be cool to have MutatePlan like:

 | Update
 { in_ :: QualifiedIdentifier
 , updCols :: [CoercibleField]
 , updBody :: Maybe LBS.ByteString
 , where_ :: [CoercibleLogicTree]
 , returning :: [FieldName]
 , applyDefs :: Bool
 , isPgrstPatch :: Bool -- a boolean for pgrst patch
 }

My plan is to convert this:

[
 { "op": "set", "path": "id", "value": "10" },
 { "op": "set", "path": "name", "value": "double d" }
]

to this when isPgrstPatch is True:

[
 {"id": "10", "name": "double d"}
]

After this, our normal update will take over to build the query. Let me know what you think.

@steve-chavez

Copy link

Member

steve-chavez commented Aug 13, 2025

@taimoorzaeem As per above discussion, we agreed that what we have now is a set operation under JSON Patch or PgrstPatch.

So instead maybe add a data UpdateOperation = SetOperation, just a single constructor for now and then a new field on Update that uses this new type.

taimoorzaeem

taimoorzaeem commented

Aug 14, 2025

View reviewed changes

src/PostgREST/ApiRequest/Payload.hs Outdated

Comment on lines 63 to 65

data PgrstPatchOp

= Set FieldName Text

-- We can add more operations in the future

Copy link

Collaborator Author

@taimoorzaeem taimoorzaeem Aug 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So instead maybe add a data UpdateOperation = SetOperation, just a single constructor for now and then a new field on Update that uses this new type.

@steve-chavez We already have this in this implementation. This would make it easy to add new operators in the future (otherwise, having only a set operation would not be any more useful than the current PATCH method).

Another reason why I think that PgrstPatch and Update should be separate is that PgrstPatch needs to be parsed before building the plan to know which operator we are using (set in this case). We don't need that in Update.

Copy link

Member

@steve-chavez steve-chavez Aug 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, fair enough. Please go ahead.

@taimoorzaeem taimoorzaeem force-pushed the feat/json-patch branch from 5a82150 to ffbb104 Compare

August 15, 2025 14:58

@taimoorzaeem taimoorzaeem changed the title ~~(削除) add: partial document update using Pgrst Patch (削除ここまで)~~ (追記) add: implement Pgrst Patch for partial document update (追記ここまで)

Aug 15, 2025

@taimoorzaeem taimoorzaeem force-pushed the feat/json-patch branch from ffbb104 to 6ea3e36 Compare

August 16, 2025 07:07

@taimoorzaeem

Copy link

Collaborator Author

taimoorzaeem commented Aug 16, 2025

@steve-chavez @wolfgangwalther Could this get a review before we add the documentation?

wolfgangwalther

wolfgangwalther reviewed

Aug 17, 2025

View reviewed changes

src/PostgREST/ApiRequest/Types.hs

Comment on lines 80 to 95

data Payload

= ProcessedJSON -- ^ Cached attributes of a JSON payload

{ payRaw :: LBS.ByteString

-- ^ This is the raw ByteString that comes from the request body. We

-- cache this instead of an Aeson Value because it was detected that for

-- large payloads the encoding had high memory usage, see

-- https://github.com/PostgREST/postgrest/pull/1005 for more details

, payKeys :: S.Set Text

-- ^ Keys of the object or if it's an array these keys are guaranteed to

-- be the same across all its objects

}

| ProcessedUrlEncoded { payArray :: [(Text, Text)], payKeys :: S.Set Text }

| RawJSON { payRaw :: LBS.ByteString }

| RawPay { payRaw :: LBS.ByteString }

Copy link

Member

@wolfgangwalther wolfgangwalther Aug 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please split refactors in separate commits, here moving the Payload type. Otherwise it's hard to review whether anything change in here.

src/PostgREST/ApiRequest/Payload.hs Outdated

| ProcessedUrlEncoded { payArray :: [(Text, Text)], payKeys :: S.Set Text }

| RawJSON { payRaw :: LBS.ByteString }

| RawPay { payRaw :: LBS.ByteString }

| PgrstPatchPay { payPgrstPatch :: [PgrstPatchOp] } -- ^ PgrstPatchPay is a list of patch updates

Copy link

Member

@wolfgangwalther wolfgangwalther Aug 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

None of the other types have an indication of "payload" in their name.

Why not just:

Suggested change

| PgrstPatchPay { payPgrstPatch :: [PgrstPatchOp] } -- ^ PgrstPatchPay is a list of patch updates

| PgrstPatch { payOps :: [PgrstPatchOp] } -- ^ PgrstPatch is a list of patch updates

Also I think the field name should be describing its content.

src/PostgREST/ApiRequest/Payload.hs Outdated

-- TODO: We need to decide what JSON "value"s are allowed in our

-- our Pgrst Patch implementation.

case op of

"set" -> Set path <$> parseString o "value"

Copy link

Member

@wolfgangwalther wolfgangwalther Aug 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we only allow string values? I think the values should be arbitrary, i.e. also any other json type, right? They should behave the same way as a regular update/patch, e.g. for composite types etc.

src/PostgREST/Query/QueryBuilder.hs Outdated

Comment on lines 175 to 176

-- PGRST PATCH: HTTP PATCH method with "vnd.pgrst.patch+json" Content-Type

mutatePlanToQuery (PgrstPatch mainQi patchCols body logicForest returnings)

Copy link

Member

@wolfgangwalther wolfgangwalther Aug 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still don't like the idea of creating different queries for pgrst patch and regular update.

I think they should be treated roughly the same:

They should both pass the full body in.
The only difference is in how the values for the body are accessed for the respective operation.

For a PgrstPatch payload, you can still do things like SET field = pgrst_body->0->'value' or so. This should remove a lot of duplicate code and allow to implement operations purely as the part after SET field = and "doing something with pgrst_body".

Copy link

Collaborator Author

@taimoorzaeem taimoorzaeem Aug 17, 2025 •

edited

Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At this point, I agree. There are a lot of duplications which I noticed when writing the tests. Let me clean this up.

test/spec/Feature/Query/PgrstPatchSpec.hs

Copy link

Member

@wolfgangwalther wolfgangwalther Aug 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this feature also work with domain representations? It needs to and should be confirmed by a test.

@taimoorzaeem taimoorzaeem force-pushed the feat/json-patch branch from 6ea3e36 to 4f1cea6 Compare

August 18, 2025 18:12

@taimoorzaeem taimoorzaeem changed the title ~~(削除) add: implement Pgrst Patch for partial document update (削除ここまで)~~ (追記) add: Pgrst Patch for partial document update (追記ここまで)

Aug 18, 2025

@taimoorzaeem taimoorzaeem mentioned this pull request

Aug 19, 2025

refactor(query): make fromJsonBodyF function more readable #4272

Merged

@taimoorzaeem taimoorzaeem force-pushed the feat/json-patch branch from 4f1cea6 to a708dac Compare

August 20, 2025 07:10

@taimoorzaeem taimoorzaeem force-pushed the feat/json-patch branch from a708dac to f42296e Compare

October 3, 2025 03:39

taimoorzaeem

taimoorzaeem commented

Oct 3, 2025

View reviewed changes

test/spec/Feature/Query/PlanSpec.hs

resHeaders `shouldSatisfy` elem ("Content-Type", "application/vnd.pgrst.plan+json; for=\"application/json\"; charset=utf-8")

resHeaders `shouldSatisfy` notZeroContentLength

resStatus `shouldBe` Status { statusCode = 200, statusMessage="OK" }

totalCost `shouldBe` 14.21

Copy link

Collaborator Author

@taimoorzaeem taimoorzaeem Oct 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, 14.21 compared to a normal update 8.23. 🤔

@taimoorzaeem


 add: Pgrst Patch for partial document update

5579bdd

Signed-off-by: Taimoor Zaeem <taimoorzaeem@gmail.com>

@taimoorzaeem taimoorzaeem force-pushed the feat/json-patch branch from f42296e to 5579bdd Compare

October 3, 2025 04:14

Labels

None yet

3 participants

@taimoorzaeem @steve-chavez @wolfgangwalther

Uh oh!

add: Pgrst Patch for partial document update #4227

Are you sure you want to change the base?

add: Pgrst Patch for partial document update #4227

Uh oh!

Conversation

@taimoorzaeem taimoorzaeem commented Jul 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

@steve-chavez steve-chavez Jul 29, 2025

Choose a reason for hiding this comment

Uh oh!

@steve-chavez steve-chavez Jul 29, 2025

Choose a reason for hiding this comment

Uh oh!

@steve-chavez steve-chavez Jul 29, 2025

Choose a reason for hiding this comment

Uh oh!

@wolfgangwalther wolfgangwalther Aug 4, 2025

Choose a reason for hiding this comment

Uh oh!

@wolfgangwalther wolfgangwalther Aug 4, 2025

Choose a reason for hiding this comment

Uh oh!

@taimoorzaeem taimoorzaeem Aug 4, 2025

Choose a reason for hiding this comment

Uh oh!

@steve-chavez steve-chavez Aug 4, 2025

Choose a reason for hiding this comment

Uh oh!

@taimoorzaeem taimoorzaeem Aug 5, 2025

Choose a reason for hiding this comment

Uh oh!

@wolfgangwalther wolfgangwalther Aug 4, 2025

Choose a reason for hiding this comment

Uh oh!

@taimoorzaeem taimoorzaeem Aug 4, 2025

Choose a reason for hiding this comment

Uh oh!

@wolfgangwalther wolfgangwalther Aug 4, 2025

Choose a reason for hiding this comment

Uh oh!

taimoorzaeem commented Aug 13, 2025

Uh oh!

steve-chavez commented Aug 13, 2025

Uh oh!

@taimoorzaeem taimoorzaeem Aug 14, 2025

Choose a reason for hiding this comment

Uh oh!

@steve-chavez steve-chavez Aug 14, 2025

Choose a reason for hiding this comment

Uh oh!

taimoorzaeem commented Aug 16, 2025

Uh oh!

@wolfgangwalther wolfgangwalther Aug 17, 2025

Choose a reason for hiding this comment

Uh oh!

@wolfgangwalther wolfgangwalther Aug 17, 2025

Choose a reason for hiding this comment

Uh oh!

@wolfgangwalther wolfgangwalther Aug 17, 2025

Choose a reason for hiding this comment

Uh oh!

@wolfgangwalther wolfgangwalther Aug 17, 2025

Choose a reason for hiding this comment

Uh oh!

@taimoorzaeem taimoorzaeem Aug 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

@wolfgangwalther wolfgangwalther Aug 17, 2025

Choose a reason for hiding this comment

Uh oh!

@taimoorzaeem taimoorzaeem Oct 3, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

@taimoorzaeem taimoorzaeem commented Jul 28, 2025 •

edited

Loading

@taimoorzaeem taimoorzaeem Aug 17, 2025 •

edited

Loading