Skip to content
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

Respond with HTTP 200 instead of 201 when no rows inserted #1070

Closed
steve-chavez opened this issue Feb 21, 2018 · 10 comments · Fixed by #2926
Closed

Respond with HTTP 200 instead of 201 when no rows inserted #1070

steve-chavez opened this issue Feb 21, 2018 · 10 comments · Fixed by #2926
Assignees
Labels
bug difficulty: medium Haskell task involving PostgreSQL IO http http compliance

Comments

@steve-chavez
Copy link
Member

Currently this is happening with POST when the payload is [], see: https://github.com/begriffs/postgrest/blob/108f3cd651a448fad065faf81a2090cee8a8d9fb/test/Feature/InsertSpec.hs#L129-L135

And when Prefer: resolution=ignore-duplicates is specified and all rows in the payload are duplicates: https://github.com/begriffs/postgrest/blob/108f3cd651a448fad065faf81a2090cee8a8d9fb/test/Feature/UpsertSpec.hs#L95-L101.

Also according to RFC, PUT should respond with 200 when the row is updated and with 201 when the row is inserted, currently it's always responding with 200.

These cases can be corrected by using the xmax system column, as described here.

@steve-chavez
Copy link
Member Author

The xmax trick wouldn't work with auto-updatable VIEWs since only TABLEs have them.

@wolfgangwalther
Copy link
Member

wolfgangwalther commented Dec 8, 2020

This could work for both tables and views, I think.

Although it probably does not play nicely with batch upserts, because once a column is updated, the GUC will be "updated".

It might be possible to adjust this, by using two GUCs as counters instead. pgrst.inserted and pgrst.updated:

  • if inserted==updated -> no rows inserted.
  • if inserted > updated -> rows inserted.
  • if updated == 0 -> no rows updated.

@taimoorzaeem
Copy link
Collaborator

@steve-chavez The design for this fix/feature seems rather vague. I am thinking of implementing it the following way:

  • Add inserted and updated fields in ResultSet here.

    data ResultSet
    = RSStandard
    { rsTableTotal :: Maybe Int64
    -- ^ count of all the table rows
    , rsQueryTotal :: Int64
    -- ^ count of the query rows
    , rsLocation :: [(BS.ByteString, BS.ByteString)]
    -- ^ The Location header(only used for inserts) is represented as a list of strings containing
    -- variable bindings like @"k1=eq.42"@, or the empty list if there is no location header.
    , rsBody :: BS.ByteString
    -- ^ the aggregated body of the query
    , rsGucHeaders :: Maybe BS.ByteString
    -- ^ the HTTP headers to be added to the response
    , rsGucStatus :: Maybe Text
    -- ^ the HTTP status to be added to the response
    }
    | RSPlan BS.ByteString -- ^ the plan of the query

  • Add the local GUCs pgrst.inserted and pgrst.updated in the query here and then read them with AS in prepareWrite statement.

    mutatePlanToQuery :: MutatePlan -> SQL.Snippet
    mutatePlanToQuery (Insert mainQi iCols body onConflct putConditions returnings _ applyDefaults) =
    "INSERT INTO " <> fromQi mainQi <> (if null iCols then " " else "(" <> cols <> ") ") <>
    fromJsonBodyF body iCols True False applyDefaults <>
    -- Only used for PUT
    (if null putConditions then mempty else "WHERE " <> intercalateSnippet " AND " (pgFmtLogicTree (QualifiedIdentifier mempty "pgrst_body") <$> putConditions)) <>
    maybe mempty (\(oncDo, oncCols) ->
    if null oncCols then
    mempty
    else
    " ON CONFLICT(" <> intercalateSnippet ", " (pgFmtIdent <$> oncCols) <> ") " <> case oncDo of
    IgnoreDuplicates ->
    "DO NOTHING"
    MergeDuplicates ->
    if null iCols
    then "DO NOTHING"
    else "DO UPDATE SET " <> intercalateSnippet ", " ((pgFmtIdent . cfName) <> const " = EXCLUDED." <> (pgFmtIdent . cfName) <$> iCols)
    ) onConflct <> " " <>
    returningF mainQi returnings
    where
    cols = intercalateSnippet ", " $ pgFmtIdent . cfName <$> iCols

  • Then, use Hasql.decoders to decode pgrst.inserted and pgrst.updated variables. Compare them and set rsGucStatus accordingly.

I am not very familiar with PostgreSQL IO yet, let me know other things that need to be changed for this fix.

@steve-chavez
Copy link
Member Author

@taimoorzaeem Looks good. Go ahead 👍

@taimoorzaeem
Copy link
Collaborator

@steve-chavez @wolfgangwalther How do I add and increment local GUCs in the middle of the query? For example, consider this:

mutatePlanToQuery :: MutatePlan -> SQL.Snippet
mutatePlanToQuery (Insert mainQi iCols body onConflct putConditions returnings _ applyDefaults) =
"INSERT INTO " <> fromQi mainQi <> (if null iCols then " " else "(" <> cols <> ") ") <>
fromJsonBodyF body iCols True False applyDefaults <>
-- Only used for PUT
(if null putConditions then mempty else "WHERE " <> intercalateSnippet " AND " (pgFmtLogicTree (QualifiedIdentifier mempty "pgrst_body") <$> putConditions)) <>
maybe mempty (\(oncDo, oncCols) ->
if null oncCols then
mempty
else
" ON CONFLICT(" <> intercalateSnippet ", " (pgFmtIdent <$> oncCols) <> ") " <> case oncDo of
IgnoreDuplicates ->
"DO NOTHING"
MergeDuplicates ->
if null iCols
then "DO NOTHING"
else "DO UPDATE SET " <> intercalateSnippet ", " ((pgFmtIdent . cfName) <> const " = EXCLUDED." <> (pgFmtIdent . cfName) <$> iCols)
) onConflct <> " " <>
returningF mainQi returnings
where
cols = intercalateSnippet ", " $ pgFmtIdent . cfName <$> iCols

To count the number of inserts and updates I would have to repeatedly read with something like current_setting('pgrst.inserted',true) and increment it with set_config('pgrst.inserted', (current_setting('pgrst.inserted',true)::int + 1)::text,true). The same goes for updates.

It seems too complicated to be added in this query. Besides I don't think we can use set_config(..) and current_setting(..) like that in an INSERT statement. How else can we handle this?

@laurenceisla
Copy link
Member

laurenceisla commented Aug 30, 2023

@taimoorzaeem Yes, I see that it gets a bit complex for batch upserts. To answer your question, the pgrst.inserted would be added to this line:

(if null putConditions then mempty else "WHERE " <> intercalateSnippet " AND " (pgFmtLogicTree (QualifiedIdentifier mempty "pgrst_body") <$> putConditions)) <>

This is how it would look like (maybe nullif is not necessary, but does some wonky things when testing):

where coalesce(set_config('pgrst.inserted', (coalesce(nullif(current_setting('pgrst.inserted', true), '')::int, 0) + 1)::text, true),'0') <> '0'

The pgrst.updated should be added to:

(if null putConditions then mempty else "WHERE " <> intercalateSnippet " AND " (pgFmtLogicTree (QualifiedIdentifier mempty "pgrst_body") <$> putConditions)) <>

And it would look like:

where coalesce(set_config('pgrst.updated', (coalesce(nullif(current_setting('pgrst.updated', true), '')::int, 0) + 1)::text, true),'0') <> '0'

After that you'd follow the logic mentioned by Wolfgang: #1070 (comment)

As an example, using the projects table from the Spec test, the the end result would follow this structure:

insert into api.projects (id, name)
-- just an example of inserted values values (1-5 already exist)
select id,name from (select 2, 'Windows 11' union all
                     select 3, 'Android' union all
                     select 6, 'NixOS' union all
                     select 5, 'Old OS' union all
                     select 7, 'New OS') pgrst_values(id,name)
where coalesce(set_config('pgrst.inserted', (coalesce(nullif(current_setting('pgrst.inserted', true), '')::int, 0) + 1)::text, true),'0') <> '0'
on conflict (id)
  do update set name = excluded.name
  where coalesce(set_config('pgrst.updated', (coalesce(nullif(current_setting('pgrst.updated', true), '')::int, 0) + 1)::text, true),'0') <> '0'
returning 1;

EDIT: Suggestion updated in my comment below #1070 (comment)

As an alternative to all of the above, we could use a single config pgrst.mutation (instead of inserted and updated), wrap the insert in a WITH statement and then get the count() of every update/insert we get. The structure would be like:

WITH pgrst_insert as (
  insert into api.projects (id, name)
  -- just an example of inserted values values (1-5 already exist)
  select id,name from (select 2, 'Windows 11' union all
                       select 3, 'Android' union all
                       select 6, 'NixOS' union all
                       select 5, 'Old OS' union all
                       select 7, 'New OS') pgrst_values(id,name)
  where 'inserted' = set_config('pgrst.mutation', 'inserted', true)
  on conflict (id)
    do update set name = excluded.name
    where 'updated' = set_config('pgrst.mutation', 'updated', true)
  returning current_setting('pgrst.mutation') as mutation
)
select count(nullif(mutation, 'updated')) as rows_inserted,
       count(nullif(mutation, 'inserted')) as rows_updated,
       count(1) as rows_total
from pgrst_insert;

I'm not sure if we need the exact quantity of updates/inserts, if not, it could be more performant I think (won't need count and possibly the CTE).

@steve-chavez
Copy link
Member Author

I'm not sure if we need the exact quantity of updates/inserts, if not, it could be more performant I think (won't need count and possibly the CTE).

Btw, to check if the queries are performant, there's a helper for pgbench tests https://github.com/PostgREST/postgrest/tree/main/test/pgbench

That isn't part of the test suite but it can be used to share results and see if a query is worth doing.

@laurenceisla
Copy link
Member

OK, I think we do not need to know how many rows are inserted/updated, only if there was at least one insert. If that's so, the query gets simpler: we'll only need to return a flag when it's inserted and finally count if that flag is greater than 0 (it appears at least once). Finally, the query would look like:

WITH pgrst_insert as (
  insert into api.projects (id, name)
  -- just an example of inserted values values (1-5 already exist)
  select id,name from (select 2, 'Windows 11' union all
                       select 3, 'Android' union all
                       select 6, 'NixOS' union all
                       select 5, 'Old OS' union all
                       select 7, 'New OS') pgrst_values(id,name)
  where '1' = set_config('pgrst.inserted', '1', true)
  on conflict (id)
    do update set name = excluded.name
    where '0' = set_config('pgrst.inserted', '0', true)
  returning current_setting('pgrst.inserted') as inserted
)
select count(nullif(inserted, '0')) > 0 as inserted
from pgrst_insert;

@taimoorzaeem
Copy link
Collaborator

@laurenceisla @steve-chavez
The problem I am facing is with returning

returning current_setting('pgrst.inserted') as inserted

Using returning, The value of of pgrst.inserted comes in the response body as {"inserted": "0", ...}. I am thinking of doing it with adding 1 to the pgrst.inserted on insert and subtracting 1 on update. If in the end result is > 0 then it is an insert otherwise an update. This way we are only updating the GUC and no extra columns are being added to the result.

@laurenceisla
Copy link
Member

laurenceisla commented Sep 6, 2023

@taimoorzaeem The returningF function could be modified to handle it so that it doesn't show in the body directly. But your new approach seems good too. I'd say go for it if it works best for you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug difficulty: medium Haskell task involving PostgreSQL IO http http compliance
4 participants