Java components for compile time injection

[marcospereira]

Status

This is ready to be reviewed.

• Documentation (Hightlights and how to use the components)
• Javadocs for all the components
• Java version for play.api.BuiltInComponentsFromContext
• Filters components like play.api.NoHttpFiltersComponents (maybe this should be done in a serie of smaller pull requests)
• *Components for subprojects

Purpose

Java components APIs similar to what we have in Scala for Compile Time Injection.

[marcospereira]

Add a private method to avoid repeatedly creating an play.api.Configuration.

[marcospereira]

This now depends of #7179 and #7180.

[wsargent]

Also see https://github.com/playframework/play-java-dagger2-example :-)

[marcospereira]

This is ready to be reviewed.

There is still some logger configuration aspects that can be improved (see documentation examples), but there is PR #7187 for that.

[gmethvin]

This is pretty cool.

It would be nice if there was some way to get Dagger to recognize the methods on the module without @Provides annotations, like Macwire does, but I don't think that's possible.

[gmethvin]

I think we need to adapt this doc a bit more to the Java audience. The only real compile-time DI framework for Java currently is Dagger, and it's a bit different from what's recommended by Adam Warski, so linking this may confuse people.

Also is the cake pattern well understood by Java devs? Can another term be used?

[marcospereira]

Yeah. Not happy with this also. Will take a look at Dagger docs and see if they have something we can link here.

[gmethvin]

Would it be better organization to move these to a play.components package?

[marcospereira]

I though about that, but decide to keep like this because Scala API is just play.api.BuiltInComponents and I didn't what to move it.

[gmethvin]

Since this injector is used to build the application, it will still be a problem when we call app.injector.instanceOf[Foo] for the global state APIs. We could just choose not to have the injector be overridable at all and make this work only without global state, but it's not hard to get it working.

We probably should write a helper method to get the default injector instance that we call from both places.

Also, we need to remember to set/unset the ObjectMapper using Json.setObjectMapper. That will definitely still be in the API even if we recommend moving to something non-static for JSON.

[marcospereira]

The trick point here is a circular dependency between some Java components (see injector() calls inside Java version of BuiltInComponents) and the injector itself (components that we need to add to have an injector on par with the Scala version).

To have a common injector initialization method, we would have to completely re-instantiate the Scala version of the components, but this shouldn't happen completely apart from the Java version (per instance, we have to honor httpFilters implementation) and we are then back to the circular dependencies here.

What I'm thinking is: if we are using dependency injection, we shouldn't touch the global state, right? Apparently this is current not true, for both compile-time and runtime di. For example, the following test:

package play;

import org.junit.Test;
import play.components.BodyParserComponents;
import play.filters.components.HttpFiltersComponents;
import play.mvc.Http;
import play.mvc.Result;
import play.mvc.Results;
import play.routing.Router;
import play.routing.RoutingDsl;
import play.test.Helpers;

import static org.hamcrest.CoreMatchers.equalTo;
import static org.junit.Assert.assertThat;

public class BuiltInComponentsFromContextTest {

    class TestBuiltInComponentsFromContext extends BuiltInComponentsFromContext implements
            HttpFiltersComponents,
            BodyParserComponents {

        public TestBuiltInComponentsFromContext(ApplicationLoader.Context context) {
            super(context);
        }

        @Override
        public Router router() {
            return new RoutingDsl(defaultScalaParser(), javaContextComponents())
                    .GET("/").routeTo(() -> Results.ok("index"))
                    .build();
        }
    }

    @Test
    public void shouldProvideAApplication() {
        ApplicationLoader.Context context = ApplicationLoader.create(Environment.simple());
        Application application = new TestBuiltInComponentsFromContext(context).application();
        Helpers.running(application, () -> {
            Http.RequestBuilder request = Helpers.fakeRequest(Helpers.GET, "/");
            Result result = Helpers.route(application, request);
            assertThat(result.status(), equalTo(Helpers.OK));
        });
    }
}

It fails because we are still touching the global state:

[error] Test play.BuiltInComponentsFromContextTest.shouldProvideAApplication failed: java.lang.RuntimeException: java.lang.InstantiationException: play.api.http.HttpConfiguration, took 1.404 sec
[error]     ...
[error] Caused by: java.lang.InstantiationException: play.api.http.HttpConfiguration
[error]     ...
[error]     at play.api.mvc.SessionCookieBaker.COOKIE_NAME(Session.scala:75)
[error]     at play.api.mvc.SessionCookieBaker.COOKIE_NAME$(Session.scala:75)
[error]     at play.api.mvc.Session$.COOKIE_NAME(Session.scala:118)

Long story short: Session.COOKIE_NAME is global state that shouldn't be touched. Removing it will make the test pass even if are not adding components to the injector.

[marcospereira]

Anyway, accessing global state inside injected components is out of the scope here. I will address it in another PR.

[gmethvin]

This seems like a great idea. The only question I have is how to deal with singletons, i.e. stateful objects that we want exactly one instance of. In scala the solution is to use a lazy val instead of a def. What's the answer for Java users? Should we provide some kind of helper? Are there any of the core components that should be made singleton?

If we clearly address the singleton problem this LGTM.

[marcospereira]

@gmethvin good question.

Since interfaces don't have state, we have two options:

1. Have a "cache" component that hold the singleton instances and make the interfaces use that "cache" to register and retrieve the singletons. I don't like this idea because it sounds complex and also because it kind of bring back the global state. Think for instance how to deal with this "cache" when we have more than one application running.

2. Delegate the singleton responsibility to BuiltInComponentsFromContext abstract class, instead of using default methods inside interfaces. So, the BuiltInComponentsFromContext will just handle these components as singletons for that specific BuiltInComponentsFromContext instance.

I more inclined to the second option, but still, users can decide to implement the interfaces without using BuiltInComponentsFromContext and then they will need to handle the singletons by themselves.

Are there any of the core components that should be made singleton?

Maybe application since it wouldn't make sense to create more than one application per BuiltInComponentsFromContext.

WDYT?

[gmethvin]

I think it's fine to do it in BuiltInComponentsFromContext. Basically we need fields for all the things that should be singletons.

As far as which components should be singletons, we could take the safe approach and just do it for anything marked @Singleton. Otherwise we should go through one by one and make sure we know which ones actually need to be singletons. The main one I can think of is ActorSystem since the actor system is expensive to create and actually spins up thread pools. Application in theory shouldn't need it, but it would probably be a good idea just to make sure the application reference is always the same.

Anything in the Context should be fine since we already have only one instance there.

Basically anything that creates thread pools, is expensive to create, or holds state should be a singleton. There may be others I haven't thought of here.

[marcospereira]

@gmethvin take a look again, regarding the singleton behavior. The components that are annotated as singletons, but were not implemented here such as are:

1. GzipFilter: maybe it should not be a singleton, since its creation is pretty lightweight.
2. play.filters.headers.SecurityHeadersFilter: the same as gzip filter
3. Formatters & FormFactory: don't need to be singletons
4. BoneCP and HikariCP connection pools should be singletons, since its creations are expensive. But maybe we should just add sensible documentation here.

Anyway, this is ready to be reviewed again.

[gmethvin]

I think we should use a lazy initialization scheme, similar to what's described here: https://stackoverflow.com/a/29138089

The reason is that prevents us having to re-order the instantiations in the constructor if we change dependencies. (Though this isn't a problem for the end user as long as we get the order correct.)

We could expose a lazily helper so adding a custom singleton is as simple as this:

private Supplier<PersonService> personService = lazily(() -> new PersonService(configuration()));
public PersonService personService() {
  return personService.get();
}

[gmethvin]

Can't these be initialized above? Would be more readable.

[marcospereira]

You mean at field declaration?

[gmethvin]

yes

[gmethvin]

I think this should be a static class somewhere else. Maybe in play.libs.F? Then we could provide a static factory like LazySupplier.lazy

[marcospereira]

Ok.

[gmethvin]

LGTM on the code/APIs. I think we should look at the documentation and perhaps provide some specific advice regarding Dagger, since in Java it's not exactly nice to wire it manually. But I think we can merge now and revisit that later.

[marcospereira]

See #7276.

[gmethvin]

@marcospereira Actually this is going to cause a bug because we use injector() in the DefaultJavaHandlerComponents to obtain instances. So we either need to add those things to the injector, or provide another way to provide action and body parser class instances. See https://github.com/playframework/playframework/blob/master/framework/src/play/src/main/scala/play/core/j/JavaAction.scala#L169