Ordering embedded resources for PATCH request is broken

[wolfgangwalther]

When embedding resources on a PATCH request, ordering the embedded resources does not work. The following test-case fails on latest main:

diff --git a/test/spec/Feature/Query/UpdateSpec.hs b/test/spec/Feature/Query/UpdateSpec.hs
index 362a00e3..53fff125 100644
--- a/test/spec/Feature/Query/UpdateSpec.hs
+++ b/test/spec/Feature/Query/UpdateSpec.hs
@@ -435,6 +435,18 @@ spec actualPgVersion = do
           , matchHeaders = [matchContentTypeJson, "Preference-Applied" <:> "return=representation"]
           }
 
+      it "with ordering" $
+        request methodPatch "/web_content?id=eq.0&select=id,name,web_content(name)&web_content.order=name.asc"
+                [("Prefer", "return=representation")]
+          [json|{"name": "tardis-patched"}|]
+          `shouldRespondWith`
+          [json|
+            [ { "id": 0, "name": "tardis-patched", "web_content": [ { "name": "bar" }, { "name": "fezz" }, { "name": "foo" } ]} ]
+          |]
+          { matchStatus  = 200
+          , matchHeaders = [matchContentTypeJson, "Preference-Applied" <:> "return=representation"]
+          }
+
       it "with filters" $
         request methodPatch "/web_content?id=eq.0&select=id,name,web_content(name)&web_content.name=like.f*"
                 [("Prefer", "return=representation")]

This started failing between v9 and v10 with commit e90391b.

Similar issues regarding embedded filters were fixed in #2618 and #2752, but I am not sure whether they were introduced at the same time.

I think it would make sense to test all embedding features systematically for all kinds of requests - possibly more is broken than just this?

[steve-chavez]

@wolfgangwalther I've been finding that users find the limited updated/delete really confusing and additionally it causes bugs like this. Also it's not that useful, I believe it's more for a backend process (not frontend) for which you can just use a function or maybe a direct pg connection.

How about if we drop the feature? I'm intending to make a breaking change for #2825

[wolfgangwalther]

How about if we drop the feature?

I never really liked it in the first place. My comment here still holds:

From my perspective, we should:

• Disallow limit and offset on mutation requests entirely. Those are not useful and likely to lead to confusion.

So I'm ok with removing it.

[wolfgangwalther]

order= is not only broken for embedded resources, but also for the top-level resource...

Against the spec schema.

GET is good:

curl http://localhost:3000/no_pk?order=a.desc

[{"a":null,"b":null},
 {"a":"2","b":"0"},
 {"a":"1","b":"0"}]

PATCH is not:

curl -X PATCH \
  -d '{"b":"1"}' \
  -H "Content-Type: application/json" \
  -H "Prefer: return=representation" \
  http://localhost:3000/no_pk?order=a.desc

[{"a":null,"b":"1"},
 {"a":"1","b":"1"},
 {"a":"2","b":"1"}]

[steve-chavez]

With #3907 done, I think closing this should only be a matter of adding more tests.

[taimoorzaeem]

With #3907 done, I think closing this should only be a matter of adding more tests.

By closing do you mean fixing it? Doesn't seem like it's still fixed, ordering is still failing with the above mentioned test.

[steve-chavez]

Doesn't seem like it's still fixed, ordering is still failing with the above mentioned test.

My bad, I thought it was already fixed by #3907. So yes, it would need fixing.

[taimoorzaeem]

Hmm, taking this example, where should the ordering be ideally?

WITH pgrst_source AS
     (UPDATE "test"."web_content"
      SET "name" = "pgrst_body"."name"
      FROM
        (SELECT $1 AS json_data) pgrst_payload,
           LATERAL
        (SELECT "name"
         FROM json_to_record(pgrst_payload.json_data) AS _("name" text)) pgrst_body
      WHERE "test"."web_content"."id" = $2 RETURNING 1)
SELECT '' AS total_result_set,
       pg_catalog.count(_postgrest_t) AS page_total, array[]::text[] AS header,
                                                          ''::text AS BODY,
                                                          nullif(current_setting('response.headers', TRUE), '') AS response_headers,
                                                          nullif(current_setting('response.status', TRUE), '') AS response_status,
                                                          '' AS response_inserted
FROM
  (SELECT *
   FROM pgrst_source) _postgrest_t

I was thinking maybe in the pgrst_body? The test passes for it.

WITH pgrst_source AS
     (UPDATE "test"."web_content"
      SET "name" = "pgrst_body"."name"
      FROM
        (SELECT $1 AS json_data) pgrst_payload,
           LATERAL
        (SELECT "name"
         FROM json_to_record(pgrst_payload.json_data) AS _("name" text) /*ORDER BY "name" ASC*/) pgrst_body
      WHERE "test"."web_content"."id" = $2 RETURNING 1)                                  ^
SELECT '' AS total_result_set,                                                           |
       pg_catalog.count(_postgrest_t) AS page_total, array[]::text[] AS header,          |
                                                          ''::text AS BODY,
                                                          nullif(current_setting('response.headers', TRUE), '') AS response_headers,
                                                          nullif(current_setting('response.status', TRUE), '') AS response_status,
                                                          '' AS response_inserted
FROM
  (SELECT *
   FROM pgrst_source) _postgrest_t

[wolfgangwalther]

I was thinking maybe in the pgrst_body?

No, that would rely on PostgreSQL updating the rows the in the order you present it with. But that's not guaranteed.

The order must happen on the result of RETURNING. So probably in the SELECT * FROM pgrst_source part?

[taimoorzaeem]

So probably in the SELECT * FROM pgrst_source part?

Hm, this works too for this test case. I'll see how to do this elegantly, because for this we are gonna have to do this in prepareWrite instead of mutatePlanToQuery 😟.

[wolfgangwalther]

It could probably also be in mutatePlanToQuery, bust just wrapped around the whole thing?

[taimoorzaeem]

Sorry, this is the actual query:

-- mutateQuery
WITH pgrst_source AS
  (UPDATE "test"."web_content"
   SET "name" = "pgrst_body"."name"
   FROM
     (SELECT $1 AS json_data) pgrst_payload,
        LATERAL
     (SELECT "name"
      FROM json_to_record(pgrst_payload.json_data) AS _("name" text)) pgrst_body
   WHERE "test"."web_content"."id" = $2 RETURNING "test"."web_content"."id",
                                                  "test"."web_content"."name")
SELECT '' AS total_result_set,
       pg_catalog.count(_postgrest_t) AS page_total, array[]::text[] AS header,
                                                          coalesce(json_agg(_postgrest_t), '[]') AS BODY,
                                                          nullif(current_setting('response.headers', TRUE), '') AS response_headers,
                                                          nullif(current_setting('response.status', TRUE), '') AS response_status,
                                                          '' AS response_inserted
FROM
  -- selectQuery
  (SELECT "web_content"."id",
          "web_content"."name",
          COALESCE("web_content_web_content_1"."web_content_web_content_1", '[]') AS "web_content"
   FROM "pgrst_source" AS "web_content"
   LEFT JOIN LATERAL
     (SELECT json_agg("web_content_web_content_1")::JSONB AS "web_content_web_content_1"
      FROM
        (SELECT "web_content_1"."name"
         FROM "test"."web_content" AS "web_content_1"
         WHERE "web_content_1"."p_web_id" = "web_content"."id"
         ORDER BY "web_content_1"."name" ASC) AS "web_content_web_content_1") AS "web_content_web_content_1" ON TRUE
   ORDER BY "name" ASC) _postgrest_t

So, we need to remove ORDER BY from selectQuery and add it to mutateQuery. Hmm, would definitely need some refactoring.

[wolfgangwalther]

So, we need to remove ORDER BY from selectQuery and add it to mutateQuery.

Wait, why? If you have it in the select part already, which is after the RETURNING, then the position is fine? Why change it?

[taimoorzaeem]

Wait, why? If you have it in the select part already, which is after the RETURNING, then the position is fine? Why change it?

Oh, I see. We just need to add the ordering in mutateQuery as well.

[wolfgangwalther]

We just need to add the ordering in mutateQuery as well.

Why do we need it in the mutateQuery at all?

[taimoorzaeem]

Why do we need it in the mutateQuery at all?

Ahh, I see, my bad, I was forgetting that I had removed the ActRelationMut constraint from here. That was all that needed. Sheesh. Works perfectly now.

postgrest/src/PostgREST/Plan.hs

Lines 772 to 778 in 4819520

	addOrders :: ResolverContext -> ApiRequest -> ReadPlanTree -> Either ApiRequestError ReadPlanTree
	addOrders ctx ApiRequest{..} rReq =
	case iAction of
	ActDb (ActRelationMut _ _) -> Right rReq
	_ -> foldr addOrderToNode (Right rReq) qsOrder
	where
	QueryParams.QueryParams{..} = iQueryParams