ceylon-sdk icon indicating copy to clipboard operation
ceylon-sdk copied to clipboard

Refactor ceylon.http modules

Open luolong opened this issue 8 years ago • 8 comments

Current implementation and design of ceylon.http.* module API is woefully inconsistent and lacks many useful features.

This issue tries to enumerate and track those:

  • [ ] Inconsistent Request Uri API between ceylon.http.server:Request and ceyon.http.client:Request
  • [ ] Inconsistent handling of headers between ceylon.http.server:Request and ceyon.http.client:Request
  • [ ] ceylon.http.server:Request has no way of enumerating all headers of a request. (#572)
  • [ ] ceylon.http.server:Request has no way of reading the body of the request as a stream of characters (or bytes)
  • [ ] ceyon.http.client:Request has no way of writing response body as a stream of characters (or bytes)

luolong avatar Nov 03 '16 21:11 luolong

Want any help with this?

xkr47 avatar Nov 04 '16 18:11 xkr47

yes @xkr47 that would probably be necessary.

Let me just outline the general plan in the following comments. The modest beginnings are already in the branch http-api-refactor.

luolong avatar Nov 04 '16 22:11 luolong

Inconsistent Request Uri API between Server and Client requests

Currently the situation is that while client Request uses URI for request URI, path and query arguments, the server Request has many separate attributes, semantics of which is somewhat mixed/unclear.

Here is the set of uri related attributes of the ceylon.http.client:Request:

shared class Request(...) {
    ...
    shared Uri uri;
    shared variable Integer port = 80;
    shared variable Boolean ssl = false;
    shared String host;
    ...
}

While client request has uri attribute with proper type, it also has few additional attributes (like host and port) realated to URI that get initialized to the values from Uri, but can be later overridden, but the URI will not reflect that change.

This is unexpected -- if setting host and port of a request issuch a common operation, both attributes should have getter and setter implementation manipulating the Uri. This way Uri will be kept in sync with request host, port and ssh attribute values.

To compare, this is the uri related parts of the ceylon.http.server:Request:

shared interface Request {
    ...
    shared formal String? queryParameter(String name);
    shared formal String[] queryParameters(String name);
    shared formal String scheme;
    shared formal String uri;
    shared formal String path;
    shared formal String relativePath;
    shared formal String queryString;
    ...
}    

(I might have missed few attributes, but the gist is here)

The server request has uri attribute of type String. While it is perfectly acceptable type for representing a request uri, it also creates a need for more than one attribute representing a portion of the URI:

  • scheme
  • path (and relativePath)
  • queryString

Also, the two methods for accessing query parameters by name -- one returning possibly empty list of strings and another returning a nullable string. I believe that the one which returns nullable string is superfluous, because empty list is equivalent to null return value in case of a missing parameter and there is no reason to make user of the API choose between one or the other.

The deeper trouble here is though that these two Request types expose completely and unexpectedly different API for dealing with Uri related data.

Using common base interface, where uri attribute is of the proper type Uri should obviate the need for extra attributes and help to bring those two APIs closer together.

luolong avatar Nov 04 '16 22:11 luolong

Sorry for some offtopic, but do you have plans to implement async http client in sdk (may be create issue for that)?

olegnekludov avatar Nov 05 '16 07:11 olegnekludov

I believe that the one which returns nullable string is superfluous

I disagree. Most of the time, I know that my client is sending a single value (or my backend is expecting a single value), so I'd rather use

if (exists param = req.queryParameter("foo"))

than

if (exists param = req.queryParameters("foo").first)

there is no reason to make user of the API choose between one or the other

Users should know what kind of data they are expecting (single or multi values), so the choice is easy to make.

bjansen avatar Nov 05 '16 07:11 bjansen

do you have plans to implement async http client in sdk (may be create issue for that)?

@olegnekludov Don't know about plans, but if you want async http now, there is decent vert.x support in Ceylon through io.vertx.ceylon.core .. I have implemented a reverse http proxy with it (https://github.com/xkr47/vhostproxy4), and use it as a front for my 10+ domains I run at home. I'm also planning on rewriting some of my current java-servlet services on top of vert.x as well.

xkr47 avatar Nov 05 '16 08:11 xkr47

@bjansen

What about:

if (nonempty params = req.queryParameters("foo")) {
    value param = params.first;
}

In any case -- if and when the server request has proper shared Uri uri attribute, the whole point will become moot, as Uri already has accessor methods for query parameters

luolong avatar Nov 07 '16 21:11 luolong

@olegnekludov

Sorry for some offtopic, but do you have plans to implement async http client in sdk (may be create issue for that)?

ceylon.http.server already has async endpoint implementation. It also has async read methods from server request and async write methods to server response.

I've not really made any experiments and I can't at this point in time vouch for what is the difference between sync write and async write methods, but I know they are there if one needs them.

luolong avatar Nov 07 '16 21:11 luolong