You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@james.apache.org by GitBox <gi...@apache.org> on 2020/10/23 06:42:34 UTC
[GitHub] [james-project] chibenwa commented on a change in pull request #256: JAMES-3389, add api to webadmin server for receiving mail over rest i…
chibenwa commented on a change in pull request #256:
URL: https://github.com/apache/james-project/pull/256#discussion_r510659597
##########
File path: server/container/guice/protocols/webadmin-mail-over-web/src/main/java/org/apache/james/modules/server/WebAdminMailOverWebModule.java
##########
@@ -0,0 +1,16 @@
+package org.apache.james.modules.server;
Review comment:
The Apache V2 license header is compulsory.
##########
File path: server/protocols/webadmin/webadmin-mail-over-web/src/main/java/org/apache/james/webadmin/routes/ReceiveMailOverWebRoutes.java
##########
@@ -0,0 +1,86 @@
+package org.apache.james.webadmin.routes;
+
+import javax.inject.Inject;
+import javax.ws.rs.POST;
+import javax.ws.rs.Path;
+import javax.ws.rs.Produces;
+
+import org.apache.james.queue.api.MailQueue;
+import org.apache.james.queue.api.MailQueueFactory;
+import org.apache.james.server.core.MailImpl;
+import org.apache.james.webadmin.Routes;
+import org.apache.james.webadmin.request.MailProps;
+import org.apache.james.webadmin.utils.ErrorResponder;
+import org.apache.james.webadmin.utils.JsonExtractor;
+import org.eclipse.jetty.http.HttpStatus;
+
+import io.swagger.annotations.Api;
+import io.swagger.annotations.ApiImplicitParam;
+import io.swagger.annotations.ApiImplicitParams;
+import io.swagger.annotations.ApiOperation;
+import io.swagger.annotations.ApiResponse;
+import io.swagger.annotations.ApiResponses;
+import spark.Service;
+
+@Api(tags = "OverWebMailReceiver")
+@Path(ReceiveMailOverWebRoutes.BASE_URL)
+@Produces("application/json")
+public class ReceiveMailOverWebRoutes implements Routes {
+
+ private final JsonExtractor<MailProps> mailPropsJsonExtractor;
+
+ public static final String BASE_URL = "/receiveMail";
+
+ private MailQueue queue;
+
+ @Override
+ public String getBasePath() {
+ return BASE_URL;
+ }
+
+ @Inject
+ public ReceiveMailOverWebRoutes(MailQueueFactory<?> queueFactory) {
+ queue = queueFactory.createQueue(MailQueueFactory.SPOOL);
+ this.mailPropsJsonExtractor = new JsonExtractor<>(MailProps.class);
+ }
+
+ @Override
+ public void define(Service service) {
+ defineReceiveMailFromWebService(service);
+ }
+
+ @POST
+ @Path("/receiveMail")
+ @ApiOperation(value = "Deleting an user")
+ @ApiImplicitParams({
+ @ApiImplicitParam(required = true, dataType = "string", name = "username", paramType = "path")
+ })
+ @ApiResponses(value = {
+ @ApiResponse(code = HttpStatus.NO_CONTENT_204, message = "OK. User is removed."),
+ @ApiResponse(code = HttpStatus.BAD_REQUEST_400, message = "Invalid input user."),
+ @ApiResponse(code = HttpStatus.INTERNAL_SERVER_ERROR_500,
+ message = "Internal server error - Something went bad on the server side.")
+ })
+ public void defineReceiveMailFromWebService(Service service) {
+ service.post(BASE_URL, (request, response) -> {
+ try {
+ //Parse the json object to org.apache.james.webadmin.request.MailProps and build the MailImpl object
+ MailImpl mail = mailPropsJsonExtractor.parse(request.body()).asMailImpl();
+ //Send to queue api for mail processing
+ queue.enQueue(mail);
+ response.body("ENQUEUED");
+ response.status(204);
+ } catch (Exception e) {
+ ErrorResponder.builder()
+ .cause(e)
+ .statusCode(500)
+ .type(ErrorResponder.ErrorType.SERVER_ERROR)
+ .message("The mail will not be sent: " + e.getMessage())
+ .haltError();
Review comment:
We have default error handling, this is not required to handle it manually here
##########
File path: server/container/guice/pom.xml
##########
@@ -73,6 +73,7 @@
<module>protocols/webadmin-mailrepository</module>
<module>protocols/webadmin-rabbitmq-mailqueue</module>
<module>protocols/webadmin-swagger</module>
+ <module>protocols/webadmin-mail-over-web</module>
Review comment:
I propose to not add yet-another module but add it to `protocols/webadmin-mailqueue`
##########
File path: server/protocols/webadmin/webadmin-mail-over-web/src/main/java/org/apache/james/webadmin/request/BodyPartProps.java
##########
@@ -0,0 +1,89 @@
+package org.apache.james.webadmin.request;
+
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Optional;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonProperty;
+
+public class BodyPartProps {
+
+ @JsonCreator
+ public BodyPartProps(
+ @JsonProperty("type") String type,
+ @JsonProperty("filename") String filename,
+ @JsonProperty("content") String content,
+ @JsonProperty("encoding") String encoding,
+ @JsonProperty("disposition") String disposition,
+ @JsonProperty("headers") Map<String, String> headers) {
+
+ Optional.ofNullable(type).ifPresent(this::setType);
+ this.filename = filename;
+ Optional.ofNullable(content).ifPresent(this::setContent);
+ this.encoding = encoding;
+ this.disposition = disposition;
+ Optional.ofNullable(headers).ifPresent(this::setHeaders);
+ }
+
+ private String type = "text/plain";
Review comment:
That's a constant.
##########
File path: server/protocols/webadmin/webadmin-mail-over-web/src/main/java/org/apache/james/webadmin/routes/ReceiveMailOverWebRoutes.java
##########
@@ -0,0 +1,86 @@
+package org.apache.james.webadmin.routes;
+
+import javax.inject.Inject;
+import javax.ws.rs.POST;
+import javax.ws.rs.Path;
+import javax.ws.rs.Produces;
+
+import org.apache.james.queue.api.MailQueue;
+import org.apache.james.queue.api.MailQueueFactory;
+import org.apache.james.server.core.MailImpl;
+import org.apache.james.webadmin.Routes;
+import org.apache.james.webadmin.request.MailProps;
+import org.apache.james.webadmin.utils.ErrorResponder;
+import org.apache.james.webadmin.utils.JsonExtractor;
+import org.eclipse.jetty.http.HttpStatus;
+
+import io.swagger.annotations.Api;
+import io.swagger.annotations.ApiImplicitParam;
+import io.swagger.annotations.ApiImplicitParams;
+import io.swagger.annotations.ApiOperation;
+import io.swagger.annotations.ApiResponse;
+import io.swagger.annotations.ApiResponses;
+import spark.Service;
+
+@Api(tags = "OverWebMailReceiver")
+@Path(ReceiveMailOverWebRoutes.BASE_URL)
+@Produces("application/json")
+public class ReceiveMailOverWebRoutes implements Routes {
+
+ private final JsonExtractor<MailProps> mailPropsJsonExtractor;
+
+ public static final String BASE_URL = "/receiveMail";
+
+ private MailQueue queue;
+
+ @Override
+ public String getBasePath() {
+ return BASE_URL;
+ }
+
+ @Inject
+ public ReceiveMailOverWebRoutes(MailQueueFactory<?> queueFactory) {
+ queue = queueFactory.createQueue(MailQueueFactory.SPOOL);
+ this.mailPropsJsonExtractor = new JsonExtractor<>(MailProps.class);
+ }
+
+ @Override
+ public void define(Service service) {
+ defineReceiveMailFromWebService(service);
+ }
+
+ @POST
+ @Path("/receiveMail")
+ @ApiOperation(value = "Deleting an user")
+ @ApiImplicitParams({
+ @ApiImplicitParam(required = true, dataType = "string", name = "username", paramType = "path")
+ })
+ @ApiResponses(value = {
+ @ApiResponse(code = HttpStatus.NO_CONTENT_204, message = "OK. User is removed."),
+ @ApiResponse(code = HttpStatus.BAD_REQUEST_400, message = "Invalid input user."),
+ @ApiResponse(code = HttpStatus.INTERNAL_SERVER_ERROR_500,
+ message = "Internal server error - Something went bad on the server side.")
+ })
+ public void defineReceiveMailFromWebService(Service service) {
+ service.post(BASE_URL, (request, response) -> {
+ try {
+ //Parse the json object to org.apache.james.webadmin.request.MailProps and build the MailImpl object
+ MailImpl mail = mailPropsJsonExtractor.parse(request.body()).asMailImpl();
+ //Send to queue api for mail processing
+ queue.enQueue(mail);
+ response.body("ENQUEUED");
+ response.status(204);
Review comment:
Use 201 + no body content
##########
File path: server/protocols/webadmin/webadmin-mail-over-web/src/main/java/org/apache/james/webadmin/request/BodyPartProps.java
##########
@@ -0,0 +1,89 @@
+package org.apache.james.webadmin.request;
+
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Optional;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonProperty;
+
+public class BodyPartProps {
+
+ @JsonCreator
+ public BodyPartProps(
+ @JsonProperty("type") String type,
+ @JsonProperty("filename") String filename,
+ @JsonProperty("content") String content,
+ @JsonProperty("encoding") String encoding,
+ @JsonProperty("disposition") String disposition,
+ @JsonProperty("headers") Map<String, String> headers) {
+
+ Optional.ofNullable(type).ifPresent(this::setType);
+ this.filename = filename;
+ Optional.ofNullable(content).ifPresent(this::setContent);
+ this.encoding = encoding;
+ this.disposition = disposition;
+ Optional.ofNullable(headers).ifPresent(this::setHeaders);
+ }
+
+ private String type = "text/plain";
+
+ private String filename;
+
+ private String content;
+
+ private String encoding;
+
+ private String disposition;
+
+ private Map<String, String> headers = new HashMap<>();
Review comment:
We tend to use immutable POJOs, using final fields (no setters) and immutable datastructures like ImmutableMap (guava)
##########
File path: server/protocols/webadmin/webadmin-mail-over-web/src/main/java/org/apache/james/webadmin/routes/ReceiveMailOverWebRoutes.java
##########
@@ -0,0 +1,86 @@
+package org.apache.james.webadmin.routes;
+
+import javax.inject.Inject;
+import javax.ws.rs.POST;
+import javax.ws.rs.Path;
+import javax.ws.rs.Produces;
+
+import org.apache.james.queue.api.MailQueue;
+import org.apache.james.queue.api.MailQueueFactory;
+import org.apache.james.server.core.MailImpl;
+import org.apache.james.webadmin.Routes;
+import org.apache.james.webadmin.request.MailProps;
+import org.apache.james.webadmin.utils.ErrorResponder;
+import org.apache.james.webadmin.utils.JsonExtractor;
+import org.eclipse.jetty.http.HttpStatus;
+
+import io.swagger.annotations.Api;
+import io.swagger.annotations.ApiImplicitParam;
+import io.swagger.annotations.ApiImplicitParams;
+import io.swagger.annotations.ApiOperation;
+import io.swagger.annotations.ApiResponse;
+import io.swagger.annotations.ApiResponses;
+import spark.Service;
+
+@Api(tags = "OverWebMailReceiver")
+@Path(ReceiveMailOverWebRoutes.BASE_URL)
+@Produces("application/json")
+public class ReceiveMailOverWebRoutes implements Routes {
+
+ private final JsonExtractor<MailProps> mailPropsJsonExtractor;
+
+ public static final String BASE_URL = "/receiveMail";
Review comment:
We should POST emails to a dedicated mailqueue.
I propose to add the endpoint in MailQueueRoutes :
`curl -XPOST http://127.0.0.1:8000/mailQueues/spool/mails -d 'payload'`
##########
File path: server/protocols/webadmin/webadmin-mail-over-web/src/test/java/org/apache/james/routes/ReceiveMailOverWebRoutesTest.java
##########
@@ -0,0 +1,104 @@
+package org.apache.james.routes;
+
+import static io.restassured.RestAssured.given;
+import static io.restassured.RestAssured.when;
+import static io.restassured.config.EncoderConfig.encoderConfig;
+import static io.restassured.config.RestAssuredConfig.newConfig;
+
+import java.nio.charset.StandardCharsets;
+
+import org.apache.james.queue.api.MailQueueFactory;
+import org.apache.james.queue.api.RawMailQueueItemDecoratorFactory;
+import org.apache.james.queue.memory.MemoryMailQueueFactory;
+import org.apache.james.util.ClassLoaderUtils;
+import org.apache.james.webadmin.WebAdminServer;
+import org.apache.james.webadmin.WebAdminUtils;
+import org.apache.james.webadmin.routes.ReceiveMailOverWebRoutes;
+import org.hamcrest.Matchers;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+import io.restassured.RestAssured;
+import io.restassured.builder.RequestSpecBuilder;
+import io.restassured.http.ContentType;
+import io.restassured.specification.RequestSpecification;
+
+public class ReceiveMailOverWebRoutesTest {
+
+ private WebAdminServer webAdminServer;
+
+ private WebAdminServer createServer() {
+ MemoryMailQueueFactory memoryMailQueueFactory = new MemoryMailQueueFactory(new RawMailQueueItemDecoratorFactory());
+ return WebAdminUtils.createWebAdminServer(new ReceiveMailOverWebRoutes(memoryMailQueueFactory)).start();
+ }
+
+ @BeforeEach
+ void setup() {
+ webAdminServer = createServer();
+ RestAssured.requestSpecification = buildRequestSpecification(webAdminServer);
+ RestAssured.enableLoggingOfRequestAndResponseIfValidationFails();
+ }
+
+ @AfterEach
+ void tearDown() {
+ webAdminServer.destroy();
+ }
+
+ RequestSpecification buildRequestSpecification(WebAdminServer server) {
+ return new RequestSpecBuilder()
+ .setContentType(ContentType.JSON)
+ .setAccept(ContentType.JSON)
+ .setBasePath(ReceiveMailOverWebRoutes.BASE_URL)
+ .setPort(server.getPort().getValue())
+ .setConfig(newConfig().encoderConfig(encoderConfig().defaultContentCharset(StandardCharsets.UTF_8)))
+ .build();
+ }
+
+ @Test
+ public void whenFromIsMissingInRequestThenRequestFails() {
+ given()
+ .body(ClassLoaderUtils.getSystemResourceAsString("json/withoutFrom.json"))
+ .when()
+ .post()
Review comment:
We generally indent given(), .when() and .then() at the same level in order to improve readability.
Here:
```
given()
.body(ClassLoaderUtils.getSystemResourceAsString("json/withoutFrom.json"))
.when()
.post()
.then()
.assertThat()
.statusCode(500)
.body("message", Matchers.containsString("Mail missing from"));
```
##########
File path: server/protocols/webadmin/webadmin-mail-over-web/src/test/java/org/apache/james/request/MailPropsTest.java
##########
@@ -0,0 +1,131 @@
+package org.apache.james.request;
+
+import static org.hamcrest.MatcherAssert.assertThat;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+
+import javax.mail.internet.MimeMessage;
+
+import org.apache.james.core.MailAddress;
+import org.apache.james.server.core.MailImpl;
+import org.apache.james.server.core.MimeMessageCopyOnWriteProxy;
+import org.apache.james.webadmin.request.BodyPartProps;
+import org.apache.james.webadmin.request.MailProps;
+import org.apache.mailet.Matcher;
+import org.hamcrest.Matchers;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+
+public class MailPropsTest {
+
+ @Test
+ public void mailImplemantationHascorrectFieldsWhenBuiltFromMailProps() throws Exception{
+ MailProps mailProps = new MailProps(
+ "test",
+ "valid@address",
+ Arrays.asList("valid1@address"),
+ Arrays.asList("valid2@address"),
+ Arrays.asList("valid3@address"),
+ "test-subject",
+ null,
+ null
Review comment:
I cannot understand this test as there is way too many arguments to this constructor, and most of them have the same type.
We could have a builder?
Or the tested input can be a JSON (the test would then deserialize the JSON first then turn it into a mail?
##########
File path: server/protocols/webadmin/webadmin-mail-over-web/src/test/java/org/apache/james/request/MailPropsTest.java
##########
@@ -0,0 +1,131 @@
+package org.apache.james.request;
+
+import static org.hamcrest.MatcherAssert.assertThat;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+
+import javax.mail.internet.MimeMessage;
+
+import org.apache.james.core.MailAddress;
+import org.apache.james.server.core.MailImpl;
+import org.apache.james.server.core.MimeMessageCopyOnWriteProxy;
+import org.apache.james.webadmin.request.BodyPartProps;
+import org.apache.james.webadmin.request.MailProps;
+import org.apache.mailet.Matcher;
+import org.hamcrest.Matchers;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+
+public class MailPropsTest {
+
+ @Test
+ public void mailImplemantationHascorrectFieldsWhenBuiltFromMailProps() throws Exception{
+ MailProps mailProps = new MailProps(
+ "test",
+ "valid@address",
+ Arrays.asList("valid1@address"),
+ Arrays.asList("valid2@address"),
+ Arrays.asList("valid3@address"),
+ "test-subject",
+ null,
+ null
+ );
+
+ MailImpl mail = mailProps.asMailImpl();
+
+ assertThat(mail.getName(), Matchers.equalTo("test"));
+ assertThat(mail.getSender().asPrettyString(), Matchers.equalTo("<va...@address>"));
+ assertThat(mail.getRecipients(), Matchers.contains(new MailAddress("valid1@address"), new MailAddress("valid2@address"), new MailAddress("valid3@address")));
+ assertThat(mail.getMessage().getHeader("Subject"), Matchers.array(Matchers.equalTo("test-subject")));
+ assertThat(mail.getMessage().getHeader("Content-Type"), Matchers.array(Matchers.equalTo("text/plain; charset=us-ascii")));
Review comment:
When we do several assertions, we wrap them in a SoftAssertions:
```
SoftAssertions.assertThatSoftly(softly -> {
softly.assertThat(...).isEqualTo("test");
softly.assertThat(...).isEqualTo("test-bis");
});
```
Also we prefer relying on assertj fluent syntax instead of hamcrest matchers.
##########
File path: server/protocols/webadmin/webadmin-mail-over-web/src/test/resources/json/mail.json
##########
@@ -0,0 +1,13 @@
+{
Review comment:
Email is a (really) complex thing, and defining a standard JSON document to represent an email is harder than when you propose here. To give you what it could look like you can have a look at the JMAP specification Email object (https://jmap.io/spec-mail.html#emails) and emailSubmission object (https://jmap.io/spec-mail.html#email-submission), as it serve a similar purpose.
Note that this simplistic format might work for you but might be too restrictive for other users.
Hence I am reluctant to add yet another non standard JSON representation of an email.
Also, emails size can be too big to be represented by a JSON object.
Finally you do not distinguish the mail enveloppe from the mail headers in your format.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org