Document runService #514
Document runService #514
Conversation
|
I would like to have @beauby check this for correctness. |
skipruntime-ts/server/src/server.ts
Outdated
| * - Write: `PATCH /v1/:collection` | ||
| * | ||
| * Update the named `collection` with the key-values entries in the request body. | ||
| * The `collection` must be the name of one of the service's collections, that is, one of the keys of the `Inputs` type parameter. |
There was a problem hiding this comment.
| * The `collection` must be the name of one of the service's collections, that is, one of the keys of the `Inputs` type parameter. | |
| * The `collection` must be the name of one of the service's input collections, that is, one of the keys of the `Inputs` type parameter. |
skipruntime-ts/server/src/server.ts
Outdated
| * The service responds to the following HTTP requests: | ||
| * - Read: `GET /v1/:resource/:key?param1=value1&...¶mN=valueN` | ||
| * | ||
| * Instantiate the named `resource` with parameters `{param1=value1,...,paramN=valueN}` and respond with the values associated with the given `key`. If the named resource has previously been instantiated with the same parameters, then it is reused. |
There was a problem hiding this comment.
We probably shouldn't make that strong a commitment about our internal caching strategy regarding resources.
There was a problem hiding this comment.
Ok. Do you think the phrasing below using "instantiate if necessary" is better/good, or should we not mention reuse at all?
skipruntime-ts/server/src/server.ts
Outdated
| * | ||
| * Instantiate the named `resource` with parameters `{param1=value1,...,paramN=valueN}` and respond with the values associated with the given `key`. If the named resource has previously been instantiated with the same parameters, then it is reused. | ||
| * | ||
| * - Subscribe: `GET /v1/:resource?param1=value1&...¶mN=valueN` |
There was a problem hiding this comment.
Currently requires an Accept: text/event-stream header, otherwise you're not getting an SSE stream but a simple JSON document representing a snapshot of the collection at current time.
skipruntime-ts/server/src/server.ts
Outdated
| * The service exposes some resources specified by a {@link SkipService}. | ||
| * Each resource has a _name_ and identifies a collection that associates _keys_ to _values_. | ||
| * The service responds to the following HTTP requests: | ||
| * - Read: `GET /v1/:resource/:key?param1=value1&...¶mN=valueN` |
There was a problem hiding this comment.
Exactly how the query parameters are parsed is not completely clear to me, but specifying this much seems safe. AFAIU what really happens is that express just parses them in its default way, subject to app.use(express.json());, and that uses a parser "based on" the qs package, but without documenting what actually happens.
There was a problem hiding this comment.
This changes in the new API, where instantiating a resource is now made using a POST /v1/streams with a JSON body looking like:
{
"resource": "foo",
"params": {
...
}
}
skipruntime-ts/server/src/server.ts
Outdated
| * - Write: `PATCH /v1/:collection` | ||
| * | ||
| * Update the named `collection` with the key-values entries in the request body. | ||
| * The `collection` must be the name of one of the service's collections, that is, one of the keys of the `Inputs` type parameter. |
There was a problem hiding this comment.
The implementation does not AFAIU check that the collection is one of the service Inputs. But I don't know how user code can come to know the actual name of other collections, such as outputs of resources, in order to issue update requests to them.
|
I'm happy to have #515 first and I can rebase and update this. Updating the docs myself and having you check them is a good way to check that I understand what is going on. |
|
Some rewording still to do, but here is a version that I think is updated for the dual-port service. |
skipruntime-ts/server/src/server.ts
Outdated
| * - `POST /v1/streams`: | ||
| * Instantiate a resource and return a UUID to subscribe to updates. | ||
| * | ||
| * Requires the request to have a `Content-Type: application/json` header. | ||
| * The body of the request will be parsed as JSON and interpreted at type `{ resource: string, params: { [param: string]: string } }`. | ||
| * Instantiate the named `resource` with parameters `params`, if necessary, and respond with a UUID that can be used to subscribe to updates. |
There was a problem hiding this comment.
Something that isn't clear to me is why in this case we read the resource and params from the body, while in other cases they are in the URL. Is this a necessary inconsistency?
There was a problem hiding this comment.
Yes and no. GET requests cannot include a body, which leaves only the url (and headers) to pass data. This has the downside of making it difficult to distinguish a string from an int, and generally to pass non-primitive values.
There was a problem hiding this comment.
To expand on my previous reply, we could replace GET /v1/resource/:resource with a POST /v1/snapshot passing the resource and params (and possibly a set of keys) as a JSON object in the request body. This would also stress the fact that doing a sync read does indeed involve instantiating a resource.
There was a problem hiding this comment.
My feeling is that it would be better if all of the requests accepted the resource and params in the same way, both for consistency and because if something is hard/impossible in either of the two, then having both will leave users with the least common denominator. So if putting them in the body or the request is possible, then we would have more control over their parsing, etc., that seems like the better option to me.
|
I think this is ready now. I reworded to improve consistency and, I think, clarity. The main thing is I removed all mention of instantiating resources "if necessary" and the docs read as if the instantiation always happens now. The reuse should not be visible to clients IIUC. The main point though is that stating that resource instances might be reused lets in a complication and unclear interaction with DELETE, where it may not be clear whether DELETE on a uuid for a reused resource deletes the original or not. To resolve this, the distinction between resource instances and stream ids would need to be exposed in the documentation, and that seems like a valueless complication. |
|
LGTM! |
No description provided.