Skip to content
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.

Sign up for GitHub

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

Jump to bottom

added Json handling and @contentType UDA #684

Merged
merged 2 commits into from
Jun 11, 2014
Merged

added Json handling and @contentType UDA #684

merged 2 commits into from
Jun 11, 2014

Conversation

UplinkCoder
Copy link
Contributor

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

@UplinkCoder UplinkCoder mentioned this pull request Jun 7, 2014
Closed
@UplinkCoder
Copy link
Contributor Author

@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
Copy link
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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@s-ludwig
Copy link
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.

@UplinkCoder
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
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).

@UplinkCoder
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@s-ludwig
Copy link
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.

@UplinkCoder
Copy link
Contributor Author

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

@s-ludwig
Copy link
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.

@UplinkCoder
Copy link
Contributor Author

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

@s-ludwig
Copy link
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).

@s-ludwig
Copy link
Member

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
@s-ludwig
Merge pull request #684 from UplinkCoder/patch-web.web
131d61c 
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
@UplinkCoder UplinkCoder deleted the patch-web.web branch June 12, 2014 05:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@UplinkCoder @s-ludwig