Unexpected error behaviour, because of failing transactions #375
Comments
|
Very interesting. I’ve thought about this before, but I wasn’t sure how often it would happen 😊 This may be an easy fix. We could modify |
|
If it isn’t too expensive to place many savepoints this is something we could merge. For more context on the Postgres feature see: I feel like I have seen an article somewhere which urges libraries to insert savepoints before each statement to handle errors, but I can’t remember where… |
|
I'm not aware of any significant performance issues with savepoints; I think they allocate a small amount of memory, but you'd probably be creating and releasing them rapidly enough that it won't be an issue. Also with #342 the number of savepoints required would be significantly reduced. |
|
Or you could just ignore "expected issues"? |
|
We’d only use savepoints in queries in not in mutations. If a mutation fails the entire document should fail as well. We could enhance the client perhaps in https://github.com/calebmer/postgraphql/blob/master/src/postgraphql/withPostGraphQLContext.ts where we would also accept an Alternatively we could enhance in a schema level resolve function which is implemented similarly to |
|
To avoid having currentPerson throw an error, I added a default value:
GraphQL returns something like this:
|
|
Another solution which doesn't require setting defaults on the database (which can be a challenge*) is to use the PG 9.6 feature of adding a second parameter create function current_person_id() returns integer as $$
select current_setting('jwt.claims.person_id', true)::integer;
$$ language sql stable;
create function current_person() returns person as $$
select * from person where id = current_person_id();
$$ language sql stable;* setting database defaults can be a challenge where you cannot control the database name (e.g. Heroku auto-provisioned database names); in these cases you can work around it with a do $$
begin
execute 'alter database ' || current_database() || ' set jwt.claims.person_id to 0';
end;
$$ language plpgsql; |
|
👍 for missing_okay. For other exceptions I'd like to run many queries and get the results where I can see them / the queries work, and to get |
|
I think GraphQL sees the fields in the root selection set of a GraphQL mutation document as independent, so I’ve wrapped each of them in savepoints. If you believe this is incorrect behaviour speak now! graphile/graphile-engine#59 |
|
@benjie this is closed now yes? |
|
Well, if summary fails now then the entire root-level field will fail... I'm not sure if that's an acceptable solution. It kind of goes against GraphQL's "nullable by default, failures happen" mentality, so I'm not super happy with it, but equally I'm not sure how to balance error capture with performance. |
|
Since I've had no pushback on this, I'm going to state that the current behaviour is the desired behaviour for v4 and that your stable functions should not throw. If they do then failures are expected. We can revisit this in v5 if it causes issues for people (or even in a point release, since going the other way wouldn't really be a breaking change). |
Hi,
when writing a query composition and one of the queries has an error, the error of one part will break the transaction for another.
I used the forum example from this repository to test this.
In the following query, I will receive all posts (as expected), but will also force an error by using
currentPersonwithout any JWT Token:This query will result in a partial result of posts, notice the absence of the (computed)
summaryproperty. The result will also feature a huge error log, for each summary:I just wonder, why results are returned when the whole query should have failed and why the order is so (in my opinion) unexpected.
Just a sidenote: I can also slightly alter the query to completely crash, by getting the
pageInfo.hasNextPage.The text was updated successfully, but these errors were encountered: