puniverse / comsat

Fibers and actors for web development
docs.paralleluniverse.co/comsat
Other
598 stars 103 forks source link

[webactors] Provide mean to access underlying servlet request (or other implementation) #76

Closed victornoel closed 7 years ago

victornoel commented 7 years ago

Hi,

When receiving an HttpRequest with a WebActor, it is not possible to access the underlying HttpServletRequest (or at least I didn't find a way to do so :) if we are sure the actor is running on a servlet. My personal need is to be able to access the current HttpSession (because it contains authentication information). Well, I actually need the HttpServletRequest itself because I'm using a library that needs it to access the session as well as the request parameters.

I was expecting to be able to simply cast it to ServletHttpRequest and access the request parameter but the class is not visible and the parameter not accessible.

Would it be possible to change that situation? I guess this must be well thought since you may not want to expose too much of the internal APIs, but if you have a clear idea of how to do that, I can provide a PR :)

Thanks!

pron commented 7 years ago

Hmm, this may require some thought. We can't have any types exposed via the API that require a specific dependency which may not exist, and the underlying engine may be different anyway. I'm open to suggestions for an API that returns an Object, which meaning depends on the implementation.

victornoel commented 7 years ago

Yep, or at least let a user of the library cast the HttpRequest to one of its underlying type? In that case HttpServletRequest itself can expose HttpServletRequest and others!

No need for a generic API I think, because if at one point there will be a cast, it may be better to simply cast the HttpRequest itself… but then, HttpServletRequest becomes itself part of the actors-servlet API (but not the actors-api).

victornoel commented 7 years ago

To be totally clear about what I mean, the idea would simply to make the class co.paralleluniverse.comsat.webactors.servlet.ServletHttpRequest public and add getters for HttpServletRequest request and HttpServletResponse response. That's all :)

pron commented 7 years ago

I'm not opposed, but it would require some refactoring. For one, the class is currently named HttpRequestWrapper (which would need to be changed).

victornoel commented 7 years ago

right, I didn't see it has been renamed apparently since 0.7.0 :) What is wrong with the current name by the way? Just that it doesn't seem a very public name?

So, would you like me to propose a PR that does that (and rename HttpRequestWrapper back to HttpServletRequest)?

pron commented 7 years ago

Yeah, it's a bad name, and yes a PR would be nice. Let's make it ServletHttpRequest, just to keep the implementation name first.

victornoel commented 7 years ago

I submitted #78! I also changed the name for the other implementations and made some choices as to what is visible publicly for each.