JAMES-4079 Fix RecipientRewriteTable adds duplicate mapping (apache#2449

)
amichair · Oct 14, 2024 · 0f484fb · 0f484fb
1 parent 5408d21
commit 0f484fb
Show file tree

Hide file tree

Showing 7 changed files with 44 additions and 112 deletions.
diff --git a/...assandra/src/main/java/org/apache/james/rrt/cassandra/CassandraRecipientRewriteTable.java b/...assandra/src/main/java/org/apache/james/rrt/cassandra/CassandraRecipientRewriteTable.java
@@ -24,7 +24,6 @@
 import jakarta.inject.Inject;
 
 import org.apache.commons.lang3.tuple.Pair;
-import org.apache.james.core.Domain;
 import org.apache.james.rrt.api.RecipientRewriteTableException;
 import org.apache.james.rrt.lib.AbstractRecipientRewriteTable;
 import org.apache.james.rrt.lib.Mapping;
@@ -79,13 +78,6 @@ public Map<MappingSource, Mappings> getAllMappings() {
             .block();
     }
 
-    @Override
-    protected Mappings mapAddress(String user, Domain domain) {
-        return cassandraRecipientRewriteTableDAO.retrieveMappings(MappingSource.fromUser(user, domain)).blockOptional()
-            .or(() -> cassandraRecipientRewriteTableDAO.retrieveMappings(MappingSource.fromDomain(domain)).blockOptional())
-            .orElse(MappingsImpl.empty());
-    }
-
     @Override
     public Stream<MappingSource> listSources(Mapping mapping) throws RecipientRewriteTableException {
         Preconditions.checkArgument(listSourcesSupportedType.contains(mapping.getType()),

diff --git a/server/data/data-file/src/main/java/org/apache/james/rrt/file/XMLRecipientRewriteTable.java b/server/data/data-file/src/main/java/org/apache/james/rrt/file/XMLRecipientRewriteTable.java
@@ -25,7 +25,6 @@
 import org.apache.commons.configuration2.HierarchicalConfiguration;
 import org.apache.commons.configuration2.ex.ConfigurationException;
 import org.apache.commons.configuration2.tree.ImmutableNode;
-import org.apache.james.core.Domain;
 import org.apache.james.rrt.api.RecipientRewriteTableException;
 import org.apache.james.rrt.lib.AbstractRecipientRewriteTable;
 import org.apache.james.rrt.lib.Mapping;
@@ -60,14 +59,6 @@ protected void doConfigure(HierarchicalConfiguration<ImmutableNode> arg0) throws
         }
     }
 
-    @Override
-    protected Mappings mapAddress(String user, Domain domain) {
-        return Optional.ofNullable(mappings)
-            .map(mappings -> RecipientRewriteTableUtil.getTargetString(user, domain, mappings))
-            .map(MappingsImpl::fromRawString)
-            .orElse(MappingsImpl.empty());
-    }
-
     @Override
     public Mappings getStoredMappings(MappingSource source) {
         return Optional.ofNullable(mappings)

diff --git a/.../data/data-file/src/test/java/org/apache/james/rrt/file/XMLRecipientRewriteTableTest.java b/.../data/data-file/src/test/java/org/apache/james/rrt/file/XMLRecipientRewriteTableTest.java
@@ -78,6 +78,11 @@ public void addMappingShouldThrowWhenMappingAlreadyExists() {
     public void testStoreAndGetMappings() {
     }
 
+    @Test
+    @Disabled("XMLRecipientRewriteTable is read only")
+    public void testStoreDuplicateMapping() {
+    }
+
     @Test
     @Disabled("XMLRecipientRewriteTable is read only")
     public void testStoreAndRetrieveRegexMapping() {

diff --git a/server/data/data-jpa/src/main/java/org/apache/james/rrt/jpa/JPARecipientRewriteTable.java b/server/data/data-jpa/src/main/java/org/apache/james/rrt/jpa/JPARecipientRewriteTable.java
@@ -113,19 +113,6 @@ public void removeMapping(MappingSource source, Mapping mapping) throws Recipien
         }
     }
 
-    @Override
-    protected Mappings mapAddress(String user, Domain domain) throws RecipientRewriteTableException {
-        Mappings userDomainMapping = getStoredMappings(MappingSource.fromUser(user, domain));
-        if (userDomainMapping != null && !userDomainMapping.isEmpty()) {
-            return userDomainMapping;
-        }
-        Mappings domainMapping = getStoredMappings(MappingSource.fromDomain(domain));
-        if (domainMapping != null && !domainMapping.isEmpty()) {
-            return domainMapping;
-        }
-        return MappingsImpl.empty();
-    }
-
     @Override
     @SuppressWarnings("unchecked")
     public Mappings getStoredMappings(MappingSource source) throws RecipientRewriteTableException {

diff --git a/...ta/data-library/src/main/java/org/apache/james/rrt/lib/AbstractRecipientRewriteTable.java b/...ta/data-library/src/main/java/org/apache/james/rrt/lib/AbstractRecipientRewriteTable.java
@@ -406,7 +406,13 @@ public void removeAliasMapping(MappingSource source, String address) throws Reci
      * It must never return null but throw RecipientRewriteTableException on errors and return an empty Mappings
      * object if no mapping is found.
      */
-    protected abstract Mappings mapAddress(String user, Domain domain) throws RecipientRewriteTableException;
+    protected Mappings mapAddress(String user, Domain domain) throws RecipientRewriteTableException {
+        Mappings mappings = getStoredMappings(MappingSource.fromUser(user, domain));
+        if (mappings.isEmpty()) {
+            mappings = getStoredMappings(MappingSource.fromDomain(domain));
+        }
+        return mappings;
+    }
 
     private void checkDomainMappingSourceIsManaged(MappingSource source) throws RecipientRewriteTableException {
         Optional<Domain> notManagedSourceDomain = source.availableDomain()

diff --git a/...ta/data-library/src/test/java/org/apache/james/rrt/lib/RecipientRewriteTableContract.java b/...ta/data-library/src/test/java/org/apache/james/rrt/lib/RecipientRewriteTableContract.java
@@ -104,6 +104,20 @@ default void testStoreAndGetMappings() throws Exception {
         assertThat(virtualUserTable().getResolvedMappings("prefix_abc", domain)).isNotEmpty();
     }
 
+    @Test
+    default void testStoreDuplicateMapping() throws Exception {
+        Domain domain = Domain.of("james");
+        MappingSource source = MappingSource.fromUser(USER, domain);
+
+        Mapping mapping = Mapping.group(ADDRESS);
+        Mapping mapping2 = Mapping.group(ADDRESS);
+
+        virtualUserTable().addMapping(source, mapping);
+        virtualUserTable().addMapping(source, mapping2);
+
+        assertThat(virtualUserTable().mapAddress(USER, domain).size()).isEqualTo(1);
+    }
+
     @Test
     default void testQuotedLocalPart() {
         assertThatCode(() -> virtualUserTable().getResolvedMappings("\"a@b\"", Domain.of("test"))).doesNotThrowAnyException();

diff --git a/...ta/data-memory/src/main/java/org/apache/james/rrt/memory/MemoryRecipientRewriteTable.java b/...ta/data-memory/src/main/java/org/apache/james/rrt/memory/MemoryRecipientRewriteTable.java
@@ -19,113 +19,50 @@
 
 package org.apache.james.rrt.memory;
 
-import java.util.ArrayList;
-import java.util.Collection;
-import java.util.List;
+import java.util.HashMap;
+import java.util.LinkedHashSet;
 import java.util.Map;
-import java.util.Optional;
-import java.util.stream.Stream;
+import java.util.Set;
+import java.util.stream.Collectors;
 
-import org.apache.commons.lang3.tuple.Pair;
-import org.apache.james.core.Domain;
-import org.apache.james.core.Username;
 import org.apache.james.rrt.lib.AbstractRecipientRewriteTable;
 import org.apache.james.rrt.lib.Mapping;
 import org.apache.james.rrt.lib.MappingSource;
 import org.apache.james.rrt.lib.Mappings;
 import org.apache.james.rrt.lib.MappingsImpl;
 
-import com.google.common.base.Objects;
-import com.google.common.collect.ImmutableMap;
-import com.google.common.collect.Multimaps;
-
 public class MemoryRecipientRewriteTable extends AbstractRecipientRewriteTable {
 
-    private static class InMemoryMappingEntry {
-        private final MappingSource source;
-        private final Mapping mapping;
-
-        public InMemoryMappingEntry(MappingSource source, Mapping mapping) {
-            this.source = source;
-            this.mapping = mapping;
-        }
-
-        public MappingSource getSource() {
-            return source;
-        }
-
-        public Mapping getMapping() {
-            return mapping;
-        }
-
-        @Override
-        public boolean equals(Object o) {
-            if (o == null || this.getClass() != o.getClass()) {
-                return false;
-            }
-
-            InMemoryMappingEntry that = (InMemoryMappingEntry) o;
-
-            return Objects.equal(this.source, that.source)
-                && Objects.equal(this.mapping, that.mapping);
-        }
-
-        @Override
-        public int hashCode() {
-            return Objects.hashCode(source, mapping);
-        }
-    }
+    private final Map<MappingSource, Set<Mapping>> table = new HashMap<>();
 
-    private final List<InMemoryMappingEntry> mappingEntries;
-
-    public MemoryRecipientRewriteTable() {
-        mappingEntries = new ArrayList<>();
+    private Mappings toMappings(Set<Mapping> mappings) {
+        return mappings == null ? MappingsImpl.empty() : MappingsImpl.fromMappings(mappings.stream());
     }
 
     @Override
     public void addMapping(MappingSource source, Mapping mapping) {
-        mappingEntries.add(new InMemoryMappingEntry(source, mapping));
+        table.computeIfAbsent(source, s -> new LinkedHashSet<>()).add(mapping);
     }
 
     @Override
     public void removeMapping(MappingSource source, Mapping mapping) {
-        mappingEntries.remove(new InMemoryMappingEntry(source, mapping));
+        Set<Mapping> mappings = table.get(source);
+        if (mappings != null) {
+            mappings.remove(mapping);
+            if (mappings.isEmpty()) {
+                table.remove(source);
+            }
+        }
     }
 
     @Override
     public Mappings getStoredMappings(MappingSource mappingSource) {
-        return retrieveMappings(mappingSource)
-            .orElse(MappingsImpl.empty());
-    }
-
-    @Override
-    protected Mappings mapAddress(String user, Domain domain) {
-        return retrieveMappings(MappingSource.fromUser(Username.fromLocalPartWithDomain(user, domain)))
-            .or(() -> retrieveMappings(MappingSource.fromDomain(domain)))
-            .orElse(MappingsImpl.empty());
+        return toMappings(table.get(mappingSource));
     }
 
     @Override
     public Map<MappingSource, Mappings> getAllMappings() {
-        return Multimaps.index(mappingEntries, InMemoryMappingEntry::getSource)
-            .asMap()
-            .entrySet()
-            .stream()
-            .map(entry -> Pair.of(entry.getKey(), toMappings(entry.getValue())))
-            .collect(ImmutableMap.toImmutableMap(Pair::getKey, Pair::getValue));
-    }
-
-    private MappingsImpl toMappings(Collection<InMemoryMappingEntry> inMemoryMappingEntries) {
-        return MappingsImpl.fromMappings(inMemoryMappingEntries
-            .stream()
-            .map(InMemoryMappingEntry::getMapping));
+        return table.entrySet().stream()
+            .collect(Collectors.toMap(e -> e.getKey(), e -> toMappings(e.getValue())));
     }
-
-    private Optional<Mappings> retrieveMappings(MappingSource mappingSource) {
-        Stream<Mapping> userEntries = mappingEntries.stream()
-            .filter(mappingEntry -> mappingEntry.source.equals(mappingSource))
-            .map(InMemoryMappingEntry::getMapping);
-        return MappingsImpl.fromMappings(userEntries).toOptional();
-    }
-
 }