springboot Deprecate SpringApplication#refresh(ApplicationContext) and provide SpringApplication#refresh(ConfigurableApplicationContext) as a replacement

The method SpringApplication#refresh is only called by SpringApplication#refreshContext(ConfigurableApplicationContext). So the application context instance passed to SpringApplication#refresh will always be a subclass of ConfigurableApplicationContext, which has a method 'refresh'. No need to assert that using Assert.isInstanceOf(AbstractApplicationContext.class, applicationContext);.

It could be simplified like this:

protected void refresh(ConfigurableApplicationContext applicationContext) {
    applicationContext.refresh();
}

Further more, the field applicationContextClass of SpringApplication is also declared as Class<? extends ConfigurableApplicationContext>.

I think it is reasonable enough to modify the method signature.

Comment From: wilkinsona

Thanks for the suggestion. While I agree that things could perhaps be simplified a little here, it would technically be a breaking API change. I'm not sure that the small benefit of the change justifies making it. May I ask what prompted you to open the issue?

Comment From: justmehyp

Thank you for your reply.

Well, I am a number-one fan of Spring framework. Since I use it, I hope it gets better. I read the source code, then I feedback something I think is better. Sometimes I am stubborn about keeping the code clean, if it makes me confused ...

Yes, it is a breaking API change. But thankfully it is an internal API, not public. It is a protected method, but no subclasses inherit SpringApplication up to now.

I noticed that parameters of SpringApplication#afterRefresh changed for introducing ApplicationArguments and removing String[]. I think it is a good strategy. If you can't change it right away, I suggest you use this strategy again as a means of transition.

In 1.2.8.RELEASE, it is:

protected void afterRefresh(ConfigurableApplicationContext context, String[] args) {
    runCommandLineRunners(context, args);
}

In v1.3.8.RELEASE, it is deprecated:

@Deprecated
protected void afterRefresh(ConfigurableApplicationContext context, String[] args) {
}

and you add a new method:

protected void afterRefresh(ConfigurableApplicationContext context, ApplicationArguments args) {
    afterRefresh(context, args.getSourceArgs());
    callRunners(context, args);
}

in v1.4.0.RELEASE, SpringApplication#afterRefresh(ConfigurableApplicationContext, String[]) was removed. Only SpringApplication#afterRefresh(ConfigurableApplicationContext, ApplicationArguments) was retained.

Comment From: philwebb

it is an internal API, not public. It is a protected method, but no subclasses inherit SpringApplication up to now.

I'm afraid we need to treat it as a public API. Just because we don't have a SpringApplication subclass in our code does not mean that someone is not relying on it.

I think it is a good strategy. If you can't change it right away, I suggest you use this strategy again as a means of transition.

I'm not sure that the additional argument would be in this case. We'd probably need to introduce a new method with a different name.

Comment From: justmehyp

I think you are right. In this case, a new method such as doRefresh could be introduced:

protected void doRefresh(ConfigurableApplicationContext applicationContext) {
    refresh(applicationContext);
}

Then mark refresh as deprecated:

@Deprecated
protected void refresh(ApplicationContext applicationContext) {
    Assert.isInstanceOf(AbstractApplicationContext.class, applicationContext);
    ((AbstractApplicationContext) applicationContext).refresh();
}