Injectable Modules, Named configuration data, and Injected Commands

[oillio]

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.

[jontejj]

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

@Qualifier
@BindingAnnotation

to not be injected into the jersey resources (https://github.com/Squarespace/jersey2-guice/issues/22) properly. Hopefully they'll accept my PR (https://github.com/Squarespace/jersey2-guice/pull/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!

[oillio]

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

[jontejj]

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 https://github.com/HubSpot/dropwizard-guice/pull/46#issuecomment-74916293 .

[oillio]

@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]

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]

@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]

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]

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]

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: https://github.com/HubSpot/dropwizard-guice/pull/46#issuecomment-75335557

[jhaber]

@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]

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 https://github.com/HubSpot/dropwizard-guice/pull/46#issuecomment-75603550 .

[oillio]

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]

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]

Fixed.

[jhaber]

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]

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]

Sure

[jhaber]

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

[oillio]

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]

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

[jhaber]

Great, I'll take a look tomorrow

[oillio]

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.

[mrserverless]

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]

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]

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:

1. 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.
2. 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]

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.

[mrserverless]

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 https://github.com/oillio/dropwizard-guice/pull/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.