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

Make AbstractTemplateEngine#init method final, use lazy instantiation #591

Merged
merged 3 commits into from
Dec 1, 2021

Conversation

decebals
Copy link
Member

The goal of this PR is to facilitate #590.
I think that now the code looks better and the performance is a little better (using lazy instantiation).

@mhagnumdw
Copy link
Member

@decebals , I took a look over and it looks ok. Tomorrow I will test it with my application that uses Freemarker. Then I come back here.

@mhagnumdw
Copy link
Member

@decebals , please see this comment: d0793e2#r61118560-permalink

@decebals
Copy link
Member Author

@decebals , please see this comment: d0793e2#r61118560-permalink

See https://github.com/pippo-java/pippo/blob/master/pippo-controller-parent/pippo-weld/src/test/java/ro/pippo/weld/ControllerInterceptorTest.java#L95

My idea is to bring each component where is used. Now many components extract dependencies from Application and ControllerApplication. Probably in the future we can remove Application and ControllerApplication entirely. It's hard to test some come components because they receive application instance as parameter and you (developer) don't know what this component uses from application.

I recognize that now is a little bit verbose to set a custom ControllerFactory. Maybe to add ControllerHandlerFactory is too much, and we should put the logic of this component in ControllerRouteFactory and to remove it (ControllerHandlerFactory).
The good news is that in the future we don't need pippo-spring, pippo-guice anymore, no need to set a custom ControllerFactory to achieve integration with DI containers.

Please feel free to say what you think, because your feedback is important to me.

@decebals
Copy link
Member Author

Remove ControllerHandlerFactory layer to simplify injection of custom ControllerFactory in DefaultControllerRouteFactory.

@mhagnumdw
Copy link
Member

mhagnumdw commented Nov 30, 2021

See https://github.com/pippo-java/pippo/blob/master/pippo-controller-parent/pippo-weld/src/test/java/ro/pippo/weld/ControllerInterceptorTest.java#L95

I did as in the test class above and in my application I got the error stack below:

Error: declares @Produces("application/json") but there is no registered ContentTypeEngine for that type!

Exception in thread "main" ro.pippo.core.PippoRuntimeException: javax.servlet.ServletException: ro.pippo.core.PippoRuntimeException: br.com.myapp.controller.cofre.CofreConsultaNgController::commandSearch declares @Produces("application/json") but there is no registered ContentTypeEngine for that type!
	at ro.pippo.jetty.JettyServer.start(JettyServer.java:84)
	at ro.pippo.core.Pippo.start(Pippo.java:158)
	at br.com.myapp.PippoLauncher.main(PippoLauncher.java:68)
Caused by: javax.servlet.ServletException: ro.pippo.core.PippoRuntimeException: br.com.myapp.controller.cofre.CofreConsultaNgController::commandSearch declares @Produces("application/json") but there is no registered ContentTypeEngine for that type!
	at ro.pippo.core.PippoFilter.init(PippoFilter.java:115)
	at ro.pippo.jetty.websocket.JettyWebSocketFilter.init(JettyWebSocketFilter.java:43)
	at org.eclipse.jetty.servlet.FilterHolder.initialize(FilterHolder.java:140)
	at org.eclipse.jetty.servlet.ServletHandler.lambda$initialize$0(ServletHandler.java:731)
	at java.util.Spliterators$ArraySpliterator.forEachRemaining(Spliterators.java:948)
	at java.util.stream.Streams$ConcatSpliterator.forEachRemaining(Streams.java:742)
	at java.util.stream.ReferencePipeline$Head.forEach(ReferencePipeline.java:580)
	at org.eclipse.jetty.servlet.ServletHandler.initialize(ServletHandler.java:755)
	at org.eclipse.jetty.servlet.ServletContextHandler.startContext(ServletContextHandler.java:379)
	at org.eclipse.jetty.server.handler.ContextHandler.doStart(ContextHandler.java:911)
	at org.eclipse.jetty.servlet.ServletContextHandler.doStart(ServletContextHandler.java:288)
	at org.eclipse.jetty.util.component.AbstractLifeCycle.start(AbstractLifeCycle.java:73)
	at org.eclipse.jetty.util.component.ContainerLifeCycle.start(ContainerLifeCycle.java:169)
	at org.eclipse.jetty.server.Server.start(Server.java:423)
	at org.eclipse.jetty.util.component.ContainerLifeCycle.doStart(ContainerLifeCycle.java:110)
	at org.eclipse.jetty.server.handler.AbstractHandler.doStart(AbstractHandler.java:97)
	at org.eclipse.jetty.server.Server.doStart(Server.java:387)
	at org.eclipse.jetty.util.component.AbstractLifeCycle.start(AbstractLifeCycle.java:73)
	at ro.pippo.jetty.JettyServer.start(JettyServer.java:81)
	... 2 more
Caused by: ro.pippo.core.PippoRuntimeException: br.com.myapp.controller.cofre.CofreConsultaNgController::commandSearch declares @Produces("application/json") but there is no registered ContentTypeEngine for that type!
	at ro.pippo.controller.ControllerHandler.validateProduces(ControllerHandler.java:313)
	at ro.pippo.controller.ControllerHandler.<init>(ControllerHandler.java:86)
	at ro.pippo.controller.DefaultControllerHandlerFactory.createRouteHandler(DefaultControllerHandlerFactory.java:37)
	at ro.pippo.controller.DefaultControllerRouteFactory.createRoute(DefaultControllerRouteFactory.java:41)
	at ro.pippo.controller.ControllerRegistry.createControllerRoutes(ControllerRegistry.java:231)
	at ro.pippo.controller.ControllerRegistry.registerControllerMethods(ControllerRegistry.java:169)
	at ro.pippo.controller.ControllerRegistry.register(ControllerRegistry.java:129)
	at ro.pippo.controller.ControllerRegistry.register(ControllerRegistry.java:95)
	at ro.pippo.controller.ControllerApplication.addControllers(ControllerApplication.java:104)
	at br.com.myapp.handlers.init.LoadApi.handle(LoadApi.java:130)
	at br.com.myapp.handlers.init.LevelHandler.handle(LevelHandler.java:46)
	at br.com.myapp.PippoApplication.onInit(PippoApplication.java:82)
	at ro.pippo.core.Application.init(Application.java:114)
	at ro.pippo.core.route.RouteDispatcher.init(RouteDispatcher.java:71)
	at ro.pippo.core.PippoFilter.init(PippoFilter.java:108)
	... 20 more

So I needed to set the ContentTypeEngines in DefaultControllerHandlerFactory. That's right? Thus:

public class PippoApplication extends ControllerApplication {
    @Override
    protected void onInit() {
        ControllerFactory controllerFactory = new GuiceControllerFactory(injector);
        ControllerHandlerFactory controllerHandlerFactory = new DefaultControllerHandlerFactory()
            .setControllerFactory(controllerFactory)
            .setContentTypeEngines(getContentTypeEngines()) // <<< I had to define here
            ;
        ControllerRouteFactory controllerRouteFactory = new DefaultControllerRouteFactory()
            .setControllerHandlerFactory(controllerHandlerFactory)
            ;
        setControllerRouteFactory(controllerRouteFactory);
    }
}

If I don't set the engines in the DefaultControllerHandlerFactory, it will instantiate an empty ContentTypeEngines in the getContentTypeEngines() method.

@mhagnumdw
Copy link
Member

Remove ControllerHandlerFactory layer to simplify injection of custom ControllerFactory in DefaultControllerRouteFactory.

oops, I'll check this change...

@mhagnumdw
Copy link
Member

@decebals , the observation I made above #591 (comment) is still valid.

It was necessary to set: .setContentTypeEngines(getContentTypeEngines()). That's right?

ControllerFactory controllerFactory = new GuiceControllerFactory(injector);
ControllerRouteFactory controllerRouteFactory = new DefaultControllerRouteFactory()
    .setControllerFactory(controllerFactory)
    .setContentTypeEngines(getContentTypeEngines()) // <<< I had to define here - getting from Application
    ;
setControllerRouteFactory(controllerRouteFactory);

@decebals
Copy link
Member Author

It was necessary to set: .setContentTypeEngines(getContentTypeEngines()). That's right?

Yes. We will see in the future if we can improve this.

@decebals
Copy link
Member Author

From what I see, we don't need ro.pippo.core.ContentTypeEngine#init(Application). Nothing happens there (less in the Gson based implementation but I think that we can move the creation of Gson in a lazy getter method).
I will come with a new PR with this improvement related to ContentTypeEngine but for the moment I want to be sure that we don't damage something in what exists already. So, check and close each PR, one by one, to isolate possible regressions.

By the way, the new release will be 2.0.0 because we will modify/improve substantial some things (break of API).

@mhagnumdw
Copy link
Member

Well... these changes worked in my application with the modifications I mentioned earlier. I will then approve this PR.

@decebals
Copy link
Member Author

Well... these changes worked in my application with the modifications I mentioned earlier. I will then approve this PR.

I don't understand. Is it works and I can merge it or do you need more time for a test?

@mhagnumdw mhagnumdw merged commit 5906228 into master Dec 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants