Require opt-in for using unnamespaced mbean exports

pom.xml

@@ -73,6 +73,13 @@
             <version>3.0.0</version>
             <optional>true</optional>
         </dependency>
+
+        <dependency>
+            <groupId>org.assertj</groupId>
+            <artifactId>assertj-core</artifactId>
+            <version>3.27.6</version>
+            <scope>test</scope>
+        </dependency>
     </dependencies>
     <build>
         <pluginManagement>

src/main/java/org/weakref/jmx/MBeanExporter.java

@@ -18,9 +18,9 @@
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.MapMaker;
 import com.google.inject.Inject;
+import jakarta.annotation.PreDestroy;
 import org.weakref.jmx.JmxException.Reason;
 
-import jakarta.annotation.PreDestroy;
 import javax.management.InstanceAlreadyExistsException;
 import javax.management.InstanceNotFoundException;
 import javax.management.MBeanRegistrationException;
@@ -47,21 +47,22 @@ public class MBeanExporter
     private final ObjectNameGenerator objectNameGenerator;
     private final Map<ObjectName, ManagedClass> exportedManagedClasses = new ConcurrentHashMap<>();
 
-    MBeanExporter()
-    {
-        this(ManagementFactory.getPlatformMBeanServer());
-    }
-
+    /**
+     * @deprecated Use {@link #MBeanExporter(MBeanServer, ObjectNameGenerator)} instead.
+     * You can obtain an instance of {@link ObjectNameGenerator} via {@link ObjectNameGenerator#defaultObjectNameGenerator()}
+     * if usage of the mbean exporter requires no namespacing.
+     */
+    @Deprecated
     public MBeanExporter(MBeanServer server)
     {
-        this(server, Optional.empty());
+        this(server, ObjectNameGenerator.defaultObjectNameGenerator());
     }
 
     @Inject
-    public MBeanExporter(MBeanServer server, Optional<ObjectNameGenerator> objectNameGenerator)
+    public MBeanExporter(MBeanServer server, ObjectNameGenerator objectNameGenerator)
     {
-        this.server = server;
-        this.objectNameGenerator = objectNameGenerator.orElseGet(ObjectNameGenerator::defaultObjectNameGenerator);
+        this.server = requireNonNull(server, "server is null");
+        this.objectNameGenerator = requireNonNull(objectNameGenerator, "objectNameGenerator is null");
         exportedObjects = new MapMaker().weakValues().makeMap();
     }
 
@@ -265,7 +266,9 @@ public Optional<Object> getExportedObject(ObjectName objectName)
      */
     public static MBeanExporter withPlatformMBeanServer()
     {
-        return new MBeanExporter(ManagementFactory.getPlatformMBeanServer());
+        return new MBeanExporter(
+                ManagementFactory.getPlatformMBeanServer(),
+                ObjectNameGenerator.defaultObjectNameGenerator());
     }
 
     private static ObjectName createObjectName(String name)

src/main/java/org/weakref/jmx/guice/GuiceMBeanExporter.java

@@ -24,7 +24,6 @@
 
 import java.util.Map;
 import java.util.Map.Entry;
-import java.util.Optional;
 import java.util.Set;
 import java.util.function.BiFunction;
 
@@ -35,15 +34,14 @@ public GuiceMBeanExporter(Set<Mapping> mappings,
             Set<SetMapping<?>> setMappings,
             Set<MapMapping<?, ?>> mapMappings,
             MBeanExporter exporter,
-            Optional<ObjectNameGenerator> objectNameGenerator,
+            ObjectNameGenerator objectNameGenerator,
             Injector injector)
     {
-        ObjectNameGenerator generator = objectNameGenerator.orElseGet(ObjectNameGenerator::defaultObjectNameGenerator);
-        export(mappings, exporter, injector, generator);
+        export(mappings, exporter, injector, objectNameGenerator);
 
         // cast to Object to get around Java's broken generics
-        exportSets(castSetMapping(setMappings), exporter, injector, generator);
-        exportMaps(castMapMappings(mapMappings), exporter, injector, generator);
+        exportSets(castSetMapping(setMappings), exporter, injector, objectNameGenerator);
+        exportMaps(castMapMappings(mapMappings), exporter, injector, objectNameGenerator);
     }
 
     @SuppressWarnings("unchecked")

src/main/java/org/weakref/jmx/guice/MBeanModule.java

@@ -16,82 +16,166 @@
 package org.weakref.jmx.guice;
 
 import com.google.inject.AbstractModule;
-import com.google.inject.Binder;
-import com.google.inject.Key;
+import com.google.inject.Inject;
+import com.google.inject.Injector;
+import com.google.inject.Provider;
 import com.google.inject.Scopes;
 import com.google.inject.TypeLiteral;
 import org.weakref.jmx.MBeanExporter;
 import org.weakref.jmx.ObjectNameGenerator;
 
+import javax.management.MBeanServer;
+
+import java.util.Optional;
+import java.util.Set;
+
 import static com.google.inject.multibindings.Multibinder.newSetBinder;
 import static com.google.inject.multibindings.OptionalBinder.newOptionalBinder;
+import static java.util.Objects.requireNonNull;
 
-public class MBeanModule
+public final class MBeanModule
         extends AbstractModule
 {
-    private ExportBuilder builder;
 
-    @Override
-    protected final void configure()
+    public static MBeanModule withUnnamespacedObjectNames()
     {
-        builder = newExporter(binder());
-
-        bind(GuiceMBeanExporter.class).asEagerSingleton();
-        bind(MBeanExporter.class).in(Scopes.SINGLETON);
-
-        newOptionalBinder(binder(), ObjectNameGenerator.class);
-        newSetBinder(binder(), new TypeLiteral<SetMapping<?>>() {});
-        newSetBinder(binder(), new TypeLiteral<MapMapping<?, ?>>() {});
+        return new MBeanModule(ObjectNameGeneratorStrategy.NO_NAMESPACE);
+    }
 
-        configureMBeans();
+    public static MBeanModule forCustomObjectNameGenerator()
+    {
+        return new MBeanModule(ObjectNameGeneratorStrategy.EXPLICIT_NAMING);
     }
 
+    private final ObjectNameGeneratorStrategy objectNameGeneratorStrategy;
+
     /**
-     * To be overridden by subclasses. E.g.,
-     *
-     * protected void configureMBeans() {
-     *    export(ManagedObject.class).as("test:name=X");
-     *    export(ManagedObject.class).annotatedWith(SomeAnnotation.class).as("test:name=Y");
-     * }
-     *
-     * When ExportBuilder is used, a raw MBeanModule can be imported to trigger the
-     * registration of exported mbeans:
-     *
-     * <pre>
-     * {@code
-     * Injector injector = Guice.createInjector(new MBeanModule(),
-     *      new AbstractModule() {
-     *          @Override
-     *          protected void configure() {
-     *              ExportBuilder builder = MBeanModule.newExporter();
-     *              builder.export(AnotherManagedObject.class).as("test:name="Z");
-     *          }
-     *      });
-     * }
-     * </pre>
-     *
-     *  @deprecated subclassing no longer supported. Use ExportBinder instead
+     * @deprecated It should be explicit choice to export MBeans without namespacing, as this can easily lead to name collisions.
+     * Use {@link #withUnnamespacedObjectNames()} if you want this behavior and use {@link #forCustomObjectNameGenerator()} if you
+     * want to provide your own naming strategy perhaps for namespacing.
      */
     @Deprecated
-    protected void configureMBeans() {
+    public MBeanModule()
+    {
+        this(ObjectNameGeneratorStrategy.LEGACY);
     }
 
-    @Deprecated
-    protected NamedBindingBuilder export(Key<?> key)
+    private MBeanModule(ObjectNameGeneratorStrategy objectNameGeneratorStrategy)
     {
-        return builder.export(key);
+        this.objectNameGeneratorStrategy = requireNonNull(objectNameGeneratorStrategy, "objectNameGeneratorStrategy is null");
     }
 
-    @Deprecated
-    protected AnnotatedExportBuilder export(Class<?> clazz)
+    @Override
+    protected void configure()
+    {
+        switch (objectNameGeneratorStrategy) {
+            case LEGACY -> {
+                bind(GuiceMBeanExporter.class)
+                        .toProvider(LegacyBindingGuiceMBeanExporterProvider.class)
+                        .asEagerSingleton();
+                bind(MBeanExporter.class)
+                        .toProvider(LegacyBindingMBeanExporterProvider.class)
+                        .in(Scopes.SINGLETON);
+                newOptionalBinder(binder(), ObjectNameGenerator.class);
+            }
+
+            case NO_NAMESPACE -> {
+                bind(GuiceMBeanExporter.class).asEagerSingleton();
+                bind(MBeanExporter.class).in(Scopes.SINGLETON);
+                bind(ObjectNameGenerator.class).toInstance(ObjectNameGenerator.defaultObjectNameGenerator());
+            }
+
+            case EXPLICIT_NAMING -> {
+                bind(GuiceMBeanExporter.class).asEagerSingleton();
+                bind(MBeanExporter.class).in(Scopes.SINGLETON);
+                // require explicit binding
+                bind(ObjectNameGenerator.class);
+            }
+        }
+
+        newSetBinder(binder(), new TypeLiteral<Mapping>() {});
+        newSetBinder(binder(), new TypeLiteral<SetMapping<?>>() {});
+        newSetBinder(binder(), new TypeLiteral<MapMapping<?, ?>>() {});
+    }
+
+    enum ObjectNameGeneratorStrategy
     {
-        return builder.export(clazz);
+        /**
+         * As it used to be - user can export mbeans with the {@link ObjectNameGenerator#defaultObjectNameGenerator() default strategy},
+         * or bind their own generator to {@code ObjectNameGenerator.class} key (or via optional binder)
+         *
+         * @deprecated exists only to support deprecated legacy mode
+         */
+        @Deprecated
+        LEGACY,
+
+        /**
+         * This strategy forces user can export mbeans with the {@link ObjectNameGenerator#defaultObjectNameGenerator() default strategy}.
+         * If custom strategy is needed, {@link #EXPLICIT_NAMING} should be used.
+         * This strategy is suitable when MBeanModule is used in top level Guice context of an application.
+         */
+        NO_NAMESPACE,
+
+        /**
+         * This strategy requires that {@link ObjectNameGenerator} is separately bound.
+         * This strategy is suitable when MBeanModule is used in a library/module Guice context,
+         * where lack of namespacing may lead to name collisions.
+         */
+        EXPLICIT_NAMING,
     }
 
-    @Deprecated
-    public static ExportBuilder newExporter(Binder binder)
+    private static class LegacyBindingGuiceMBeanExporterProvider
+            implements Provider<GuiceMBeanExporter>
     {
-    	return new ExportBuilder(newSetBinder(binder, Mapping.class));
+        private final Set<Mapping> mappings;
+        private final Set<SetMapping<?>> setMappings;
+        private final Set<MapMapping<?, ?>> mapMappings;
+        private final MBeanExporter exporter;
+        private final ObjectNameGenerator objectNameGenerator;
+        private final Injector injector;
+
+        @Inject
+        public LegacyBindingGuiceMBeanExporterProvider(
+                Set<Mapping> mappings,
+                Set<SetMapping<?>> setMappings,
+                Set<MapMapping<?, ?>> mapMappings,
+                MBeanExporter exporter,
+                Optional<ObjectNameGenerator> objectNameGenerator,
+                Injector injector)
+        {
+            this.mappings = requireNonNull(mappings, "mappings is null");
+            this.setMappings = requireNonNull(setMappings, "setMappings is null");
+            this.mapMappings = requireNonNull(mapMappings, "mapMappings is null");
+            this.exporter = requireNonNull(exporter, "exporter is null");
+            this.objectNameGenerator = objectNameGenerator.orElseGet(ObjectNameGenerator::defaultObjectNameGenerator);
+            this.injector = requireNonNull(injector, "injector is null");
+        }
+
+        @Override
+        public GuiceMBeanExporter get()
+        {
+            return new GuiceMBeanExporter(mappings, setMappings, mapMappings, exporter, objectNameGenerator, injector);
+        }
     }
 
+    private static class LegacyBindingMBeanExporterProvider
+            implements Provider<MBeanExporter>
+    {
+
+        private final MBeanServer server;
+        private final ObjectNameGenerator objectNameGenerator;
+
+        @Inject
+        public LegacyBindingMBeanExporterProvider(MBeanServer server, Optional<ObjectNameGenerator> objectNameGenerator)
+        {
+            this.server = requireNonNull(server, "server is null");
+            this.objectNameGenerator = objectNameGenerator.orElseGet(ObjectNameGenerator::defaultObjectNameGenerator);
+        }
+
+        @Override
+        public MBeanExporter get()
+        {
+            return new MBeanExporter(server, objectNameGenerator);
+        }
+    }
 }