Refactored usage of Optional<> as method parameters - #2720
Conversation
There was a problem hiding this comment.
Pull request overview
Refactors the config metatype lookup helper and its consumers to avoid using Optional as a method parameter (and as a generic parameter for functional interface callbacks), simplifying callback usage across the config command code.
Changes:
- Updated
MetaServiceCaller.doWithMetaTypeto acceptFunction<MetaInfo, T>instead ofFunction<Optional<MetaInfo>, T>. - Updated
MetaCommandcallbacks to work withMetaInfodirectly (with null handling). - Refactored
ConfigurationPropertyCompletermetatype property collection to avoidOptional<MetaInfo>parameters and use a stream-based collection approach.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| config/core/src/main/java/org/apache/karaf/config/core/MetaServiceCaller.java | Changes the metatype callback signature away from Optional and adjusts invocation behavior. |
| config/command/src/main/java/org/apache/karaf/config/command/MetaCommand.java | Updates command implementation callbacks to match the new doWithMetaType function signature. |
| config/command/src/main/java/org/apache/karaf/config/command/completers/ConfigurationPropertyCompleter.java | Updates metatype-property extraction logic to avoid Optional parameters and collect properties via streams. |
Comments suppressed due to low confidence (1)
config/core/src/main/java/org/apache/karaf/config/core/MetaServiceCaller.java:89
doWithMetaTypeno longer invokes the callback when no metatype is found (or whenMetaTypeServiceis unavailable) and returnsnullinstead. Call sites currently rely on the callback being invoked with a "missing" value (e.g.,ConfigurationPropertyCompleterexpects a non-nullListtoaddAll, andMetaCommandexpects to print a "No meta type definition" message). This change can lead to aNullPointerExceptionand/or silently skipping output.
public static <T> T doWithMetaType(BundleContext context, String pid, Function<MetaInfo, T> function) {
ServiceReference<MetaTypeService> ref = context.getServiceReference(MetaTypeService.class);
if (ref != null) {
try {
MetaTypeService metaService = context.getService(ref);
MetaInfo metaInfo = getMetatype(context, metaService, pid);
if (metaInfo != null) {
return function.apply(metaInfo);
}
} finally {
context.ungetService(ref);
}
}
return null;
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| MetaTypeService metaService = context.getService(ref); | ||
| MetaInfo metaInfo = getMetatype(context, metaService, pid); | ||
| return function.apply(Optional.ofNullable(metaInfo)); | ||
| if (metaInfo != null) { |
There was a problem hiding this comment.
This change seems to have an effect on semantics for the null case: before, function.apply of an empty Optional was returned, now the fall-through may just return null.
This could lead to an NPE in the statement from the other file above (return MetaServiceCaller.doWithMetaType(...)).
There's only one place that uses
Optionalas method parameters, and it is generally recommended to be avioded. I've reworked the use case so that theOptionals are no longer used as method parameters or generic parameters for other functional interfaces