Removing servletContext from BootStrap.init closure

[gsartori]

Since we can inject ServletContext in BootStrap.groovy I am proposing to cleanup the init closure from this:

class BootStrap {

    def init = { servletContext ->
    }
    def destroy = {
    }
}

to this:

class BootStrap {

    ServletContext servletContext

    def init = {
    }

    def destroy = {
    }

}

Benefits:

• Compatible with the current solution, porting a Grails application requires no init code change
• By itself it documents how to retrieve the ServletContext from other artifacts like Controllers, Services, etc..
• If we are using the IntelliJ COMMUNITY EDITION without the Grails plugin, the IDE will still support code completion on servletContext
• It's cleaner

[matrei]

Would it not be more clean and correct to just add the type to the Closure parameter?

class BootStrap {

    def init = { ServletContext servletContext ->
    }

    def destroy = {
    }
}

[jeffscottbrown]

Maybe @jeffscottbrown has the history why this particular object is injected as a parameter?

I honestly don't remember but in the 100s of Grails apps that I have been involved with I don't think I have ever referenced the servletContext in that context. Of course it is possible I am forgetting one or some, but I wouldn't miss it if weren't there.

[gsartori]

@jeffscottbrown @matrei we are using it to get the contextPath.

My proposal exists not because I don't think it can be useful, but because I don't see the reason to pass, as a variable, an object that is a Singleton (and without altering its state). It is alreday available as something that is "decoupled" but Grails is "wiring" it into a closure call (?!)

So I'm not proposing to remove it, not even to change grails-core, but just the application template. Infact it already works with no changes to Grails.

[jeffscottbrown]

Be aware that I think with code like this if no changes were made to core, while inside of the init closure I think you would have access to 2 references that point to the servletContext, one is the field servletContext and the other is the implicit “it” arg to the closure.

class BootStrap {
    ServletContext servletContext

    def init = {
    }

    def destroy = {
    }
}

[gsartori]

To be honest I've already done the changes to grails-core to avoid passing servletContext to the init closure, if it is fine with everybody I will submit the PR.

[gsartori]

It's a minor thing but I think we should document it in the Grails 7 release notes

[gsartori]

PR:

• Grails Doc - BootStrap init servletContext cleanup grails-doc#950
• Grails Forge - BootStrap servletContext refactoring grails-forge#507
• Grails - BootStrap init closure servletContext cleanup #13999

[codeconsole]

Well, this broke my app....

I use it

servletContext.setAttribute('availableLocales', localeService.getAvailableLocales())

What was the benefit in removing it? couldn't DefaultGrailsBootstrapClass support both cases with or without it?

[jdaugherty]

We didn't remove it. You just need to inject it to your Bootstrap class now.

[codeconsole]

Yes, I understand that. I already fixed my app accordingly before seeing this discussion. What I am trying to understand is what was the benefit in removing it as opposed to having the argument optional?

[gsartori]

I believe with Grails 7 we are giving Grails to the future, we have the opportunity to cleanup things.

Passing a Singleton as an argument of a closure from an architectural point of view is an anti-pattern, it means coupling/wiring something that is naturally decoupled. I don't know if there was a time where we werent able to inject the ServletContext, maybe that's the reason why it was implemented that way.

But yes I believe we should document it in the "What's new" section.

[jdaugherty]

@codeconsole - it's cleanup. I'd suggest you switch to a milestone for more stability. We're about to seriously break the snapshot builds after M2 due to the artifact / package renames.

[rainboyan]

In my opinion, it is simple in design and seemingly generic but can be easily abused and difficult to maintain, so I don't recommend using BootStrap. But I'm curious to see what scenarios you use in your real-world projects? I'm willing to recommend some better alternatives.

[rainboyan]

I also think that using methods instead of Closures is the better way to go. And make sure that this change is backwards compatible, with only a few modifications to support both ways.

class BootStrap {

    ServletContext servletContext

    def init() {
    }

    def destroy() {
    }

}


public class DefaultGrailsBootstrapClass {

    public void callInit(ServletContext servletContext) {
        Closure<?> init = getInitClosure();
        if (init != null) {
            Class<?>[] parameterTypes = init.getParameterTypes();
            if (parameterTypes != null) {
                init = init.curry(new Object[] { servletContext });
            }
            Environment.executeForCurrentEnvironment(init);
        }
        else {
            // Invoke the init() method
            InvokerHelper.invokeMethod(this.instance, 'init', new Object[0]);
        }
    }

}

[jdaugherty]

Longer term, I'd rather us support the Application class as a bootstrap class similar to spring boot. In the short term, I'm fine with this change but you'll need to add test coverage for both mechanisms.

[gsartori]

@jdaugherty can you point me to some docs/examples of how Spring does the bootstrap? I tried looking for it but I think I'm using the wrong keywords

[matrei]

@gsartori For init() look at implementing ApplicationRunner or CommandLineRunner interfaces or annotating a method with @PostConstruct. For destroy() look at implementing DisposableBean or adding @PreDestroy to a method.

[gsartori]

@matrei Okay, I see. Once we remove the BootStrap.groovy, are we planning to decide for one of the Spring options as our "standard" to include in Application.groovy as an "empty" implentation (like the current init closure)?

I would prefer we provide a predefined way to initialize the application instead of letting the developers study Spring to do it.

[jdaugherty]

@gsartori #14012 is the ticket to add @SpringBootApplication or @SpringBootConfiguration to the Application class. A side effect of this is that the Application.class will effectively become @Configuration which means you can declare beans directly on the application class.

I don't think we can fully remove bootstrap in 7. Adopting spring conventions for bean wiring, init, and destroy will mean that we need to fully adopt them and that effort is likely too large for this release.