Replace Optional with Spring's ObjectProvider to manage the optionali… #5573
Conversation
kratosmy
force-pushed
the
main
branch
2 times, most recently
from
c163eda
to
5814bea
Compare
…ty of injection points more clearly.
| if (!armeriaServerConfigurators.stream().findAny().isPresent() && | ||
| !armeriaServerBuilderConsumers.stream().findAny().isPresent()) { |
There was a problem hiding this comment.
Optional) Not strong on this, but intelliJ is complaining and generally inverse conditions are slightly harder to reason about (although I think this case is simple enough)
| if (!armeriaServerConfigurators.stream().findAny().isPresent() && | |
| !armeriaServerBuilderConsumers.stream().findAny().isPresent()) { | |
| if (armeriaServerConfigurators.stream().findAny().isEmpty() && | |
| armeriaServerBuilderConsumers.stream().findAny().isEmpty()) { |
There was a problem hiding this comment.
Thanks for your suggestion and yes I have used isEmpty() at first, but local test failed due to this, not sure the exact reason because error message is garbled characters.
There was a problem hiding this comment.
Optional.isEmpty() is introduced in Java 11 but our baseline of Java is 8.
| @@ -69,16 +69,16 @@ public Server armeriaServer( | |||
| ArmeriaSettings armeriaSettings, | |||
| InternalServices internalService, | |||
| Optional<MeterRegistry> meterRegistry, | |||
| Optional<List<MetricCollectingServiceConfigurator>> metricCollectingServiceConfigurators, | |||
| ObjectProvider<MetricCollectingServiceConfigurator> metricCollectingServiceConfigurators, | |||
There was a problem hiding this comment.
Note: This API has been around since 4.3, so there's probably next to no user impact. The change is now that beans are correctly ordered by spring's priority system
| @@ -69,16 +69,16 @@ public Server armeriaServer( | |||
| ArmeriaSettings armeriaSettings, | |||
| InternalServices internalService, | |||
| Optional<MeterRegistry> meterRegistry, | |||
There was a problem hiding this comment.
Question: Can we also use ObjectProvider for this type?
There was a problem hiding this comment.
Yes, we can. But it may require something like meterIdPrefixFunction.getIfAvailable() ? meterIdPrefixFunction.getIfAvailable() : MeterIdPrefixFunction.ofDefault("armeria.server"), seems not so concise.
There was a problem hiding this comment.
Yeah, let's keep it as is. Thanks!
Motivation:
Optional is not as compatible as ObjectProvider, which is included in Spring Framework since Spring 4.3, so use ObjectProvider would improve the programmatic resolution of Beans.
Modifications:
Two file are affected, AbstractArmeriaAutoConfiguration.java and ArmeriaReactiveWebServerFactoryAutoConfiguration.java
Result:
Closes #5527 .