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.

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

Non string_body requests #17

Closed
0x00002a opened this issue Jun 22, 2021 · 9 comments · Fixed by #28
Closed

Non string_body requests #17

0x00002a opened this issue Jun 22, 2021 · 9 comments · Fixed by #28
Labels
enhancement New feature or request
Milestone

Comments

@0x00002a
Copy link
Contributor

0x00002a commented Jun 22, 2021

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:

  • Only parse the header when we first receive the message. matches for the endpoints can use this instead.
  • Template request in the same manner as response
  • Wait to parse the body until handle in endpoint_http

My current thoughts on the customisation of request setup:

  • Have the Func passed to add in router be optionally something which has a function which takes in a header and returns a variant of requests for it.
  • If it doesn't have this then just default construct the body (and require that it is default constructable at the add for nice errors)
  • Have connection pass down a "generator" object instead of a request
  • The generator object can give the header of the request and also the body via a templated rvalue method (templated on the type of the body), which reads the remainder of the message from the stream
  • The generator would have to be internal to connection since the stream type differs depending on Derived
  • To cross the endpoint_http interface boundry the generator will be a variant based on connection_plain and connection_tls
  • endpoint_http_regex will have the pile of std::visits needed to sort out the message in handle
  • endpoint_http_files will be removed and implemented in terms of endpoint_http_regex in router

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

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

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
Copy link
Owner

Tectu commented Jun 24, 2021

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

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
Copy link
Owner

Tectu commented Jun 25, 2021

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 9160424.

@0x00002a
Copy link
Contributor Author

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
Copy link
Owner

Tectu commented Jun 25, 2021

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

0x00002a commented Jun 25, 2021

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
Copy link
Owner

Tectu commented Jun 25, 2021

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 ;-)

@Tectu Tectu mentioned this issue Jun 28, 2021
@Tectu Tectu added the enhancement New feature or request label Jun 28, 2021
@Tectu Tectu added this to the v0.1 milestone Jun 28, 2021
@Tectu Tectu closed this as completed in #28 Jun 29, 2021
This was referenced Jun 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants