Copilot commented on code in PR #11048:
URL: https://github.com/apache/cloudstack/pull/11048#discussion_r3202644704
##########
framework/spring/module/src/main/java/org/apache/cloudstack/spring/module/model/impl/DefaultModuleDefinitionSet.java:
##########
@@ -314,24 +318,35 @@ public Map<String, ApplicationContext> getContextMap() {
@Override
public Resource[] getConfigResources(String name) {
- Set<Resource> resources = new LinkedHashSet<Resource>();
-
- ModuleDefinition original = null;
- ModuleDefinition def = original = modules.get(name);
-
- if (def == null)
+ ModuleDefinition def = modules.get(name);
+ if (def == null) {
return new Resource[] {};
+ }
+
+ Set<Resource> resources = new LinkedHashSet<>();
resources.addAll(def.getContextLocations());
- while (def != null) {
- resources.addAll(def.getInheritableContextLocations());
- def = modules.get(def.getParentName());
+ resources.addAll(collectInheritedResources(def));
+
+ resources.addAll(def.getOverrideContextLocations());
+
+ return resources.toArray(Resource[]::new);
+ }
+
+ private Set<Resource> collectInheritedResources(final ModuleDefinition
def) {
+ if (def == null) {
+ return Collections.emptySet();
}
- resources.addAll(original.getOverrideContextLocations());
+ if (configResourcesMap.containsKey(def.getName())) {
+ return configResourcesMap.get(def.getName());
Review Comment:
`collectInheritedResources` does a `containsKey` + `get` on
`configResourcesMap`, which is a redundant double lookup. Prefer a single `get`
with a null check (or `computeIfAbsent`) to keep the memoization fast and
simpler.
##########
framework/spring/module/src/main/java/org/apache/cloudstack/spring/module/model/impl/DefaultModuleDefinitionSet.java:
##########
@@ -61,6 +62,9 @@ public class DefaultModuleDefinitionSet implements
ModuleDefinitionSet {
String root;
Map<String, ModuleDefinition> modules;
Map<String, ApplicationContext> contexts = new HashMap<String,
ApplicationContext>();
+
+ Map<String, Set<Resource>> configResourcesMap = new HashMap<String,
Set<Resource>>();
Review Comment:
`configResourcesMap` is used to memoize only *inheritable* (parent-chain)
context resources, not the full set of config resources. The current name makes
it easy to misinterpret the cache’s contents and could lead to incorrect future
reuse. Consider renaming it to reflect that it stores inherited/inheritable
context resources (or alternatively cache the full result of
`getConfigResources`).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
