Tectu / malloy

A cross-platform C++20 library providing embeddable server & client components for HTTP and WebSocket.
BSD 3-Clause "New" or "Revised" License
66 stars 8 forks source link

Non string_body requests #17

Closed 0x00002a closed 3 years ago

0x00002a commented 3 years ago

This one is a good deal more complex than #13 imo. I've made a start on a possible implementation but its early stages and I might end up scrapping it. The main issue is that request bodies sometimes need some setup before being used (e.g. file_body needs opening before reading out). How that is actually exposed as a configuration point is one of the sticking points. My ideas on how this can be done outside of handling request setup:

My current thoughts on the customisation of request setup:

This should allow extensive customisation while keeping the interface simple for the usual use-case. I am concerned about the Fields option in header though, this currently does not leave room for that to be custom. Not sure if its in-scope for this library (I've never used them myself) but still worth mentioning. Also currently this does not include websockets which I think also need something similar. Possibly could use the same logic and just pass in request.body() which would happen to be std::string by default.

As I said at the beginning, I have an implementation along these lines (not quite complete) and I'd appreciate thoughts and feedback on this

0x00002a commented 3 years ago

If we went the route of optionally passing a struct with overloaded call operator, instead of a function (as suggested above for setting up requests), it could also be used as the customisation point for #4.

0x00002a commented 3 years ago

Yet another idea with this. It could allow easy integration with stuff like nlohmann::json too. The appropriate mimetypes could be set automatically, it would use string_body always, and would transform it to a nlohamnn::json before being passed to the handler. It would even be quite easy for users to write instead of being part of the library.

Tectu commented 3 years ago

This is certainly a step in the right direction. All of the good stuff that is on the ToDo list for this library. I am very glad that you dropped by :p

Do you already have a proof-of-concept of this?

0x00002a commented 3 years ago

Not really, I've got early stage stuff in feat-custom-requests but it doesn't even compile yet and the commit history is a mess. Should have some time to work on it today/this evening though

Tectu commented 3 years ago

The commit history shouldn't be a problem. You can use the squash feature of git. It allows to "condense" multiple commits into one.

This can also be done after you created a PR when I merge it into the main library - I already did that with your previous PR #14, see 916042427c491381ed6732b566e68912ccb2b6ac.

0x00002a commented 3 years ago

So I'm most of the way there now with http. Wondering how to do the websocket side of things. Honestly I quite like the idea of scraping the current interface and going with a concept that has handle_message(...) or similar. This would also allow it to define its own buffer types and writing interface similar to how bodies work for beast::http. It took me a bit to work out how the current interface works, conceptually I think it makes more sense to have an object used for the websocket session (which takes the send in its ctor or something) since its a two-way connection that lives on indefinetly (rather than the one-shot nature of http, for which the handler functions make sense imo).

Interested in your thoughts on this. Especially on how we can do the buffers since I have very little experience with the websocket side of beast.

Tectu commented 3 years ago

The current implementation of the websocket interface is far from what I'd like it to be. Unfortunately this was created under some pressure of an up-stream project that used malloy and desperately needed WebSocket server capabilities.

I can't get into details right now regarding what I think would be best. However, having examined your contributions so far I think that you are very capable of providing a decent, well implemented solution design :p

Especially on how we can do the buffers since I have very little experience with the websocket side of beast.

There's a chance that I am misunderstanding. We should be able to re-use the various buffer implementations provided by beast. My personal take on this is that we alias any beast type that is exposed to the public API for consistency reason. While we are proud to base this library on beast we don't want to enforce the user of understanding the various types etc. used by beast. We're already doing this for types such as http::status, http::method and so on. This also makes it easier to maintain if future decision would lead to changing the implementation to something different.

0x00002a commented 3 years ago

Yeah I'm hoping to reuse the http body types from beast. But I'm not sure how that would actually work, and whether there are any issues there with assumptions about httpness (I'd guess not but like I said, not much experience :p).

Also I feel like we should give the option of completely handing off to a customsation point (which gets given the socket), once the routing part is done, since this libraries job is routing and not websocket connection management (at least thats my understanding, if I'm wrong here please correct me). It would mean exposing quite a few implementation details of the library though so I'm not sure if its a good idea.

However, having examined your contributions so far I think that you are very capable of providing a decent, well implemented solution design :p

Thank you :)

Tectu commented 3 years ago

Yeah I'm hoping to reuse the http body types from beast. But I'm not sure how that would actually work, and whether there are any issues there with assumptions about httpness (I'd guess not but like I said, not much experience :p).

I might be completely wrong here (especially as I know HTTP better than WS too) but I don't think that that is possible - or wise to do. As far as I can tell, you just want to provide a binary stream (eg. byte buffers on each end - but then full duplex) and build the application on top of that. If you can find some evidence of this in specifications or other decent sources (like not your typical "I'm a web dev"-blog posts) I'd be highly interested in reading that.

Also I feel like we should give the option of completely handing off to a customsation point (which gets given the socket), once the routing part is done, since this libraries job is routing and not websocket connection management (at least thats my understanding, if I'm wrong here please correct me). It would mean exposing quite a few implementation details of the library though so I'm not sure if its a good idea.

In general I agree. For example, currently it seems difficult to close a websocket connection from the server side through an enpoint - exactly because of the reason you mentioned: The handler has no access to the connection itself and can only receive bytes and send bytes. This is actually an issue that one of my applications currently still has. It would be neat if we could only expose some kind of handler rather than the connection itself. The connection will keep itself alive anyway until it is explicitly closed. So maybe we can define a handler interface for the typical ioctl tasks.

Thank you :)

Nah, thank you ;-)