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 <joerghoh@users.noreply.github.com>
* Update src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java
Co-authored-by: Jörg Hoh <joerghoh@users.noreply.github.com>
* Update src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java
Co-authored-by: Jörg Hoh <joerghoh@users.noreply.github.com>
* 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 <joerghoh@users.noreply.github.com>
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 @@
* 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.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.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.numberOfVanityPathBloomFalsePositives -- the total number of vanity path lookup that passed the bloom filter but were false positives
+ * 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 @@
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 @@
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 @@
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 @@
* 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;
}
/**
@@ -168,12 +181,28 @@
}
/**
+ * 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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
}
private boolean doAddAlias(final Resource resource) {
- return loadAlias(resource, this.aliasMapsMap);
+ return loadAlias(resource, this.aliasMapsMap, null, null);
}
/**
@@ -737,8 +762,8 @@
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 @@
if (!probablyPresent) {
// filtered by Bloom filter
- this.vanityPathBloomNegative.incrementAndGet();
+ this.vanityPathBloomNegatives.incrementAndGet();
}
}
@@ -777,7 +802,7 @@
}
if (mapEntries == null && probablyPresent) {
// Bloom filter had a false positive
- this.vanityPathBloomFalsePositive.incrementAndGet();
+ this.vanityPathBloomFalsePositives.incrementAndGet();
}
}
@@ -1156,7 +1181,9 @@
* 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 @@
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 @@
/**
* 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,58 +1267,71 @@
if (containingResource == null) {
log.warn("containingResource is null for alias on {}, skipping.", resource.getPath());
return false;
- }
+ } else {
+ final Resource parent = containingResource.getParent();
- final Resource parent = containingResource.getParent();
-
- if (parent == null) {
- log.warn("{} is null for alias on {}, skipping.", containingResource == resource ? "parent" : "grandparent",
- resource.getPath());
- return false;
- }
- else {
- // resource the alias is for
- String resourceName = containingResource.getName();
-
- // parent path of that resource
- String parentPath = parent.getPath();
-
- boolean hasAlias = false;
-
- // require properties
- final ValueMap props = resource.getValueMap();
- final String[] aliasArray = props.get(ResourceResolverImpl.PROP_ALIAS, String[].class);
-
- 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;
- }
- }
+ 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());
}
}
-
- return hasAlias;
}
}
/**
+ * 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;
+
+ 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)) {
+ 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;
+ }
+
+ /**
* Given a resource, check whether the name is "jcr:content", in which case return the parent resource
* @param resource resource to check
* @return parent of jcr:content resource (may be null), otherwise the resource itself
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.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 @@
context.registerService(MetricsService.class, metricsService);
context.registerInjectActivateService(metrics);
}
-
+
@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));
-
- }
-
- 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));
- return result[0];
+ 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 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, 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 @@
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 @@
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);
- 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);
+
+ final Field detectedConflictingAliasesField = MapEntries.class.getDeclaredField("detectedConflictingAliases");
+ detectedConflictingAliasesField.setAccessible(true);
+ this.detectedConflictingAliases = (AtomicLong) detectedConflictingAliasesField.get(mapEntries);
}
@Override
@@ -232,16 +243,19 @@
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 @@
assertNotNull(aliasMap);
assertTrue(aliasMap.containsKey("child"));
assertEquals(Collections.singletonList("alias"), aliasMap.get("child"));
+ assertEquals(1, detectedConflictingAliases.get());
}
@Test
