james-server-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From btell...@apache.org
Subject [1/7] james-project git commit: JAMES-2445 Adding/removing groups/forwards should be indempotent
Date Tue, 03 Jul 2018 08:51:39 GMT
Repository: james-project
Updated Branches:
  refs/heads/master 21bb9757b -> 8992243e9


JAMES-2445 Adding/removing groups/forwards should be indempotent


Project: http://git-wip-us.apache.org/repos/asf/james-project/repo
Commit: http://git-wip-us.apache.org/repos/asf/james-project/commit/5d70b940
Tree: http://git-wip-us.apache.org/repos/asf/james-project/tree/5d70b940
Diff: http://git-wip-us.apache.org/repos/asf/james-project/diff/5d70b940

Branch: refs/heads/master
Commit: 5d70b940011af916daf329d10b27e12654991a48
Parents: 21bb975
Author: benwa <btellier@linagora.com>
Authored: Mon Jul 2 12:16:47 2018 +0700
Committer: benwa <btellier@linagora.com>
Committed: Tue Jul 3 15:49:54 2018 +0700

----------------------------------------------------------------------
 .../rrt/api/MappingAlreadyExistsException.java  | 30 ++++++++++++++++++++
 .../rrt/lib/AbstractRecipientRewriteTable.java  |  3 +-
 .../james/webadmin/routes/ForwardRoutes.java    | 15 ++++++++--
 .../james/webadmin/routes/GroupsRoutes.java     | 23 ++++++++++-----
 .../routes/DomainMappingsRoutesTest.java        | 16 +++++++++++
 .../webadmin/routes/ForwardRoutesTest.java      | 26 ++++++++++++-----
 .../james/webadmin/routes/GroupsRoutesTest.java | 26 ++++++++++++-----
 src/site/markdown/server/manage-webadmin.md     |  8 +++---
 8 files changed, 118 insertions(+), 29 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/james-project/blob/5d70b940/server/data/data-api/src/main/java/org/apache/james/rrt/api/MappingAlreadyExistsException.java
----------------------------------------------------------------------
diff --git a/server/data/data-api/src/main/java/org/apache/james/rrt/api/MappingAlreadyExistsException.java
b/server/data/data-api/src/main/java/org/apache/james/rrt/api/MappingAlreadyExistsException.java
new file mode 100644
index 0000000..4d80ad8
--- /dev/null
+++ b/server/data/data-api/src/main/java/org/apache/james/rrt/api/MappingAlreadyExistsException.java
@@ -0,0 +1,30 @@
+/****************************************************************
+ * Licensed to the Apache Software Foundation (ASF) under one   *
+ * or more contributor license agreements.  See the NOTICE file *
+ * distributed with this work for additional information        *
+ * regarding copyright ownership.  The ASF licenses this file   *
+ * to you under the Apache License, Version 2.0 (the            *
+ * "License"); you may not use this file except in compliance   *
+ * with the License.  You may obtain a copy of the License at   *
+ *                                                              *
+ *   http://www.apache.org/licenses/LICENSE-2.0                 *
+ *                                                              *
+ * Unless required by applicable law or agreed to in writing,   *
+ * software distributed under the License is distributed on an  *
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY       *
+ * KIND, either express or implied.  See the License for the    *
+ * specific language governing permissions and limitations      *
+ * under the License.                                           *
+ ****************************************************************/
+package org.apache.james.rrt.api;
+
+public class MappingAlreadyExistsException extends RecipientRewriteTableException {
+
+    public MappingAlreadyExistsException(String msg) {
+        super(msg);
+    }
+
+    public MappingAlreadyExistsException(String msg, Throwable t) {
+        super(msg, t);
+    }
+}

http://git-wip-us.apache.org/repos/asf/james-project/blob/5d70b940/server/data/data-library/src/main/java/org/apache/james/rrt/lib/AbstractRecipientRewriteTable.java
----------------------------------------------------------------------
diff --git a/server/data/data-library/src/main/java/org/apache/james/rrt/lib/AbstractRecipientRewriteTable.java
b/server/data/data-library/src/main/java/org/apache/james/rrt/lib/AbstractRecipientRewriteTable.java
index bb42e52..328de7a 100644
--- a/server/data/data-library/src/main/java/org/apache/james/rrt/lib/AbstractRecipientRewriteTable.java
+++ b/server/data/data-library/src/main/java/org/apache/james/rrt/lib/AbstractRecipientRewriteTable.java
@@ -34,6 +34,7 @@ import org.apache.james.core.User;
 import org.apache.james.domainlist.api.DomainList;
 import org.apache.james.domainlist.api.DomainListException;
 import org.apache.james.lifecycle.api.Configurable;
+import org.apache.james.rrt.api.MappingAlreadyExistsException;
 import org.apache.james.rrt.api.RecipientRewriteTable;
 import org.apache.james.rrt.api.RecipientRewriteTableException;
 import org.apache.james.rrt.lib.Mapping.Type;
@@ -313,7 +314,7 @@ public abstract class AbstractRecipientRewriteTable implements RecipientRewriteT
     private void checkDuplicateMapping(MappingSource source, Mapping mapping) throws RecipientRewriteTableException
{
         Mappings mappings = getUserDomainMappings(source);
         if (mappings != null && mappings.contains(mapping)) {
-            throw new RecipientRewriteTableException("Mapping " + mapping + " for " + source.asString()
+ " already exist!");
+            throw new MappingAlreadyExistsException("Mapping " + mapping + " for " + source.asString()
+ " already exist!");
         }
     }
 

http://git-wip-us.apache.org/repos/asf/james-project/blob/5d70b940/server/protocols/webadmin/webadmin-data/src/main/java/org/apache/james/webadmin/routes/ForwardRoutes.java
----------------------------------------------------------------------
diff --git a/server/protocols/webadmin/webadmin-data/src/main/java/org/apache/james/webadmin/routes/ForwardRoutes.java
b/server/protocols/webadmin/webadmin-data/src/main/java/org/apache/james/webadmin/routes/ForwardRoutes.java
index 935f88a..8ea0ec0 100644
--- a/server/protocols/webadmin/webadmin-data/src/main/java/org/apache/james/webadmin/routes/ForwardRoutes.java
+++ b/server/protocols/webadmin/webadmin-data/src/main/java/org/apache/james/webadmin/routes/ForwardRoutes.java
@@ -41,6 +41,7 @@ import javax.ws.rs.Produces;
 import org.apache.james.core.MailAddress;
 import org.apache.james.core.User;
 import org.apache.james.domainlist.api.DomainListException;
+import org.apache.james.rrt.api.MappingAlreadyExistsException;
 import org.apache.james.rrt.api.RecipientRewriteTable;
 import org.apache.james.rrt.api.RecipientRewriteTableException;
 import org.apache.james.rrt.lib.Mapping;
@@ -167,8 +168,16 @@ public class ForwardRoutes implements Routes {
         ensureUserExist(forwardBaseAddress);
         MailAddress destinationAddress = parseMailAddress(request.params(FORWARD_DESTINATION_ADDRESS));
         MappingSource source = MappingSource.fromUser(User.fromLocalPartWithDomain(forwardBaseAddress.getLocalPart(),
forwardBaseAddress.getDomain()));
-        recipientRewriteTable.addForwardMapping(source, destinationAddress.asString());
-        return halt(HttpStatus.CREATED_201);
+        addForward(source, destinationAddress);
+        return halt(HttpStatus.NO_CONTENT_204);
+    }
+
+    private void addForward(MappingSource source, MailAddress destinationAddress) throws
RecipientRewriteTableException {
+        try {
+            recipientRewriteTable.addForwardMapping(source, destinationAddress.asString());
+        } catch (MappingAlreadyExistsException e) {
+            // ignore
+        }
     }
 
     private void ensureUserExist(MailAddress mailAddress) throws UsersRepositoryException
{
@@ -201,7 +210,7 @@ public class ForwardRoutes implements Routes {
         MailAddress destinationAddressToBeRemoved = parseMailAddress(request.params(FORWARD_DESTINATION_ADDRESS));
         MappingSource source = MappingSource.fromUser(User.fromLocalPartWithDomain(baseAddress.getLocalPart(),
baseAddress.getDomain()));
         recipientRewriteTable.removeForwardMapping(source, destinationAddressToBeRemoved.asString());
-        return halt(HttpStatus.OK_200);
+        return halt(HttpStatus.NO_CONTENT_204);
     }
 
     @GET

http://git-wip-us.apache.org/repos/asf/james-project/blob/5d70b940/server/protocols/webadmin/webadmin-data/src/main/java/org/apache/james/webadmin/routes/GroupsRoutes.java
----------------------------------------------------------------------
diff --git a/server/protocols/webadmin/webadmin-data/src/main/java/org/apache/james/webadmin/routes/GroupsRoutes.java
b/server/protocols/webadmin/webadmin-data/src/main/java/org/apache/james/webadmin/routes/GroupsRoutes.java
index 2625a46..85771f8 100644
--- a/server/protocols/webadmin/webadmin-data/src/main/java/org/apache/james/webadmin/routes/GroupsRoutes.java
+++ b/server/protocols/webadmin/webadmin-data/src/main/java/org/apache/james/webadmin/routes/GroupsRoutes.java
@@ -43,6 +43,7 @@ import org.apache.james.core.MailAddress;
 import org.apache.james.core.User;
 import org.apache.james.domainlist.api.DomainList;
 import org.apache.james.domainlist.api.DomainListException;
+import org.apache.james.rrt.api.MappingAlreadyExistsException;
 import org.apache.james.rrt.api.RecipientRewriteTable;
 import org.apache.james.rrt.api.RecipientRewriteTableException;
 import org.apache.james.rrt.lib.Mapping;
@@ -55,7 +56,6 @@ import org.apache.james.webadmin.Constants;
 import org.apache.james.webadmin.Routes;
 import org.apache.james.webadmin.utils.ErrorResponder;
 import org.apache.james.webadmin.utils.ErrorResponder.ErrorType;
-import org.apache.james.webadmin.utils.JsonExtractException;
 import org.apache.james.webadmin.utils.JsonTransformer;
 import org.eclipse.jetty.http.HttpStatus;
 import org.slf4j.Logger;
@@ -64,6 +64,7 @@ import org.slf4j.LoggerFactory;
 import com.github.steveash.guavate.Guavate;
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.collect.ImmutableSortedSet;
+
 import io.swagger.annotations.Api;
 import io.swagger.annotations.ApiImplicitParam;
 import io.swagger.annotations.ApiImplicitParams;
@@ -149,22 +150,30 @@ public class GroupsRoutes implements Routes {
                 MAILADDRESS_ASCII_DISCLAIMER)
     })
     @ApiResponses(value = {
-        @ApiResponse(code = HttpStatus.OK_200, message = "OK", response = List.class),
+        @ApiResponse(code = HttpStatus.NO_CONTENT_204, message = "OK", response = List.class),
         @ApiResponse(code = HttpStatus.BAD_REQUEST_400, message = GROUP_ADDRESS + " or group
structure format is not valid"),
         @ApiResponse(code = HttpStatus.FORBIDDEN_403, message = "server doesn't own the domain"),
         @ApiResponse(code = HttpStatus.CONFLICT_409, message = "requested group address is
already used for another purpose"),
         @ApiResponse(code = HttpStatus.INTERNAL_SERVER_ERROR_500,
             message = "Internal server error - Something went bad on the server side.")
     })
-    public HaltException addToGroup(Request request, Response response) throws JsonExtractException,
AddressException, RecipientRewriteTableException, UsersRepositoryException, DomainListException
{
+    public HaltException addToGroup(Request request, Response response) throws RecipientRewriteTableException,
UsersRepositoryException, DomainListException {
         MailAddress groupAddress = parseMailAddress(request.params(GROUP_ADDRESS));
         Domain domain = groupAddress.getDomain();
         ensureRegisteredDomain(domain);
         ensureNotShadowingAnotherAddress(groupAddress);
         MailAddress userAddress = parseMailAddress(request.params(USER_ADDRESS));
         MappingSource source = MappingSource.fromUser(User.fromLocalPartWithDomain(groupAddress.getLocalPart(),
domain));
-        recipientRewriteTable.addGroupMapping(source, userAddress.asString());
-        return halt(HttpStatus.CREATED_201);
+        addGroupMember(source, userAddress);
+        return halt(HttpStatus.NO_CONTENT_204);
+    }
+
+    private void addGroupMember(MappingSource source, MailAddress userAddress) throws RecipientRewriteTableException
{
+        try {
+            recipientRewriteTable.addGroupMapping(source, userAddress.asString());
+        } catch (MappingAlreadyExistsException e) {
+            // do nothing
+        }
     }
 
     private void ensureRegisteredDomain(Domain domain) throws DomainListException {
@@ -202,14 +211,14 @@ public class GroupsRoutes implements Routes {
         @ApiResponse(code = HttpStatus.INTERNAL_SERVER_ERROR_500,
             message = "Internal server error - Something went bad on the server side.")
     })
-    public HaltException removeFromGroup(Request request, Response response) throws JsonExtractException,
AddressException, RecipientRewriteTableException {
+    public HaltException removeFromGroup(Request request, Response response) throws RecipientRewriteTableException
{
         MailAddress groupAddress = parseMailAddress(request.params(GROUP_ADDRESS));
         MailAddress userAddress = parseMailAddress(request.params(USER_ADDRESS));
         MappingSource source = MappingSource
             .fromUser(
                 User.fromLocalPartWithDomain(groupAddress.getLocalPart(), groupAddress.getDomain()));
         recipientRewriteTable.removeGroupMapping(source, userAddress.asString());
-        return halt(HttpStatus.OK_200);
+        return halt(HttpStatus.NO_CONTENT_204);
     }
 
     @GET

http://git-wip-us.apache.org/repos/asf/james-project/blob/5d70b940/server/protocols/webadmin/webadmin-data/src/test/java/org/apache/james/webadmin/routes/DomainMappingsRoutesTest.java
----------------------------------------------------------------------
diff --git a/server/protocols/webadmin/webadmin-data/src/test/java/org/apache/james/webadmin/routes/DomainMappingsRoutesTest.java
b/server/protocols/webadmin/webadmin-data/src/test/java/org/apache/james/webadmin/routes/DomainMappingsRoutesTest.java
index b41c1ba..0d36762 100644
--- a/server/protocols/webadmin/webadmin-data/src/test/java/org/apache/james/webadmin/routes/DomainMappingsRoutesTest.java
+++ b/server/protocols/webadmin/webadmin-data/src/test/java/org/apache/james/webadmin/routes/DomainMappingsRoutesTest.java
@@ -23,6 +23,7 @@ import static com.jayway.restassured.RestAssured.delete;
 import static com.jayway.restassured.RestAssured.given;
 import static com.jayway.restassured.RestAssured.put;
 import static com.jayway.restassured.RestAssured.when;
+import static com.jayway.restassured.RestAssured.with;
 import static org.apache.james.webadmin.WebAdminServer.NO_CONFIGURATION;
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.entry;
@@ -106,6 +107,21 @@ class DomainMappingsRoutesTest {
         }
 
         @Test
+        void addDomainMappingShouldBeIdempotent() {
+            with()
+                .body("to.com")
+                .put("from.com");
+
+            given()
+                .body("to.com")
+            .when()
+                .put("from.com")
+            .then()
+                .statusCode(HttpStatus.NO_CONTENT_204)
+                .body(isEmptyString());
+        }
+
+        @Test
         void getDomainMappingsShouldReturnAllDomainMappings() throws RecipientRewriteTableException
{
             String alias1 = "to_1.com";
             String alias2 = "to_2.com";

http://git-wip-us.apache.org/repos/asf/james-project/blob/5d70b940/server/protocols/webadmin/webadmin-data/src/test/java/org/apache/james/webadmin/routes/ForwardRoutesTest.java
----------------------------------------------------------------------
diff --git a/server/protocols/webadmin/webadmin-data/src/test/java/org/apache/james/webadmin/routes/ForwardRoutesTest.java
b/server/protocols/webadmin/webadmin-data/src/test/java/org/apache/james/webadmin/routes/ForwardRoutesTest.java
index 27fc7ab..89d9ded 100644
--- a/server/protocols/webadmin/webadmin-data/src/test/java/org/apache/james/webadmin/routes/ForwardRoutesTest.java
+++ b/server/protocols/webadmin/webadmin-data/src/test/java/org/apache/james/webadmin/routes/ForwardRoutesTest.java
@@ -19,6 +19,7 @@
 
 package org.apache.james.webadmin.routes;
 
+import static com.jayway.restassured.RestAssured.given;
 import static com.jayway.restassured.RestAssured.when;
 import static com.jayway.restassured.RestAssured.with;
 import static org.apache.james.webadmin.Constants.SEPARATOR;
@@ -173,19 +174,30 @@ class ForwardRoutesTest {
         }
 
         @Test
-        void putUserInForwardShouldReturnCreated() {
+        void putUserInForwardShouldReturnNoContent() {
             when()
                 .put(ALICE + SEPARATOR + "targets" + SEPARATOR + BOB)
             .then()
-                .statusCode(HttpStatus.CREATED_201);
+                .statusCode(HttpStatus.NO_CONTENT_204);
         }
 
         @Test
-        void putUserWithSlashInForwardShouldReturnCreated() {
+        void putUserShouldBeIdempotent() {
+            given()
+                .put(ALICE + SEPARATOR + "targets" + SEPARATOR + BOB);
+
+            when()
+                .put(ALICE + SEPARATOR + "targets" + SEPARATOR + BOB)
+            .then()
+                .statusCode(HttpStatus.NO_CONTENT_204);
+        }
+
+        @Test
+        void putUserWithSlashInForwardShouldReturnNoContent() {
             when()
                 .put(BOB + SEPARATOR + "targets" + SEPARATOR + ALICE_WITH_ENCODED_SLASH)
             .then()
-                .statusCode(HttpStatus.CREATED_201);
+                .statusCode(HttpStatus.NO_CONTENT_204);
         }
 
         @Test
@@ -215,11 +227,11 @@ class ForwardRoutesTest {
         }
 
         @Test
-        void putUserInForwardWithEncodedSlashShouldReturnCreated() {
+        void putUserInForwardWithEncodedSlashShouldReturnNoContent() {
             when()
                 .put(ALICE_WITH_ENCODED_SLASH + SEPARATOR + "targets" + SEPARATOR + BOB)
             .then()
-                .statusCode(HttpStatus.CREATED_201);
+                .statusCode(HttpStatus.NO_CONTENT_204);
         }
 
         @Test
@@ -337,7 +349,7 @@ class ForwardRoutesTest {
             when()
                 .delete(ALICE + SEPARATOR + "targets" + SEPARATOR + BOB)
             .then()
-                .statusCode(HttpStatus.OK_200);
+                .statusCode(HttpStatus.NO_CONTENT_204);
         }
 
         @Test

http://git-wip-us.apache.org/repos/asf/james-project/blob/5d70b940/server/protocols/webadmin/webadmin-data/src/test/java/org/apache/james/webadmin/routes/GroupsRoutesTest.java
----------------------------------------------------------------------
diff --git a/server/protocols/webadmin/webadmin-data/src/test/java/org/apache/james/webadmin/routes/GroupsRoutesTest.java
b/server/protocols/webadmin/webadmin-data/src/test/java/org/apache/james/webadmin/routes/GroupsRoutesTest.java
index 61fb855..5da0d8c 100644
--- a/server/protocols/webadmin/webadmin-data/src/test/java/org/apache/james/webadmin/routes/GroupsRoutesTest.java
+++ b/server/protocols/webadmin/webadmin-data/src/test/java/org/apache/james/webadmin/routes/GroupsRoutesTest.java
@@ -21,6 +21,7 @@ package org.apache.james.webadmin.routes;
 
 import static com.jayway.restassured.RestAssured.given;
 import static com.jayway.restassured.RestAssured.when;
+import static com.jayway.restassured.RestAssured.with;
 import static org.apache.james.webadmin.Constants.SEPARATOR;
 import static org.apache.james.webadmin.WebAdminServer.NO_CONFIGURATION;
 import static org.assertj.core.api.Assertions.assertThat;
@@ -141,6 +142,17 @@ class GroupsRoutesTest {
         }
 
         @Test
+        void putShouldBeIdempotent() {
+            with()
+                .put(GROUP1 + SEPARATOR + USER_A);
+
+            given()
+                .put(GROUP1 + SEPARATOR + USER_A)
+            .then()
+                .statusCode(HttpStatus.NO_CONTENT_204);
+        }
+
+        @Test
         void getNotRegisteredGroupShouldReturnNotFound() {
             Map<String, Object> errors = when()
                 .get("unknown@domain.travel")
@@ -159,19 +171,19 @@ class GroupsRoutesTest {
         }
 
         @Test
-        void putUserInGroupShouldReturnCreated() {
+        void putUserInGroupShouldReturnNoContent() {
             when()
                 .put(GROUP1 + SEPARATOR + USER_A)
             .then()
-                .statusCode(HttpStatus.CREATED_201);
+                .statusCode(HttpStatus.NO_CONTENT_204);
         }
 
         @Test
-        void putUserWithSlashInGroupShouldReturnCreated() {
+        void putUserWithSlashInGroupShouldReturnNoContent() {
             when()
                 .put(GROUP1 + SEPARATOR + USER_WITH_ENCODED_SLASH)
             .then()
-                .statusCode(HttpStatus.CREATED_201);
+                .statusCode(HttpStatus.NO_CONTENT_204);
         }
 
         @Test
@@ -211,11 +223,11 @@ class GroupsRoutesTest {
         }
 
         @Test
-        void putUserInGroupWithEncodedSlashShouldReturnCreated() {
+        void putUserInGroupWithEncodedSlashShouldReturnNoContent() {
             when()
                 .put(GROUP_WITH_ENCODED_SLASH + SEPARATOR + USER_A)
             .then()
-                .statusCode(HttpStatus.CREATED_201);
+                .statusCode(HttpStatus.NO_CONTENT_204);
         }
 
         @Test
@@ -344,7 +356,7 @@ class GroupsRoutesTest {
             when()
                 .delete(GROUP1 + SEPARATOR + USER_A)
             .then()
-                .statusCode(HttpStatus.OK_200);
+                .statusCode(HttpStatus.NO_CONTENT_204);
         }
 
         @Test

http://git-wip-us.apache.org/repos/asf/james-project/blob/5d70b940/src/site/markdown/server/manage-webadmin.md
----------------------------------------------------------------------
diff --git a/src/site/markdown/server/manage-webadmin.md b/src/site/markdown/server/manage-webadmin.md
index a161a97..8600ae5 100644
--- a/src/site/markdown/server/manage-webadmin.md
+++ b/src/site/markdown/server/manage-webadmin.md
@@ -1100,7 +1100,7 @@ Will add member@domain.com to group@domain.com, creating the group if
needed
 
 Response codes:
 
- - 200: Success
+ - 204: Success
  - 400: Group structure or member is not valid
  - 403: Server does not own the requested domain
  - 409: Requested group address is already used for another purpose
@@ -1115,7 +1115,7 @@ Will remove member@domain.com from group@domain.com, removing the group
if group
 
 Response codes:
 
- - 200: Success
+ - 204: Success
  - 400: Group structure or member is not valid
 
 ## Creating address forwards
@@ -1187,7 +1187,7 @@ Will add destination@domain.com to user@domain.com, creating the forward
if need
 
 Response codes:
 
- - 200: Success
+ - 204: Success
  - 400: Forward structure or member is not valid
  - 403: Server does not own the requested domain
  - 404: Requested forward address does not match an existing user
@@ -1202,7 +1202,7 @@ Will remove destination@domain.com from user@domain.com, removing the
forward if
 
 Response codes:
 
- - 200: Success
+ - 204: Success
  - 400: Forward structure or member is not valid
 
 ## Administrating mail repositories


---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org


Mime
View raw message