AbstractRefreshableApplicationContext should not appear in the type hierarchy of our application context implementations

[snicoll]

Several ApplicationContext implementations we use inherits from AbstractRefreshableApplicationContext which is a hierarchy we shouldn't be using in modern-day arrangements:

• AnnotationConfigWebApplicationContext
• AnnotationConfigReactiveWebApplicationContext

While the latter is in our code base, the former is in the framework code base with no clear sign we shouldn't be using it anymore. Those contexts don't hold a BeanFactory instance before they refresh and therefore prevent any use of functional bean registration.

A potential replacement would be a GenericWebApplicationContext sub-class with a AnnotationConfigUtils.registerAnnotationConfigProcessors(ctx) invocation.

[wilkinsona]

Our move to Framework 5.2 is related to this. In 5.2, AnnotationConfigRegistry has two new methods. AnnotationConfigReactiveWebApplicationContext implements AnnotationConfigRegistry and, as a result, does not compile against 5.2 snapshots. AnnotationConfigReactiveWebServerApplicationContext has the same problem.

@snicoll Did you have anything in mind here? Will it help with implementing the new methods on AnnotationConfigRegistry?

[snicoll]

This is related indirectly. We have contexts in the hierarchy that ultimately do not depend on GenericApplicationContext and some methods where not available. @jhoeller provided a pragmatic fix for that that explains why there are two new methods to implement in 5.2.

It looks like this will be interesting for AnnotationConfigReactiveWebApplicationContext. AnnotationConfigReactiveWebApplicationContext should stop extending from AbstractRefreshableConfigApplicationContext. There is a class affected in framework and I am waiting for Juergen to apply a consistent fix here.

[philwebb]

AnnotationConfigReactiveWebApplicationContext intentionally mirrors the hierarchy of frameworks AnnotationConfigWebApplicationContext and I think it's mainly used in tests. We should follow the lead of Spring Framework, but I doubt we can change it to not extend AbstractRefreshableConfigApplicationContext at this point.

[snicoll]

@philwebb it's mainly used in tests you are right. As for the regular AnnotationConfigWebApplicationContext it turns out to be in that hierarchy for no proper reason anymore and @jhoeller can't change that at this point.

I've introduced a AnnotationConfigServletWebApplicationContext in
8bf3bfd that, I believe, what Juergen would have liked in framework and use that type in our test infrastructure. I've also changed AnnotationConfigReactiveWebApplicationContext.

[philwebb]

@snicoll I've added a couple of extra commits on a local branch.

The changes you've made look good to me. My only hesitation is with AnnotationConfigReactiveWebApplicationContext since it's a breaking change. We have the same problem as Juergen there. Ideally we'd leave it alone and create a new version, but we really really want to use the existing name. I toyed with AnnotationConfigGenericReactiveWebApplicationContext, but it just sucks!

Since that context is just for reactive code, and since we've probably not got many users working with it directly, I think we should just risk changing the base class. I do think we should perhaps re-introduce deprecated versions of methods from the old class (I've done that on my branch). We can't make them work, but at least we can give people hints that they're deprecated. We should also add additional constructors I think.

Overall, I'm +1 for the changes.

[snicoll]

Thanks for the review @philwebb - FYI, I've dropped philwebb@2ce95ef from your polish. I didn't migrate those on purpose as this module can't have a compile dependency on spring-boot.