knpuniversity / rest

Screencast code, script and HTTP Methods for "RESTful APIs in the Real World Episode 1"
http://knpuniversity.com/screencast/rest
Other
55 stars 53 forks source link

Ongoing issue during the coding of the tutorial #1

Closed weaverryan closed 10 years ago

weaverryan commented 10 years ago

My own notes of things to not forget about or revisit as I create the project for this tutorial:

weaverryan commented 10 years ago
willdurand commented 10 years ago

There is no preferred answer about the plural vs singular question. What matters the most is to be consistent.

willdurand commented 10 years ago

A) If the client sends extra properties (or perhaps a property that exists in the representation, but is unmutable), is it better just to ignore these properties or return a validation error?

I'd say it depends. Do we want to be "strict" or pragmatic?

B) What do you think about allowing the client to specify which embedded resources they want - e.g. /places?embed=checkins,merchant,current_opp.images? It seems very flexible for the client. And if we use this, is this reasonably possible with the serializer?

OData to the rescue! A good read from François: http://redotheweb.com/2012/08/09/how-to-design-rest-apis-for-mobile.html.

C) Have you seen https://github.com/thephpleague/fractal? I'm not sure its mature or flexible enough, but I did like how explicit it was to create your representations, especially if your representations begin to differ from your model (instead of using VirtualProperty's etc). Just wanted to see if you had any strong thoughts one way or another :).

Yeah, and I don't really like this package, sorry. I don't like the way representations are created, and I don't think it is worth using it.

weaverryan commented 10 years ago

OData to the rescue! A good read from François: http://redotheweb.com/2012/08/09/how-to-design-rest-apis-for-mobile.html.

Oh, so this idea of ?expand= is already a semi-standard, which appears to "work" exactly like embed in the URL I was giving. That's awesome. Now, is this easily possible with the serializer? Without manually giving each "expandable" relationship property its own group? Or is that exactly what we should do.

willdurand commented 10 years ago

I think we should have a whitelist of known relationship properties.

weaverryan commented 10 years ago
{
    "type": "http://someUrl.com/errorCodes#ErrorCode",
    "title": "Human readable description",
    "errors": {
        "firstName": "Please enter a name"
    }
}

Do you see any issues? Also, it seems that you might prefer vnd.error?

Thanks!

willdurand commented 10 years ago

D) I'm using Symfony's validator component, though I don't really want to focus on this [...]

I'm fine with your API Problem response. vnd.error is not really self-explaining.

E) For 404, 403 and 500 errors, we'll need an ExceptionListener that transforms these into the proper "Api Problem" JSON structure. [...]

Yeah, it is better. You can use a basic middleware in Silex. It is not difficult, and I think people will understand it.

F) Can you take a quick glance at the Programmer controller so far and see if you spot any horrible things? :)

Ok, quickly read it, and it looks good to me. Will do an extensive review tomorrow.


BTW, I don't get email notifications from GitHub...

weaverryan commented 10 years ago

Next!

willdurand commented 10 years ago

If it is awkward, then don't use a custom type, application/json is fine for errors. I would not create my own type here. However, I don't get the "code" stuff.

Let's keep everything simple: an error response is made a message for the end-user, and optionally a message for the developer. Most of the time, API error reponses are either not accurate or dumb. According to Apigility guys, if I remember correctly, we must return the right HTTP status code (which already means a lot, facebook does not do that for instance...), a message for the end-user (which should be translated so that clients could use it directly), a link to the error explanation for the developer or, at least, a message for the developer (in English), in other words, something meaningful for the developer.

willdurand commented 10 years ago

Yes, PATCH is hard to use (well, tedious). So, either we don't use it (:+1:) or we explain it the right way, not the wrong way. For a small flag, I would rather expose it as a subresource:

/api/programmers/1/angry

(yes/no)

willdurand commented 10 years ago

For I, I like the FOSRestBundle way actually, one action = one resource (or collection), no matter the format you serve to represent it. Using two controllers, you basically break this, as you distinguish HTML and JSON/XML. However, these are formats, and resources should not depend upon them. Is it understandable?

willdurand commented 10 years ago

F) Can you take a quick glance at the Programmer controller so far and see if you spot any horrible things? :) https://github.com/knpuniversity/rest/blob/finish/src/KnpU/CodeBattle/Controller/Api/ProgrammerController.php. This is only done through ch7 (well, ch7 is not fully done) - so we're looking at a basic CRUD API without any idea of linking or using any outside libraries to help us serialize, etc.

I don't really like the updateProgrammerFromRequest() method, but apart from that I'm fine with the code. For instance, in newAction() I expect a programmer to be created, not "updated from the request". It makes the code less readable, and less self-explanatory.

weaverryan commented 10 years ago

Yea, awesome! Now things are getting hard, as I expected! More questions :) - or follow-ups from above:

willdurand commented 10 years ago

J) well, it can be form-urlencoded too:

value=true|false

or simply:

true|false

K) exposing sub-resources sounds better to me, but if you really really want to use PATCH, feel free.

L) 2 URIs for each resource is wrong to me.

weaverryan commented 10 years ago

L) 2 URIs for each resource is wrong to me.

This is tough :). It means then that we must introduce a view layer and use one controller for everything. I think that's quite complex. And again, something like GitHub does this:

Does using a different subdomain make the difference? Because then it's technically 2 different applications (and hey, maybe the HTML version is using the API behind the scenes to get data)? Thoughts?

... update ... the more I think about it, the more I think we should mention this issue earlier, and then perhaps show how to fix it later. It's a cool point because we can say "why just return JSON and XML? If you do a good job, this URL can return HTML too, using most of the same code".

willdurand commented 10 years ago

I have a long answer but it is late here, and I just can't think about anything anymore at the moment. Let's not change anything by now. More to come tomorrow :sleeping:

willdurand commented 10 years ago

I don't think it is complex to serve either HTML and JSON using the same controller. That is why I expect from my students for example. IMO, one of the main benefits is that it makes the transition to Symfony/FOSRestBundle smoother/easier.

Does using a different subdomain make the difference? Because then it's technically 2 different applications

Probably, then it makes sense to have different urls.

Here, we build an API (a REST-ish one), and a single app, hence the idea of not duplicating the code. Maybe we can explain our choice to separate the API part than the HTML/Website part, but what are the pros/cons?

weaverryan commented 10 years ago

Ok cool :). So I think I'd like to unify HTML with JSON later in the tutorial. Early-on, there are just a few pieces we're missing, like content-negotiation and needing a layer to gather the information from the request body or from the POST variables. But once we've laid out the foundation, it seems very cool to me to basically say "look, JSON and HTML aren't really different, let's see how easy it will be to unify things now".

Early-on, I think we should make these changes:

What do you think? :)

willdurand commented 10 years ago

Tip top! :+1:

willdurand commented 10 years ago

Is everything ok on your side?

weaverryan commented 10 years ago

@willdurand Yes, sorry for the silence ! Big travels, then when we got back, we still had plenty to work on! We've recorded a bunch of chapters now, and now I need to turn a few more chapters into rough copies for your review. I think we will then be nearing finishing episode 1. I'll ping you this week or next with a few more things to review. I'll open up a new issue with each question (hence closing this).

Thanks for checking in :)

willdurand commented 10 years ago

Alright! :heart_eyes: