jcoglan commented on PR #5858:
URL: https://github.com/apache/couchdb/pull/5858#issuecomment-4562936096
Been looking into how the errors should be passed from `mango_native_proc`
back to the client and how this affects the use of `{forbidden, _}` tuples.
Many places in the codebase throw or catch (or otherwise create/use) the
structure `{forbidden, Msg}`. `forbidden` is an atom and `Msg` is usually a
binary, though it can be a string, a list (e.g. `["by_node not an object"]`) or
a tuple (e.g. `chttpd_auth_request` throws `{forbidden, {Error, Reason}}`).
The specific code path for VDUs is that the actual VDU engine (a JS process
or `mango_native_proc`) throws a JS object like `{ forbidden: Msg }`, which is
`{[{<<"forbidden">>, Msg}]}` in Erlang.
`couch_query_servers:validate_doc_update()` catches `{[{<<"forbidden">>,
Msg}]}` and converts it into `{forbidden, Msg}` i.e. a normal Erlang pair with
`forbidden` as an atom. This is re-thrown and passed back through fabric and
other request-handling machinery until it ends up in the HTTP layer.
`chttpd` has two handlers relevant here:
- `error_info({forbidden, Msg})` produces a 403 response with `{ "error":
"forbidden", "reason": Msg }`
- `error_info({forbidden, Error, Msg})` produces a 403 response with `{
"error": Error, "reason": Msg }`
The problem we have is: how to send the list of failure objects from
`mango_native_proc` back to the client. The current design means it's safe for
`couch_query_servers` to throw a 3-tuple and `chttpd` will be able to handle
it, so there's no problem there vis-a-vis rolling upgrades. However, this only
lets us change the `error` field in the response, whereas we probably want to
keep `"error": "forbidden"` as the response for failed Mango VDU validations.
If we have `couch_query_servers` throw the 2-tuple `{forbidden,
{[{<<"failures">>, Failures}]}}` (`Failures` is the list of validation
failures) then this ends up putting the failures in the `reason` field of the
response, e.g.:
```json
{
"error": "forbidden",
"reason": {
"failures": [
{
"path": ["newDoc", "ok"],
"message": "must be present"
}
]
}
}
```
Technically, this is not a breaking change; it was already legal for a JS
VDU to throw `{ forbidden: { failures: [...] } }` and this would end producing
the response above. However there has been concern that making `reason` not be
a string would be surprising to most users and could break existing programs,
so should be considered a breaking change.
If we want to retain `reason` as a string and put the `failures` list
somewhere else then we need to invent some other way for `couch_query_servers`
to throw an error message and failure list, and put these into the HTTP
response. One such way is to make it throw `{forbidden, {failures, Failures}}`,
and then make `chttpd:send_error()` inspect `ReasonStr`. If it's `{failures,
Failures}` then we make it put this in the response:
```json
"reason": "document is not valid",
"failures": Failures
```
Otherwise we make it just emit `"reason": ReasonStr` as it currently does.
The problem with this is that it couples some generic HTTP error handling code
to the specifics of VDUs, which seems like a bad idea. We could weaken this
coupling by making `couch_query_servers` throw a more complex object containing
the `reason` and any additional fields, and make `send_error()` emit all that
data into the response, i.e. it lets `ReasonStr` be either a string, or a set
of JSON fields. This removes the coupling but still adds some complexity to how
errors are communicated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
