ricellis commented on code in PR #5792: URL: https://github.com/apache/couchdb/pull/5792#discussion_r2588365256
########## src/docs/rfcs/018-declarative-vdu.md: ########## @@ -0,0 +1,2249 @@ +--- +name: Formal RFC +about: Submit a formal Request For Comments for consideration by the team. +title: '' +labels: rfc, discussion +assignees: '' + +--- + +[NOTE]: # ( ^^ Provide a general summary of the RFC in the title above. ^^ ) + +# Introduction + +## Abstract + +[NOTE]: # ( Provide a 1-to-3 paragraph overview of the requested change. ) +[NOTE]: # ( Describe what problem you are solving, and the general approach. ) + +## Requirements Language + +[NOTE]: # ( Do not alter the section below. Follow its instructions. ) + +The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT", +"SHOULD", "SHOULD NOT", "RECOMMENDED", "MAY", and "OPTIONAL" in this +document are to be interpreted as described in +[RFC 2119](https://www.rfc-editor.org/rfc/rfc2119.txt). + +## Terminology + +[TIP]: # ( Provide a list of any unique terms or acronyms, and their definitions here.) + +--- + +# Detailed Description + +This document specifies a system of declarative document validation and +authorization for CouchDB. It lets users write rules for validating document +updates using expressions that can be evaluated inside the main CouchDB process, +instead of having to invoke JavaScript code and incurring the overhead of +round-tripping documents to the external JavaScript engine. + + +## Design documents + +Users encode validation rules by storing a design document with the following +fields: + +- `language`: must have the value `"query"` +- `validate_doc_update`: contains an _Extended Mango_ expression that encodes Review Comment: This type of polymorphism pattern that occurs in CouchDB APIs increases the complexity of modelling Couch types in strongly typed languages (i.e. Go, Java) and with technologies like OpenAPI. | ddoc language | `validate_doc_update` type | | --- | --- | | `javascript` | `string` | | `query` | `object` | One could argue for, say, the use of `oneOf` and a `discriminator` based on `language` in OpenAPI, but such a change is a breaking one for anyone with an already existing model type representing a design document. I am, sadly, very aware that this `string`/`object` problem already exists for the `map` property of a design document, see for example https://github.com/IBM/cloudant-go-sdk/issues/507, and I'd be reluctant to see it propagate. ########## src/docs/rfcs/018-declarative-vdu.md: ########## @@ -0,0 +1,2249 @@ +--- +name: Formal RFC +about: Submit a formal Request For Comments for consideration by the team. +title: '' +labels: rfc, discussion +assignees: '' + +--- + +[NOTE]: # ( ^^ Provide a general summary of the RFC in the title above. ^^ ) + +# Introduction + +## Abstract + +[NOTE]: # ( Provide a 1-to-3 paragraph overview of the requested change. ) +[NOTE]: # ( Describe what problem you are solving, and the general approach. ) + +## Requirements Language + +[NOTE]: # ( Do not alter the section below. Follow its instructions. ) + +The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT", +"SHOULD", "SHOULD NOT", "RECOMMENDED", "MAY", and "OPTIONAL" in this +document are to be interpreted as described in +[RFC 2119](https://www.rfc-editor.org/rfc/rfc2119.txt). + +## Terminology + +[TIP]: # ( Provide a list of any unique terms or acronyms, and their definitions here.) + +--- + +# Detailed Description + +This document specifies a system of declarative document validation and +authorization for CouchDB. It lets users write rules for validating document +updates using expressions that can be evaluated inside the main CouchDB process, +instead of having to invoke JavaScript code and incurring the overhead of +round-tripping documents to the external JavaScript engine. + + +## Design documents + +Users encode validation rules by storing a design document with the following +fields: + +- `language`: must have the value `"query"` +- `validate_doc_update`: contains an _Extended Mango_ expression that encodes + the desired validation rules +- `defs`: (optional) a set of named Extended Mango expressions that may be + referenced from the `validate_doc_update` expression, as a way to encode + reusable functions + + +## Handling write requests + +When a document is updated, the `validate_doc_update` (VDU) fields of all the +design docs in the database are evaluated, and all of them must return a +successful response in order for the update to be accepted. For existing VDUs +written in JavaScript, those continue to be evaluated using the JavaScript +engine. For VDUs in docs with the field `"language": "query"`, the VDU is +evaluated using the functions in the `mango` application, particularly the +`mango_selector` module. This module will need additional functionality to +handle Extended Mango expressions as described below. + + +### Input to declarative VDUs + +JavaScript-based VDUs are functions that accept four arguments: the old and new +versions of the document, the user context, and the database security object. +The input to a declarative VDU is a virtual JSON document with the following +top-level properties: + +- `$newDoc`: the body of the new version of the document that the client is + attempting to write +- `$oldDoc`: the body of the previous leaf revision of the document that the new + revision would follow on from +- `$userCtx`: the user context containing the current user's `name` and an array + of `roles` the user possesses +- `$secObj`: the database security object, containing arrays of users and roles + with admin access, and users and roles with member access + +Example of a user context: + + { + "db": "movies", + "name": "Alice", + "roles": ["_admin"] + } + +Example of a security object: + + { + "admins": { + "names": ["Bob"], + "roles": [] + }, + "members": { + "names": ["Mike", "Alice"], + "roles": [] + } + } + +To evaluate a declarative VDU against this virtual JSON document, evaluate the +Extended Mango expression in the `validate_doc_update` field. This will produce +a list of _failures_ that describe the ways in which the input does not match +the selector. Evaluation is considered successful if this list is empty. + +If the expression produces a non-empty list, then no further expressions from +other design docs are evaluated, the write is rejected, and a representation of +the failures is returned to the client. If the expression produces an empty +list, then expressions from other design docs are evaluated. If none of the +`validate_doc_update` fields in any design doc produces a failure, the write is +accepted. + + +### Responses to write requests + +If the selector expression in the `validate_doc_update` field returns an empty +list of failures, then the write is accepted and proceeds as normal, leading to +a 201 or 202 response. + +If any of the selectors fails, then either its list of failures or a custom +error response is returned to the caller, with a 401 or 403 status code as +indicated by the selector itself. For example, imagine a design doc contains the +following: + + "validate_doc_update": { + "$all": [ + { + "$userCtx.roles": { "$all": ["_admin"] }, + "$error": "unauthorized" + }, + { + "$newDoc.type": { "$in": ["movie", "director"] }, + "$error": "forbidden" + } + ] + } + +To evaluate this expression, the following steps are performed: + +- Check whether the user context's `roles` array contains the value `"_admin"`. + If it does not, return a 401 response to the client. +- Check whether the new doc's `type` field has the value `"movie"` or + `"director"`. If it does not, return a 403 response to the client. +- Otherwise, accept the write and return a 201 or 202. + +The body of the response contains two fields: + +- `error`: this is either `"unauthorized"` or `"forbidden"` +- `reason`: this contains either a custom error message, or the list of failures + generated by the first non-matching selector. + +If no custom `$reason` is set, then the `reason` field contains a list of +failures like so: + + { + "error": "forbidden", + "reason": { + "failures": [ + { + "path": ["$newDoc", "type"], + "type": "in", + "params": ["movie", "director"] + } + ] + } + } + +This is consistent with the current working of JavaScript VDUs. Such functions +can call `throw({ forbidden: obj })` where `obj` is an object, and it will be +passed back to the client as JSON, i.e. it is already possible for user-defined +VDUs to generate responses like that above. Review Comment: Whilst I see the argument that it is already possible for JS VDUs to pass any error structure they like I feel that formally specifying `object` types into the `reason` property is a breaking API change from the existing `string` usages enshrined in system defined error responses. Just because someone _can_ change that already with their own functions doesn't mean that they _should_. My concern here is that this change means using declarative VDU comes with a burden to check and potentially rewrite all post-write error handling code to handle both cases. That feels like a barrier to adoption or an avenue for unanticipated breakages. I accept the value of providing more validation information, but I don't agree with overloading the `reason` property with a different type to do it. Keeping `reason` a `string` at least would mean existing code continues to work on new VDU validation failures even if it doesn't provide as much information. Applications that want to use the extra information have coding to do anyway so they could as easily obtain that from some other part of the error structure. -- 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]
