This is an automated email from the ASF dual-hosted git repository.
reschke pushed a commit to branch master
in repository
https://gitbox.apache.org/repos/asf/sling-org-apache-sling-resourceresolver.git
The following commit(s) were added to refs/heads/master by this push:
new 6459b96 SLING-12402: ResourceResolver: add additional log entry when
there are many conflicting or invalid aliases (also add metrics) (#125)
6459b96 is described below
commit 6459b962980084edf4daa8c08ddc33ead9ff1acd
Author: Julian Reschke <[email protected]>
AuthorDate: Tue Oct 29 17:01:51 2024 +0100
SLING-12402: ResourceResolver: add additional log entry when there are many
conflicting or invalid aliases (also add metrics) (#125)
* SLING-12402: metrics for broken aliases (WIP)
* SLING-12402: metrics for conflicting aliases (WIP)
* SLING-12402: metrics for conflicting aliases - adjust log message (WIP)
* SLING-12402: metrics for defunct aliases - add warning after init when
over (minimal) threshold
* SLING-12402: metrics for defunct aliases - add warning after init when
over (minimal) threshold - whitespace fix
* SLING-12402: metrics for defunct aliases - add warning after init when
over (minimal) threshold - improve test coverage and fix typos in metrics names
(sic)
* SLING-12402: metrics for defunct aliases - add warning after init when
over (minimal) threshold - slightly simplify tests
* Update
src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java
Co-authored-by: Jörg Hoh <[email protected]>
* Update
src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java
Co-authored-by: Jörg Hoh <[email protected]>
* Update
src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java
Co-authored-by: Jörg Hoh <[email protected]>
* SLING-12402: metrics for defunct aliases - add warning after init when
over (minimal) threshold - restore metrics names
* SLING-12402: metrics for defunct aliases - add warning after init when
over (minimal) threshold - explain why we clear the 'conflicts queues'
* SLING-12402: metrics for defunct aliases - add warning after init when
over (minimal) threshold - tune logging
* SLING-12402: metrics for defunct aliases - add warning after init when
over (minimal) threshold - fix javadoc
* SLING-12402: metrics for defunct aliases - add warning after init when
over (minimal) threshold - transform collecting lists into method parameters
* SLING-12402: metrics for defunct aliases - add warning after init when
over (minimal) threshold - remove unused code
* SLING-12402: metrics for defunct aliases - fix log message templates
* SLING-12402: metrics for defunct aliases - refactor loadAlias
* SLING-12402: metrics for defunct aliases - refactor loadAlias
* SLING-12402: metrics for defunct aliases - reduce Cognitive Complexity in
refactored code once more
---------
Co-authored-by: Jörg Hoh <[email protected]>
---
.../impl/ResourceResolverMetrics.java | 77 +++++++----
.../resourceresolver/impl/mapping/MapEntries.java | 151 +++++++++++++--------
.../impl/ResourceResolverMetricsTest.java | 86 +++++++++---
.../impl/mapping/MapEntriesTest.java | 19 ++-
4 files changed, 231 insertions(+), 102 deletions(-)
diff --git
a/src/main/java/org/apache/sling/resourceresolver/impl/ResourceResolverMetrics.java
b/src/main/java/org/apache/sling/resourceresolver/impl/ResourceResolverMetrics.java
index c073727..4c19e48 100644
---
a/src/main/java/org/apache/sling/resourceresolver/impl/ResourceResolverMetrics.java
+++
b/src/main/java/org/apache/sling/resourceresolver/impl/ResourceResolverMetrics.java
@@ -36,14 +36,16 @@ import org.osgi.service.component.annotations.Reference;
* Export metrics for the resource resolver bundle:
*
* org.apache.sling.resourceresolver.numberOfResourcesWithAliasesOnStartup --
the total number of resources with sling:alias properties found on startup
+ * org.apache.sling.resourceresolver.numberOfDetectedConflictingAliases --
the total number of detected invalid aliases
+ * org.apache.sling.resourceresolver.numberOfDetectedInvalidAliases -- the
total number of detected invalid aliases
+ *
*
org.apache.sling.resourceresolver.numberOfResourcesWithVanityPathsOnStartup --
the total number of resources with sling:vanityPath properties found on startup
- * org.apache.sling.resourceresolver.numberOfVanityPaths -- the total number
of vanity paths in the cache
- * org.apache.sling.resourceresolver.numberOfVanityPathLookups -- the total
number of vanity path lookups
- * org.apache.sling.resourceresolver.numberOfVanityPathBloomNegatives -- the
total number of vanity path lookups filtered by the bloom filter
* org.apache.sling.resourceresolver.numberOfVanityPathBloomFalsePositives --
the total number of vanity path lookup that passed the bloom filter but were
false positives
- * org.apache.sling.resourceresolver.numberOfAliases -- the total number of
aliases
- * org.apache.sling.resourceresolver.unclosedResourceResolvers -- the total
number of unclosed resource resolvers
+ * org.apache.sling.resourceresolver.numberOfVanityPathBloomNegatives -- the
total number of vanity path lookups filtered by the bloom filter
+ * org.apache.sling.resourceresolver.numberOfVanityPathLookups -- the total
number of vanity path lookups
+ * org.apache.sling.resourceresolver.numberOfVanityPaths -- the total number
of vanity paths in the cache
*
+ * org.apache.sling.resourceresolver.unclosedResourceResolvers -- the total
number of unclosed resource resolvers
*/
@@ -70,12 +72,12 @@ public class ResourceResolverMetrics {
private Supplier<Long> numberOfVanityPathLookupsSupplier = ZERO_SUPPLIER;
// number of vanity path lookups filtered by Bloom filter
- private ServiceRegistration<Gauge<Long>>
numberOfVanityPathBloomNegativeGauge;
- private Supplier<Long> numberOfVanityPathBloomNegativeSupplier =
ZERO_SUPPLIER;
+ private ServiceRegistration<Gauge<Long>>
numberOfVanityPathBloomNegativesGauge;
+ private Supplier<Long> numberOfVanityPathBloomNegativesSupplier =
ZERO_SUPPLIER;
// number of vanity path lookups passing the Bloom filter but being false
positives
- private ServiceRegistration<Gauge<Long>>
numberOfVanityPathBloomFalsePositiveGauge;
- private Supplier<Long> numberOfVanityPathBloomFalsePositiveSupplier =
ZERO_SUPPLIER;
+ private ServiceRegistration<Gauge<Long>>
numberOfVanityPathBloomFalsePositivesGauge;
+ private Supplier<Long> numberOfVanityPathBloomFalsePositivesSupplier =
ZERO_SUPPLIER;
// number of resources with aliased children
private ServiceRegistration<Gauge<Long>>
numberOfResourcesWithAliasedChildrenGauge;
@@ -85,18 +87,27 @@ public class ResourceResolverMetrics {
private ServiceRegistration<Gauge<Long>>
numberOfResourcesWithAliasesOnStartupGauge;
private Supplier<Long> numberOfResourcesWithAliasesOnStartupSupplier =
ZERO_SUPPLIER;
- private Counter unclosedResourceResolvers;
+ // total number of detected invalid aliases on startup
+ private ServiceRegistration<Gauge<Long>>
numberOfDetectedInvalidAliasesGauge;
+ private Supplier<Long> numberOfDetectedInvalidAliasesSupplier =
ZERO_SUPPLIER;
+
+ // total number of detected conflicting aliases on startup
+ private ServiceRegistration<Gauge<Long>>
numberOfDetectedConflictingAliasesGauge;
+ private Supplier<Long> numberOfDetectedConflictingAliasesSupplier =
ZERO_SUPPLIER;
+ private Counter unclosedResourceResolvers;
@Activate
protected void activate(BundleContext bundleContext) {
- numberOfVanityPathsGauge = registerGauge(bundleContext, METRICS_PREFIX
+ ".numberOfVanityPaths", () -> numberOfVanityPathsSupplier );
- numberOfResourcesWithVanityPathsOnStartupGauge =
registerGauge(bundleContext, METRICS_PREFIX +
".numberOfResourcesWithVanityPathsOnStartup", () ->
numberOfResourcesWithVanityPathsOnStartupSupplier );
- numberOfVanityPathLookupsGauge = registerGauge(bundleContext,
METRICS_PREFIX + ".numberOfVanityPathLookups", () ->
numberOfVanityPathLookupsSupplier );
- numberOfVanityPathBloomNegativeGauge = registerGauge(bundleContext,
METRICS_PREFIX + ".numberOfVanityPathBloomNegatives", () ->
numberOfVanityPathBloomNegativeSupplier );
- numberOfVanityPathBloomFalsePositiveGauge =
registerGauge(bundleContext, METRICS_PREFIX +
".numberOfVanityPathBloomFalsePositives", () ->
numberOfVanityPathBloomFalsePositiveSupplier );
+ numberOfVanityPathsGauge = registerGauge(bundleContext, METRICS_PREFIX
+ ".numberOfVanityPaths", () -> numberOfVanityPathsSupplier);
+ numberOfResourcesWithVanityPathsOnStartupGauge =
registerGauge(bundleContext, METRICS_PREFIX +
".numberOfResourcesWithVanityPathsOnStartup", () ->
numberOfResourcesWithVanityPathsOnStartupSupplier);
+ numberOfVanityPathLookupsGauge = registerGauge(bundleContext,
METRICS_PREFIX + ".numberOfVanityPathLookups", () ->
numberOfVanityPathLookupsSupplier);
+ numberOfVanityPathBloomNegativesGauge = registerGauge(bundleContext,
METRICS_PREFIX + ".numberOfVanityPathBloomNegatives", () ->
numberOfVanityPathBloomNegativesSupplier);
+ numberOfVanityPathBloomFalsePositivesGauge =
registerGauge(bundleContext, METRICS_PREFIX +
".numberOfVanityPathBloomFalsePositives", () ->
numberOfVanityPathBloomFalsePositivesSupplier);
numberOfResourcesWithAliasedChildrenGauge =
registerGauge(bundleContext, METRICS_PREFIX +
".numberOfResourcesWithAliasedChildren", () ->
numberOfResourcesWithAliasedChildrenSupplier);
- numberOfResourcesWithAliasesOnStartupGauge =
registerGauge(bundleContext, METRICS_PREFIX +
".numberOfResourcesWithAliasesOnStartup", () ->
numberOfResourcesWithAliasesOnStartupSupplier );
+ numberOfResourcesWithAliasesOnStartupGauge =
registerGauge(bundleContext, METRICS_PREFIX +
".numberOfResourcesWithAliasesOnStartup", () ->
numberOfResourcesWithAliasesOnStartupSupplier);
+ numberOfDetectedInvalidAliasesGauge = registerGauge(bundleContext,
METRICS_PREFIX + ".numberOfDetectedInvalidAliases", () ->
numberOfDetectedInvalidAliasesSupplier);
+ numberOfDetectedConflictingAliasesGauge = registerGauge(bundleContext,
METRICS_PREFIX + ".numberOfDetectedConflictingAliases", () ->
numberOfDetectedConflictingAliasesSupplier);
unclosedResourceResolvers = metricsService.counter(METRICS_PREFIX +
".unclosedResourceResolvers");
}
@@ -105,10 +116,12 @@ public class ResourceResolverMetrics {
numberOfVanityPathsGauge.unregister();
numberOfResourcesWithVanityPathsOnStartupGauge.unregister();
numberOfVanityPathLookupsGauge.unregister();
- numberOfVanityPathBloomNegativeGauge.unregister();
- numberOfVanityPathBloomFalsePositiveGauge.unregister();
- numberOfResourcesWithAliasesOnStartupGauge.unregister();
+ numberOfVanityPathBloomNegativesGauge.unregister();
+ numberOfVanityPathBloomFalsePositivesGauge.unregister();
numberOfResourcesWithAliasedChildrenGauge.unregister();
+ numberOfResourcesWithAliasesOnStartupGauge.unregister();
+ numberOfDetectedInvalidAliasesGauge.unregister();
+ numberOfDetectedConflictingAliasesGauge.unregister();
}
/**
@@ -139,16 +152,16 @@ public class ResourceResolverMetrics {
* Set the number of vanity path lookups in the system that were filtered
* @param supplier a supplier returning the number of vanity path lookups
*/
- public void setNumberOfVanityPathBloomNegativeSupplier(Supplier<Long>
supplier) {
- numberOfVanityPathBloomNegativeSupplier = supplier;
+ public void setNumberOfVanityPathBloomNegativesSupplier(Supplier<Long>
supplier) {
+ numberOfVanityPathBloomNegativesSupplier = supplier;
}
/**
* Set the number of vanity path lookups in the system that were not
catched by the Bloom filter
* @param supplier a supplier returning the number of vanity path lookups
*/
- public void setNumberOfVanityPathBloomFalsePositiveSupplier(Supplier<Long>
supplier) {
- numberOfVanityPathBloomFalsePositiveSupplier = supplier;
+ public void
setNumberOfVanityPathBloomFalsePositivesSupplier(Supplier<Long> supplier) {
+ numberOfVanityPathBloomFalsePositivesSupplier = supplier;
}
/**
@@ -167,13 +180,29 @@ public class ResourceResolverMetrics {
numberOfResourcesWithAliasesOnStartupSupplier = supplier;
}
+ /**
+ * Set the supplier for the number of invalid aliases
+ * @param supplier a supplier returning the number of invalid aliases
+ */
+ public void setNumberOfDetectedInvalidAliasesSupplier(Supplier<Long>
supplier) {
+ numberOfDetectedInvalidAliasesSupplier = supplier;
+ }
+
+ /**
+ * Set the supplier for the number of duplicate aliases
+ * @param supplier a supplier returning the number of conflicting aliases
+ */
+ public void setNumberOfDetectedConflictingAliasesSupplier(Supplier<Long>
supplier) {
+ numberOfDetectedConflictingAliasesSupplier = supplier;
+ }
+
/**
* Increment the counter for the number of unresolved resource resolvers
*/
public void reportUnclosedResourceResolver() {
unclosedResourceResolvers.increment();
}
-
+
/**
* Create a gauge metrics.
*
diff --git
a/src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java
b/src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java
index 44b83df..801c66c 100644
---
a/src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java
+++
b/src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java
@@ -146,14 +146,19 @@ public class MapEntries implements
private Map<String, Map<String, Collection<String>>> aliasMapsMap;
private final AtomicLong aliasResourcesOnStartup;
+ private final AtomicLong detectedConflictingAliases;
+ private final AtomicLong detectedInvalidAliases;
+
+ // keep track of some defunct aliases for diagnostics (thus size-limited)
+ private static final int MAX_REPORT_DEFUNCT_ALIASES = 50;
private final ReentrantLock initializing = new ReentrantLock();
private final AtomicLong vanityCounter;
private final AtomicLong vanityResourcesOnStartup;
private final AtomicLong vanityPathLookups;
- private final AtomicLong vanityPathBloomNegative;
- private final AtomicLong vanityPathBloomFalsePositive;
+ private final AtomicLong vanityPathBloomNegatives;
+ private final AtomicLong vanityPathBloomFalsePositives;
private byte[] vanityBloomFilter;
@@ -180,6 +185,8 @@ public class MapEntries implements
this.stringInterpolationProvider = stringInterpolationProvider;
this.aliasResourcesOnStartup = new AtomicLong(0);
+ this.detectedConflictingAliases = new AtomicLong(0);
+ this.detectedInvalidAliases = new AtomicLong(0);
this.useOptimizeAliasResolution = doInit();
@@ -188,8 +195,9 @@ public class MapEntries implements
this.vanityCounter = new AtomicLong(0);
this.vanityResourcesOnStartup = new AtomicLong(0);
this.vanityPathLookups = new AtomicLong(0);
- this.vanityPathBloomNegative = new AtomicLong(0);
- this.vanityPathBloomFalsePositive = new AtomicLong(0);
+ this.vanityPathBloomNegatives = new AtomicLong(0);
+ this.vanityPathBloomFalsePositives = new AtomicLong(0);
+
initializeVanityPaths();
this.metrics = metrics;
@@ -197,10 +205,13 @@ public class MapEntries implements
this.metrics.get().setNumberOfVanityPathsSupplier(vanityCounter::get);
this.metrics.get().setNumberOfResourcesWithVanityPathsOnStartupSupplier(vanityResourcesOnStartup::get);
this.metrics.get().setNumberOfVanityPathLookupsSupplier(vanityPathLookups::get);
-
this.metrics.get().setNumberOfVanityPathBloomNegativeSupplier(vanityPathBloomNegative::get);
-
this.metrics.get().setNumberOfVanityPathBloomFalsePositiveSupplier(vanityPathBloomFalsePositive::get);
+
this.metrics.get().setNumberOfVanityPathBloomNegativesSupplier(vanityPathBloomNegatives::get);
+
this.metrics.get().setNumberOfVanityPathBloomFalsePositivesSupplier(vanityPathBloomFalsePositives::get);
this.metrics.get().setNumberOfResourcesWithAliasedChildrenSupplier(() -> (long)
aliasMapsMap.size());
this.metrics.get().setNumberOfResourcesWithAliasesOnStartupSupplier(aliasResourcesOnStartup::get);
+
this.metrics.get().setNumberOfDetectedConflictingAliasesSupplier(detectedConflictingAliases::get);
+
this.metrics.get().setNumberOfDetectedInvalidAliasesSupplier(detectedInvalidAliases::get);
+
this.metrics.get().setNumberOfResourcesWithAliasesOnStartupSupplier(detectedInvalidAliases::get);
}
}
@@ -235,14 +246,28 @@ public class MapEntries implements
return this.factory.isOptimizeAliasResolutionEnabled();
}
+ List<String> conflictingAliases = new ArrayList<>();
+ List<String> invalidAliases = new ArrayList<>();
+
boolean isOptimizeAliasResolutionEnabled =
this.factory.isOptimizeAliasResolutionEnabled();
//optimization made in SLING-2521
if (isOptimizeAliasResolutionEnabled) {
try {
- final Map<String, Map<String, Collection<String>>>
loadedMap = this.loadAliases(resolver);
+ final Map<String, Map<String, Collection<String>>>
loadedMap = this.loadAliases(resolver, conflictingAliases, invalidAliases);
this.aliasMapsMap = loadedMap;
-
+
+ // warn if there are more than a few defunct aliases
+ if (conflictingAliases.size() >=
MAX_REPORT_DEFUNCT_ALIASES) {
+ log.warn("There are {} conflicting aliases; excerpt:
{}", conflictingAliases.size(), conflictingAliases);
+ } else if (!conflictingAliases.isEmpty()) {
+ log.warn("There are {} conflicting aliases: {}",
conflictingAliases.size(), conflictingAliases);
+ }
+ if (invalidAliases.size() >= MAX_REPORT_DEFUNCT_ALIASES) {
+ log.warn("There are {} invalid aliases; excerpt: {}",
invalidAliases.size(), invalidAliases);
+ } else if (!invalidAliases.isEmpty()) {
+ log.warn("There are {} invalid aliases: {}",
invalidAliases.size(), invalidAliases);
+ }
} catch (final Exception e) {
logDisableAliasOptimization(e);
@@ -580,7 +605,7 @@ public class MapEntries implements
}
private boolean doAddAlias(final Resource resource) {
- return loadAlias(resource, this.aliasMapsMap);
+ return loadAlias(resource, this.aliasMapsMap, null, null);
}
/**
@@ -737,8 +762,8 @@ public class MapEntries implements
if (current >= Long.MAX_VALUE - 100000) {
// reset counters when we get close the limit
this.vanityPathLookups.set(1);
- this.vanityPathBloomNegative.set(0);
- this.vanityPathBloomFalsePositive.set(0);
+ this.vanityPathBloomNegatives.set(0);
+ this.vanityPathBloomFalsePositives.set(0);
log.info("Vanity Path metrics reset to 0");
}
@@ -748,7 +773,7 @@ public class MapEntries implements
if (!probablyPresent) {
// filtered by Bloom filter
- this.vanityPathBloomNegative.incrementAndGet();
+ this.vanityPathBloomNegatives.incrementAndGet();
}
}
@@ -777,7 +802,7 @@ public class MapEntries implements
}
if (mapEntries == null && probablyPresent) {
// Bloom filter had a false positive
- this.vanityPathBloomFalsePositive.incrementAndGet();
+ this.vanityPathBloomFalsePositives.incrementAndGet();
}
}
@@ -1156,7 +1181,9 @@ public class MapEntries implements
* Load aliases - Search for all nodes (except under /jcr:system) below
* configured alias locations having the sling:alias property
*/
- private Map<String, Map<String, Collection<String>>> loadAliases(final
ResourceResolver resolver) {
+ private Map<String, Map<String, Collection<String>>> loadAliases(final
ResourceResolver resolver,
+ List<String> conflictingAliases, List<String> invalidAliases) {
+
final Map<String, Map<String, Collection<String>>> map = new
ConcurrentHashMap<>();
final String baseQueryString = generateAliasQuery();
@@ -1177,7 +1204,7 @@ public class MapEntries implements
long processStart = System.nanoTime();
while (it.hasNext()) {
count += 1;
- loadAlias(it.next(), map);
+ loadAlias(it.next(), map, conflictingAliases, invalidAliases);
}
long processElapsed = System.nanoTime() - processStart;
long resourcePerSecond = (count * TimeUnit.SECONDS.toNanos(1) /
(processElapsed == 0 ? 1 : processElapsed));
@@ -1231,7 +1258,8 @@ public class MapEntries implements
/**
* Load alias given a resource
*/
- private boolean loadAlias(final Resource resource, Map<String, Map<String,
Collection<String>>> map) {
+ private boolean loadAlias(final Resource resource, Map<String, Map<String,
Collection<String>>> map,
+ List<String> conflictingAliases, List<String> invalidAliases) {
// resource containing the alias
final Resource containingResource = getResourceToBeAliased(resource);
@@ -1239,55 +1267,68 @@ public class MapEntries implements
if (containingResource == null) {
log.warn("containingResource is null for alias on {}, skipping.",
resource.getPath());
return false;
- }
-
- final Resource parent = containingResource.getParent();
+ } else {
+ final Resource parent = containingResource.getParent();
- if (parent == null) {
- log.warn("{} is null for alias on {}, skipping.",
containingResource == resource ? "parent" : "grandparent",
- resource.getPath());
- return false;
+ if (parent == null) {
+ log.warn("{} is null for alias on {}, skipping.",
containingResource == resource ? "parent" : "grandparent",
+ resource.getPath());
+ return false;
+ } else {
+ final String[] aliasArray =
resource.getValueMap().get(ResourceResolverImpl.PROP_ALIAS, String[].class);
+ if (aliasArray == null) {
+ return false;
+ } else {
+ return loadAliasFromArray(aliasArray, map,
conflictingAliases, invalidAliases, containingResource.getName(),
+ parent.getPath());
+ }
+ }
}
- else {
- // resource the alias is for
- String resourceName = containingResource.getName();
+ }
- // parent path of that resource
- String parentPath = parent.getPath();
+ /**
+ * Load alias given a an alias array, return success flag.
+ */
+ private boolean loadAliasFromArray(final String[] aliasArray, Map<String,
Map<String, Collection<String>>> map,
+ List<String> conflictingAliases, List<String> invalidAliases,
final String resourceName, final String parentPath) {
- boolean hasAlias = false;
+ boolean hasAlias = false;
- // require properties
- final ValueMap props = resource.getValueMap();
- final String[] aliasArray =
props.get(ResourceResolverImpl.PROP_ALIAS, String[].class);
+ log.debug("Found alias, total size {}", aliasArray.length);
- if (aliasArray != null) {
- log.debug("Found alias, total size {}", aliasArray.length);
- // the order matters here, the first alias in the array must
come first
- for (final String alias : aliasArray) {
- if (isAliasInvalid(alias)) {
- log.warn("Encountered invalid alias '{}' under parent
path '{}'. Refusing to use it.", alias, parentPath);
- } else {
- Map<String, Collection<String>> parentMap =
map.computeIfAbsent(parentPath, key -> new ConcurrentHashMap<>());
- Optional<String> siblingResourceNameWithDuplicateAlias
= parentMap.entrySet().stream()
- .filter(entry ->
!entry.getKey().equals(resourceName)) // ignore entry for the current resource
- .filter(entry ->
entry.getValue().contains(alias))
- .findFirst().map(Map.Entry::getKey);
- if (siblingResourceNameWithDuplicateAlias.isPresent())
{
- log.warn(
- "Encountered duplicate alias '{}' under
parent path '{}'. Refusing to replace current target {} with {}.",
- alias, parentPath,
siblingResourceNameWithDuplicateAlias.get(), resourceName);
- } else {
- Collection<String> existingAliases =
parentMap.computeIfAbsent(resourceName, name -> new CopyOnWriteArrayList<>());
- existingAliases.add(alias);
- hasAlias = true;
- }
+ // the order matters here, the first alias in the array must come first
+ for (final String alias : aliasArray) {
+ if (isAliasInvalid(alias)) {
+ long invalids = detectedInvalidAliases.incrementAndGet();
+ log.warn("Encountered invalid alias '{}' under parent path
'{}' (total so far: {}). Refusing to use it.",
+ alias, parentPath, invalids);
+ if (invalidAliases != null && invalids <
MAX_REPORT_DEFUNCT_ALIASES) {
+ invalidAliases.add((String.format("'%s'/'%s'", parentPath,
alias)));
+ }
+ } else {
+ Map<String, Collection<String>> parentMap =
map.computeIfAbsent(parentPath, key -> new ConcurrentHashMap<>());
+ Optional<String> siblingResourceNameWithDuplicateAlias =
parentMap.entrySet().stream()
+ .filter(entry -> !entry.getKey().equals(resourceName))
// ignore entry for the current resource
+ .filter(entry -> entry.getValue().contains(alias))
+ .findFirst().map(Map.Entry::getKey);
+ if (siblingResourceNameWithDuplicateAlias.isPresent()) {
+ long conflicting =
detectedConflictingAliases.incrementAndGet();
+ log.warn(
+ "Encountered duplicate alias '{}' under parent
path '{}'. Refusing to replace current target '{}' with '{}' (total duplicated
aliases so far: {}).",
+ alias, parentPath,
siblingResourceNameWithDuplicateAlias.get(), resourceName, conflicting);
+ if (conflictingAliases != null && conflicting <
MAX_REPORT_DEFUNCT_ALIASES) {
+ conflictingAliases.add((String.format("'%s': '%s'/'%s'
vs '%s'/'%s'", parentPath, resourceName,
+ alias,
siblingResourceNameWithDuplicateAlias.get(), alias)));
}
+ } else {
+ Collection<String> existingAliases =
parentMap.computeIfAbsent(resourceName, name -> new CopyOnWriteArrayList<>());
+ existingAliases.add(alias);
+ hasAlias = true;
}
}
-
- return hasAlias;
}
+
+ return hasAlias;
}
/**
diff --git
a/src/test/java/org/apache/sling/resourceresolver/impl/ResourceResolverMetricsTest.java
b/src/test/java/org/apache/sling/resourceresolver/impl/ResourceResolverMetricsTest.java
index 14075c6..e35ac74 100644
---
a/src/test/java/org/apache/sling/resourceresolver/impl/ResourceResolverMetricsTest.java
+++
b/src/test/java/org/apache/sling/resourceresolver/impl/ResourceResolverMetricsTest.java
@@ -30,14 +30,14 @@ import org.junit.Test;
import org.mockito.Mockito;
public class ResourceResolverMetricsTest {
-
+
@Rule
public OsgiContext context = new OsgiContext();
-
+
private MetricsService metricsService;
-
+
private ResourceResolverMetrics metrics;
-
+
@Before
public void setup() {
metrics = new ResourceResolverMetrics();
@@ -45,26 +45,70 @@ public class ResourceResolverMetricsTest {
context.registerService(MetricsService.class, metricsService);
context.registerInjectActivateService(metrics);
}
-
+
+ @Test
+ public void testGaugesAliases() {
+ // get gauges
+ Gauge<Long> numberOfResourcesWithAliasedChildren =
getGauge("numberOfResourcesWithAliasedChildren");
+ Gauge<Long> numberOfResourcesWithAliasesOnStartup =
getGauge("numberOfResourcesWithAliasesOnStartup");
+ Gauge<Long> numberOfDetectedInvalidAliasesGauge =
getGauge("numberOfDetectedInvalidAliases");
+ Gauge<Long> numberOfDetectedConflictingAliases =
getGauge("numberOfDetectedConflictingAliases");
+
+ // check initial Values
+ assertThat(numberOfResourcesWithAliasedChildren.getValue(), is(0L));
+ assertThat(numberOfResourcesWithAliasesOnStartup.getValue(), is(0L));
+ assertThat(numberOfDetectedInvalidAliasesGauge.getValue(), is(0L));
+ assertThat(numberOfDetectedConflictingAliases.getValue(), is(0L));
+
+ // set values
+ metrics.setNumberOfResourcesWithAliasedChildrenSupplier(() -> 8L);
+ metrics.setNumberOfResourcesWithAliasesOnStartupSupplier(() -> 9L);
+ metrics.setNumberOfDetectedInvalidAliasesSupplier(() -> 10L);
+ metrics.setNumberOfDetectedConflictingAliasesSupplier(() -> 11L);
+
+ // check values
+ assertThat(numberOfResourcesWithAliasedChildren.getValue(), is(8L));
+ assertThat(numberOfResourcesWithAliasesOnStartup.getValue(), is(9L));
+ assertThat(numberOfDetectedInvalidAliasesGauge.getValue(), is(10L));
+ assertThat(numberOfDetectedConflictingAliases.getValue(), is(11L));
+ }
+
@Test
- public void testGauges() {
- Gauge<Long> vanityPaths =
getGauge(ResourceResolverMetrics.METRICS_PREFIX + ".numberOfVanityPaths");
- Gauge<Long> aliases = getGauge(ResourceResolverMetrics.METRICS_PREFIX
+ ".numberOfResourcesWithAliasedChildren");
- assertThat(vanityPaths.getValue(),is(0L));
- assertThat(aliases.getValue(),is(0L));
-
- metrics.setNumberOfResourcesWithAliasedChildrenSupplier(() -> 3L);
- metrics.setNumberOfVanityPathsSupplier(() -> 2L);
- assertThat(vanityPaths.getValue(),is(2L));
- assertThat(aliases.getValue(),is(3L));
-
+ public void testGaugesVanityPaths() {
+ // get gauges
+ Gauge<Long> numberOfVanityPaths = getGauge("numberOfVanityPaths");
+ Gauge<Long> numberOfResourcesWithVanityPathsOnStartup =
getGauge("numberOfResourcesWithVanityPathsOnStartup");
+ Gauge<Long> numberOfVanityPathLookups =
getGauge("numberOfVanityPathLookups");
+ Gauge<Long> numberOfVanityPathBloomNegatives =
getGauge("numberOfVanityPathBloomNegatives");
+ Gauge<Long> numberOfVanityPathBloomFalsePositives =
getGauge("numberOfVanityPathBloomFalsePositives");
+
+ // check initial Values
+ assertThat(numberOfVanityPaths.getValue(), is(0L));
+ assertThat(numberOfResourcesWithVanityPathsOnStartup.getValue(),
is(0L));
+ assertThat(numberOfVanityPathLookups.getValue(), is(0L));
+ assertThat(numberOfVanityPathBloomNegatives.getValue(), is(0L));
+ assertThat(numberOfVanityPathBloomFalsePositives.getValue(), is(0L));
+
+ // set values
+ metrics.setNumberOfVanityPathsSupplier(() -> 3L);
+ metrics.setNumberOfResourcesWithVanityPathsOnStartupSupplier(() -> 4L);
+ metrics.setNumberOfVanityPathLookupsSupplier(() -> 5L);
+ metrics.setNumberOfVanityPathBloomNegativesSupplier(() -> 6L);
+ metrics.setNumberOfVanityPathBloomFalsePositivesSupplier(() -> 7L);
+
+ // check values
+ assertThat(numberOfVanityPaths.getValue(), is(3L));
+ assertThat(numberOfResourcesWithVanityPathsOnStartup.getValue(),
is(4L));
+ assertThat(numberOfVanityPathLookups.getValue(), is(5L));
+ assertThat(numberOfVanityPathBloomNegatives.getValue(), is(6L));
+ assertThat(numberOfVanityPathBloomFalsePositives.getValue(), is(7L));
}
-
+
private Gauge<Long> getGauge(String name) {
- String filter = String.format("(%s=%s)", Gauge.NAME,name);
- Gauge<Long>[] result = context.getServices(Gauge.class,filter);
- assertThat(result.length,is(1));
+ String filter = String.format("(%s=%s)", Gauge.NAME,
ResourceResolverMetrics.METRICS_PREFIX + "." + name);
+ @SuppressWarnings("unchecked")
+ Gauge<Long>[] result = context.getServices(Gauge.class, filter);
+ assertThat(result.length, is(1));
return result[0];
}
-
}
diff --git
a/src/test/java/org/apache/sling/resourceresolver/impl/mapping/MapEntriesTest.java
b/src/test/java/org/apache/sling/resourceresolver/impl/mapping/MapEntriesTest.java
index 4402741..75b5e50 100644
---
a/src/test/java/org/apache/sling/resourceresolver/impl/mapping/MapEntriesTest.java
+++
b/src/test/java/org/apache/sling/resourceresolver/impl/mapping/MapEntriesTest.java
@@ -100,6 +100,9 @@ public class MapEntriesTest extends
AbstractMappingMapEntriesTest {
private EventAdmin eventAdmin;
private Map<String, Map<String, String>> aliasMap;
+ private AtomicLong detectedInvalidAliases;
+ private AtomicLong detectedConflictingAliases;
+
private int testSize = 5;
private int pageSize;
@@ -162,10 +165,18 @@ public class MapEntriesTest extends
AbstractMappingMapEntriesTest {
Optional<ResourceResolverMetrics> metrics = Optional.empty();
mapEntries = new MapEntries(resourceResolverFactory, bundleContext,
eventAdmin, stringInterpolationProvider, metrics);
+
final Field aliasMapField =
MapEntries.class.getDeclaredField("aliasMapsMap");
aliasMapField.setAccessible(true);
+ this.aliasMap = (Map<String, Map<String, String>>)
aliasMapField.get(mapEntries);
+
+ final Field detectedInvalidAliasesField =
MapEntries.class.getDeclaredField("detectedInvalidAliases");
+ detectedInvalidAliasesField.setAccessible(true);
+ this.detectedInvalidAliases = (AtomicLong)
detectedInvalidAliasesField.get(mapEntries);
- this.aliasMap = ( Map<String, Map<String, String>>)
aliasMapField.get(mapEntries);
+ final Field detectedConflictingAliasesField =
MapEntries.class.getDeclaredField("detectedConflictingAliases");
+ detectedConflictingAliasesField.setAccessible(true);
+ this.detectedConflictingAliases = (AtomicLong)
detectedConflictingAliasesField.get(mapEntries);
}
@Override
@@ -232,16 +243,19 @@ public class MapEntriesTest extends
AbstractMappingMapEntriesTest {
assertNotNull(aliasMap);
assertTrue(aliasMap.containsKey("child"));
assertEquals(List.of("foo", "bar", "qux", " "), aliasMap.get("child"));
+ assertEquals(3, detectedInvalidAliases.get());
}
@Test
public void test_alias_support_invalid() {
- for (String invalidAlias : List.of(".", "..", "foo/bar", "# foo", ""))
{
+ List<String> invalidAliases = List.of(".", "..", "foo/bar", "# foo",
"");
+ for (String invalidAlias : invalidAliases) {
prepareMapEntriesForAlias(false, false, invalidAlias);
mapEntries.doInit();
Map<String, Collection<String>> aliasMap =
mapEntries.getAliasMap("/parent");
assertEquals(Collections.emptyMap(), aliasMap);
}
+ assertEquals(invalidAliases.size(), detectedInvalidAliases.get());
}
private void prepareMapEntriesForAlias(boolean onJcrContent, boolean
withNullParent, String... alias) {
@@ -310,6 +324,7 @@ public class MapEntriesTest extends
AbstractMappingMapEntriesTest {
assertNotNull(aliasMap);
assertTrue(aliasMap.containsKey("child"));
assertEquals(Collections.singletonList("alias"),
aliasMap.get("child"));
+ assertEquals(1, detectedConflictingAliases.get());
}
@Test
