Injectable Modules, Named configuration data, and Injected Commands #46

oillio · 2015-01-13T02:09:01Z

We have made a number of additions to dropwizard-guice.
I'm thinking the upgrade to Dropwizard 0.8 might be a good chance to see if any of these additions would be of interest to the project (the attached code is currently based off my 0.8 update PR).

The additions are (usage descriptions can be found in the updated readme):

Injectable Modules Allows for singletons with config data, etc. This is accomplished through double injection. The InitModules are initialized first, at Dropwizard initialization. During the Dropwizard run phase, after the environment and config data is added, the rest of the modules are injected and a child injector is created from these modules.�
Named config data Each field in the Configuration object is bound and named based on the field name (done recursively for any sub-config objects found in scope). This allows you to inject a String with @nAmed("defaultPerson.name") instead of injecting the configuration object and doing config.getDefaultPerson().getName(). We have found this to be particularly useful when sharing Bundles between projects. The code using the config data doesn't have to take a dependency on the actual config object.
InjectedCommands A set of Command types that allow for full injection of config data and other modules. These were needed because Command objects must be initialized and passed to Dropwizard before the Configuration and Environment data exists.
Integration Tests The start of tests for these features and the rest of dropwizard-guice.

Altered GuiceBundle to allow setting of ServletContextListeners

…ting

…ting injector annotation just to be make it clear it's primarly used for configuring based on setting annotations?

support Shiro)

jontejj · 2015-02-14T11:07:11Z

Fantastic work @oillio and @yunspace. I found a bug in jersey2-guice that caused

@Qualifier
@BindingAnnotation

to not be injected into the jersey resources (Squarespace/jersey2-guice#22) properly. Hopefully they'll accept my PR (Squarespace/jersey2-guice#23) and then let's upgrade the dependency. I thought you should know as I first suspected that it was the order we initialize things in that was the culprit, but it wasn't!

jontejj · 2015-02-14T11:33:50Z

src/test/java/com/hubspot/dropwizard/guice/sample/command/SimpleCommand.java

+    }
+
+    @Run
+    public void run(@Named("defaultName") String name) {


Also inject HelloWorldConfiguration as a method parameter? Really neat feature:)

oillio · 2015-02-18T18:14:41Z

@jontejj Thanks. Let me know when jersey2-guice accepts your patch and I will rev the dependency version.

jontejj · 2015-02-19T00:16:56Z

Will do. I think he tried to merge my PR but got formatting issues. Soon...
On Feb 18, 2015 7:14 PM, "Dan Jasek" [email protected] wrote:

@jontejj https://github.com/jontejj Thanks. Let me know when
jersey2-guice accepts your patch and I will rev the dependency version.

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

oillio · 2015-02-20T23:01:28Z

@yunspace and @jontejj
I am getting tired of maintaining two branches and it looks like HubSpot has all but abandoned this project. I think Elias is no longer with HubSpot as well.

I am thinking about forking the project, registering it with Maven Central, and using this branch as the mainline. The three of us seem to be the most active users. So, a survey:

Do you agree that moving to a fork is waranted?

Do you like the improvements made in this branch, and do you agree this should be the mainline?

jhaber · 2015-02-20T23:09:42Z

We have definitely not abandoned this project! We have 200+ services in production using dropwizard-guice and have no plans to use something else. Unfortunately we are still on dropwizard 0.6, but I can try to get this and #45 merged into master and cut a new release

mrserverless · 2015-02-21T03:54:55Z

@oillio @jontejj @HiJon89 whilst I think staying on the mainline is good to leverage the existing community base, I do feel that it may be slightly overwhelming for HubSpot to support new features and issues related to dropwizard 0.8 when the bulk of their services are on 0.6.

Also #45 is practically a different code base using a third party library. The core GuiceContainer is replaced by HK2Linker and friends. All the tests written so far are also aimed at DW0.8 using Jersey2-Guice

There is no doubt the good folks at HubSpot have built a pretty stable foundation in dropwizard-guice. Rather than spreading support across versions 0.6, 0.7 and 0.8 which are all drastically different, perhaps now is a good time to make a fork and delegate DW 0.8 + Jersey2 support to teams who are actively building 0.8 services.

Just my 2 cents. But if we do need a fork, I actually have Continuous Delivery pipeline and unofficial BinTray repository set up for #45 (just need to change namespace etc):
https://snap-ci.com/Trunkplatform/dropwizard-guice/branch/master
https://bintray.com/trunkplatform/dropwizard-modules/dropwizard-jersey2-guice/view

oillio · 2015-02-21T05:49:53Z

Thanks @HiJon89. Sorry to jump the gun. After 3 moths without a commit or comment I was just getting a bit nervous.

I don't care who owns the project so long as we get timely releases and code reviews; I don't want to have to continue to maintain a private build and branch.

If you need help maintaining the 0.8 branch, I am happy to help.

mrserverless · 2015-02-21T06:39:13Z

I'm happy to help support 0.8 release also if this project will remain active and timely. Once the 0.8 changes are merged, I will re-point my CI and BinTray setup back to here.

eliast · 2015-02-22T01:00:19Z

We are still here. My office is a few blocks from HubSpot and I am using 0.7 but I could move easily to 0.8 since I only have two services in my new company so far.

I am away this weekend so let's get this sorted out Monday. I have been meaning to catch up on .8 and jersey 2.

—
Sent from Mailbox

On Fri, Feb 20, 2015 at 6:01 PM, Dan Jasek [email protected]
wrote:

@yunspace and @jontejj
I am getting tired of maintaining two branches and it looks like HubSpot has all but abandoned this project. I think Elias is no longer with HubSpot as well.
I am thinking about forking the project, registering it with Maven Central, and using this branch as the mainline. The three of us seem to be the most active users. So, a survey:
Do you agree that moving to a fork is waranted?

Do you like the improvements made in this branch, and do you agree this should be the mainline?

Reply to this email directly or view it on GitHub:
#46 (comment)

jhaber · 2015-02-23T18:39:52Z

@oillio I don't like the idea of binding configuration properties using @Named because of the chance of collisions with apps that might already be binding similarly named properties, thoughts on making a new annotation for this purpose such as @Configuration?

jontejj · 2015-02-23T21:20:15Z

Great suggestion Jonathan. Once my jersey2-guice fix is in we can even
support injecting it into the resource classes.
On Feb 23, 2015 7:39 PM, "Jonathan Haber" [email protected] wrote:

@oillio https://github.com/oillio I don't like the idea of binding
configuration properties using @nAmed because of the chance of collisions
with apps that might already be binding similarly named properties,
thoughts on making a new annotation for this purpose such as
@configuration?

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

oillio · 2015-02-24T00:13:05Z

That sounds good to me and should be easy to implement. I think it will also help me fix another annoyance with the current design.

Mind if I call it @config instead?

oillio · 2015-02-25T00:42:42Z

OK, I converted it to use a new annotation called @config.
I also added the ability to specify a root config class, to which the provided path will be relative.
So, for instance, if you have a configuration setup like this:

public class MyConfiguration extends Configuration {
    private FooConfig foo;
}

public class FooConfig {
    private String data;
}

You can get to data with either of these attributes:

@Config("foo.data")

@Config(FooConfig.class, "data")

This is particularly useful if FooConfig, and the classes using "data" are in an external library. This way, the library doesn't have to know where exactly FooConfig is located within the parent project's config object, just so long as it is there somewhere.

oillio · 2015-02-25T01:06:16Z

Fixed.

jhaber · 2015-02-25T17:08:53Z

I'm going to see if I can make dropwizard-guice extensible enough to support these use-cases easily (maybe with a new type of Bundle that can add Guice bindings?), I would rather keep the core library lean (maybe a separate module with common extensions?)

oillio · 2015-02-25T17:27:08Z

Sounds good. The @config stuff can be pretty cleanly moved to an independent Guice module. Do you want me to split that feature out into a separate pull request?

jhaber · 2015-02-25T19:11:11Z

Sure

jhaber · 2015-02-25T19:12:13Z

Also, I meant a separate Maven module for the common extensions

oillio · 2015-02-25T20:45:16Z

Yep. I was going to pull the @config stuff out of DropwizardEnvironmentModule into it's own Guice module. So, when you go to implement the extension hooks, it will be easier to refactor it out into the extension Maven module.

oillio · 2015-02-26T22:00:01Z

I pulled out the @config functionality from this PR and moved it to its own PR.

jhaber · 2015-02-26T22:02:45Z

Great, I'll take a look tomorrow

oillio · 2015-02-27T19:40:39Z

When pulling the @config code out I noticed that, with the double injection feature, we may not need the user to set the configurationClass.

Is there any scenario where the configurationClass should not be configuration.getClass()?

If the answer is no, I should be able to remove setConfigClass and significantly simplify DropwizardEnvironmentModule.

Break out configuration code. Implement InjectedCommands. Added tests. Various bug fixes and refactorings.

Conflicts: .gitignore README.md pom.xml src/main/java/com/hubspot/dropwizard/guice/AutoConfig.java src/main/java/com/hubspot/dropwizard/guice/GuiceBundle.java src/test/java/com/hubspot/dropwizard/guice/AutoConfigTest.java src/test/java/com/hubspot/dropwizard/guice/GuiceBundleTest.java src/test/java/com/hubspot/dropwizard/guice/objects/ExplicitResource.java src/test/java/com/hubspot/dropwizard/guice/objects/InjectedTask.java

mrserverless · 2015-03-07T12:10:35Z

I'm going to try and see if I can merge the 2 test applications: hello-world.yml and test-config.yml into a single test suite. A single test application will hopefully make it easier to demonstrate the various new functionalities to users of this tool (including myself).

oillio · 2015-03-07T22:00:26Z

Sounds good. Thanks.
I based my test application off of the pre-existing example app. It could definitely do for some cleanup.

As it stands, it is a bit difficult to follow the tests and understand why certain bits are setup the way they are. I wanted to test end to end functionality, going through the Jersey integration, and I couldn't come up with a better way to do it. If you have any better ideas...

mrserverless · 2015-03-08T03:51:49Z

As I began splitting the tests into 2 groups: DoubleInjectApplication and SingleInjectApplication. It became more clear that the code should also be split up, into either:

GuiceBundle and DoubleInjectBundle. Where DoubleInjectBundle would come with all the new Command objects and extra tests. Similar to DW's HibernateBundle vs ScanningHibernateBundle where users have a choice.
Or potentially as separate maven modules: dropwizard-guice vs dropwizard-guice-doubleinject. Similar to guice vs guice-assistedinject maven dependencies.

I believe this was what @HiJon89 was hinting at by splitting the extensions? Whilst the double injection functionality is quite powerful and forms the basis for @Config; it does come across more as an advanced feature and should be an optional add-on for users who can't satisfy their needs using single module inject.

What does everyone think? In the mean while I've made a rough start on option 1, but haven't progressed very far. Will submit a PR to @oillio's branch when it's in a good state.

oillio · 2015-03-08T15:52:48Z

I worry that splitting them up adds additional complexity for the user with little gain. Under what scenario would a user not want to use double injection?
For the base case, the user can configure their bundle just as they did before. Their modules will be added during the run phase, there shouldn't be any performance hit to the double injection, and everything should work as expected. Even better so as their modules will be added after the environment and configuration objects are added to the injector, so no singleton issues, etc.

For more advanced users, they can add injected fields to their modules and add initialization modules as needed without any configuration change or change to performance or functionality. If we split up the functionality into two modules, the transition would take additional effort and understanding.

I would agree that the @config functionality could be an optional addon. There is additional configuration required to make it work as expected and a potential performance hit for complex configuration objects.

…Merged tests into single test suite with a single config.yml and shared packages. Also: * Removed test-config.yml and TestModule, to use hello-world.yml and HelloWorldModule * Removed ConfigData (covered by JITDao) and HelloWorldConfig.otherData (unused) * Removed InjectedEnvironmentCommandTest.test_simple_case() which is repeated. * Added extra test in IntegrationTest to cater specifically eager singletons as per HubSpot#19

mrserverless · 2015-03-09T12:56:13Z

Complexity for the users is a fair point. But I think it's also reasonable that many users just use single injection for simple injections and don't have any issues related to configuration or environment. But regardless, both usages are covered by the tests.

In the spirit of keeping the library lean and easy to understand, I've move things around a bit. All the double injection related stuff are in a separate sub-package, along with their tests. It's a lot clearer now (I hope) who is using who, and may form a good basis for future refactoring.

I wrote new test catering for #19. where users want to create a eager singleton DBI that takes both Configuration and Environment module. Everything passes, except InjectedConfiguredCommandTest for some reason beyond my understanding. I've created oillio#3

In regards to @oillio previous question:

Is there any scenario where the configurationClass should not be configuration.getClass()?

I don't understand why the user needs to set Configuration class either. Can anyone shed some light? Or in the spirit of TDD, the best way probably write a test case for both scenarios and see if the results end up the same.

Refactored double injection related code and tests into sub package

…op the module. This can happen when running a Command that does not have an environment when modules are defined that depend on the environment.

sskrla and others added 6 commits February 24, 2014 22:24

Adding two phase injection, helps with setting up guice-persist.

06517c5

Altered GuiceBundle to allow setting of ServletContextListeners

cc09752

Merge pull request #1 from piemonkey/master

c4f4648

Altered GuiceBundle to allow setting of ServletContextListeners

Adding support for @Inject able modules. Perhaps we should use a @set…

bc23644

…ting injector annotation just to be make it clear it's primarly used for configuring based on setting annotations?

Binding configuration keys to objects implicitly

1637b47

Adding initialization injector binding of ServletContext (primaraly to

1f3c92c

support Shiro)

oillio mentioned this pull request Jan 20, 2015

DropWizard 0.8 upgrade using GuiceIntoHK2Bridge #47

Closed

jontejj reviewed Feb 14, 2015
View reviewed changes

oillio mentioned this pull request Feb 26, 2015

Config #49

Open

Target DropWizard 0.8.

df973aa

Break out configuration code. Implement InjectedCommands. Added tests. Various bug fixes and refactorings.

oillio force-pushed the doubler branch from f36321d to df973aa Compare March 2, 2015 18:28

mrserverless and others added 3 commits March 16, 2015 22:59

renamed 'doubleinject' package to 'command'

d106f2e

Merge pull request #3 from yunspace/doubler

38e3fcd

Refactored double injection related code and tests into sub package

Do not error out if injection fails for a module. warn instead and dr…

daa20ab

…op the module. This can happen when running a Command that does not have an environment when modules are defined that depend on the environment.

mrserverless mentioned this pull request Sep 1, 2015

Enable EagerSingletons using Managed and Guice Provider #70

Closed

vvondra mentioned this pull request Jan 22, 2017

Injecting Jersey request-scoped services for explicit bindings #87

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Injectable Modules, Named configuration data, and Injected Commands #46

Injectable Modules, Named configuration data, and Injected Commands #46

oillio commented Jan 13, 2015

jontejj commented Feb 14, 2015

jontejj Feb 14, 2015

oillio commented Feb 18, 2015

jontejj commented Feb 19, 2015

oillio commented Feb 20, 2015

jhaber commented Feb 20, 2015

mrserverless commented Feb 21, 2015

oillio commented Feb 21, 2015

mrserverless commented Feb 21, 2015

eliast commented Feb 22, 2015

Do you like the improvements made in this branch, and do you agree this should be the mainline?

jhaber commented Feb 23, 2015

jontejj commented Feb 23, 2015

oillio commented Feb 24, 2015

oillio commented Feb 25, 2015

oillio commented Feb 25, 2015

jhaber commented Feb 25, 2015

oillio commented Feb 25, 2015

jhaber commented Feb 25, 2015

jhaber commented Feb 25, 2015

oillio commented Feb 25, 2015

oillio commented Feb 26, 2015

jhaber commented Feb 26, 2015

oillio commented Feb 27, 2015

mrserverless commented Mar 7, 2015

oillio commented Mar 7, 2015

mrserverless commented Mar 8, 2015

oillio commented Mar 8, 2015

mrserverless commented Mar 9, 2015

Injectable Modules, Named configuration data, and Injected Commands #46

Are you sure you want to change the base?

Injectable Modules, Named configuration data, and Injected Commands #46

Conversation

oillio commented Jan 13, 2015

jontejj commented Feb 14, 2015

jontejj Feb 14, 2015

Choose a reason for hiding this comment

oillio commented Feb 18, 2015

jontejj commented Feb 19, 2015

oillio commented Feb 20, 2015

jhaber commented Feb 20, 2015

mrserverless commented Feb 21, 2015

oillio commented Feb 21, 2015

mrserverless commented Feb 21, 2015

eliast commented Feb 22, 2015

Do you like the improvements made in this branch, and do you agree this should be the mainline?

jhaber commented Feb 23, 2015

jontejj commented Feb 23, 2015

oillio commented Feb 24, 2015

oillio commented Feb 25, 2015

oillio commented Feb 25, 2015

jhaber commented Feb 25, 2015

oillio commented Feb 25, 2015

jhaber commented Feb 25, 2015

jhaber commented Feb 25, 2015

oillio commented Feb 25, 2015

oillio commented Feb 26, 2015

jhaber commented Feb 26, 2015

oillio commented Feb 27, 2015

mrserverless commented Mar 7, 2015

oillio commented Mar 7, 2015

mrserverless commented Mar 8, 2015

oillio commented Mar 8, 2015

mrserverless commented Mar 9, 2015