This is an automated email from the ASF dual-hosted git repository. chetanm pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/sling-org-apache-sling-commons-log.git
commit c45b141519dd553cfeeaf8f61e7dc9d93410121b Author: Chetan Mehrotra <[email protected]> AuthorDate: Tue Oct 31 19:46:35 2017 +0530 SLING-3049 - Make Logback Stacktrace Packaging data support OSGi aware Ensure that bundle info is only provided if there is a single owner of any package. If same package is provided by multiple bundles then no bundle info would be provided --- pom.xml | 7 ++++ .../log/logback/internal/LogbackManager.java | 2 +- .../internal/stacktrace/PackageInfoCollector.java | 23 ++++++++--- .../logback/integration/PackagingDataTestUtil.java | 37 +++++++++++++---- .../stacktrace/PackageInfoCollectorTest.java | 48 +++++++++++++++++++++- 5 files changed, 101 insertions(+), 16 deletions(-) diff --git a/pom.xml b/pom.xml index 0b5fca3..5989a74 100644 --- a/pom.xml +++ b/pom.xml @@ -48,6 +48,7 @@ <slf4j.version>1.7.21</slf4j.version> <logback.version>1.2.3</logback.version> <pax-exam.version>3.5.0</pax-exam.version> + <sling.java.version>8</sling.java.version> <bundle.build.dir> ${basedir}/target @@ -392,6 +393,12 @@ <version>1.3</version> <scope>test</scope> </dependency> + <dependency> + <groupId>org.mockito</groupId> + <artifactId>mockito-core</artifactId> + <version>2.11.0</version> + <scope>test</scope> + </dependency> </dependencies> <profiles> diff --git a/src/main/java/org/apache/sling/commons/log/logback/internal/LogbackManager.java b/src/main/java/org/apache/sling/commons/log/logback/internal/LogbackManager.java index 9f357c3..582d487 100644 --- a/src/main/java/org/apache/sling/commons/log/logback/internal/LogbackManager.java +++ b/src/main/java/org/apache/sling/commons/log/logback/internal/LogbackManager.java @@ -813,7 +813,7 @@ public class LogbackManager extends LoggerContextAwareBase { private void registerPackageInfoCollector() { //Weaving hook once registered would not be removed upon config changed if (logConfigManager.isPackagingDataEnabled()) { - Properties props = new Properties(); + Dictionary<String,Object> props = new Hashtable<>(); props.put(Constants.SERVICE_VENDOR, "Apache Software Foundation"); props.put(Constants.SERVICE_DESCRIPTION, PACKAGE_INFO_COLLECTOR_DESC); diff --git a/src/main/java/org/apache/sling/commons/log/logback/internal/stacktrace/PackageInfoCollector.java b/src/main/java/org/apache/sling/commons/log/logback/internal/stacktrace/PackageInfoCollector.java index e80eed1..cd78ba9 100644 --- a/src/main/java/org/apache/sling/commons/log/logback/internal/stacktrace/PackageInfoCollector.java +++ b/src/main/java/org/apache/sling/commons/log/logback/internal/stacktrace/PackageInfoCollector.java @@ -18,15 +18,18 @@ */ package org.apache.sling.commons.log.logback.internal.stacktrace; -import java.util.Map; +import java.util.Collections; +import java.util.HashSet; +import java.util.Set; import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.ConcurrentMap; import org.osgi.framework.Bundle; import org.osgi.framework.hooks.weaving.WeavingHook; import org.osgi.framework.hooks.weaving.WovenClass; public class PackageInfoCollector implements WeavingHook{ - private final Map<String, String> pkgInfoMapping = new ConcurrentHashMap<>(); + private final ConcurrentMap<String, Set<String>> pkgInfoMapping = new ConcurrentHashMap<>(); @Override public void weave(WovenClass wovenClass) { @@ -38,7 +41,10 @@ public class PackageInfoCollector implements WeavingHook{ } void add(Bundle bundle, String className) { - pkgInfoMapping.put(getPackageName(className), getInfo(bundle)); + String packageName = getPackageName(className); + + Set<String> infos = pkgInfoMapping.computeIfAbsent(packageName, k -> Collections.synchronizedSet(new HashSet<>())); + infos.add(getInfo(bundle)); } String getBundleInfo(String className) { @@ -46,10 +52,17 @@ public class PackageInfoCollector implements WeavingHook{ return null; } String packageName = getPackageName(className); - return pkgInfoMapping.get(packageName); + Set<String> infos = pkgInfoMapping.get(packageName); + + //If multiple infos are found then we cannot determine the exact version + //so better not to provide any info + if (infos == null || infos.size() > 1 || infos.size() == 0) { + return null; + } + return infos.iterator().next(); } - static String getInfo(Bundle bundle) { + private static String getInfo(Bundle bundle) { return bundle.getSymbolicName() + ":" + bundle.getVersion(); } diff --git a/src/test/java/org/apache/sling/commons/log/logback/integration/PackagingDataTestUtil.java b/src/test/java/org/apache/sling/commons/log/logback/integration/PackagingDataTestUtil.java index 05de164..8e9b0da 100644 --- a/src/test/java/org/apache/sling/commons/log/logback/integration/PackagingDataTestUtil.java +++ b/src/test/java/org/apache/sling/commons/log/logback/integration/PackagingDataTestUtil.java @@ -20,10 +20,9 @@ package org.apache.sling.commons.log.logback.integration; import java.io.InputStream; +import java.net.URL; -import org.apache.sling.commons.log.logback.integration.bundle.PackageDataActivator; -import org.apache.sling.commons.log.logback.integration.bundle.TestRunnable; -import org.ops4j.pax.tinybundles.core.InnerClassStrategy; +import org.ops4j.pax.tinybundles.core.TinyBundle; import org.osgi.framework.Constants; import static org.ops4j.pax.tinybundles.core.TinyBundles.bundle; @@ -39,12 +38,32 @@ public class PackagingDataTestUtil { public static final String TEST_BUNDLE_NAME = "packagedatatest"; public static InputStream createTestBundle() { - return bundle() - .set(Constants.BUNDLE_ACTIVATOR, PackageDataActivator.class.getName()) + //Avoid referring to test bundle classes otherwise they get loaded in 2 bundles i.e. + //pax exam probe bundle and our packagedatatest. So we refer only by class name strings + String activatorClassName = "org.apache.sling.commons.log.logback.integration.bundle.PackageDataActivator"; + TinyBundle tb = bundle() + .set(Constants.BUNDLE_ACTIVATOR, activatorClassName) .set(Constants.BUNDLE_SYMBOLICNAME, TEST_BUNDLE_NAME) - .set(Constants.BUNDLE_VERSION, TEST_BUNDLE_VERSION) - .add(PackageDataActivator.class, InnerClassStrategy.NONE) - .add(TestRunnable.class, InnerClassStrategy.NONE) - .build(withBnd()); + .set(Constants.BUNDLE_VERSION, TEST_BUNDLE_VERSION); + add(tb, "org.apache.sling.commons.log.logback.integration.bundle.TestRunnable"); + add(tb, activatorClassName); + return tb.build(withBnd()); + } + + private static void add(TinyBundle tb, String className) { + String name = asResource(className); + tb.add(name, asResourceURL(name)); + } + + private static String asResource( String klass ) { + return klass.replace( '.', '/' ) + ".class"; + } + + private static URL asResourceURL(String klass ) { + URL u = PackagingDataTestUtil.class.getResource("/" + klass); + if (u == null) { + throw new RuntimeException("No resource found for "+klass); + } + return u; } } diff --git a/src/test/java/org/apache/sling/commons/log/logback/internal/stacktrace/PackageInfoCollectorTest.java b/src/test/java/org/apache/sling/commons/log/logback/internal/stacktrace/PackageInfoCollectorTest.java index f3ec3f5..ec982be 100644 --- a/src/test/java/org/apache/sling/commons/log/logback/internal/stacktrace/PackageInfoCollectorTest.java +++ b/src/test/java/org/apache/sling/commons/log/logback/internal/stacktrace/PackageInfoCollectorTest.java @@ -20,14 +20,60 @@ package org.apache.sling.commons.log.logback.internal.stacktrace; import org.junit.Test; +import org.osgi.framework.Bundle; +import org.osgi.framework.Version; +import org.osgi.framework.hooks.weaving.WovenClass; +import org.osgi.framework.wiring.BundleWiring; -import static org.junit.Assert.*; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNull; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; public class PackageInfoCollectorTest { + private PackageInfoCollector collector = new PackageInfoCollector(); + @Test public void packageName() throws Exception{ assertEquals("com.foo", PackageInfoCollector.getPackageName("com.foo.TestClass")); assertEquals("", PackageInfoCollector.getPackageName("packageInDefaultClass")); } + @Test + public void getBundleInfo() throws Exception{ + Bundle bundle = newBundle("foo.bundle", "0.0.7"); + collector.weave(newWovenClass(bundle, "com.example.Foo")); + + assertEquals("foo.bundle:0.0.7",collector.getBundleInfo("com.example.Foo")); + assertEquals("foo.bundle:0.0.7",collector.getBundleInfo("com.example.Bar")); + assertNull(collector.getBundleInfo("com.example2.Bar")); + } + + /** + * For case where same package is present in multiple bundles then no bundle info is provided + */ + @Test + public void duplicates() throws Exception{ + collector.weave(newWovenClass(newBundle("foo.bundle", "0.0.7"), "com.example.Foo")); + collector.weave(newWovenClass(newBundle("foo.bundle2", "0.0.8"), "com.example.Bar")); + + assertNull(collector.getBundleInfo("com.example.Bar")); + } + + private Bundle newBundle(String name, String version){ + Bundle b = mock(Bundle.class); + when(b.getSymbolicName()).thenReturn(name); + when(b.getVersion()).thenReturn(Version.parseVersion(version)); + return b; + } + + private WovenClass newWovenClass(Bundle bundle, String className){ + WovenClass woven = mock(WovenClass.class); + BundleWiring wiring = mock(BundleWiring.class); + when(woven.getBundleWiring()).thenReturn(wiring); + + when(wiring.getBundle()).thenReturn(bundle); + when(woven.getClassName()).thenReturn(className); + return woven; + } } \ No newline at end of file -- To stop receiving notification emails like this one, please contact "[email protected]" <[email protected]>.
