keystonejs / keystone-classic

Node.js CMS and web app framework
http://v4.keystonejs.com
MIT License
14.64k stars 2.2k forks source link

[0.4] API Inconsistencies #2557

Closed TheDucc closed 8 years ago

TheDucc commented 8 years ago

First of all: Everything is AWESOME. And thanks for all the consistent and hard work. That being said.

I have been working with the built in API for the last couple of days and I have noticed some inconsistencies that I would like to make note of here. I based my code almost exclusively on the very helpful code in Jed's KeystoneApiExample.md at URL: https://gist.github.com/JedWatson/9741171

In keystone.js I did: keystone.set( 'cors allow origin', true )

And in routes/index.js I did this:

app.get('/api/users/list', [keystone.middleware.cors, keystone.middleware.api], routes.api.users.list);
app.all('/api/users/create', [keystone.middleware.cors, keystone.middleware.api], routes.api.users.create);
app.get('/api/users/:id', [keystone.middleware.cors, keystone.middleware.api], routes.api.users.get);
app.all('/api/users/:id/update', [keystone.middleware.cors, keystone.middleware.api], routes.api.users.update);
app.get('/api/users/:id/remove', [keystone.middleware.cors, keystone.middleware.api], routes.api.users.remove);

And then I tried to use them one by one. List went great.

When I moved onto update I kept getting an empty req.body until I added 'Content-Type': 'application/json' to the header of my http.post.

When I tried to re-use the front-end update code to create a user, I got a few new issues. I was getting a pre-flight error, the request to /api/users/create was coming in as an OPTION (not POST), and the req.body was once again empty. Until I manually set the header to 'Content-Type': 'application/x-www-form-urlencoded'

And this is the inconsistency that I would like help understanding.

But while I have your attention I also had a little trouble with the create code I copied

exports.create = function(req, res) {
    var item = new User.model(),
        data = (req.method == 'POST') ? req.body : req.query;
    item.getUpdateHandler(req).process(data, function(err) {
    if (err) return res.apiError('error', err);
        res.apiResponse({
            user: item
        });
    });
}

This did not work for me. Though the data var logged out as:

{ username: 'newuser',
  name: { first: 'New', last: 'User' },
  email: 'new@email.com',
  password: 'new' }

I got an error:

{ message: 'Validation failed',
      name: 'ValidationError',
      errors:
       { password:
          { name: 'ValidatorError',
            path: 'password',
            message: 'Passwords must match',
            type: 'required' } } }

When I changed the code to match the user create in updates/0.0.1-admins.js; and everything worked fine. I would also like some help understanding this as well. Though I assume that I am just not understanding how posts are different from users, and the consequences of getUpdateHandler related to each.

Thanks already for reading this much.

dmd

JedWatson commented 8 years ago

Hi @TheDucc, thanks for such detailed feedback! You've done some great sleuthing here.

At the moment, we're working on a brand new API (see admin/server/api/item/update.js for where it's currently at)

The underlying functionality - List.validateInput and List.updateItem are basically going to replace the UpdateHandler, which is one of the last pieces left preventing use of custom field types (if you look under the hood, you'll see it has hard-coded handlers for certain field types, and does things that have been replace by the new FieldType internal API)

We're about to rewrite the UpdateHandler to use the new internals as well so the public API doesn't break between 0.3 and 0.4

I'm also planning to expose new methods to create API endpoint handlers, to give you an idea what this will look like:

app.post('/api/things', keystone.list('Thing').getCreateAPIHandler(options));
app.post('/api/things/:id', keystone.list('Thing').getUpdateAPIHandler(options));

... or you could use more specific functionality if you need more control; we're planning a kind of Matryoshka doll series of APIs and API generators offering increasingly granular functionality.

I'll do my best to get an updated example working for 0.4 asap and add the information here when it's ready.

mxstbr commented 8 years ago

Closing this since @JedWatson answered everything quite thoroughly—see #2566 for the newest update!