Improve Fess Ingest feature implementation

src/main/java/org/codelibs/fess/ingest/IngestFactory.java

@@ -15,24 +15,34 @@
  */
 package org.codelibs.fess.ingest;
 
+import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.Comparator;
+import java.util.List;
 
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
 
 /**
  * Factory class for managing and organizing document ingesters.
- * The factory maintains a sorted collection of ingesters based on their priority
- * and provides methods to add new ingesters and retrieve the current collection.
+ * The factory maintains a sorted array of ingesters based on their priority.
  *
- * Ingesters are automatically sorted by priority, with lower numbers having higher priority.
+ * <p>Ingesters are automatically sorted by priority, with lower numbers having higher priority.
+ * This class is designed for initialization-time registration only and is not thread-safe.</p>
+ *
+ * <p><strong>IMPORTANT:</strong> The {@code add()} method should only be called during
+ * the initialization phase (typically via DI container) before the factory is accessed
+ * by multiple threads. Runtime modification is not supported.</p>
  */
 public class IngestFactory {
     /** Logger instance for this class */
     private static final Logger logger = LogManager.getLogger(IngestFactory.class);
 
+    /** Comparator for sorting ingesters by priority */
+    private static final Comparator<Ingester> PRIORITY_COMPARATOR = Comparator.comparingInt(Ingester::getPriority);
+
     /** Array of registered ingesters, sorted by priority */
-    private Ingester[] ingesters = {};
+    private Ingester[] ingesters = new Ingester[0];
 
     /**
      * Default constructor.
@@ -43,27 +53,51 @@ public IngestFactory() {
 
     /**
      * Adds an ingester to the factory.
-     * The ingester is inserted into the collection and the array is re-sorted by priority.
-     * This method is thread-safe.
+     * If an ingester with the same class already exists, it will be replaced.
      *
-     * @param ingester the ingester to add
+     * <p><strong>IMPORTANT:</strong> This method is NOT thread-safe. It should only
+     * be called during the initialization phase before the factory is used.</p>
+     *
+     * @param ingester the ingester to add (must not be null)
+     * @throws IllegalArgumentException if ingester is null
      */
-    public synchronized void add(final Ingester ingester) {
+    public void add(final Ingester ingester) {
+        if (ingester == null) {
+            throw new IllegalArgumentException("Ingester cannot be null");
+        }
+
         if (logger.isDebugEnabled()) {
-            logger.debug("Loaded {}", ingester.getClass().getSimpleName());
+            logger.debug("Loading {}", ingester.getClass().getSimpleName());
+        }
+
+        // Convert to list for manipulation
+        final List<Ingester> list = new ArrayList<>(Arrays.asList(ingesters));
+
+        // Remove existing ingester of the same class if present
+        list.removeIf(existing -> existing.getClass().equals(ingester.getClass()));
+
+        // Add new ingester and sort by priority
+        list.add(ingester);
+        list.sort(PRIORITY_COMPARATOR);
+
+        // Update array
+        ingesters = list.toArray(new Ingester[0]);
+
+        if (logger.isDebugEnabled()) {
+            logger.debug("Loaded {} with priority {}", ingester.getClass().getSimpleName(), ingester.getPriority());
         }
-        final Ingester[] newIngesters = Arrays.copyOf(ingesters, ingesters.length + 1);
-        newIngesters[ingesters.length] = ingester;
-        Arrays.sort(newIngesters, (o1, o2) -> o1.priority - o2.priority);
-        ingesters = newIngesters;
     }
 
     /**
      * Returns the array of registered ingesters sorted by priority.
      * The returned array contains all ingesters in priority order
      * (lower priority numbers first).
      *
-     * @return the sorted array of ingesters
+     * <p>The array is returned directly for read-only access. Modifications
+     * to the array will not affect future calls to this method, but callers
+     * should treat the array as immutable.</p>
+     *
+     * @return the array of ingesters
      */
     public Ingester[] getIngesters() {
         return ingesters;

src/main/java/org/codelibs/fess/ingest/Ingester.java

@@ -29,12 +29,25 @@
  * extract additional metadata, or perform other transformations during the
  * indexing process.
  *
- * Ingesters are processed in priority order, with lower numbers having higher priority.
+ * <p>Ingesters are processed in priority order, with lower numbers having higher priority.
+ * Valid priority values range from 0 to 999, with a default value of 99.</p>
+ *
+ * <p>Implementations should override the appropriate {@code process()} methods
+ * to implement their specific document transformation logic.</p>
  */
 public abstract class Ingester {
 
+    /** Minimum allowed priority value */
+    public static final int MIN_PRIORITY = 0;
+
+    /** Maximum allowed priority value */
+    public static final int MAX_PRIORITY = 999;
+
+    /** Default priority value */
+    public static final int DEFAULT_PRIORITY = 99;
+
     /** Priority of this ingester (lower numbers = higher priority) */
-    protected int priority = 99;
+    protected int priority = DEFAULT_PRIORITY;
 
     /**
      * Default constructor.
@@ -57,9 +70,14 @@ public int getPriority() {
      * Sets the priority of this ingester.
      * Lower numbers indicate higher priority.
      *
-     * @param priority the priority value to set
+     * @param priority the priority value to set (must be between 0 and 999)
+     * @throws IllegalArgumentException if priority is outside valid range
      */
     public void setPriority(final int priority) {
+        if (priority < MIN_PRIORITY || priority > MAX_PRIORITY) {
+            throw new IllegalArgumentException(
+                    "Priority must be between " + MIN_PRIORITY + " and " + MAX_PRIORITY + ", but was: " + priority);
+        }
         this.priority = priority;
     }
 
@@ -128,4 +146,43 @@ protected Map<String, Object> process(final Map<String, Object> target) {
         return target;
     }
 
+    /**
+     * Indicates whether some other object is "equal to" this one.
+     * Two ingesters are considered equal if they are of the same class.
+     *
+     * @param obj the reference object with which to compare
+     * @return true if this object is the same as the obj argument; false otherwise
+     */
+    @Override
+    public boolean equals(final Object obj) {
+        if (this == obj) {
+            return true;
+        }
+        if (obj == null || getClass() != obj.getClass()) {
+            return false;
+        }
+        return true;
+    }
+
+    /**
+     * Returns a hash code value for the object.
+     * The hash code is based on the class type.
+     *
+     * @return a hash code value for this object
+     */
+    @Override
+    public int hashCode() {
+        return getClass().hashCode();
+    }
+
+    /**
+     * Returns a string representation of this ingester.
+     *
+     * @return a string representation including class name and priority
+     */
+    @Override
+    public String toString() {
+        return getClass().getSimpleName() + "{priority=" + priority + "}";
+    }
+
 }

src/test/java/org/codelibs/fess/ingest/IngestFactoryTest.java

@@ -15,35 +15,162 @@
  */
 package org.codelibs.fess.ingest;
 
+import java.util.Map;
+
 import org.codelibs.fess.unit.UnitFessTestCase;
 
+/**
+ * Test class for IngestFactory.
+ * Tests basic functionality of ingester registration and sorting.
+ */
 public class IngestFactoryTest extends UnitFessTestCase {
 
-    public void test_add_1() {
+    public void test_add_sortedOrder() {
         IngestFactory factory = new IngestFactory();
         factory.add(new TestIngester(1));
         factory.add(new TestIngester(2));
         factory.add(new TestIngester(3));
         Ingester[] ingesters = factory.getIngesters();
+        assertEquals(3, ingesters.length);
         assertEquals(1, ingesters[0].getPriority());
         assertEquals(2, ingesters[1].getPriority());
         assertEquals(3, ingesters[2].getPriority());
     }
 
-    public void test_add_2() {
+    public void test_add_reversedOrder() {
         IngestFactory factory = new IngestFactory();
         factory.add(new TestIngester(3));
         factory.add(new TestIngester(2));
         factory.add(new TestIngester(1));
         Ingester[] ingesters = factory.getIngesters();
+        assertEquals(3, ingesters.length);
         assertEquals(1, ingesters[0].getPriority());
         assertEquals(2, ingesters[1].getPriority());
         assertEquals(3, ingesters[2].getPriority());
     }
 
+    public void test_add_duplicateClass() {
+        IngestFactory factory = new IngestFactory();
+        factory.add(new TestIngester(1));
+        factory.add(new TestIngester(2)); // Same class, should replace
+        Ingester[] ingesters = factory.getIngesters();
+        assertEquals(1, ingesters.length); // Only one ingester should remain
+        assertEquals(2, ingesters[0].getPriority()); // Latest one should be kept
+    }
+
+    public void test_add_null() {
+        IngestFactory factory = new IngestFactory();
+        try {
+            factory.add(null);
+            fail("Expected IllegalArgumentException");
+        } catch (IllegalArgumentException e) {
+            assertEquals("Ingester cannot be null", e.getMessage());
+        }
+    }
+
+    public void test_addSameClassDifferentPriorities() {
+        IngestFactory factory = new IngestFactory();
+        factory.add(new TestIngester(10));
+        assertEquals(1, factory.getIngesters().length);
+        assertEquals(10, factory.getIngesters()[0].getPriority());
+
+        // Add same class with different priority - should replace
+        factory.add(new TestIngester(5));
+        assertEquals(1, factory.getIngesters().length);
+        assertEquals(5, factory.getIngesters()[0].getPriority());
+
+        // Add again with even different priority - should replace again
+        factory.add(new TestIngester(20));
+        assertEquals(1, factory.getIngesters().length);
+        assertEquals(20, factory.getIngesters()[0].getPriority());
+    }
+
+    public void test_multipleDifferentClassesSamePriorityStability() {
+        IngestFactory factory = new IngestFactory();
+
+        // Add multiple ingesters with same priority
+        TestIngester t1 = new TestIngester(5);
+        AnotherTestIngester t2 = new AnotherTestIngester(5);
+        ThirdTestIngester t3 = new ThirdTestIngester(5);
+
+        factory.add(t1);
+        factory.add(t2);
+        factory.add(t3);
+
+        Ingester[] ingesters = factory.getIngesters();
+        assertEquals(3, ingesters.length);
+
+        // All should have same priority
+        assertEquals(5, ingesters[0].getPriority());
+        assertEquals(5, ingesters[1].getPriority());
+        assertEquals(5, ingesters[2].getPriority());
+    }
+
+    public void test_largeNumberOfIngesters() {
+        IngestFactory factory = new IngestFactory();
+
+        // Add 100 ingesters with different priorities
+        for (int i = 0; i < 100; i++) {
+            if (i % 3 == 0) {
+                factory.add(new TestIngester(i));
+            } else if (i % 3 == 1) {
+                factory.add(new AnotherTestIngester(i));
+            } else {
+                factory.add(new ThirdTestIngester(i));
+            }
+        }
+
+        // Verify sorting and that ingesters were added
+        Ingester[] ingesters = factory.getIngesters();
+        assertTrue(ingesters.length > 0);
+        assertTrue(ingesters.length <= 100);
+
+        for (int i = 1; i < ingesters.length; i++) {
+            assertTrue("Ingesters not sorted at index " + i, ingesters[i - 1].getPriority() <= ingesters[i].getPriority());
+        }
+    }
+
+    public void test_addMultiple_samePriority() {
+        IngestFactory factory = new IngestFactory();
+        factory.add(new TestIngester(1));
+        factory.add(new AnotherTestIngester(1));
+        factory.add(new ThirdTestIngester(1));
+
+        Ingester[] ingesters = factory.getIngesters();
+        assertEquals(3, ingesters.length);
+        // All should have same priority
+        assertEquals(1, ingesters[0].getPriority());
+        assertEquals(1, ingesters[1].getPriority());
+        assertEquals(1, ingesters[2].getPriority());
+    }
+
     private static class TestIngester extends Ingester {
         public TestIngester(int priority) {
             this.priority = priority;
         }
+
+        @Override
+        protected Map<String, Object> process(Map<String, Object> target) {
+            target.put("test", "value");
+            return target;
+        }
+    }
+
+    private static class AnotherTestIngester extends Ingester {
+        public AnotherTestIngester(int priority) {
+            this.priority = priority;
+        }
+
+        @Override
+        protected Map<String, Object> process(Map<String, Object> target) {
+            target.put("another", "value");
+            return target;
+        }
+    }
+
+    private static class ThirdTestIngester extends Ingester {
+        public ThirdTestIngester(int priority) {
+            this.priority = priority;
+        }
     }
 }