Call to action - Please review the new API Spec

[JedWatson]

I've written up a spec for the new Admin API and would really like some feedback on it.

The endpoints in the spec are all implemented and in use, and I've cleaned them up slightly as I've been writing up how they work.

The next part of the plan is to make these generally available through List methods (so you can customise them and easily add user-facing versions for your app) so it's really important to get them right and make sure the design is one that we're happy to live with, so please shout out with any feedback you've got (PRs are also welcome!)

Specs are here: https://github.com/keystonejs/keystone/blob/master/admin/server/api/Readme.md

cc @mxstbr @molomby @josephg @wmertens @creynders

[wmertens]

You repeated the Sign In header

[JedWatson]

@wmertens thanks! fixed.

[wmertens]

How are errors for nested data handled? E.g. you have an schema with an array of objects and there should be at least 2 items in the array, and there is only one item and it is missing one of the values in its object?

[JedWatson]

That would be up to the Field to define how it provides validation errors - see the example for the password and email fields. An array-type field (e.g TextArray) would do something similar, for nested schema fields I expect it would nest a validation errors array.

[wmertens]

Suppose we make natural keys possible, e.g. a list of countries uses the international 2-letter code, how can we change the API? GET/POST /api/{list}/key/{key}?

[JedWatson]

Why would we want to do that for the Admin UI?

In user land you could bind the routes to whatever you like in the express router, and probably provide a custom function for querying an item (default to findById)

... but yes, given we've got a concept of autokey on lists, we could support that kind of URL if we wanted to.

[wmertens]

Also, I wonder if the API responses should not be a level lower. If a list has an error field, that could falsely trigger error code when getting an answer.
So responses could be {ok:<answer>}. If you don't get a JSON object with a single ok key back, you know you weren't talking to the Keystone API.

[wmertens]

I also wonder if it's really necessary to use HTTP error codes. A proxy could misunderstand, and if you do opportunistic getLoggedIn calls that respond with 401 the browser prints that in the console.

If however the answer is always a JSON object with extra keys for the various error conditions, you always know you are talking to the API and you don't try to parse what a captive portal sends you.

[wmertens]

Meaning, if the response is anything but 200, you know there is an error with the network or service. Actual API errors are handled by the UI code, not by the browser.

[wmertens]

Is there a reason to keep the item results in the legacy format? I never understood why the fields are sent as their own object under fields.

[geloescht]

Maybe the concept of expandRelationshipFields could be expanded a little, to also allow population of only certain relationship fields. Should the API return a top-level error if a field cannot be populated (due to a missing referenced item) or a nested error?
I agree with wmertens that itens should never be returned at the top level of JSON data. Apart from err, we might want to add other stuff such as timing statistics, that might collide with field names. I like status codes, but I am not an expert on how they interact with proxies.

[wmertens]

On Mon, Jul 25, 2016 at 6:03 PM geloescht notifications@github.com wrote:

I agree with wmertens that itens should never be returned at the top level
of JSON data. Apart from err, we might want to add other stuff such as
timing statistics, that might collide with field names. I like status
codes, but I am not an expert on how they interact with proxies.

The way I see it, HTTP status codes are meant for generic interaction
between browsers and servers. However, in this case we have very specific
interaction of a client implementing the Keystone API and Keystone. HTTP
error codes just add extra state to the interaction with no benefit I can
think of. In the end, the client expects the promise for data to settle
with the data or a rejection, and that collapses the errors again into a
single error state.

Another reason: Doing it this way makes the API transport agnostic, so you
could easily implement API calls in React server side rendering as direct
keystone calls and the reducers would get the same data.

[webteckie]

In the Query Params for Get Items, doesn't it make sense to have expandRelationshipFields be either a Boolean OR a list of fields? What if a client wants more than just {id, name} and wants to avoid extra round trips?

[webteckie]

In the Duplicate User case, why is it a good idea to return back all info about the duplicate user?

[wmertens]

There's also #558 - and the json-api spec also proscribes a sub-object for data.

I'm not sure that implementing the full JSON-API spec brings anything interesting though.

[christroutner]

As a developer that uses KeystoneJS on a daily basis, I just want to say thank you for creating this spec. I can't wait to see it implemented. It's going to make my life a lot easier!

[JedWatson]

Thanks @christroutner, glad to hear it! In good news: the spec is already implemented, so you can start testing it out today (although while it's still tied into the Admin UI, you need to be signed in which may or may not be viable depending on your project. going to give you the ability to expose a custom version of it in your application routes very soon!)

[JedWatson]

@wmertens

Is there a reason to keep the item results in the legacy format? I never understood why the fields are sent as their own object under fields.

To separate the "root" concepts of id and name, and the structured values in the fields. Keystone always wants to know the ID and Name of documents, regardless of what their paths in the schema are. This keeps the actual schema isolated.

Similar argument to the one JSON API makes (maybe we could align them more closely?)

The way I see it, HTTP status codes are meant for generic interaction
between browsers and servers. However, in this case we have very specific
interaction of a client implementing the Keystone API and Keystone. HTTP
error codes just add extra state to the interaction with no benefit I can
think of.

I'm actually hoping to standardise this as something that can be used in projects as well. So if you're writing a React Native app with a Keystone back-end, just tell keystone to generate you the API Endpoints you want and off you go. Ideally, everything (including status codes) should behave as close to "best practice" and consistently as possible

Trying to remove as much of the special behaviour in Keystone as possible, without overcomplicating things or losing the bits that make it really nice to work with. Hard line to walk 😉

@geloescht @webteckie being able to specify only some paths in expandRelationshipFields makes sense in theory, but I couldn't actually think of a real-world case where it would be useful to support that complexity. If you want field data out of relationships, at the moment I think it's best to keep it simple in the API and make additional calls to fetch the related items. Otherwise things get recursive pretty quickly, and flattening out the options for that gets really complex / custom / magical. We can revisit that decision once 0.4 ships, though!

I agree with nesting data in the return structure, will update the spec accordingly. It'll make the GET {list} and GET {list}/:id endpoints more consistent too, I guess, which is nice!

In the Duplicate User case, why is it a good idea to return back all info about the duplicate user?

It's not, really, that's just me enumerating the complete data we get back from mongoose. Any lower-level error like that is passed back as mongoose (or the driver) provides it. Not ideal but I'd prefer not to add any additional middle-man processing at this stage on lower-level errors that could occur.

[wmertens]

Ok, so for status codes I suppose that they can reflect the error that is in the object, but the error itself should be sufficient to map to the status, and the rest of the application layer should have no idea about response objects or error codes.

[wmertens]

In another project I made subclasses of Error for various types of errors, and the rest of the code returns promises for values. Any errors are thrown, and the Express API has a wrapper to call the functions, returning the value as json and sending the errors as json too. Nice separation of concerns.

This is also more or less how Koa 2 works, as far as I understand.

[VinayaSathyanarayana]

Can we have the documentation as a swagger/open api spec --> http://swagger.io/
If there is a swagger spec, one can leverage automatic code generation facilities of swagger.

[ctaepper]

you are not following the REST specs, but i assume you know that and the whole thing is not supposed to be a full blown REST API?

• when creating a new item in list, one should POST /api/{list}
• when deleting an item, one should DELETE /api/{list}/{id}
• when updating an item, one should PUT /api/{list}/{id} containing the whole item, or PATCH /api/{list}/{id} containing only the properties which get updated

[wmertens]

Frankly, I have yet to see a benefit from closely adhering to rest. Api calls don't get easier, in fact the opposite. If you make all calls require the same verb, that's less code to maintain. Overlapping routes are cute, but each call could have its own endpoint and it would be less code. POST /list/create, POST /item/read/id etc. You could add aliases for GET so just browsing also works.

[ctaepper]

@wmertens i'm sorry, i don't want to start an argument, but you clearly never had to consume a large REST API or even developed one yourself. Surely, technically you can just use POST for all endpoints... you can also only use DIV's everywhere in you html... but then you totally lack semantic meaning of your markup, or - in this case - the route definitions of your API

[mxstbr]

but you clearly never had to consume a large REST API or even developed one yourself.

No need to start insulting people here @ctaepper, refrain from doing that again, keep it technical please. This is a professional discussion, not a mud-slinging contest.

[ctaepper]

sorry, this was not supposed to be an insult. i only tried to make my point about semantics as clear as possible :)

[mxstbr]

As a side question, has any of the new file upload capability (#3228) been implemented yet? I figure I might as well extend my test script to go through all the API calls, including file uploads.

We are extremely hard at work on that, in fact Jed is working on this as we speak! It's going to be done super duper soon, but it's not yet enabled – see #3269 for instructions on how to test that. (you'll need to change two lines in a file)

[mxstbr]

The index.js file has the test code, which can be accessed by bring up that page in a web browser with a JavaScript console.

Also, this is awesome! 🎉

[creynders]

@JedWatson Cool! Great job with the APi doc.

However, I am a fan of real REST API's, for these reasons:

• Using verbs in URL's limits and cripples them. For instance I like to provide both a id-based and slug-based identification mechanism, e.g.
  GET /api/users/muthafucka aliases GET /api/users/123456. However, this means that with POST /api/users/create for instance, you'll need to sanitize the slugs on specific keywords
• REST is far more humanly readable IMO, compare POST /api/users/123456/delete?filter[foo]=whatever&q=kjhkjashkdj to DELETE /api/users/123456?filter[foo]=whatever&q=kjhkjashkdj This applies to calls made from non-browser clients, log streams etc. It also makes consuming them a lot easier
• I don't see the benefits of having a fields field for returned resources. It breaks the symmetry, since the returned resource data structures don't match the sent resource data structures.

In my experience REST works really great. You request resources, which are returned in full or partially. And you send resources in full or partially to update them. It's the symmetry and simpleness which is great. It also respects the separation of concerns by extracting the action from the URI.
And it allows for a lot of future expansion w/o getting in the way, which I've encountered with almost all other schemes, where your API grows and you need to start coding around your own API to allow the features you want. It gets really hard to manage and keep clean both code and APIwise.

I've consolidated and modified a number of guides, picking what I've found to work best and compiled it to a guide, maybe it can provide some inspiration: https://iminds-livinglabs.github.io/rest-api-recommendations/index.html

[creynders]

@JedWatson

I haven't added a spec for it but we have a draft implementation of an endpoint you can call to update multiple items. It's currently /api/{list}/update, and an array of items (including ID and new data) are sent in the body.

That's what PATCH is for. Many people think it's only used to partially update a single resource directly, but that's not correct. A collection is also a resource, i.e. if you want to update multiple resources in a collection it's a partial update of the collection, which is why PATCH is allowed to do this. E.g. you could

PATCH /api/users
[
    {
        id: 123456,
        name: "Jules Winfield"
    },
    {
        id: 789123,
        name: "Mia Wallace"
    }
]

Actually, the way PATCH normally is handled in most frameworks is incorrect. PATCH is supposed to describe a set of changes applied to one or more resources.

The PATCH method affects the resource identified by the Request-URI, and it also MAY have side effects on other resources; i.e., new resources may be created, or existing ones modified, by the application of a PATCH.

See RFC-5789

That said, obviously it's an easy way of updating a single resource by simply sending out the updated values and using the ID from the request URI.
Aaaaanyway, what I'm working towards in my projects is to allow PATCH be a compositional action, where:

1. if an ID is present in the request URI, it applies value updates to a single resource

2. if the request URI points to a collection it's a set of changes existing out of a composition of REST actions, e.g.

   PATCH /api/users
   [
       {
           verb: "PATCH",
           data: {
               id: 123456,
               name: "Jules Winfield"
           }
       },
       {
           verb: "DELETE",
           data: {
               id: 789123
           }
       },
   ]
   

Which solves all problems of updating and deleting multiple resources.

And finally, any operations on entities that aren't resources are POST's to actions, e.g. POST /api/search

Not saying this is what should be implemented in keystone. Just defending the sanity and flexibility of REST API's 😁

[christroutner]

I just wanted to update this thread with a compliment on the implementation of the /api/session call. This is going to make it so much easier to detect if a user is logged in and control the content displayed to them. Also, the fact that api calls for non-logged-in callers return the sign-in page is perfect.

I started a new repository last night, keystone-api-tests, where I'll be writing a test script for the new Admin API and Files field. The code used in this test script will also be used in the API documentation I'll be contributing. It's very simple right now, with hard to read code and only three tests, but I will be expanding and improving it.

FYI, the /api/sessions call returns an empty Object for a user that is not logged in, which is detectable on the front end with jQuery using the command $.isEmptyObject(data). However, according to the spec, it should return undefined. I'm not sure if that is the same thing.

[mxstbr]

Awesome @christroutner, looking forward to some other discrepancies you'll find!

[christroutner]

After reading through the spec and this thread again, I'm not sure what the proper way to UPDATE a list item is. I've tried both of the following and neither one works:

$.post('/keystone/api/users/update', userItem, function(data) {debugger;})
$.post('/keystone/api/users/'+userItem.id+'/update', userItem, function(data) {debugger;})

I tried to dig into the code, but couldn't figure out the correct path. I'm also using a version of the master branch that is a few weeks old.

The first instruction will return a 403 error, so I'm included to think this is the one that should work. The second one returns a 404 error, but is how I've seen it done before.

I've updated the keystone-api-test repository and included test cases for this situation. If you want to try it out, just clone the repo into the public/ directory of a working Keystone install, then open ip:3000/keystone-api-tests/tests.html in your browser.

[mxstbr]

It's POST /keystone/api/users/userId, no /update! (see admin/client/utils/List.js for all the API calls we make from the admin interface)

[christroutner]

Thanks for the quick reply, Max. I fixed the URL in my POST, but I'm still getting a 403 forbidden error. I made sure I'm logged into the system. I even updated my installation of Keystone to the latest commit (cbf9ff4).

I've updated the keystone-api-test repo to reflect the change is POST URL. I've also tried doing a Update to other list items besides just the User model, and it doesn't work for those either.

I'll try to fire up node-inspector and step through the code in admin/client/utils/List.js to figure out what's going on.

[wmertens]

@christroutner note that Node v6.3+ has the --inspect flag now, which works wonderfully.

[christroutner]

I ran the test script in the keystone-api-tests repository while running KeystoneJS under node inspector. Keep in mind that I'm running the latest master commit (cbf9ff4).

When I ran the post(/keystone/api/users/userID) test again, the file keystone/admin/client/utils/List.js didn't get executed at all. Instead, keystone/admin/server/api/item/update.js was executed. That file was throwing a 403 error because of an invalid CSRF token. Here is the code from that file. I stopped it at the debugger statement:

(function (exports, require, module, __filename, __dirname) { module.exports = function (req, res) {
    var keystone = req.keystone;
    if (!keystone.security.csrf.validate(req)) {
        debugger;
        return res.apiError(403, 'invalid csrf');
    }
    req.list.model.findById(req.params.id, function (err, item) {
        if (err) return res.status(500).json({ err: 'database error', detail: err });
        if (!item) return res.status(404).json({ err: 'not found', id: req.params.id });
        req.list.validateInput(item, req.body, function (err) {
            if (err) return res.status(400).json(err);
            req.list.updateItem(item, req.body, { files: req.files }, function (err) {
                if (err) return res.status(500).json(err);
                res.json(req.list.getData(item));
            });
        });
    });
};

});

I manually ran req.keystone.security.csrf.getSecret(req) and it returned the CSRF token string "CunSnobyfWscEg==". Next I manually ran req.keystone.security.csrf.validate(req), which returned false. I ran req.keystone.security.csrf.requestToken(req) and it returned an empty string.

During this entire time, I'm logged in and can access the Admin UI at /keystone, so this clearly seems to be a CSRF issue.

Any thoughts?

[christroutner]

If anyone can try out a POST call to update a list item successfully, let me know. If there is some environment variable I'm missing I'll dig into it. I'd be curious to see if other people get the same experience I did.

[danielmahon]

@JedWatson @wmertens Just wanted to throw my hat in the ring here. What happened with the GraphQL discussion? I've been working on a React/Keystone app for a bit now and was able to rather easily use https://github.com/RisingStack/graffiti-mongoose to hook up the models to the GraphQL API. This was simple because I didn't need to create a single GraphQL schema, it converts the mongoose one. There are only a few fields which it doesn't seem to like (createdBy, relationships, etc). I'm looking into if that's an easy fix from either end. Thoughts?

[danielmahon]

OK, I lied. Must of had a typo somewhere. Now it seems to work perfectly (so far) with keystone:

const keystone = require('keystone');
const _ = require('lodash');
const graphqlHTTP = require('express-graphql');
const { getSchema } = require('@risingstack/graffiti-mongoose');

// Setup Route Bindings
exports = module.exports = (app) => {
  // Import keystone schemas
  const options = {
    mutation: false, // mutation fields can be disabled
    allowMongoIDMutation: false, // mutation of mongo _id can be enabled
  };
  const models = _.map(keystone.lists, list => list.model);
  const graphQLSchema = getSchema(models, options);

  // GraphQL
  app.all('/api/graphql', graphqlHTTP({ schema: graphQLSchema, graphiql: true }));

};

[wmertens]

nice!

There is a problem with this though, mongoose is not the final arbiter of
permissions here. So with this endpoint you are allowing anyone full access
to the DB, or am I mistaken?

This would be a great plugin, something like
"require('keystone-graphql')(myKeystoneInstance)". It would need to lazily
build the schema on first connect due to the ordering of the settings I
think.

On Fri, Sep 30, 2016 at 5:09 AM Daniel Mahon notifications@github.com
wrote:

OK, I lied. Must of had a typo somewhere. Now it seems to work perfectly
(so far) with keystone:

const keystone = require('keystone');
const _ = require('lodash');
const graphqlHTTP = require('express-graphql');
const { getSchema } = require('@risingstack/graffiti-mongoose');

// Setup Route Bindings
exports = module.exports = (app) => {
// Import keystone schemas
const options = {
mutation: false, // mutation fields can be disabled
allowMongoIDMutation: false, // mutation of mongo _id can be enabled
};
const models = _.map(keystone.lists, val => val.model);
const graphQLSchema = getSchema(models, options);

// GraphQL
app.all('/api/graphql', graphqlHTTP({ schema: graphQLSchema, graphiql: true }));

};

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
keystonejs/keystone#3209 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AADWlrnajm4p3L_3cYC8A5aHGjmEGnn2ks5qvH1sgaJpZM4JUObx
.

[LorbusChris]

Regarding GraphQL, another approach that I've been thinking of making is to wrap Keystone's API in it (and that neat Dataloader) like described in this post on the graphql blog (see the server-side REST wrapper part).

That way permissions (and possibly roles in the future) would be enforced.

This may even be worth automating with a getGraphQLSchema function specifically for the Keystone API, too.

[zenflow]

Regarding GraphQL, another approach that I've been thinking of making is to wrap Keystone's API in it (and that neat Dataloader) like described in this post on the graphql blog (see the server-side REST wrapper part).

@LorbusChris The intro to that blog post seems to imply that that approach is suggested mostly as a shortcut to upgrade legacy APIs. I can see it has it's drawbacks in portability, complexity and performance (the application making lots and lost of localhost http connections to itself). Hosts like Heroku don't support this. As a framework I think it would be better to follow graphql.org's recommended best practices:
https://graphql.org/learn/thinking-in-graphs/#business-logic-layer

[JedWatson]

This can be closed now; the API in v4 is stable and v5 is using GraphQL

[zenflow]

and v5 is using GraphQL

@JedWatson Exciting words! Is there anywhere we can follow and or contribute to v5 yet?

[gautamsi]

https://github.com/keystonejs/keystone-5