Add 'PGRST' error code to differentiate from PostgreSQL errors

[laurenceisla]

Closes #1917

Todo:

• Add error codes for PostgREST errors
• Modify test specs with the new error codes

[wolfgangwalther]

Right now, the code would be:

• a specific number for PostgreSQL errors
• a custom code for hasql errors ("PGxxx"), but a different one for each error
• a generic code for PostgREST errors ("PGRST")

This combination is... not really useful. Or let's say it does not really achieve either of the following two goals:

• Have a way to tell apart PostgreSQL errors, from PostgREST errors, from hasql errors
• Have a way to check for specific PostgREST errors

One way to achieve both would be to add another field, e.g. called source: postgresql|hasql|postgrest - and then use code for a different number for each error. Or do the same with just code and add a prefix for hasql and postgrest to avoid collisions. No prefix = Postgresql. This PR goes kind of halfway - the PG prefix is very misleading and PGRST is not specific enough.

Maybe just rename the PGxxx (hasql) errors to something like HASQL_xxx and change all PGRST to something like PGRST_xxx.

[steve-chavez]

True, the PGRST is not helpful because it contains "PG", which could be interpreted as a PostgreSQL error.

Or do the same with just code and add a prefix for hasql and postgrest to avoid collisions. No prefix = Postgresql.

I think there are libraries relying on the code contents and modifying it would cause a breaking change that we can avoid. Misread that one, if the code for PostgreSQL is left untouched then there'd be no breaking change.

One way to achieve both would be to add another field, e.g. called source: postgresql|hasql|postgrest

I like this one better. I'd say just have a source: PostgREST/PostgreSQL, because for end users Hasql would be the same as PostgREST. Then we can put Hasql specific errors in the code part.

[steve-chavez]

One way to achieve both would be to add another field, e.g. called source: postgresql|hasql|postgrest

Hm, though having another field could break some libraries with strict typing for the error body. That was mostly the main idea for just having two additional codes: PGRST(all PostgREST errors) and PGCON(anything related to the connection), which seems simpler imho; and then just document this difference.

[steve-chavez]

Or do the same with just code and add a prefix for hasql and postgrest to avoid collisions. No prefix = Postgresql.

One problem with prefixing the code, with say PostgREST: HasqlError, is that it's not really a "code" - not a five-character error. Although maybe that doesn't matter for this case.

[laurenceisla]

One way to achieve both would be to add another field, e.g. called source: postgresql|hasql|postgrest

I like this one better. I'd say just have a source: PostgREST/PostgreSQL, because for end users Hasql would be the same as PostgREST.

@wolfgangwalther @steve-chavez I like this approach too. My initial suggestion was to add "PostgREST: ..." in the message field and not touch the code field at all. What Wolfgang suggested would be more akin to it without needing to modify the message.

and then use code for a different number for each error.

I think we should arrive to an agreement on how to name these error codes, if at all. Otherwise, just the new source field without specifying the error code should be enough for the purpose of this PR (which is to know the error source).

[steve-chavez]

One thing with adding the source field is that we lose some sense of transparency to PostgreSQL.

The docs for this PR made a lot of sense and it was great that we could map our error fields to pg error fields(MESSAGE, DETAIL, HINT, ERRCODE).

With the source field we lose this and the user would have to check our docs anyway to know the difference between source: PostgreSQL/PostgREST.

So I think we should just go back to code: "PGRST" for all of our errors and leave pg errors as is. The PGRST prefix is well known since we already use it for singular or plural.

@wolfgangwalther @laurenceisla WDYT?

[wolfgangwalther]

So I think we should just go back to code: "PGRST" for all of our errors and leave pg errors as is. The PGRST prefix is well known since we already use it for singular or plural.

Well, let's just make it a code prefix then and give all our errors a PGRST01, PGRST02, ... code? This way we can easily assemble a FAQ-style list of error codes with hints on how to solve them later on. We can probably even add a URI to the error message as hint or something, because those codes would allow static URIs to our docs?

[laurenceisla]

I found it convenient to add a prefix for each group of errors that are defined in the code. This is to keep an order in the file and to identify the groups more easily. If no prefix was present, then the sequence of error codes would "jump" from group to group; e. g. if the last Error has a PGRST30 code, and the last ApiRequestError has a PGRST10 then the next ApiRequestError added would be PGRST31.

The GerenalErrorPrefix corresponds to the Error data type and has a prefix of 0 (because it has the errors that are not "grouped" by default) and has ApiRequestError and PgError (Hasql errors) as constructors.

[steve-chavez]

I think that might be useful and it's similar to how pg classifies their errors: https://www.postgresql.org/docs/13/errcodes-appendix.html#ERRCODES-TABLE

So one group could be for JWT related errors, another for API errors, another for connection errors and so on.

[steve-chavez]

We can probably even add a URI to the error message as hint or something, because those codes would allow static URIs to our docs?

@wolfgangwalther This is a great idea that Laurence is currently working on.

Also, regarding a URL on the error body, I've just seen the Problem Details for HTTP APIs RFC(7807) that presents a type field for this. Maybe we can adopt that.

[steve-chavez]

The RFC mentions that:

Note that both "type" and "instance" accept relative URIs; this means
that they must be resolved relative to the document's base URI, as
per [RFC3986], Section 5.

So the above idea about specifying type wouldn't be complete/correct unless we allowed to configure the base URI for the errors. That can be left for another PR.

[wolfgangwalther]

So the above idea about specifying type wouldn't be complete/correct unless we allowed to configure the base URI for the errors. That can be left for another PR.

I don't think we need any base URI to configure here. The URIs will all point to PostgREST docs, no? That will be an absolute URI in all cases.

Note, that the RFC does not say that URIs "must" be relative. It merely says, relative URIs are accepted, too, and if used, they must resolve in the matter described. There are some examples in the RFC that use absolute URIs and it also has this:

New problem type definitions MUST document:

1. a type URI (typically, with the "http" or "https" scheme),

[steve-chavez]

The URIs will all point to PostgREST docs, no? That will be an absolute URI in all cases.

Hm, I don't think it would be right to do that. I assume that external services(example) or open data initiatives(example) that use us for their API would like to keep their users on their own site, so they should be able to override the error base URI.

Also, I'm not sure if we should add the type field, OpenAPI also has a way to describe errors https://swagger.io/docs/specification/describing-responses/#status-codes

[laurenceisla]

Codecov is giving a warning for these errors codes and their error instances, but I don't think they can be tested because they are caused by unexpected bugs. We might have to tank the score loss here.

[wolfgangwalther]

The URIs will all point to PostgREST docs, no? That will be an absolute URI in all cases.

Hm, I don't think it would be right to do that. I assume that external services(example) or open data initiatives(example) that use us for their API would like to keep their users on their own site, so they should be able to override the error base URI.

Maybe we can add a base URI for that later, but I don't think we need it right away. A base URI would require to have those external services have the same general URI structure, where just the base can be replaced.

A more flexible approach would be to rewrite those error responses via nginx sub_filter or body_filter_by_lua to their own URIs - and that's something that works better when we have a https://postgrest.org/... prefix there, that they can match.

Also, I'm not sure if we should add the type field, OpenAPI also has a way to describe errors https://swagger.io/docs/specification/describing-responses/#status-codes

If we indeed move the OpenAPI output to "contrib", we shouldn't assume we have OpenAPI at all in the main app. I think linking to our docs via the type field (or any other field, type just happens to be in some kind of standard, that we can fall back to) is very valuable.