pippo icon indicating copy to clipboard operation
pippo copied to clipboard

Add support for Inject

Open decebals opened this issue 3 years ago • 32 comments

https://github.com/pippo-java/pippo/issues/554

decebals avatar Nov 19 '21 22:11 decebals

Now everything (almost) is configurable via DI container and the things remain perfectly the same until now if no DI container is detected. Only one new tiny dependency is added (javax.inject) with scope provided . I tested on pippo-demo and pippo-test projects to see that things work perfectly without inject. I tested with Spring and Guice, in complex scenario (inject new webserver, new template engine, ..) and things work as expected.

From the beginning, we wanted this feature to be as unobtrusive as possible, with as less as possible changes. My implementation started from the idea that both Spring and Guice (the most important DI containers from Java) have support for optional inject for the fields annotated with standard javax.inject.Inject. The idea is to use java.util.Optional<T> option for that fields. In theory the using of java.util.Optional is discouraged for fields (according documentation) but many people (included Spring and Guice teams that implemented a such support in their libraries) consider a perfect fit and I agree with that. My idea was to add lazy initialization in getters (Application and ControllerApplication) and to initialize the injected fields that are not touched by injection.

I think that we can do the code more compact but this is another story. For example for each inject aware field we have a declaration and a lazy initialization getter (and in some cases a setter):

@Inject
private Optional<ErrorHandler> errorHandler = Optional.empty();

public ErrorHandler getErrorHandler() {
    if (!errorHandler.isPresent()) {
        errorHandler = Optional.of(new DefaultErrorHandler(this));
    }

    return errorHandler.get();
}

public void setErrorHandler(ErrorHandler errorHandler) {
    this.errorHandler = Optional.of(errorHandler);
}

I prefer something more verbose/light as:

@Inject
private Optional<ErrorHandler> errorHandler;

public ErrorHandler getErrorHandler() {
    return OptionalUtils.setOnNull(Supplier<ErrorHandler>).get();
}

Also the proposed implementation with lazy initialization getters come with some improvements from performance point of view because the objects are initialized only on request.

That is all. I will add in my next two comments how I tested with Spring and Guice. If this PR will be accepted, after merge I will update pippo-demo project (pippo-demo-spring and pippo-demo-guice). With this PR the pippo-spring and pippo-guice are no longer needed and should be deleted.

decebals avatar Nov 20 '21 12:11 decebals

For Spring Test, I modified pippo-demo-spring

public class SpringApplication3 extends ControllerApplication {

    @Inject
    private List<? extends Controller> controllers;

    @Override
    protected void onInit() {
        // add routes for static content
        addPublicResourceRoute();
        addWebjarsResourceRoute();

        addControllers(controllers.toArray(new Controller[0]));
    }

}
@Configuration
@ComponentScan
public class SpringConfiguration3 extends SpringConfiguration {

    @Bean
    public ContactService contactService() {
        return new InMemoryContactService();
    }

    @Bean
    public TemplateEngine templateEngine() {
        return new SimpleTemplateEngine();
    }

    @Bean
    public Router router() {
        return new CustomRouter();
    }

    @Bean
    public WebServer webServer() {
        return new TjwsServer();
    }

    @Bean
    public PippoSettings pippoSettings() {
        return new PippoSettings();
    }

    @Bean
    public Application application() {
        return new SpringApplication3();
    }

    @Bean
    public Pippo pippo() {
        return new Pippo(application()).setServer(webServer());
    }

}
public class SpringDemo3 {

    public static void main(String[] args) {
        ApplicationContext context = new AnnotationConfigApplicationContext(SpringConfiguration3.class);
        Pippo pippo = context.getBean(Pippo.class);
        pippo.start();
    }

}
@Path
@Component
public class ContactsController extends Controller {

    @Inject
    private ContactService contactService;

    @Inject
    private TemplateEngine templateEngine;

    @GET
    public void sayHello() {
        StringWriter writer = new StringWriter();
        Map<String, Object> model = new HashMap<>();
        model.put("name", "Decebal");
        templateEngine.renderString("Hello ${name}", model, writer);
        getResponse().send(writer.toString());
    }

}

decebals avatar Nov 20 '21 13:11 decebals

For Guice Test, I modified pippo-demo-guice

public class GuiceApplication3 extends ControllerApplication {

    @Inject
    private List<? extends Controller> controllers;

    @Override
    protected void onInit() {
        // add routes for static content
        addPublicResourceRoute();
        addWebjarsResourceRoute();

        addControllers(controllers.toArray(new Controller[0]));
    }

}
public class GuiceModule3 extends AbstractModule {

    @Override
    protected void configure() {
        bind(ContactService.class).to(InMemoryContactService.class).asEagerSingleton();
        bind(Application.class).to(GuiceApplication3.class).asEagerSingleton();
        bind(Router.class).to(CustomRouter.class).in(Scopes.SINGLETON);
        bind(TemplateEngine.class).to(SimpleTemplateEngine.class).asEagerSingleton();
        bind(WebServer.class).to(TjwsServer.class).in(Scopes.SINGLETON);

        bind(Pippo.class);

        bindOptionalApplication();
        bindOptionalControllerApplication();
    }

    @Singleton
    @Provides
    @Inject
    public List<? extends Controller> controllers(ContactsController contacts) {
        return Arrays.asList(contacts);
    }

    private void bindOptionalApplication() {
        OptionalBinder.newOptionalBinder(binder(), ContentTypeEngines.class);
        OptionalBinder.newOptionalBinder(binder(), ErrorHandler.class);
        OptionalBinder.newOptionalBinder(binder(), HttpCacheToolkit.class);
        OptionalBinder.newOptionalBinder(binder(), Languages.class);
        OptionalBinder.newOptionalBinder(binder(), Messages.class);
        OptionalBinder.newOptionalBinder(binder(), MimeTypes.class);
        OptionalBinder.newOptionalBinder(binder(), Router.class);
        OptionalBinder.newOptionalBinder(binder(), WebSocketRouter.class);
        OptionalBinder.newOptionalBinder(binder(), RequestResponseFactory.class);
        OptionalBinder.newOptionalBinder(binder(), RoutePreDispatchListenerList.class);
        OptionalBinder.newOptionalBinder(binder(), RoutePostDispatchListenerList.class);
        OptionalBinder.newOptionalBinder(binder(), TemplateEngine.class);
        OptionalBinder.newOptionalBinder(binder(), new TypeLiteral<RouteHandler<?>>(){});
        OptionalBinder.newOptionalBinder(binder(), new TypeLiteral<List<Initializer>>(){});
    }

    private void bindOptionalControllerApplication() {
        OptionalBinder.newOptionalBinder(binder(), ControllerFactory.class);
        OptionalBinder.newOptionalBinder(binder(), ControllerInitializationListenerList.class);
        OptionalBinder.newOptionalBinder(binder(), ControllerInstantiationListenerList.class);
        OptionalBinder.newOptionalBinder(binder(), ControllerInvokeListenerList.class);
        OptionalBinder.newOptionalBinder(binder(), new TypeLiteral<List<MethodParameterExtractor>>(){});
    }

}
public class GuiceDemo3 {

    public static void main(String[] args) {
        Injector injector = Guice.createInjector(new GuiceModule3());
        Pippo pippo = injector.getInstance(Pippo.class);
        pippo.start();
    }

}
@Path
public class ContactsController extends Controller {

    @Inject
    private ContactService contactService;

    @Inject
    private TemplateEngine templateEngine;

    @GET
    public void sayHello() {
        StringWriter writer = new StringWriter();
        Map<String, Object> model = new HashMap<>();
        model.put("name", "Decebal");
        templateEngine.renderString("Hello ${name}", model, writer);
        getResponse().send(writer.toString());
    }

}

I don't like the complexity of GuiceModule3 (extra bindOptionalApplication and bindOptionalControllerApplication methods) but probably we can improve the code here (disclaimer: I'm not a good Guice connoisseur),

decebals avatar Nov 20 '21 13:11 decebals

    @Singleton
    @Provides
    @Inject
    public List<? extends Controller> controllers(ContactsController contacts) {
        return Arrays.asList(contacts);
    }

Sorry I didn't understand why ContactsController contacts. contacts ?!

What if I have dozens of controllers?

mhagnumdw avatar Nov 20 '21 23:11 mhagnumdw

@decebals , will you still make changes or can I test this version in my application?

mhagnumdw avatar Nov 20 '21 23:11 mhagnumdw

@decebals , will you still make changes or can I test this version in my application?

I think that you can test it. The code is perfect functional from what I see until now. Maybe little adjustments in time.

decebals avatar Nov 21 '21 08:11 decebals

    @Singleton
    @Provides
    @Inject
    public List<? extends Controller> controllers(ContactsController contacts) {
        return Arrays.asList(contacts);
    }

Sorry I didn't understand why ContactsController contacts. contacts ?!

What if I have dozens of controllers?

Your question is good. In my last (relative big) project I use Pippo with Spring. In Spring this part with gathering all controllers and inject them in application is easy and automatically. All you have to do is to add Component annotation on each controller class and enable component scan ( @ComponentScan) in configuration. In Guice I think that you need to register each controller by hand, no support for component scan in core (at least as far as I know). I am not a Guice guy and I am sure that the example code presented by me related to Guice can be improved. Maybe there's someone here who can help us.

decebals avatar Nov 21 '21 09:11 decebals

Your question is good. In my last (relative big) project I use Pippo with Spring. In Spring this part with gathering all controllers and inject them in application is easy and automatically. All you have to do is to add Component annotation on each controller class and enable component scan ( @ComponentScan) in configuration. In Guice I think that you need to register each controller by hand, no support for component scan in core (at least as far as I know). I am not a Guice guy and I am sure that the example code presented by me related to Guice can be improved. Maybe there's someone here who can help us.

In the end I think that I solved the problem in an elegant way using Guice Multibinding and Reflections. Now the code looks like:

public class GuiceModule3 extends AbstractModule {

    @Override
    protected void configure() {
        bind(ContactService.class).to(InMemoryContactService.class).asEagerSingleton();
        bind(Application.class).to(GuiceApplication3.class).asEagerSingleton();
        bind(Router.class).to(CustomRouter.class).in(Scopes.SINGLETON);
        bind(TemplateEngine.class).to(SimpleTemplateEngine.class).asEagerSingleton();
        bind(WebServer.class).to(TjwsServer.class).in(Scopes.SINGLETON);

        bind(Pippo.class);

        bindControllers();

        bindOptionalApplication();
        bindOptionalControllerApplication();
    }

    private void bindControllers() {
        // retrieve controller classes
        Reflections reflections = new Reflections(getClass().getPackage().getName());
        Set<Class<? extends Controller>> controllers = reflections.getSubTypesOf(Controller.class);

        // bind found controllers
        Multibinder<Controller> multibinder = Multibinder.newSetBinder(binder(), Controller.class);
        controllers.forEach(controller -> multibinder.addBinding().to(controller));
    }

}
public class GuiceApplication3 extends ControllerApplication {

    @Inject
    private Set<Controller> controllers;

    @Override
    protected void onInit() {
        // add routes for static content
        addPublicResourceRoute();
        addWebjarsResourceRoute();

        addControllers(controllers.toArray(new Controller[0]));
    }

}

I tested with multiple controllers and the result is good.

decebals avatar Nov 21 '21 10:11 decebals

I already use the Reflections lib and it is very good!

mhagnumdw avatar Nov 22 '21 18:11 mhagnumdw

@decebals , will you still make changes or can I test this version in my application?

I think that you can test it. The code is perfect functional from what I see until now. Maybe little adjustments in time.

My boot is heavily modified, so for now I won't be able to test it thoroughly. But I have good news: with this version my application continues to work normally.

mhagnumdw avatar Nov 22 '21 19:11 mhagnumdw

I'm trying to adapt my application to this model...

@decebals , I use the EntityManager configured via the com.google.inject.persist.jpa.JpaPersistModule.JpaPersistModule Guice module.

The JpaPersistModule receives a set of properties via the properties(Map<?,?> properties) method. I got these properties from the ro.pippo.core.Application.getPippoSettings() instance. Do you have any idea what the best way to do this is now?

Application.getPippoSettings() will not be available when creating the Guice module.

mhagnumdw avatar Nov 22 '21 21:11 mhagnumdw

For Guice Test, I modified pippo-demo-guice

public class GuiceApplication3 extends ControllerApplication {

    @Inject
    private List<? extends Controller> controllers;

    // ...
}

For me, Guice's dependency injection just only worked like this:

@Inject
private Set<Controller> controllers;

obs: Set or List

mhagnumdw avatar Nov 26 '21 16:11 mhagnumdw

obs: Set or List

Yes, it's Set instead of List. Sorry for inconvenient. I will update the snippet code.

decebals avatar Nov 26 '21 17:11 decebals

In https://github.com/pippo-java/pippo/pull/590#issuecomment-974789415, it's Set instead of List. Probably you copied the initial code.

decebals avatar Nov 26 '21 17:11 decebals

I'm trying to adapt my application to this model...

@decebals , I use the EntityManager configured via the com.google.inject.persist.jpa.JpaPersistModule.JpaPersistModule Guice module.

The JpaPersistModule receives a set of properties via the properties(Map<?,?> properties) method. I got these properties from the ro.pippo.core.Application.getPippoSettings() instance. Do you have any idea what the best way to do this is now?

Application.getPippoSettings() will not be available when creating the Guice module.

I inject PippoSettings in Spring also, together with Application and other services. When I need PippoSettings I injected where I need it. My approach with application.properties is hybrid, the same file is used by PippoSettings (internal stuff like server port, ..) but it is also used by Spring. I injected the properties in my services via Spring @Value annotation. So, if I need one or more properties in a service (or other component outside Pippo), I don't retrieve that information via PippoSettings, but using the DI container support for properties.

As I mentioned in https://github.com/pippo-java/pippo/issues/565, the pippo - spring integration is good enough for me and without this PR. This PR is useful when you want to fine tuning the pippo stack from DI (Spring, Guice), entirely.

decebals avatar Nov 26 '21 17:11 decebals

I obtained relative (~~only one problem related to gathering all controllers in a set/list collection~~) good result with Avaje Inject library. It's a dependency injection library inspired by Dagger2, with very good performance and a small size (48 KB - version 5.13). It's useful when the total size of application matters, but you want to use a DI container.

The code in this case looks like:

//@Singleton
public class AvajeApplication extends ControllerApplication {

    private List<Controller> controllers;

//    @Inject
    public AvajeApplication(List<Controller> controllers) {
        this.controllers = controllers;
    }

    @Override
    protected void onInit() {
        // add routes for static content
        addPublicResourceRoute();
        addWebjarsResourceRoute();

        addControllers(controllers.toArray(new Controller[0]));
    }

}
@Factory
public class AvajeConfiguration {

    @Bean
    public ContactService contactService() {
        return new InMemoryContactService();
    }

    @Bean
    public PippoSettings pippoSettings() {
        return new PippoSettings();
    }

//    @Bean
//    public List<Controller> controllers(ContactsController contactsController) {
//        System.out.println("AvajeConfiguration.controllers");
//        return Collections.singletonList(contactsController);
//    }

    @Bean
    public Application application(ContactsController contactsController, TestController testController) {
        return new AvajeApplication(Arrays.asList(contactsController, testController));
    }

//    @Bean
//    public Pippo pippo(Application application, WebServer webServer) {
//        return new Pippo(application).setServer(webServer);
//    }

}
public class AvajeDemo {

    public static void main(String[] args) {
        BeanScope beanScope = BeanScope.newBuilder().build();
        Pippo pippo = beanScope.get(Pippo.class);
        pippo.start();
    }

}
@Path
@Singleton
public class ContactsController extends Controller {

    @Inject
    ContactService contactService;

    @Inject
    TemplateEngine templateEngine;

    @GET
    public void index() {
        getResponse().bind("contacts", contactService.getContacts());
        getResponse().render("contacts");
    }

}

decebals avatar Nov 26 '21 17:11 decebals

I'm trying to adapt my application to this model... @decebals , I use the EntityManager configured via the com.google.inject.persist.jpa.JpaPersistModule.JpaPersistModule Guice module. The JpaPersistModule receives a set of properties via the properties(Map<?,?> properties) method. I got these properties from the ro.pippo.core.Application.getPippoSettings() instance. Do you have any idea what the best way to do this is now? Application.getPippoSettings() will not be available when creating the Guice module.

I inject PippoSettings in Spring also, together with Application and other services. When I need PippoSettings I injected where I need it. My approach with application.properties is hybrid, the same file is used by PippoSettings (internal stuff like server port, ..) but it is also used by Spring. I injected the properties in my services via Spring @Value annotation. So, if I need one or more properties in a service (or other component outside Pippo), I don't retrieve that information via PippoSettings, but using the DI container support for properties.

As I mentioned in #565, the pippo - spring integration is good enough for me and without this PR. This PR is useful when you want to fine tuning the pippo stack from DI (Spring, Guice), entirely.

Hi! I understand that it is possible to inject PippoSettings into a Guice component. But this only works after Guice is ready. In my case I would need PippoSettings when creating the Guice modules.

To explain it better, something like this:

Injector injector = Guice.createInjector(
    new PippoGuiceModule(),
    new AppJpaPersistModule("persistenceUnitName", pippoSettings), // <<< need PippoSettings instance here
    new AppGuiceModule()
);

GuiceInjector.set(injector);
Pippo pippo = injector.getInstance(Pippo.class);

pippo.start();

ps: I'm looking for a way to work around this problem.

mhagnumdw avatar Nov 26 '21 18:11 mhagnumdw

    private void bindControllers() {
        // retrieve controller classes
        Reflections reflections = new Reflections(getClass().getPackage().getName());
        Set<Class<? extends Controller>> controllers = reflections.getSubTypesOf(Controller.class);

        // bind found controllers
        Multibinder<Controller> multibinder = Multibinder.newSetBinder(binder(), Controller.class);
        controllers.forEach(controller -> multibinder.addBinding().to(controller));
    }

We might have abstract controllers, so maybe it's better to avoid bind errors (at least in Guice):

Reflections reflections = new Reflections(getClass().getPackage().getName(), new SubTypesScanner());
Set<Class<? extends Controller>> controllers = reflections.getSubTypesOf(Controller.class)
    .stream()
    .filter(clazz -> clazz.isAnnotationPresent(ro.pippo.controller.Path.class))
    .collect(Collectors.toSet())

Or some other logic that checks if it's a concrete class.

mhagnumdw avatar Nov 26 '21 18:11 mhagnumdw

Hi! I understand that it is possible to inject PippoSettings into a Guice component. But this only works after Guice is ready. In my case I would need PippoSettings when creating the Guice modules.

To explain it better, something like this:

Injector injector = Guice.createInjector(
    new PippoGuiceModule(),
    new AppJpaPersistModule("persistenceUnitName", pippoSettings), // <<< need PippoSettings instance here
    new AppGuiceModule()
);

GuiceInjector.set(injector);
Pippo pippo = injector.getInstance(Pippo.class);

pippo.start();

ps: I'm looking for a way to work around this problem.

I don't visualize your implementation. How AppJpaPersistModule looks like?

decebals avatar Nov 26 '21 18:11 decebals

Hi! I understand that it is possible to inject PippoSettings into a Guice component. But this only works after Guice is ready. In my case I would need PippoSettings when creating the Guice modules.

What about https://stackoverflow.com/questions/39734343/injecting-a-dependency-into-guice-module?

decebals avatar Nov 26 '21 18:11 decebals

I don't visualize your implementation. How AppJpaPersistModule looks like?

Oh, sorry 😅, I forgot to mention it's a class of mine. It's just a wrapper for Guice's JpaPersistModule.

It goes something like this:

public class AppJpaPersistModule implements Module {

    private final PippoSettings settings;

    public JPAGuiceModule(PippoSettings settings) {
        this.settings = settings;
    }

    @Override
    public void configure(Binder binder) {
        JpaPersistModule jpaModule = new JpaPersistModule(Constantes.PU_NAME);
        jpaModule.properties( ... ); // TODO: get properties from PippoSettings and add here
        binder.install(jpaModule);
    }

}

ps: But I think I'll change the strategy so I don't need PippoSettings there... I'm still not sure what it's going to look like... I'm still seeing it...

mhagnumdw avatar Nov 26 '21 19:11 mhagnumdw

@decebals , I use Freemarker and it's not working. The problem is that the ro.pippo.freemarker.FreemarkerTemplateEngine.init(Application) method is not being called.

To make it work I did:

  • Annotated the ro.pippo.core.AbstractTemplateEngine.init(Application) method with @javax.inject.Inject
  • Annotated the ro.pippo.freemarker.FreemarkerTemplateEngine.init(Application) method with @javax.inject.Inject

I configure the Guice module like this:

@Override
protected void configure() {
    bind(Application.class).to(PippoApplication.class).asEagerSingleton();
    bind(TemplateEngine.class).to(FreemarkerTemplateEngine.class).asEagerSingleton();
    // ...
}

It would be nice to be able to leave the annotation just on the AbstractTemplateEngine class.

mhagnumdw avatar Nov 28 '21 22:11 mhagnumdw

It would be nice to be able to leave the annotation just on the AbstractTemplateEngine class.

@mhagnumdw I think that I have a solution based on #591. Please review #591 and if you consider that is OK I can merge it in master branch (and in inject branch) and I will continue work on this PR.

decebals avatar Nov 29 '21 14:11 decebals

@decebals , please update from master.

mhagnumdw avatar Dec 01 '21 17:12 mhagnumdw

@decebals , please update from master.

Done.

decebals avatar Dec 01 '21 17:12 decebals

@decebals , please update from master.

mhagnumdw avatar Dec 21 '21 22:12 mhagnumdw

@decebals , please update from master.

Done

decebals avatar Dec 22 '21 19:12 decebals

Currently I register a content type like this: registerContentTypeEngine(GsonEngine.class).

What do you think we also use dependency injection to register ContentTypeEngine?

In Guice we can use MapBinder, in Spring I think it's also called MapBinder.

If you agree, could you implement it? So I would validate doing the tests in my application.

mhagnumdw avatar Dec 24 '21 22:12 mhagnumdw

If you agree, could you implement it? So I would validate doing the tests in my application.

Sure, I will do it. Now I am in a mini vacation with family.

decebals avatar Dec 28 '21 09:12 decebals

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

sonarqubecloud[bot] avatar Nov 22 '22 18:11 sonarqubecloud[bot]