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

Allow other strategies than META-INF/spring.factories for configuring Spring Boot infrastructure #15704

Closed
sdeleuze opened this issue Jan 14, 2019 · 12 comments
Labels
status: superseded An issue that has been superseded by another

Comments

@sdeleuze
Copy link
Contributor

sdeleuze commented Jan 14, 2019

In order to make Spring Boot a better citizen with GraalVM native image generation, in Spring Fu I would like to replace META-INF/spring.factories mechanism by a custom way to provide ApplicationContextInitializer, ApplicationListener, PropertySourceLoader, EnvironmentPostProcessor and SpringApplicationRunListener instances. But SpringFactoriesLoader usage is currently impossible to disable. I think @dsyer have also a similar need.

In my use case, the functional configuration provided by the user will be used to provide the implementations. This will make the configuration more modular and will make Boot infrastructure a better citizen with GraalVM native image which requires currently a huge mount of reflection configuration because these classes are defined in a property file invisible to GraalVM static code analysis.

In term of software design, I tend to think providing constructors or setters where one can provide a list of instances instead of using internally META-INF/spring.factories, and making SpringFactoriesLoader only the default way to get implementations for those classes would improve testability, extensibility, and would make dependencies more explicit.

I am also wondering if such improvement could be used to replace SpringFactoriesLoader (which was initially designed to customize Spring Framework internals for Scala support if I am not mistaken) by another mechanism like Java ServiceLoader which is natively compatible with GraalVM native image.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jan 14, 2019
@philwebb
Copy link
Member

philwebb commented Jan 15, 2019

I think there are maybe a few different requirements in this issue that we need to clarify or possibly split out.

In order to make Spring Boot a better citizen with GraalVM native image generation, in Spring Fu I would like to replace META-INF/spring.factories mechanism.

Do we need to totally replace spring.factories in order to have it work with GraalVM? Is the Java ServiceLoader the only way to offer this kind of extensible behavior? We can certainly consider switching to it for some of the things we currently load, but:

  • Not all items in spring.factories are currenlty an implementation of something (for example auto-configuration classes are keys on the annotation class)
  • We don't always want to instantiate instances (for example, auto-configuration needs to use ASM parsing)
  • We can't just drop existing support

SpringFactoriesLoader usage is currently impossible to disable. I think @dsyer have also a similar need.

Is this a different requirement? Which specific usages do you need to disable and why?

GraalVM native image which requires currently a huge mount of reflection configuration because these classes are defined in a property file invisible to GraalVM static code analysis

Are there any other options before we consider a wholesale migration to a different mechanism? Perhaps GraalVM should offer an extension mechanism that would allow us to make it aware of the information without needing an additional configuration?

In term of software design, I tend to think providing constructors or setters where one can provide a list of instances instead of using internally META-INF/spring.factories, and making SpringFactoriesLoader only the default way to get implementations for those classes would improve testability, extensibility, and would make dependencies more explicit.

This is a good point, although I'm not sure it's always possible since we need a way to decouple things. For example, spring-boot-autoconfigure contributes an ConditionEvaluationReportLoggingListener which is picked up by SpringApplication. We can't directly call SpringApplication.setListeners because it's the user that creates the SpringApplication instance. We also don't really want a SpringApplication subclass specifically for auto-configured applications. We need a way for modules to contribute items without the thing that they contribute them too needing to know about them.

I am also wondering if such improvement could be used to replace SpringFactoriesLoader (which was initially designed to customize Spring Framework internals for Scala support if I am not mistaken) by another mechanism like Java ServiceLoader

I wasn't aware of the history of SpringFactoriesLoader, but I'm not sure that that should sway our decision to migrate away from it. Migrating could be an option but we'll probably need to support spring.factories for some time to keep existing code compatible. I also imagine that there are other users of SpringFactoriesLoader that will need to migrate if that's really the only option to get GraalVM support.

@sdeleuze sdeleuze changed the title Allow programmatic configuration instead of META-INF/spring.factories Allow other strategies than META-INF/spring.factories for configuring Boot infrastructure Jan 15, 2019
@dsyer
Copy link
Member

dsyer commented Jan 15, 2019

An example of the problem I think we need to address is that SpringApplication always reads spring.factories in its constructor. It's impossible to override that. It's just a bit too hard to test and/or change. Having a place where we can choose the strategy to contribute list of ApplicationContextInitializer, ApplicationListener, etc. and make spring.factories just the default strategy would help.

So something as simple as protected method in SpringApplication would not be enough, but on the right track. I have been playing with a proof of concept with a customizer interface and an annotation that you can put on your @SpringBootApplication class. (That way tests will pick it up as well, eventually, if we also modify the test context loader). So a user writes public class

@SpringBootApplication
@CustomizedWith(SpringFooCustomizer.class) 
public class MyApplication {...}

And SpringApplication notices this and uses SpringFooCustomizer instead of the default for creating listeners and initializers. In my PoC the customizer can also itself be annotated as @CustomizedWith, so it's easy to compose and inherit the defaults if you want to without the user app having to refer to them directly.

@sdeleuze
Copy link
Contributor Author

sdeleuze commented Jan 15, 2019

I want to clarify that this issue is about making Boot infrastructure configurable with other strategies than using META-INF/spring.factories for configuring its infrastructure. I am not asking a concrete alternative for it (yet).

That would enable experimenting on other flavors like ServiceLoader, @dsyer @SpringApplicationCustomizers, or Spring Fu functional approach where configuration is explicitly done by the user via a DSL that will provide suppliers of listeners, initializers, etc.

I mentioned GraalVM as a concrete use case for such approach. In such native world, ASM parsing is not an option, ServiceLoader is supported by default so may be worth to try, and programmatic configuration can be analyzed by GraalVM compiler, unlike property based class configuration like META-INF/spring.factories. I am currently trying to identify if there are other pluggable mechanisms that could fit our need, and will explore that possibility as well.

SpringFactoriesLoader is currently mandatory because used in:

  • SpringApplication constructor and SpringApplication#run()
  • ConfigFileApplicationListener#loadPostProcessors and ConfigFileApplicationListener.Loader#Loader
  • FailureAnalyzers constructor
  • TemplateAvailabilityProviders constructor
  • etc.

What I would like is a more flexible approach where those classes can be configured by providing List<ApplicationContextInitializer> or List<ApplicationListener> without relying on SpringFactoriesLoader for their implementation, and allow a library/framework/application/test which does not use org.springframework.boot package to provide these List<ApplicationContextInitializer> or List<ApplicationListener> which another mechanism with some extension point to be decided. Having to extend Spring Boot classes to override protected method would be fine to me or any kind of mechanism that does not force me to modify classes like SpringApplication or ConfigFileApplicationListener and duplicate a lot of code.

@snicoll snicoll changed the title Allow other strategies than META-INF/spring.factories for configuring Boot infrastructure Allow other strategies than META-INF/spring.factories for configuring Spring Boot infrastructure Jan 21, 2019
@wilkinsona wilkinsona added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Feb 7, 2019
@wilkinsona wilkinsona added this to the 2.2.x milestone Feb 7, 2019
@wilkinsona wilkinsona modified the milestones: 2.2.x, 2.x Apr 17, 2019
@wilkinsona
Copy link
Member

It's not clear that the benefits on offer here justify the time it'll take to implement. The opportunity cost is too high at the moment.

@dsyer
Copy link
Member

dsyer commented Apr 17, 2019

What? But we had a prototype and a plan to improve it. The cost of implementation is not high, is it? Or was there something missing?

@wilkinsona
Copy link
Member

There was stuff missing (#15955 and #15956). We need to implement all three issues to have a coherent solution and, unfortunately, I don't think we have time right now. We can look at reprioritising things if you really need this in 2.2.

@dsyer
Copy link
Member

dsyer commented Apr 17, 2019

I can see there are still some design decisions pending (#15956 in particular). The rest seems mainly mechanical, and it would be much appreciated in 2.2. I can send some pull requests next week probably, if it helps.

@wilkinsona
Copy link
Member

Some pull requests could certainly help. It would also help to have some more information about exactly what use case the functionality will unlock. We still don't feel like we understand the full picture. Also, in discussing this general requirement with @sdeleuze recently, he was in agreement that waiting till after 2.2 would give us the time to know exactly what's needed and the form that it should take.

@dsyer
Copy link
Member

dsyer commented Apr 30, 2019

Here's a concrete proposal: https://github.com/dsyer/spring-boot/tree/gh-15704. It takes the prototype from @wilkinsona and strips down the ExtensionResolver, leaving the 2 extensions with non-default constructors as protected methods in SpringApplication (instead of spring.factories). Notwithstanding the discussions in #15955 and #15956, this seems like a good compromise to me because the non-default constructor contract was always awkward, and they are really uncommon extension points (I think I used the run listener one once).

@wilkinsona wilkinsona added the for: team-attention An issue we'd like other members of the team to review label May 9, 2019
@philwebb philwebb modified the milestones: 2.x, 2.2.x May 10, 2019
@wilkinsona wilkinsona removed the for: team-attention An issue we'd like other members of the team to review label May 10, 2019
@wilkinsona wilkinsona self-assigned this May 10, 2019
@wilkinsona
Copy link
Member

wilkinsona commented May 22, 2019

We had moved this into 2.2.x, but #16880 has muddied the waters with this one again so it's back in 2.x now. In addition to #16880, we also have Andy Clement's Graal feature work with which I think there's some overlap here. We're back to really needing an answer to this:

It would also help to have some more information about exactly what use case the functionality will unlock.

Without it, I don't think we can justify adding public API that may be quickly superseded by #16880 or the Graal feature work.

@wilkinsona wilkinsona modified the milestones: 2.2.x, 2.x May 22, 2019
@dsyer
Copy link
Member

dsyer commented May 22, 2019

I think if we implemented #16880 I would say this would be low priority again for me. The main use case is really just flexibility, and the ability to override something without needing reflection. But I don't think that's urgent for 2.2 if we can add the conditional processing.

@wilkinsona wilkinsona removed their assignment May 28, 2019
@sdeleuze
Copy link
Contributor Author

sdeleuze commented Sep 7, 2020

I agree and I also think we should at some point improve spring.factories support on Spring Framework side to have a better support for Spring Boot advanced patterns. I close this issue in order to focus on #16880.

@sdeleuze sdeleuze closed this as completed Sep 7, 2020
@snicoll snicoll added status: superseded An issue that has been superseded by another and removed theme: graal type: enhancement A general enhancement labels Sep 7, 2020
@snicoll snicoll removed this from the 2.x milestone Sep 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: superseded An issue that has been superseded by another
Projects
None yet
Development

No branches or pull requests

6 participants