search

vibe-d / vibe.d

Official vibe.d development
MIT License
1.15k stars 284 forks source link

Future of the REST generator #1040

Open Geod24 opened 9 years ago

Geod24 commented 9 years ago

I'd like to express here my plans for a new REST module.

==> @before / @after I think this goes without saying that we should replace this. I wrote alot about it, and I'm going so I'm going to be concise: the interface should only bear informations that are equally relevant to the client and the server. I think it would be easier to design if client and server where splitted (see #842 ). While separating, we still need to provide a way for users to access the request (for example, for logging purpose), and the response if they have very specific need. My solution to this problem I'll call 'UDI' (user defined inheritance). The idea is that you can generate a base type (in our case, interface) using a 'model' (in our case, the interface that we used to inherit from), the actual implementation, and a set of rules defined in the template.

ATM, rules are still to be defined, but an example of what I'm aiming for is: An user defines an interface IAPI, just like he used to (@before / @after are deprecated). The will be absolutely no change on the client side (except deprecation of @before / @after). Server side: User implements class API : RESTServer!(API, IAPI) instead of class API : IAPI. RESTServer is a template that will generate an interface 'on-the-fly' that will match the implementation, given the interface is valid. We can define 3 basic rules: 1) If the signature of an overload in API matches one in in IAPI, include the overload in G (generated interface). This is the normal use case that we had. 2) If the signature of an overload in API has a set of parameters that are HTTPServerRequest[, HTTPServerResponse] then the same set of parameter as a method in IAPI, then we add this signature to G. 3) If anything differs, error out / return case 1 and let the compiler error out.

Of course, only methods with matching names are tried.

Code example:

interface IAPI {
  void foo(string bar);
}

class API : RESTServer!(API, IAPI) {
  // This is valid of course
  override void foo(string bar);
}
class API2 : RESTServer!(API2, IAPI) {
  // This would also be valid
  override void foo(HTTPServerRequest req, HTTPServerResponse res, string bar);
}

This will be available on 2.067 and up (because before it segfault the compiler).

Note: This could also be handled by implementing a separate class and an interface. But it'll force the generator to do more checks and would be less user friendly IMO (and departs too much from the current approach).

Geod24 commented 9 years ago

Answer from @s-ludwig in this thread:

To be frank, I really dislike that approach. The nice thing about the current approach where the HTTP part is almost completely hidden from the service implementation is that it can be used locally, too. With this change, you'd basically always have to use a REST connection to talk to it, even if just locally from the same process. It also makes it very easy to break the REST idiom by looking at req.session or similar (at least you have to jump through some hoops to do that now).

I'm not sure I understand correctly. You don't have to get the HTTPServer* objects, because most of the time you might not want to. But when you need to, they are here. Also, this will be totally opt-in: we will keep the current generator and add special handling for RESTServer template until, at least, we drop support for 2.066.

s-ludwig commented 9 years ago

The HTTPServer* objects have been deliberately excluded, so that the server implementation stays as free from HTTP specific code as possible. Moving such accesses to external annotations was definitely a big win, even if the current @before/@after might be a little too low level.

I think a better direction would be to think about ways to annotate the interface that work for both sides.

Geod24 commented 9 years ago

Why would we want to forbid the user to use HTTP specific code ? I know REST doesnt mean HTTP in theory, but I have yet to be pointed to one interface that isnt based on it. I'd also understand if we wish to prevent users from doing unRESTful stuff in REST handler, but this has to be enforced by useful artifacts, not strict restrictions. One example is header/body/queryParam. Those are annotations that are meaningful to both client and server. Other planned changes includes matching a route via content type (which is why the overload case is not solved, yet). But for some use case you definitely need to be able to do that.

mihails-strasuns commented 9 years ago

(will respond later this week)

s-ludwig commented 9 years ago

I've mentioned it in my original reply. Ideally, the server implementation should be usable locally without any HTTP protocol in-between, just like a normal class. This was one of the basic initial design decisions. It also implies that other protocols instead of just HTTP could be used.

not strict restrictions.

I've never talked about strict restrictions. @before is quite at the right level in that regard - it doesn't influence the code or the method signature, but gives access to the raw request if really needed. The idea is always to make the "right" way easy and convenient and the "wrong" way deliberately a bit less straight forward, but not impossible.

One example is header/body/queryParam.

Those are fine, they are just additional (HTTP specific) annotations that do neither change the semantics, nor the function signature or body and can thus be ignored when no HTTP is involved.

Geod24 commented 9 years ago

I've mentioned it in my original reply. Ideally, the server implementation should be usable locally without any HTTP protocol in-between, just like a normal class. This was one of the basic initial design decisions. It also implies that other protocols instead of just HTTP could be used.

Okay, wasn't aware of that. At first I find it weird, but it can make sense to call the server locally. However, regarding the change of protocol, see my earlier comment. I'll investigate how the new scheme can fit in those requirements.

@before is quite at the right level in that regard - it doesn't influence the code or the method signature, but gives access to the raw request if really needed.

It does influence the function signature: you end up adding an extra parameter to the interface, for something that is server only. So if you do logging in your handler and you want to add the IP address, which is a server-side only modification, you will end up adding a @before parameter to the interface.

Also, the rest example use @before for authentication. My first project using vibe.web.rest was very simple, and inspired from it. It was 2 binaries (one client library wrapping my API, and my API itself). Only a folder was shared between the projects, which contained the interfaces definitions. I wanted to authenticate my users (on a RESTful manner - using basic authentication at the time). It looked a bit like this:

interface User {
@before!authenticate("user_ctx")
  void postMessage(string message, UserContext usr_ctx);
  string getMessage(uint msgId, UserContext usr_ctx);
}

UserContext authenticate(HTTPServerRequest req, HTTPServerResponse res) {
  import mysql;
  import server.someStuff;
  // Mysql authentication logic...
  // Create username / userId if successful.
  return UserContext(username, userId);
}

Now what went wrong ? Well, in my client application, the interface didn't compile because the UDA was referencing authenticate. But I couldn't bring authenticate in, because it had reference to the internals of my server (I already had to separate UserContext), and to MySQL. I gave it a very long though (this was discussed with @Dicebot at DConf2014), and after some months of trying to patch @before's design, I realized it was not possible this way, because the server often needs more information that what we currently provide to it (even if most of the time, such things won't be needed). Also, I know that this specific use case can now be solved with @headerParam, but I'm asking you to consider the effect of @before on encapsulation. I also know I could have written a different interface for my client library, stripped of @before and the additional datastructure, but that's just a workaround.

Those are fine, they are just additional (HTTP specific) annotations that do neither change the semantics, nor the function signature or body and can thus be ignored when no HTTP is involved.

Could you provide an example of a REST API not based on HTTP ?

s-ludwig commented 9 years ago

You are right of course that an additional parameter is needed. I usually use a default value for those parameters, though, so that doesn't matter so much in practice. I don't want to defend the @before mechanism as perfect, but directly adding a HTTPServerRequest parameter is a definitive step back.

Could you provide an example of a REST API not based on HTTP ?

It's not about REST specifically, it could be some binary protocol for example. The important thing is just that the same code can be used to generate different protocol implementations.

A proposal for a different approach: Use special parameter types similar to the validation types. These types could have a special method static T fromRestRequest(HTTPServerRequest req, HTTPServerResponse res) (or similar) that is recognized by the REST interface generator. Other protocol generators could have similar methods, so that the same type could support any number of protocols. For the client side, there could be the reverse void toRestRequest(HTTPClientRequest req) method.

Geod24 commented 9 years ago

It's not about REST specifically, it could be some binary protocol for example. The important thing is just that the same code can be used to generate different protocol implementations.

I agree to that from a purist POV, but in practice different protocols often have different considerations. Some protocols will have state, some other will care about packing, etc... In practice it's rarely possible to implement many protocols behind the same interface, and you end up resorting on the weakest one. It would be very disappointing to have the rest module limited by such consideration when some very popular framework (RoR) are doing just fine. I think Vibe could be a very solid candidate in this area given it's model of concurrency, it's efficiency, and the modeling power of D. But restricting the module because we might integrate it with protobuf seems like early optimization.

A proposal for a different approach: Use special parameter types similar to the validation types.

You mean something like:

interface IAPI {
  void foo(string name, MetaParam par);
}

struct MetaParam {
  static MetaParam fromRestRequest(HTTPServerRequest req, HTTPServerResponse res) { ...}
  void toRestRequest(HTTPClientRequest req) { ... }
}

?

s-ludwig commented 9 years ago

That doesn't make much sense to me. The interface defines an API, an API that should always be stateless. Now it doesn't matter which kind of protocol is used for implementing the actual RPC calls (REST is a kind of RPC mechanism). Packing or protocol specific state is handled by the protocol implementation, but who says that this has to affect the API? It may be that you instead want to create a stateful API of course, for whatever reason, but just because that may be the case doesn't mean that being able to reuse code for a stateless API is of no value.

It would be very disappointing to have the rest module limited by such consideration

There are multiple possible approaches to this and the functionality doesn't have to be limited at all, just represented differently. The question is just how well each approach separates different concerns.

Regarding RoR, popularity simply doesn't matter for this. I can't judge how well its approach works in practice in terms of creating proper REST interfaces (it relies purely on convention, right?). But that it works for creating REST interfaces in general is not the question. The question is if there isn't an approach that works just as well (or better), but that doesn't sacrifice the separation of transport and implementation.

You mean something like: ...

Yeah that would be about it. It might take one or two more things to support passing more data to those methods (such as the server/client class instance) and to break hard dependencies from the type to the server or client code.

mihails-strasuns commented 9 years ago

So, finally got to it.

1) I agree with @Geod24 that current situation is sub-optimal and overly purist. As soon as you need to make authentication some sort of state becomes anavoidable and @before / @after screw REST client quite a bit. Not many projects out there really care for "true" RESTful designs.

2) At the same time the fact that current API implementation is completely decoupled from HTTP bits is incredibly valuable and was one of killer features for me. This allows for very elegant code reusage when generating output for different contexts (for example, both REST for AJAX queires and server-side generation for disabled JavaScript) and this must stay.

One approach I was thinking about is to make is to introduce additional higher level abstraction on top of existing REST implementation that could keep existing object as a private field but have access to HTTP request / response objects. It could also become intermediate step from vibe.web.rest to vibe.web.web for better code reusage / consistency.

I hope to find some time to sit down and brainstorm with @Geod24 on this topic next time we meet in Berlin :)

Geod24 commented 9 years ago

So we finally had a chat over some food (And i'm finally writting that down). Here's an extract. @Dicebot may patch some memory error ;)

interface IAPI {}
class CoreAPI : API {}
class HTTPAPI : Something {
 HTTPServerRequest req;
   HTTPServerResponse res;
      // Possibility to hijack REST methods
      // Possibility to have a "prepare" and "finalize" call respectively before and after each method
}

While this approach offers some niceness, I feel it plays against the stateless approach we currently have in the REST module. If you store the context in an object, it means you need a pool of object, which is not currently the case. We also risk some escaping of references, leading to more leaks, and abuse such as #425 .

One thing I want to clear front up is that having access to the context is not against REST's principles. It does makes it easier to violate them though.

So, bearing in mind we want to keep the "single definition for client and server" part, as well as the "stateless" approach, but we can't put client/server specific data on the interface. We however want to keep as much information about the 'exchange' as possible in the interface.

So, taking a step back to look at the whole picture, the interface is nothing but an IDL (ironically). It works quite well for us since most of the features of REST interface are expressible in D interface.

The fact that the interface is just an IDL is the reason why I proposed to update the generator to allow some on-the-fly codegen. This change is, again, purely additive, and a last resort option. We should add enough abstraction to make it useless in 99% of use case, but we need to cover 100% of use case, and this feature will cover it. This discussion actually gave me another idea to fill a current gap in the interface (see below). Last but not least, the REST module should be a building block. I don't see it as a high-level functionality because it doesn't provide what you need to easily build a REST API, such as, once again, authentication. Almost nobody wants to implement authentication themselve, and it's a processus that's error prone. Other thing like collections/objects handling might be built on top instead of expanding the scope.

So, revised plan:

Btw, at some point we'll need to update documentation :fearful:

Geod24 commented 9 years ago

If there is any one interested in contributing to this module, or just in the state of things, here's my current "TODO" list (very, very broad). I tried to order it by priority (or what's blocking what).

Design

Grunt work

Compiler tasks (nice but optional)

Testing