Pmd features

.gitignore

@@ -4,6 +4,7 @@
 *.sublime-workspace
 *.iml
 build/
+bin/
 .idea/
 .gradle/
 node_modules/

xtraplatform-features/src/main/java/de/ii/xtraplatform/features/app/ConnectorFactoryImpl.java

@@ -22,6 +22,7 @@
 import java.util.Objects;
 import java.util.Optional;
 import java.util.Set;
+import java.util.concurrent.locks.ReentrantLock;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -36,6 +37,7 @@ public class ConnectorFactoryImpl implements ConnectorFactory {
 
   private final Lazy<Set<ConnectorFactory2<?, ?, ?>>> connectorFactories;
   private final Map<String, Set<Runnable>> disposeListeners;
+  private final ReentrantLock lock = new ReentrantLock();
 
   @Inject
   public ConnectorFactoryImpl(Lazy<Set<ConnectorFactory2<?, ?, ?>>> connectorFactories) {
@@ -44,45 +46,73 @@ public ConnectorFactoryImpl(Lazy<Set<ConnectorFactory2<?, ?, ?>>> connectorFacto
   }
 
   @Override
-  public synchronized FeatureProviderConnector<?, ?, ?> createConnector(
+  public FeatureProviderConnector<?, ?, ?> createConnector(
       String providerType, String providerId, ConnectionInfo connectionInfo) {
-    final String connectorType = connectionInfo.getConnectorType();
+    lock.lock();
+    try {
+      final String connectorType = connectionInfo.getConnectorType();
 
-    if (getFactory(providerType, connectorType).isEmpty()) {
-      throw new IllegalStateException(
-          String.format(
-              "Connector with type %s for provider type %s is not supported.",
-              connectorType, providerType));
-    }
+      if (getFactory(providerType, connectorType).isEmpty()) {
+        throw new IllegalStateException(
+            String.format(
+                "Connector with type %s for provider type %s is not supported.",
+                connectorType, providerType));
+      }
+
+      ConnectorFactory2<?, ?, ?> connectorFactory2 = getFactory(providerType, connectorType).get();
+
+      if (connectionInfo.isShared()) {
+        Optional<FeatureProviderConnector<?, ?, ?>> shared =
+            findSharedConnector(connectorFactory2, connectionInfo);
 
-    ConnectorFactory2<?, ?, ?> connectorFactory2 = getFactory(providerType, connectorType).get();
-
-    if (connectionInfo.isShared()) {
-      Optional<? extends FeatureProviderConnector<?, ?, ?>> match =
-          connectorFactory2.instances().stream()
-              .filter(connector -> connector.canBeSharedWith(connectionInfo, false).first())
-              .findFirst();
-
-      if (match.isPresent()) {
-        Tuple<Boolean, String> fullMatch = match.get().canBeSharedWith(connectionInfo, true);
-
-        if (fullMatch.first()) {
-          LOGGER.debug("Joining shared pool.");
-          match
-              .get()
-              .getRefCounter()
-              .ifPresent(refs -> LOGGER.debug("Shared pool consumers: {}", refs.incrementAndGet()));
-
-          return match.get();
-        } else {
-          throw new IllegalStateException(
-              String.format(
-                  "Connection pool cannot be shared with provider %s: %s",
-                  match.get().getProviderId(), fullMatch.second()));
+        if (shared.isPresent()) {
+          return shared.get();
         }
       }
+
+      return createNewConnector(
+          connectorFactory2, providerId, connectorType, providerType, connectionInfo);
+    } finally {
+      lock.unlock();
     }
+  }
 
+  private Optional<FeatureProviderConnector<?, ?, ?>> findSharedConnector(
+      ConnectorFactory2<?, ?, ?> connectorFactory2, ConnectionInfo connectionInfo) {
+    Optional<? extends FeatureProviderConnector<?, ?, ?>> match =
+        connectorFactory2.instances().stream()
+            .filter(connector -> connector.canBeSharedWith(connectionInfo, false).first())
+            .findFirst();
+
+    if (match.isEmpty()) {
+      return Optional.empty();
+    }
+
+    Tuple<Boolean, String> fullMatch = match.get().canBeSharedWith(connectionInfo, true);
+
+    if (!fullMatch.first()) {
+      throw new IllegalStateException(
+          String.format(
+              "Connection pool cannot be shared with provider %s: %s",
+              match.get().getProviderId(), fullMatch.second()));
+    }
+
+    LOGGER.debug("Joining shared pool.");
+    match
+        .get()
+        .getRefCounter()
+        .ifPresent(refs -> LOGGER.debug("Shared pool consumers: {}", refs.incrementAndGet()));
+
+    return Optional.of(match.get());
+  }
+
+  @SuppressWarnings("PMD.AvoidCatchingGenericException")
+  private FeatureProviderConnector<?, ?, ?> createNewConnector(
+      ConnectorFactory2<?, ?, ?> connectorFactory2,
+      String providerId,
+      String connectorType,
+      String providerType,
+      ConnectionInfo connectionInfo) {
     try {
       LOGGER.debug("Creating new pool.");
       FeatureProviderConnector<?, ?, ?> connector =
@@ -96,7 +126,7 @@ public ConnectorFactoryImpl(Lazy<Set<ConnectorFactory2<?, ?, ?>>> connectorFacto
 
       return connector;
 
-    } catch (Throwable e) {
+    } catch (Exception e) {
       throw new IllegalStateException(
           String.format(
               "Connector with type %s for provider type %s could not be created.",
@@ -106,35 +136,44 @@ public ConnectorFactoryImpl(Lazy<Set<ConnectorFactory2<?, ?, ?>>> connectorFacto
   }
 
   @Override
-  public synchronized void disposeConnector(FeatureProviderConnector<?, ?, ?> connector) {
-    int refs = 0;
-    if (connector.getRefCounter().isPresent()) {
-      LOGGER.debug("Leaving shared pool.");
-      refs = connector.getRefCounter().get().decrementAndGet();
-      LOGGER.debug("Shared pool consumers: {}", refs);
-    }
+  public void disposeConnector(FeatureProviderConnector<?, ?, ?> connector) {
+    lock.lock();
+    try {
+      int refs = 0;
+      if (connector.getRefCounter().isPresent()) {
+        LOGGER.debug("Leaving shared pool.");
+        refs = connector.getRefCounter().get().decrementAndGet();
+        LOGGER.debug("Shared pool consumers: {}", refs);
+      }
 
-    if (refs == 0) {
-      boolean deleted =
-          getFactory(connector.getType()).get().deleteInstance(connector.getProviderId());
-      if (deleted && LOGGER.isDebugEnabled()) {
-        LOGGER.debug("Deleted unused pool.");
+      if (refs == 0) {
+        boolean deleted =
+            getFactory(connector.getType()).get().deleteInstance(connector.getProviderId());
+        if (deleted && LOGGER.isDebugEnabled()) {
+          LOGGER.debug("Deleted unused pool.");
+        }
       }
-    }
 
-    if (disposeListeners.containsKey(connector.getProviderId())) {
-      disposeListeners.get(connector.getProviderId()).forEach(Runnable::run);
-      disposeListeners.get(connector.getProviderId()).clear();
+      if (disposeListeners.containsKey(connector.getProviderId())) {
+        disposeListeners.get(connector.getProviderId()).forEach(Runnable::run);
+        disposeListeners.get(connector.getProviderId()).clear();
+      }
+    } finally {
+      lock.unlock();
     }
   }
 
   @Override
-  public synchronized void onDispose(
-      FeatureProviderConnector<?, ?, ?> connector, Runnable runnable) {
-    if (!disposeListeners.containsKey(connector.getProviderId())) {
-      disposeListeners.put(connector.getProviderId(), new HashSet<>());
+  public void onDispose(FeatureProviderConnector<?, ?, ?> connector, Runnable runnable) {
+    lock.lock();
+    try {
+      if (!disposeListeners.containsKey(connector.getProviderId())) {
+        disposeListeners.put(connector.getProviderId(), new HashSet<>());
+      }
+      disposeListeners.get(connector.getProviderId()).add(runnable);
+    } finally {
+      lock.unlock();
     }
-    disposeListeners.get(connector.getProviderId()).add(runnable);
   }
 
   private Optional<ConnectorFactory2<?, ?, ?>> getFactory(String type, String subType) {

xtraplatform-features/src/main/java/de/ii/xtraplatform/features/app/FeatureChangeHandlerImpl.java

@@ -22,6 +22,7 @@
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
+@SuppressWarnings("PMD.DoNotUseThreads")
 public class FeatureChangeHandlerImpl implements FeatureChanges {
 
   private static final Logger LOGGER = LoggerFactory.getLogger(FeatureChangeHandlerImpl.class);
@@ -30,6 +31,7 @@ public class FeatureChangeHandlerImpl implements FeatureChanges {
   private final List<DatasetChangeListener> datasetListeners;
   private final List<FeatureChangeListener> featureListeners;
 
+  @SuppressWarnings("PMD.CloseResource")
   public FeatureChangeHandlerImpl() {
     ThreadPoolExecutor threadPoolExecutor =
         (ThreadPoolExecutor)

xtraplatform-features/src/main/java/de/ii/xtraplatform/features/app/LocalSchemaFragmentResolver.java

@@ -145,6 +145,7 @@ private Map<String, FeatureSchema> merge(
   // property element names per object type (notably GML with `objectTypeNamespaces`) read this
   // tag at runtime; without it they'd inherit the containing feature's objectType, which is
   // wrong for properties that come from a different schema fragment than the feature itself.
+  @SuppressWarnings("PMD.CompareObjectsWithEquals")
   private static FeatureSchema tagOrigin(FeatureSchema property, String originType) {
     if (originType == null) {
       return property;
@@ -153,9 +154,11 @@ private static FeatureSchema tagOrigin(FeatureSchema property, String originType
     Map<String, FeatureSchema> taggedChildren = null;
     for (Map.Entry<String, FeatureSchema> e : property.getPropertyMap().entrySet()) {
       FeatureSchema childTagged = tagOrigin(e.getValue(), originType);
+      // reference comparison is intentional here: tagOrigin returns the same instance
+      // when nothing changed, so this is a cheap way to detect an actual change
       if (childTagged != e.getValue()) {
         if (taggedChildren == null) {
-          taggedChildren = new LinkedHashMap<>(property.getPropertyMap());
+          taggedChildren = copyOf(property.getPropertyMap());
         }
         taggedChildren.put(e.getKey(), childTagged);
       }
@@ -173,6 +176,10 @@ private static FeatureSchema tagOrigin(FeatureSchema property, String originType
     return b.build();
   }
 
+  private static Map<String, FeatureSchema> copyOf(Map<String, FeatureSchema> propertyMap) {
+    return new LinkedHashMap<>(propertyMap);
+  }
+
   private FeatureSchema resolve(String ref, FeatureProviderDataV2 data) {
     String key = getKey(ref);