New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

added Json handling and @contentType UDA #684

Merged
merged 2 commits into from Jun 11, 2014

Conversation

Projects
None yet
2 participants
@UplinkCoder
Contributor

UplinkCoder commented Jun 7, 2014

this change adds the possibility to serve arbitrary data (if convertable to ubyte[]) with a custom contentType

@UplinkCoder

This comment has been minimized.

Show comment
Hide comment
@UplinkCoder

UplinkCoder Jun 9, 2014

Contributor

@s-ludwig what do you think about this API addition ?
is it more fit for rest ?
or can the restmodule be depreacatd for web.web ?

Contributor

UplinkCoder commented Jun 9, 2014

@s-ludwig what do you think about this API addition ?
is it more fit for rest ?
or can the restmodule be depreacatd for web.web ?

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Jun 9, 2014

Member

The REST and web modules have different scopes. The REST module in particular is meant to provide a full RPC mechanism, if needed, and is supposed to enforce a stateless protocol. The web module on the other hand is strongly related to session and HTML based interfaces. In theory, JSON seems to fit more with the REST module, but of course there are many use cases for non-REST JSON, too.

The functionality itself is fine, IMO. I just have a few thoughts left to be sorted out:

  • Duplicate functionality, can already be achieved by
    • adding a HTTPServerResponse parameter and calling writeJsonBody
    • using a OutputStream parameter and using serializeToJson (except for the content-type)
  • Return value vs. parameter: the latter would allow to write the response and then do some other computations, resulting in a lower latency
  • Generality: Would be trivial to extend, but for efficiency and type safety it would be desirable to allow arbitrary return types and use serializeToJson on them to directly serialize to the wire
  • Forward compatibility: Maybe there is another use case for the return value that may be forever lost, if the return value is already in use this way (I don't have a concrete example for this right now, but maybe something with WebSockets or similar?)
Member

s-ludwig commented Jun 9, 2014

The REST and web modules have different scopes. The REST module in particular is meant to provide a full RPC mechanism, if needed, and is supposed to enforce a stateless protocol. The web module on the other hand is strongly related to session and HTML based interfaces. In theory, JSON seems to fit more with the REST module, but of course there are many use cases for non-REST JSON, too.

The functionality itself is fine, IMO. I just have a few thoughts left to be sorted out:

  • Duplicate functionality, can already be achieved by
    • adding a HTTPServerResponse parameter and calling writeJsonBody
    • using a OutputStream parameter and using serializeToJson (except for the content-type)
  • Return value vs. parameter: the latter would allow to write the response and then do some other computations, resulting in a lower latency
  • Generality: Would be trivial to extend, but for efficiency and type safety it would be desirable to allow arbitrary return types and use serializeToJson on them to directly serialize to the wire
  • Forward compatibility: Maybe there is another use case for the return value that may be forever lost, if the return value is already in use this way (I don't have a concrete example for this right now, but maybe something with WebSockets or similar?)
@UplinkCoder

This comment has been minimized.

Show comment
Hide comment
@UplinkCoder

UplinkCoder Jun 9, 2014

Contributor

I think the lost return value can be avoided with a simple UDA
@keepReturn or somehing like that.
I think if you want to return Json you will serialize it in your function
but of course one could add another UDA
EDIT:
adding HTTPServerResponse would be contrary to the idea of web.web
my first implementation of this did export writeBody of the requestContext.
But using Json as a return value of a function is much easier and just works out of the box.

Contributor

UplinkCoder commented Jun 9, 2014

I think the lost return value can be avoided with a simple UDA
@keepReturn or somehing like that.
I think if you want to return Json you will serialize it in your function
but of course one could add another UDA
EDIT:
adding HTTPServerResponse would be contrary to the idea of web.web
my first implementation of this did export writeBody of the requestContext.
But using Json as a return value of a function is much easier and just works out of the box.

@UplinkCoder

This comment has been minimized.

Show comment
Hide comment
@UplinkCoder

UplinkCoder Jun 10, 2014

Contributor

I added the possibity to use HTTPRequestDelegateas Methods It seems to integrate with the rest,
this enables .Forward Compatibility and maximal control

Contributor

UplinkCoder commented Jun 10, 2014

I added the possibity to use HTTPRequestDelegateas Methods It seems to integrate with the rest,
this enables .Forward Compatibility and maximal control

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Jun 10, 2014

Member

Do you have a use case for returning a HTTPServerRequestDelegate? The code doesn't look like it would work (ParameterIdentifierTuple!overload would pass the names of the function parameters as function parameters?).

Regarding the return value and @keepReturn or any other UDA, yes that would of course be possible, but I just want to make sure that the default behavior is the right one. One of the goals is to have as few annotations as possible. ...but the return value approach generally seems to be the right one, so this should be fine. I'd just generalize it and add some constraints on the value of @contentType at some point.

Member

s-ludwig commented Jun 10, 2014

Do you have a use case for returning a HTTPServerRequestDelegate? The code doesn't look like it would work (ParameterIdentifierTuple!overload would pass the names of the function parameters as function parameters?).

Regarding the return value and @keepReturn or any other UDA, yes that would of course be possible, but I just want to make sure that the default behavior is the right one. One of the goals is to have as few annotations as possible. ...but the return value approach generally seems to be the right one, so this should be fine. I'd just generalize it and add some constraints on the value of @contentType at some point.

@UplinkCoder

This comment has been minimized.

Show comment
Hide comment
@UplinkCoder

UplinkCoder Jun 10, 2014

Contributor

HTTPServerRequestDelegate is what handleWebsockets returns.
and because this delegate has accses to req and res of the RequestContextit can be used to work around the default behavior of web.web should the need arise.
I have an almost running vibenots with web.web.
Though, it fails just like before because of some fiber-ownership issues ...

Contributor

UplinkCoder commented Jun 10, 2014

HTTPServerRequestDelegate is what handleWebsockets returns.
and because this delegate has accses to req and res of the RequestContextit can be used to work around the default behavior of web.web should the need arise.
I have an almost running vibenots with web.web.
Though, it fails just like before because of some fiber-ownership issues ...

@UplinkCoder

This comment has been minimized.

Show comment
Hide comment
@UplinkCoder

UplinkCoder Jun 10, 2014

Contributor

It is used like this

 auto getHello()
        {

                void writeHello (HTTPServerRequest req, HTTPServerResponse res) {
                        res.writeBody("<html><body><h1> Hello World </h1></body></html>","text/html");
                }
                return &writeHello;
        }
Contributor

UplinkCoder commented Jun 10, 2014

It is used like this

 auto getHello()
        {

                void writeHello (HTTPServerRequest req, HTTPServerResponse res) {
                        res.writeBody("<html><body><h1> Hello World </h1></body></html>","text/html");
                }
                return &writeHello;
        }
@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Jun 10, 2014

Member

That doesn't seem to buy much over

void getHello(HTTPServerRequest req, HTTPServerResponse res)
{
    void writeHello(HTTPServerRequest req, HTTPServerResponse res) {
        res.writeBody("<html><body><h1> Hello World </h1></body></html>","text/html");
    }
    writeHello(req, res);
}

I'd rather add specific support for web sockets than to litter the API with such special cases to achieve something with a handful of less characters to type. It will make everything harder to understand (the general web.web concept is already quite non-obvious due to the declarative nature, I think it should be kept as small as possible).

Member

s-ludwig commented Jun 10, 2014

That doesn't seem to buy much over

void getHello(HTTPServerRequest req, HTTPServerResponse res)
{
    void writeHello(HTTPServerRequest req, HTTPServerResponse res) {
        res.writeBody("<html><body><h1> Hello World </h1></body></html>","text/html");
    }
    writeHello(req, res);
}

I'd rather add specific support for web sockets than to litter the API with such special cases to achieve something with a handful of less characters to type. It will make everything harder to understand (the general web.web concept is already quite non-obvious due to the declarative nature, I think it should be kept as small as possible).

@UplinkCoder

This comment has been minimized.

Show comment
Hide comment
@UplinkCoder

UplinkCoder Jun 10, 2014

Contributor

IMO It does not really affect the API as such.
as normal user just sees the neat trick the his delegates are automaticly called

Contributor

UplinkCoder commented Jun 10, 2014

IMO It does not really affect the API as such.
as normal user just sees the neat trick the his delegates are automaticly called

@UplinkCoder

This comment has been minimized.

Show comment
Hide comment
@UplinkCoder

UplinkCoder Jun 10, 2014

Contributor

I don't where this is too complicated ... maybe I'm overlooking something here ..

Contributor

UplinkCoder commented Jun 10, 2014

I don't where this is too complicated ... maybe I'm overlooking something here ..

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Jun 10, 2014

Member

If you see code like that and don't know this special rule, you can just wonder what happens. It's not this particular feature, but if this becomes a kitchen sink of little mini-helpers and rules, then the source code of applications written with this can become impenetrable. It also facilitates bad code, such as:

auto getWS() {
    return handleWebSockets(...);
}

This will create a new heap delegate for every request, which may have considerable performance impact. So to support something like that, I'd rather support it in a way that forces an efficient approach.

Member

s-ludwig commented Jun 10, 2014

If you see code like that and don't know this special rule, you can just wonder what happens. It's not this particular feature, but if this becomes a kitchen sink of little mini-helpers and rules, then the source code of applications written with this can become impenetrable. It also facilitates bad code, such as:

auto getWS() {
    return handleWebSockets(...);
}

This will create a new heap delegate for every request, which may have considerable performance impact. So to support something like that, I'd rather support it in a way that forces an efficient approach.

@UplinkCoder

This comment has been minimized.

Show comment
Hide comment
@UplinkCoder

UplinkCoder Jun 10, 2014

Contributor

hmm I get the efficiency-argument.
this is indeed a problem ...
What do you propose then ?

Contributor

UplinkCoder commented Jun 10, 2014

hmm I get the efficiency-argument.
this is indeed a problem ...
What do you propose then ?

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Jun 10, 2014

Member

The most intuitive way to me seems to be supporting the same signature as WebSocketHandshakeDelegate:

void getWS(scope WebSocket ws)
{
    // ...
}

The web interface generator can then pass that to handleWebSockets behind the scenes.

Member

s-ludwig commented Jun 10, 2014

The most intuitive way to me seems to be supporting the same signature as WebSocketHandshakeDelegate:

void getWS(scope WebSocket ws)
{
    // ...
}

The web interface generator can then pass that to handleWebSockets behind the scenes.

@UplinkCoder

This comment has been minimized.

Show comment
Hide comment
@UplinkCoder

UplinkCoder Jun 10, 2014

Contributor

hmm,
I'll implement it tomarrow alright ?
what about the special handling of Json returing methods
shall I commit a rebased PR ?

Contributor

UplinkCoder commented Jun 10, 2014

hmm,
I'll implement it tomarrow alright ?
what about the special handling of Json returing methods
shall I commit a rebased PR ?

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Jun 10, 2014

Member

Would be very welcome, if you implement that. A rebase of your branch would also be good (ideally skipping the two HTTPRequestDelegate commits). No need to open a new PR (it would have been better to have one PR per feature, but let's not needlessly complicate the process here).

Member

s-ludwig commented Jun 10, 2014

Would be very welcome, if you implement that. A rebase of your branch would also be good (ideally skipping the two HTTPRequestDelegate commits). No need to open a new PR (it would have been better to have one PR per feature, but let's not needlessly complicate the process here).

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Jun 11, 2014

Member

Thanks for rebase. I'll merge now, so that the WS additions are separate.

Member

s-ludwig commented Jun 11, 2014

Thanks for rebase. I'll merge now, so that the WS additions are separate.

s-ludwig added a commit that referenced this pull request Jun 11, 2014

Merge pull request #684 from UplinkCoder/patch-web.web
Add Json handling and @contentType UDA + an alias for WebSocket handler delegates.

@s-ludwig s-ludwig merged commit 131d61c into vibe-d:master Jun 11, 2014

1 check failed

continuous-integration/travis-ci The Travis CI build failed
Details

@UplinkCoder UplinkCoder deleted the UplinkCoder:patch-web.web branch Jun 12, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment