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

add servlet support #314

Closed
wants to merge 2 commits into from
Closed

Conversation

jiacai2050
Copy link

@jiacai2050 jiacai2050 commented Nov 15, 2017

ref: weavejester/compojure#176

Add a new option to define servlet mapping

{:servlet-mapping {"/hello" (HelloServlet.)}}

@weavejester
Copy link
Member

I think this would be better tested in a third-party library first.

@jiacai2050
Copy link
Author

jiacai2050 commented Nov 15, 2017

This adapter has been used for almost two years in my company, it use the same tricks:

If requests are not matched against servlet contexts, they are passed to the original handler.

@weavejester
Copy link
Member

What's your reasoning for including this change in the Ring Jetty adapter itself if there is a third-party adapter already available? Do you believe it's a common enough use-case to include an option for?

Have you considered other solutions, such as converting a Java servlet into a Ring handler?

@jiacai2050
Copy link
Author

jiacai2050 commented Nov 16, 2017

Well, the adapter I referred is mainly to be used with hystrix, what if you have another servlet? Maybe write another adapter. And I have done this when using druid's StatViewServlet.

Converting a Java servlet into a Ring handler is indeed another solution, and I have done this several times when port a Java web application into a Ring-based one. I'm not sure if sevlet-mapping integration is common for others, I just want this feature now and then.
I hope Ring can support servlet out of box, no more no less.

@weavejester
Copy link
Member

Could this be achieved instead with the existing :configurator option? Or a function that converts a servlet into a handler function?

@jiacai2050
Copy link
Author

configurator option doesn't work for this. Since the adapter has already defined a handler, if set to another, then the original one will be lost.

I can't image what a general function converting servlet into a handler look like, after all it's servlet that does the real business, turning a request map to a HttpServletRequest is reasonable, but not otherwise.

@weavejester
Copy link
Member

One possibility is that we provide an option to allow the adapter to include the servlet keys that are already added when a handler is compiled into a servlet. Given this, converting a servlet into a handler becomes more straightforward.

I'm leery about adding a :servlet-mapping key to the adapter, as it provides a "back door" that exists outside the adapter specification. I'd prefer to have this tested in a third-party adapter first - one more general purpose than ring-jetty-hystrix-adapter - than add this as an option to Ring's official adapter without fully thinking through the consequences.

@jiacai2050
Copy link
Author

OK, I get your point.
I'm fine with the solution. I will contribute another adapter based on my code, and let the time tell.
Thanks for the hard work you have done with Ring.

@jiacai2050 jiacai2050 closed this Nov 16, 2017
@jiacai2050
Copy link
Author

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants