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

DropWizard 0.8 upgrade using GuiceIntoHK2Bridge #47

Closed
wants to merge 0 commits into from
Closed

DropWizard 0.8 upgrade using GuiceIntoHK2Bridge #47

wants to merge 0 commits into from

Conversation

mrserverless
Copy link
Contributor

To address issue #37 I've updated the code to incoroprate Guice/HK2 Bridge. A quick summary:

  • removed jersey.spi.inject references and related classes such as GuiceComponentProviderFactory, JerseyContainerModule
  • GuiceContainer is instantiated by injection instead of new GuiceContainer()
  • GuiceContainer calls HK2 bridge during init()
  • wrote tests and always use javax.inject.Inject annotations.

I noticed there is pull request #45 which uses jersey2-guice. It would be interesting to compare the two different approaches.

@mrserverless
Copy link
Contributor Author

I wrote some tests to cover AutoConfig. In the process of testing, found out that dropwizard 0.8.x Tasks require a mandatory name in the constructor. I resolved this by adding a @Named injection in the Task constructor. Reflected this in my test cases and updated README.md

@jontejj
Copy link

jontejj commented Jan 19, 2015

IMO we should go with this one because it's better tested. https://github.com/stickfigure/gwizard choose jersey2-guice, don't know why but perhaps a discussion with them would help in taking a decision?

@jontejj
Copy link

jontejj commented Jan 19, 2015

From their readme:
"Jersey1 worked great with Guice. For Jersey2, the team wrote their own DI framework from scratch (HK2) and littered it with global state, config files in META-INF/service, and other JavaEE-isms that make my skin crawl. In the year and a half that Jersey2 has been released, nobody has managed to make it play 100% nicely with Guice. The most valiant attempt uses reflection to punch values into private final fields in HK2's static globals. It will likely break in future point releases of Jersey.

If I sound bitter writing this, it's because I wasted a stupid amount of time getting it to work. By comparison, RESTEasy comes pre-baked with Guice integration that worked almost immediately."

@mrserverless
Copy link
Contributor Author

@jontejj under the hood the jersey2-guice library would be doing similar things as HK2 guice-bridge, ie linking ServiceLocator to Guice Injector etc. I suppose one difference would be that jersey2-guice is specifically written for Jersey2 where as the guice-bridge is more generic.

Personally I don't like HK2 either. But would prefer using HK2 provided tools directly rather than going through 3rd party code, potentially less layers of support when issues come up. I agree with the gwizard author in that: the jersey2-guice adapter (used by this module) may break with future Jersey point releases.

We've used HK2 guice-bridge for our Jersey2 GAE app in prod environment for the second half of 2014. We didn't come across any issues, which is why I'm quite confident in migrating it over to DW. Also reflected by the tests.

I'll upgrade this pull request to use 0.8.0-rc2 soon. If anyone wishes to go for a test run, the forked artifact can be accessed form our company bintray: https://bintray.com/trunkplatform/dropwizard-modules/trunkplatform%3Adropwizard-guice/view

@jontejj
Copy link

jontejj commented Jan 20, 2015

We'll definitely give that a go!
On Jan 20, 2015 12:19 AM, "Yun Zhi Lin" [email protected] wrote:

@jontejj https://github.com/jontejj under the hood the jersey2-guice
library would be doing similar things as HK2 guice-bridge, ie linking
ServiceLocator to Guice Injector etc. I suppose one difference would be
that jersey2-guice is specifically written for Jersey2 where as the
guice-bridge is more generic.

Personally I don't like HK2 either. But would prefer using HK2 provided
tools directly rather than going through 3rd party code, potentially less
layers of support when issues come up. I agree with the gwizard author in
that: the jersey2-guice adapter (used by this module) may break with
future Jersey point releases.

We've used HK2 guice-bridge for our Jersey2 GAE app in prod environment
for the second half of 2014. We didn't come across any issues, which is why
I'm quite confident in migrating it over to DW. Also reflected by the tests.

I'll upgrade this pull request to use 0.8.0-rc2 soon. If anyone wishes to
go for a test run, the forked artifact can be accessed form our company
bintray:
https://bintray.com/trunkplatform/dropwizard-modules/trunkplatform%3Adropwizard-guice/view


Reply to this email directly or view it on GitHub
#47 (comment)
.

@jontejj
Copy link

jontejj commented Jan 20, 2015

I tried your 0.8.0-rc2-201501201620 release, During the initialization phase in dropwizard I get "No implementation for javax.ws.rs.core.HttpHeaders was bound." https://github.com/HubSpot/dropwizard-guice/blob/f61ffd2b71857733421d41d4981a59a3289abaa1/src/main/java/com/hubspot/dropwizard/guice/JerseyContainerModule.java seem to be removed?

autoConfig.run(environment, injector);

//then
Set<Class<?>> components = environment.jersey().getResourceConfig().getClasses();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps verify that the instance is creatable with guice?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean to verify healthcheck can be created by guice and that the registered health check is also same as the instance created by guice? Can do, will take a look tmr.

@oillio
Copy link
Contributor

oillio commented Jan 20, 2015

I wrote the jersey2-guice version.
Nice job @yunspace, the test suite looks nice.

I think either solution will work fine.
One major advantage of jersey2-guice is that it handles Guice JIT bindings.

I also wrote the beginnings of a test suite. It is located in my other PR (along with some other goodies I have been working on): #46
The suite isn't as extensive or as clean as the one by @yunspace and focuses more on my new features. Also, it is built as integration tests rather than unit tests.

@mrserverless
Copy link
Contributor Author

Thanks for the compliments @oillio, your jersey2-guice PR inspired me to get this one in. The lack of support for JIT bindings in guide-bridge is painful indeed, if jersey2-guice handles it that's pretty awesome.

Yes I agree either solution will work fine. Perhaps it's wise to offer the option of letting the user choose which implementation to use. Ie having 2 subprojects, similar to dropwizard's freemarker vs mustache modules. Of course we would need a thorough test suite to ensure consistent functionality across the two.

@mrserverless
Copy link
Contributor Author

I'm closing this PR for now. Until I find time to add support for JIT bindings and various other features, I'll be leveraging the jersey2-guice implementation (#45) which offers most of the functionalities my project currently need.

I've squashed the commits and did some clean up in my code base. It's available at https://github.com/yunspace/dropwizard-guice/tree/guice-bridge if anyone is interested.

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.

3 participants