Skip to content

Commit

Permalink
API controller: support user providing a query factory
Browse files Browse the repository at this point in the history
Previously, the `handle` method took in a request object, did some
extra parsing and validation, built a query from the parsed + validated
request, and then transformed that query with the user’s transform
function. Finally, it ran the new query to get a Result, and returned
an HTTPResponse created from that result.

I’ve now broken this up over many separate functions (that are then
optionally still called in `handle`), which is a good thing in a
general/abstract sense, but also enables a specific use case. In
particular, the idea is that, rather than the library constructing the
initial query and then the user getting to transform that, the user
should be able to control how the initial query is constructed.

This may seem like a distinction without a difference, because you’d
imagine that the user could take the initial query (passed to their
transform function) and just discard it in the transform to replace it
with whatever query they want. But the problem with that is request
validation.

Imagine if the user wants to handle some request like `POST /sign-in`,
where the credentials are passed in an Authorization header. Even
though this is a POST request, the user wants to map it to a FindQuery
that’ll read the credentials and use them to look up the resulting
user, or return an error. The problem is that, to invoke their query
transform, the library first has to construct an initial query that it
can hand the query transform as an argument. And, fundamentally, it has
no idea how to construct a query for a POST request with no body and no
`type` parameter, since it’s basically assuming all POST requests
result in CreateQuerys or AddToRelationshipQuerys. So, the library
tries to validate the request in preparation for creating such a query,
and the lack of a body causes the library to throw an error, so the
user’s transform never runs.

So, we replace the idea of a query transform with a query factory -- a
function the user can provide to build the query -- and we make a
separate `APIController.makeQuery` function public, so that the user
can simply compose their own function with `makeQuery` to recreate the
effect of query transforms.

But this reveals another problem, which is present whether the final
query comes from running the user's transform function (as before) or a
user factory function. Namely: when and how beforeSave/beforeRender
transforms should run and generally interact with query construction.

In short, `beforeSave` transforms have to run as part of/prior to
constructing the query, because we have no way to take a query, once
constructed, and know conceptually which parts of it were constructed
with info from the request document that should be transformed. (E.g.,
in a bulk delete query, resource identifier objects from the document
end up in the query's Criteria.) In other words, we really can't we
really can't apply `beforeSave` to a query -- but only to an incoming
request document, where we know what parts are resources and resource
identifiers. Moreover, it might be confusing/inelegant for the query
that the query factory returns to not actually be the query that runs,
which is another argument for not trying to apply beforeSave to a
returned query.

But, in our old query transform world, applying `beforeSave` as part of
query construction posed a problem: the user's code only ran after the
initial query was constructed, which made it impossible for them to
access the raw, untransformed data as the end user input it in the
request.

So, now we solve this by giving the query factory access to the
untransformed request document, to read the raw data, and a function
for transforming a document, so it can get the transformed dat it needs
and put it in the query.

That runs into another small problem, though, namely: that the
`Resource` class is mutable, so running `beforeSave` produces big side
effects; at the moment you do the transform, the untransformed
resources that get passed to the factory are mutated too. That's really
icky/not ideal, but it's tolerable: the factory function just has to
read the data it needs off the untransformed doc before calling the
transform function or `makeQuery`, which has to call the transform
function.

So, that solves how to apply `beforeSave`. Then, the question is
`beforeRender`. It turns out there's an asymmetry here, because we can
take a query and transform it to be the same query but with
`beforeRender` applied. That's because beforeRender would be applied in
`query.resulting`, and we always have a document that we're applying it
to at that point.

That made me tempted to take the query factory's result and wrap it in
`beforeRender` automatically, as that would ensure that the user never
forgets to call beforeRender on the result, which could be a security
issue. However, I ultimately decided against this because: 1) I again
didn't like the idea that the query returned by the the query factory
isn't actually the query that's run and; 2) when the user's making
their own queries (which should hopefully be rare), they already have
to remember to apply beforeSave to be safe, so having to remember to
apply beforeRender (which will be automatic if they're using
`makeQuery` as a base and composing the `query.returning` function it
generates) seemed like not too much extra burden.

Note: a big nice outcome of this commit is that all the library’s
built-in query construction logic is unit testable, because
APIController.makeQuery is a pure function. Before we’d reified the
idea of the query and broken construction apart from running, we’d have
needed to do an integration test where we spied on adapter methods.

This commit also includes another of other changes; see UPGRADING.md
diff in this commit.
  • Loading branch information
ethanresnick committed Jan 8, 2018
1 parent 5e5c6ff commit 569fa64
Show file tree
Hide file tree
Showing 19 changed files with 718 additions and 336 deletions.
46 changes: 23 additions & 23 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -25,43 +25,43 @@ Check out the [full, working v3 example repo](https://github.com/ethanresnick/js
var adapter = new API.dbAdapters.Mongoose(models);
var registry = new API.ResourceTypeRegistry({
"people": {
urlTemplates: {
"self": "/people/{id}"
},
beforeRender: function(resource, req, res) {
if(!userIsAdmin(req)) resource.removeAttr("password");
return resource;
}
},
"places": {
urlTemplates: {"self": "/places/{id}"}
}
"places": {}
}, {
"dbAdapter": adapter
"dbAdapter": adapter,
"urlTemplates": {
"self": "/{type}/{id}"
}
});

// Initialize the automatic documentation.
var DocsController = new API.controllers.Documentation(registry, {name: 'Example API'});

// Tell the lib the host name our API is served from; needed for security.
var opts = { host: 'example.com' }
const opts = { host: 'example.com' };

// Set up a front controller, passing it controllers that'll be used
// to handle requests for API resources and for the auto-generated docs.
var Front = new API.httpStrategies.Express(
new API.controllers.API(registry),
new API.controllers.Documentation(registry, {name: 'Example API'})
);

// Set up our controllers
var APIController = new API.controllers.API(registry);
var Front = new API.httpStrategies.Express(APIController, DocsController, opts);
var requestHandler = Front.apiRequest.bind(Front);
// Render the docs at /
app.get("/", Front.docsRequest);

// Add routes for basic list, read, create, update, delete operations
app.get("/:type(people|places)", requestHandler);
app.get("/:type(people|places)/:id", requestHandler);
app.post("/:type(people|places)", requestHandler);
app.patch("/:type(people|places)/:id", requestHandler);
app.delete("/:type(people|places)/:id", requestHandler);
app.get("/:type(people|places)", Front.apiRequest);
app.get("/:type(people|places)/:id", Front.apiRequest);
app.post("/:type(people|places)", Front.apiRequest);
app.patch("/:type(people|places)/:id", Front.apiRequest);
app.delete("/:type(people|places)/:id", Front.apiRequest);

// Add routes for adding to, removing from, or updating resource relationships
app.post("/:type(people|places)/:id/relationships/:relationship", requestHandler);
app.patch("/:type(people|places)/:id/relationships/:relationship", requestHandler);
app.delete("/:type(people|places)/:id/relationships/:relationship", requestHandler);
app.post("/:type(people|places)/:id/relationships/:relationship", Front.apiRequest);
app.patch("/:type(people|places)/:id/relationships/:relationship", Front.apiRequest);
app.delete("/:type(people|places)/:id/relationships/:relationship", Front.apiRequest);


app.listen(3000);
Expand Down
20 changes: 19 additions & 1 deletion UPGRADING.md
Original file line number Diff line number Diff line change
@@ -1,7 +1,25 @@
# 3.0.0-beta.4

## New Features
- The APIController, and the ExpressStrategy, now have functions for sending a JSON:API any `Result` back to the user. This is convenient if you ahve a `Result` that you created manually or that otherwise came from somewhere other than executing a query in `APIController.handle`.

- The public methods on the Express strategy are now bound methods, so you no-longer have to bind their `this` when passing them around.

- The library now prints deprecation warnings using the `depd` module.

## Breaking Changes
- Query.returning and query.catch can now be async (returning a `Promise<Result>` rather than a `Result` directly). Accordingly, if you were wrapping one of those functions, you'll now have to `await` the return value.
- Query.returning and query.catch can now be async (returning a `Promise<Result>` rather than a `Result` directly). Accordingly, if you were composing with one of those functions, you'll now have to `await` the return value.

- Request body and query parsing now happen slightly earlier than before. Accordingly, if the user's request had multiple errors (e.g. an invalid body and an invalid method), the error returned may be different than before, because the first check that fails is the one whose error is reported.

- The library's internal `Request` object no longer has a `primary` key; instead it has a `document` key which holds the whole request body parsed as a `Document` instance (or undefined for requests with no body). This lays the ground work for supporting sideposting and other features where there's query-related data in the request document outside of the primary data.

- In the fifth argument passed to user-defined `beforeSave` and `beforeRender` functions, which is an object, the request and response objects generated by your HTTP framework (e.g., express, hapi, node) should now be accessed at the `serverReq` and `serverRes` properties, rather than `frameworkReq` and `frameworkRes`. Those old names are deprecated and will be removed when v3 launches.

- `ExpressStrategy.sendError` should now be provided with the `next` function as an argument; not providing this will be an error when v3 is finalized.

- On `APIController` signature of `handle` method has changed, and the `responseFromExternalError` method has been renamed to `responseFromError`. These changes should only effect you if you have written a custom HTTP strategy.

# 3.0.0-beta.3

## Bugfixes
Expand Down
84 changes: 76 additions & 8 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 3 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,8 @@
"@types/url-template": "^2.0.28",
"content-type": "1.x.x",
"dasherize": "2.0.x",
"debug": "^2.6.8",
"debug": "^3.1.0",
"depd": "^1.1.1",
"flat": "^1.2.1",
"immutable": "^3.7.5",
"jade": "1.11.x",
Expand All @@ -61,6 +62,7 @@
"qs": "^6.5.0",
"ramda": "^0.24.1",
"raw-body": "2.x.x",
"supports-color": "^4.5.0",
"url-template": "^2.0.4",
"vary": "^1.1.1"
},
Expand Down
Loading

0 comments on commit 569fa64

Please sign in to comment.