Configurable redesign (#261)

* Configurable redesign * Remove code duplicates by introducing ChildClassConfigurable * Apply review comments
ArDoCo · Aug 23, 2023 · 14550b5 · 14550b5
1 parent 12b0c10
commit 14550b5
Show file tree

Hide file tree

Showing 37 changed files with 231 additions and 696 deletions.
diff --git a/...mon/src/main/java/edu/kit/kastel/mcse/ardoco/core/configuration/AbstractConfigurable.java b/...mon/src/main/java/edu/kit/kastel/mcse/ardoco/core/configuration/AbstractConfigurable.java
@@ -25,21 +25,9 @@ public abstract class AbstractConfigurable implements IConfigurable {
 
     private SortedMap<String, String> lastAppliedConfiguration = new TreeMap<>();
 
-    protected final <E> List<E> findByClassName(List<String> selected, List<E> instances) {
-        List<E> target = new ArrayList<>(0);
-        for (var clazz : selected) {
-            var elem = instances.stream()
-                    .filter(e -> e.getClass().getSimpleName().equals(clazz))
-                    .findFirst()
-                    .orElseThrow(() -> new IllegalArgumentException("Could not find " + clazz));
-            target.add(elem);
-        }
-        return target;
-    }
-
     @Override
     public final void applyConfiguration(SortedMap<String, String> additionalConfiguration) {
-        applyConfiguration(additionalConfiguration, this.getClass());
+        applyConfiguration(additionalConfiguration, this, this.getClass());
         delegateApplyConfigurationToInternalObjects(additionalConfiguration);
         this.lastAppliedConfiguration = new TreeMap<>(additionalConfiguration);
     }
@@ -51,23 +39,49 @@ public SortedMap<String, String> getLastAppliedConfiguration() {
 
     protected abstract void delegateApplyConfigurationToInternalObjects(SortedMap<String, String> additionalConfiguration);
 
-    private void applyConfiguration(SortedMap<String, String> additionalConfiguration, Class<?> currentClass) {
-        if (currentClass == Object.class || currentClass == AbstractConfigurable.class)
+    private void applyConfiguration(SortedMap<String, String> additionalConfiguration, AbstractConfigurable configurable, Class<?> currentClassInHierarchy) {
+        if (currentClassInHierarchy == Object.class || currentClassInHierarchy == AbstractConfigurable.class)
             return;
 
-        var fields = currentClass.getDeclaredFields();
+        if (currentClassInHierarchy.getAnnotation(NoConfiguration.class) != null) {
+            logger.debug("Skipping configuration for class {}", currentClassInHierarchy.getSimpleName());
+            return;
+        }
+
+        var fields = currentClassInHierarchy.getDeclaredFields();
         for (Field field : fields) {
             if (!field.isAnnotationPresent(Configurable.class)) {
                 continue;
             }
-            Configurable c = field.getAnnotation(Configurable.class);
-            String key = c.key().isBlank() ? (currentClass.getSimpleName() + CLASS_ATTRIBUTE_CONNECTOR + field.getName()) : c.key();
+            String key = getKeyOfField(configurable, currentClassInHierarchy, field);
             if (additionalConfiguration.containsKey(key)) {
                 setValue(field, additionalConfiguration.get(key));
             }
         }
 
-        applyConfiguration(additionalConfiguration, currentClass.getSuperclass());
+        applyConfiguration(additionalConfiguration, configurable, currentClassInHierarchy.getSuperclass());
+    }
+
+    /**
+     * Returns the key (for the configuration file) of a field. If the field is marked as ChildClassConfigurable, the key is based on the class of the
+     * configurable object. Otherwise, the key is based on the class where the field is defined.
+     * 
+     * @param configurable            the configurable object
+     * @param currentClassInHierarchy the class where the field is defined
+     * @param field                   the field
+     * @return the key of the field
+     */
+    public static String getKeyOfField(AbstractConfigurable configurable, Class<?> currentClassInHierarchy, Field field) {
+        Configurable configurableAnnotation = field.getAnnotation(Configurable.class);
+        ChildClassConfigurable childClassConfigurableAnnotation = field.getAnnotation(ChildClassConfigurable.class);
+
+        if (childClassConfigurableAnnotation != null && !configurableAnnotation.key().isBlank()) {
+            throw new IllegalStateException("You cannot define a key for a field that is marked as ChildClassConfigurable.");
+        }
+
+        String classOfDefinition = childClassConfigurableAnnotation == null ? currentClassInHierarchy.getSimpleName() : configurable.getClass().getSimpleName();
+
+        return configurableAnnotation.key().isBlank() ? (classOfDefinition + CLASS_ATTRIBUTE_CONNECTOR + field.getName()) : configurableAnnotation.key();
     }
 
     private void setValue(Field field, String value) {

diff --git a/...n/src/main/java/edu/kit/kastel/mcse/ardoco/core/configuration/ChildClassConfigurable.java b/...n/src/main/java/edu/kit/kastel/mcse/ardoco/core/configuration/ChildClassConfigurable.java
@@ -0,0 +1,18 @@
+/* Licensed under MIT 2023. */
+package edu.kit.kastel.mcse.ardoco.core.configuration;
+
+import java.lang.annotation.Documented;
+import java.lang.annotation.ElementType;
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+import java.lang.annotation.Target;
+
+/**
+ * This annotation marks a field that is configurable as configured by child class. That means that the key that is used to configure the field is based on the
+ * actual class (not on the class where the configurable field is defined).
+ */
+@Retention(RetentionPolicy.RUNTIME)
+@Documented
+@Target(ElementType.FIELD)
+public @interface ChildClassConfigurable {
+}
diff --git a/...k/common/src/main/java/edu/kit/kastel/mcse/ardoco/core/configuration/NoConfiguration.java b/...k/common/src/main/java/edu/kit/kastel/mcse/ardoco/core/configuration/NoConfiguration.java
@@ -0,0 +1,18 @@
+/* Licensed under MIT 2023. */
+package edu.kit.kastel.mcse.ardoco.core.configuration;
+
+import java.lang.annotation.Documented;
+import java.lang.annotation.ElementType;
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+import java.lang.annotation.Target;
+
+/**
+ * This annotation is used to mark classes that should not be configured. This means that the fields of the class will not be modified. The cascade
+ * configuration will be applied to the fields of the class.
+ */
+@Retention(RetentionPolicy.RUNTIME)
+@Documented
+@Target(ElementType.TYPE)
+public @interface NoConfiguration {
+}
diff --git a/...common/src/main/java/edu/kit/kastel/mcse/ardoco/core/pipeline/AbstractExecutionStage.java b/...common/src/main/java/edu/kit/kastel/mcse/ardoco/core/pipeline/AbstractExecutionStage.java
@@ -2,7 +2,13 @@
 package edu.kit.kastel.mcse.ardoco.core.pipeline;
 
 import java.util.List;
+import java.util.SortedMap;
 
+import org.eclipse.collections.api.factory.Lists;
+import org.eclipse.collections.api.list.MutableList;
+
+import edu.kit.kastel.mcse.ardoco.core.configuration.ChildClassConfigurable;
+import edu.kit.kastel.mcse.ardoco.core.configuration.Configurable;
 import edu.kit.kastel.mcse.ardoco.core.data.DataRepository;
 import edu.kit.kastel.mcse.ardoco.core.pipeline.agent.PipelineAgent;
 
@@ -11,18 +17,26 @@
  * Inconsistency-Checker.
  * <p>
  * Implementing classes need to implement {@link #initializeState()} that cares for setting up the state for processing.
- * Additionally, implementing classes need to implement {@link #getEnabledAgents()} that returns the listof enabled {@link PipelineAgent pipeline agents}
  */
 public abstract class AbstractExecutionStage extends Pipeline {
 
+    private final MutableList<PipelineAgent> agents;
+
+    @Configurable
+    @ChildClassConfigurable
+    private List<String> enabledAgents;
+
     /**
      * Constructor for ExecutionStages
-     *
+     * 
+     * @param agents         the agents that could be executed by this pipeline
      * @param id             the id of the stage
      * @param dataRepository the {@link DataRepository} that should be used
      */
-    protected AbstractExecutionStage(String id, DataRepository dataRepository) {
+    protected AbstractExecutionStage(List<? extends PipelineAgent> agents, String id, DataRepository dataRepository) {
         super(id, dataRepository);
+        this.agents = Lists.mutable.withAll(agents);
+        this.enabledAgents = this.agents.collect(PipelineAgent::getId);
     }
 
     @Override
@@ -37,8 +51,10 @@ protected final void preparePipelineSteps() {
     protected final void initialize() {
         initializeState();
 
-        for (var agent : getEnabledAgents()) {
-            this.addPipelineStep(agent);
+        for (var agent : agents) {
+            if (enabledAgents.contains(agent.getId())) {
+                this.addPipelineStep(agent);
+            }
         }
     }
 
@@ -47,10 +63,11 @@ protected final void initialize() {
      */
     protected abstract void initializeState();
 
-    /**
-     * Return the enabled {@link PipelineAgent agents}
-     *
-     * @return the list of agents
-     */
-    protected abstract List<PipelineAgent> getEnabledAgents();
+    @Override
+    protected void delegateApplyConfigurationToInternalObjects(SortedMap<String, String> additionalConfiguration) {
+        super.delegateApplyConfigurationToInternalObjects(additionalConfiguration);
+        for (var agent : agents) {
+            agent.applyConfiguration(additionalConfiguration);
+        }
+    }
 }
diff --git a/...rk/common/src/main/java/edu/kit/kastel/mcse/ardoco/core/pipeline/agent/PipelineAgent.java b/...rk/common/src/main/java/edu/kit/kastel/mcse/ardoco/core/pipeline/agent/PipelineAgent.java
@@ -1,8 +1,12 @@
 /* Licensed under MIT 2022-2023. */
 package edu.kit.kastel.mcse.ardoco.core.pipeline.agent;
 
+import java.util.ArrayList;
 import java.util.List;
+import java.util.SortedMap;
 
+import edu.kit.kastel.mcse.ardoco.core.configuration.ChildClassConfigurable;
+import edu.kit.kastel.mcse.ardoco.core.configuration.Configurable;
 import edu.kit.kastel.mcse.ardoco.core.data.DataRepository;
 import edu.kit.kastel.mcse.ardoco.core.pipeline.AbstractExecutionStage;
 import edu.kit.kastel.mcse.ardoco.core.pipeline.Pipeline;
@@ -11,13 +15,21 @@
  * This class represents a pipeline agent that calculates some results for an {@link AbstractExecutionStage} execution
  * stage}.
  *
- * Implementing classes need to override {@link #getEnabledPipelineSteps()}.
+ * Implementing classes need to override.
  * Additionally, sub-classes are free to override {@link #initializeState()} to execute code at the beginning of the initialization before the main processing.
  */
 public abstract class PipelineAgent extends Pipeline implements Agent {
 
-    protected PipelineAgent(String id, DataRepository dataRepository) {
+    private final List<Informant> informants;
+
+    @Configurable
+    @ChildClassConfigurable
+    private List<String> enabledInformants;
+
+    protected PipelineAgent(List<? extends Informant> informants, String id, DataRepository dataRepository) {
         super(id, dataRepository);
+        this.informants = new ArrayList<>(informants);
+        this.enabledInformants = informants.stream().map(Informant::getId).toList();
     }
 
     @Override
@@ -31,8 +43,10 @@ protected final void preparePipelineSteps() {
      */
     protected final void initialize() {
         initializeState();
-        for (var informant : getEnabledPipelineSteps()) {
-            this.addPipelineStep(informant);
+        for (var informant : informants) {
+            if (enabledInformants.contains(informant.getId())) {
+                this.addPipelineStep(informant);
+            }
         }
     }
 
@@ -43,10 +57,10 @@ protected void initializeState() {
         // do nothing here
     }
 
-    /**
-     * Return the enabled pipeline steps (informants)
-     * 
-     * @return the list of Informants
-     */
-    protected abstract List<Informant> getEnabledPipelineSteps();
+    @Override
+    protected void delegateApplyConfigurationToInternalObjects(SortedMap<String, String> additionalConfiguration) {
+        super.delegateApplyConfigurationToInternalObjects(additionalConfiguration);
+        informants.forEach(filter -> filter.applyConfiguration(additionalConfiguration));
+    }
+
 }
diff --git a/...ine-core/src/main/java/edu/kit/kastel/mcse/ardoco/core/execution/ConfigurationHelper.java b/...ine-core/src/main/java/edu/kit/kastel/mcse/ardoco/core/execution/ConfigurationHelper.java
@@ -100,7 +100,7 @@ protected static void processConfigurationOfClass(Map<String, String> configs, C
     private static void fillConfigs(AbstractConfigurable object, List<Field> fields, Map<String, String> configs) throws IllegalAccessException {
         for (Field f : fields) {
             f.setAccessible(true);
-            var key = f.getDeclaringClass().getSimpleName() + AbstractConfigurable.CLASS_ATTRIBUTE_CONNECTOR + f.getName();
+            var key = AbstractConfigurable.getKeyOfField(object, f.getDeclaringClass(), f);
             var rawValue = f.get(object);
             var value = getValue(rawValue);
             if (configs.containsKey(key)) {

diff --git a/...core/src/test/java/edu/kit/kastel/mcse/ardoco/core/execution/ConfigurationHelperTest.java b/...core/src/test/java/edu/kit/kastel/mcse/ardoco/core/execution/ConfigurationHelperTest.java
@@ -11,6 +11,7 @@
 
 import edu.kit.kastel.mcse.ardoco.core.architecture.Deterministic;
 import edu.kit.kastel.mcse.ardoco.core.configuration.AbstractConfigurable;
+import edu.kit.kastel.mcse.ardoco.core.configuration.ChildClassConfigurable;
 import edu.kit.kastel.mcse.ardoco.core.configuration.Configurable;
 
 @Deterministic
@@ -77,6 +78,54 @@ void testBasicConfigurable() throws Exception {
 
     }
 
+    @Test
+    void testBaseAndChildConfigurable() throws Exception {
+        SortedMap<String, String> configs = new TreeMap<>();
+        ConfigurationHelper.processConfigurationOfClass(configs, TestBaseConfigurable.class);
+        Assertions.assertEquals(1, configs.size());
+        Assertions.assertEquals("1", configs.get(TestBaseConfigurable.class.getSimpleName() + AbstractConfigurable.CLASS_ATTRIBUTE_CONNECTOR + "value"));
+
+        configs = new TreeMap<>();
+        ConfigurationHelper.processConfigurationOfClass(configs, TestChildConfigurable.class);
+        Assertions.assertEquals(1, configs.size());
+        Assertions.assertEquals("2", configs.get(TestChildConfigurable.class.getSimpleName() + AbstractConfigurable.CLASS_ATTRIBUTE_CONNECTOR + "value"));
+
+        configs = new TreeMap<>(Map.of(//
+                TestBaseConfigurable.class.getSimpleName() + AbstractConfigurable.CLASS_ATTRIBUTE_CONNECTOR + "value", "42", //
+                TestChildConfigurable.class.getSimpleName() + AbstractConfigurable.CLASS_ATTRIBUTE_CONNECTOR + "value", "43" //
+        ));
+
+        var t1 = new TestBaseConfigurable();
+        t1.applyConfiguration(configs);
+        Assertions.assertEquals(42, t1.value);
+
+        var t2 = new TestChildConfigurable();
+        t2.applyConfiguration(configs);
+        Assertions.assertEquals(43, t2.value);
+    }
+
+    private static class TestBaseConfigurable extends AbstractConfigurable {
+        @Configurable
+        @ChildClassConfigurable
+        public int value;
+
+        public TestBaseConfigurable() {
+            value = 1;
+        }
+
+        @Override
+        protected void delegateApplyConfigurationToInternalObjects(SortedMap<String, String> additionalConfiguration) {
+            // NOP
+        }
+    }
+
+    private static final class TestChildConfigurable extends TestBaseConfigurable {
+        public TestChildConfigurable() {
+            super();
+            value = 2;
+        }
+    }
+
     private static final class TestConfigurable extends AbstractConfigurable {
 
         @Configurable

diff --git a/...ava/edu/kit/kastel/mcse/ardoco/core/codetraceability/SadCodeTraceabilityLinkRecovery.java b/...ava/edu/kit/kastel/mcse/ardoco/core/codetraceability/SadCodeTraceabilityLinkRecovery.java
@@ -4,30 +4,16 @@
 import java.util.List;
 import java.util.SortedMap;
 
-import org.eclipse.collections.api.factory.Lists;
-import org.eclipse.collections.api.list.MutableList;
-
 import edu.kit.kastel.mcse.ardoco.core.api.codetraceability.CodeTraceabilityState;
 import edu.kit.kastel.mcse.ardoco.core.codetraceability.agents.ArchitectureLinkToCodeLinkTransformerAgent;
 import edu.kit.kastel.mcse.ardoco.core.common.util.DataRepositoryHelper;
-import edu.kit.kastel.mcse.ardoco.core.configuration.Configurable;
 import edu.kit.kastel.mcse.ardoco.core.data.DataRepository;
 import edu.kit.kastel.mcse.ardoco.core.pipeline.AbstractExecutionStage;
-import edu.kit.kastel.mcse.ardoco.core.pipeline.agent.Agent;
-import edu.kit.kastel.mcse.ardoco.core.pipeline.agent.PipelineAgent;
 
 public class SadCodeTraceabilityLinkRecovery extends AbstractExecutionStage {
 
-    private final MutableList<PipelineAgent> agents;
-
-    @Configurable
-    private List<String> enabledAgents;
-
     public SadCodeTraceabilityLinkRecovery(DataRepository dataRepository) {
-        super(SadCodeTraceabilityLinkRecovery.class.getSimpleName(), dataRepository);
-
-        agents = Lists.mutable.of(new ArchitectureLinkToCodeLinkTransformerAgent(dataRepository));
-        enabledAgents = agents.collect(Agent::getId);
+        super(List.of(new ArchitectureLinkToCodeLinkTransformerAgent(dataRepository)), SadCodeTraceabilityLinkRecovery.class.getSimpleName(), dataRepository);
     }
 
     public static SadCodeTraceabilityLinkRecovery get(SortedMap<String, String> additionalConfigs, DataRepository dataRepository) {
@@ -44,17 +30,4 @@ protected void initializeState() {
             dataRepository.addData(CodeTraceabilityState.ID, codeTraceabilityState);
         }
     }
-
-    @Override
-    protected List<PipelineAgent> getEnabledAgents() {
-        return findByClassName(enabledAgents, agents);
-    }
-
-    @Override
-    protected void delegateApplyConfigurationToInternalObjects(SortedMap<String, String> additionalConfiguration) {
-        super.delegateApplyConfigurationToInternalObjects(additionalConfiguration);
-        for (var agent : agents) {
-            agent.applyConfiguration(additionalConfiguration);
-        }
-    }
 }