stefanseifert commented on code in PR #6:
URL:
https://github.com/apache/sling-org-apache-sling-caconfig-impl/pull/6#discussion_r1054150738
##########
src/main/java/org/apache/sling/caconfig/impl/ConfigurationResolverImpl.java:
##########
@@ -83,15 +91,54 @@ private void activate(Config config) {
@Override
public @NotNull ConfigurationBuilder get(@NotNull Resource resource) {
- return new ConfigurationBuilderImpl(resource, this,
- configurationResourceResolvingStrategy,
configurationPersistenceStrategy,
- configurationInheritanceStrategy,
configurationOverrideMultiplexer, configurationMetadataProvider,
- configBucketNames);
+ if (resource == null) {
+ return buildFor(resource);
Review Comment:
i do not understand this line - it is dead code assuming resource is never
null according to @NotNull annotation
##########
src/main/java/org/apache/sling/caconfig/impl/ConfigurationResolverImpl.java:
##########
@@ -83,15 +91,54 @@ private void activate(Config config) {
@Override
public @NotNull ConfigurationBuilder get(@NotNull Resource resource) {
- return new ConfigurationBuilderImpl(resource, this,
- configurationResourceResolvingStrategy,
configurationPersistenceStrategy,
- configurationInheritanceStrategy,
configurationOverrideMultiplexer, configurationMetadataProvider,
- configBucketNames);
+ if (resource == null) {
+ return buildFor(resource);
+ }
+ return resolveFromCache(resource);
}
@Override
public @NotNull Collection<String> configBucketNames() {
return configBucketNames;
}
+
+ /**
+ * Resolve the ConfigurationBuilder
+ * already resolved instances are stored in the PropertyMap of the
ResourceResolver, which shares
+ * its life-cycle with the ResourceResolver; so we don't need to care
about any cleanup.
+ * @param resource the resource
+ * @return the resolved ConfigurationBuilder or null
+ */
+ private @NotNull ConfigurationBuilder resolveFromCache (@NotNull Resource
resource) {
+
+ ResourceResolver rr = resource.getResourceResolver();
+ Map<String,Object> map = rr.getPropertyMap();
+
+ @SuppressWarnings("unchecked")
+ Map<String,ConfigurationBuilder> configurationBuilderCache =
(Map<String, ConfigurationBuilder>) map.get(CACHE_KEY);
+ if (configurationBuilderCache == null) {
+ configurationBuilderCache = new HashMap<>();
+ map.put(CACHE_KEY, configurationBuilderCache);
+ }
+
+ if (!configurationBuilderCache.containsKey(resource.getPath())) {
+ ConfigurationBuilder cb = buildFor(resource);
+ configurationBuilderCache.put(resource.getPath(), cb);
+ LOG.trace("created configurationBuilde for {}",resource.getPath());
Review Comment:
typo -> configurationBuilder
##########
src/main/java/org/apache/sling/caconfig/impl/ConfigurationResolverImpl.java:
##########
@@ -83,15 +91,54 @@ private void activate(Config config) {
@Override
public @NotNull ConfigurationBuilder get(@NotNull Resource resource) {
- return new ConfigurationBuilderImpl(resource, this,
- configurationResourceResolvingStrategy,
configurationPersistenceStrategy,
- configurationInheritanceStrategy,
configurationOverrideMultiplexer, configurationMetadataProvider,
- configBucketNames);
+ if (resource == null) {
+ return buildFor(resource);
+ }
+ return resolveFromCache(resource);
}
@Override
public @NotNull Collection<String> configBucketNames() {
return configBucketNames;
}
+
+ /**
+ * Resolve the ConfigurationBuilder
+ * already resolved instances are stored in the PropertyMap of the
ResourceResolver, which shares
+ * its life-cycle with the ResourceResolver; so we don't need to care
about any cleanup.
+ * @param resource the resource
+ * @return the resolved ConfigurationBuilder or null
+ */
+ private @NotNull ConfigurationBuilder resolveFromCache (@NotNull Resource
resource) {
+
+ ResourceResolver rr = resource.getResourceResolver();
+ Map<String,Object> map = rr.getPropertyMap();
+
+ @SuppressWarnings("unchecked")
+ Map<String,ConfigurationBuilder> configurationBuilderCache =
(Map<String, ConfigurationBuilder>) map.get(CACHE_KEY);
+ if (configurationBuilderCache == null) {
+ configurationBuilderCache = new HashMap<>();
+ map.put(CACHE_KEY, configurationBuilderCache);
+ }
+
+ if (!configurationBuilderCache.containsKey(resource.getPath())) {
Review Comment:
consider using Map.computeIfAbsent (here and also above for the property map)
--
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]
