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 05:43:14 UTC
[GitHub] [james-project] andrevka opened a new pull request #256: JAMES-3389, add api to webadmin server for receiving mail over rest i…
andrevka opened a new pull request #256:
URL: https://github.com/apache/james-project/pull/256
----------------------------------------------------------------
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
[GitHub] [james-project] mbaechler commented on a change in pull request #256: JAMES-3389, add api to webadmin server for receiving mail over rest i…
Posted by GitBox <gi...@apache.org>.
mbaechler commented on a change in pull request #256:
URL: https://github.com/apache/james-project/pull/256#discussion_r512581016
##########
File path: pom.xml
##########
@@ -1860,6 +1860,11 @@
<artifactId>james-server-webadmin-jmap</artifactId>
<version>${project.version}</version>
</dependency>
+ <dependency>
+ <groupId>${james.groupId}</groupId>
+ <artifactId>james-server-webadmin-mail-over-web</artifactId>
Review comment:
I personally like that separated module
##########
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(
Review comment:
this ordering will not pass checkstyle rules. You should run checkstyle and adjust the coding style accordingly.
Rules are mainly Java official guidelines
##########
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")
Review comment:
it looks like you left some things after your copy/paste
##########
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:
I suggest `POST /mail-transfer-service/` (you don't know if it will be delivered locally or not, right? you want it to go into the mailet pipeline I guess)
##########
File path: server/protocols/webadmin/webadmin-mail-over-web/src/main/java/org/apache/james/webadmin/request/MailProps.java
##########
@@ -0,0 +1,189 @@
+package org.apache.james.webadmin.request;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import java.util.UUID;
+import java.util.function.Predicate;
+
+import javax.activation.DataHandler;
+import javax.mail.Header;
+import javax.mail.MessagingException;
+import javax.mail.internet.AddressException;
+import javax.mail.internet.MimeMessage;
+import javax.mail.internet.PreencodedMimeBodyPart;
+import javax.mail.util.ByteArrayDataSource;
+
+import org.apache.james.core.builder.MimeMessageBuilder;
+import org.apache.james.server.core.MailImpl;
+import org.apache.james.server.core.MimeMessageCopyOnWriteProxy;
+
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.github.fge.lambdas.Throwing;
+import com.google.common.annotations.VisibleForTesting;
+
+public class MailProps {
+
+ private String name = UUID.randomUUID().toString();
Review comment:
we tend to set values to attributes only in the constructor for a better readability
##########
File path: server/protocols/webadmin/webadmin-mail-over-web/src/test/resources/json/mail.json
##########
@@ -0,0 +1,13 @@
+{
Review comment:
I agree that defining new formats is almost always a bad idea: it reduces the value of the feature by preventing interoperability with others tools, it will probably have new design flaws (existing formats have know design flow and people are able to deal with them).
So I would rather send the mail as a `message/rfc822` payload.
----------------------------------------------------------------
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
[GitHub] [james-project] andrevka closed pull request #256: JAMES-3389, add api to webadmin server for receiving mail over rest i…
Posted by GitBox <gi...@apache.org>.
andrevka closed pull request #256:
URL: https://github.com/apache/james-project/pull/256
----------------------------------------------------------------
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
[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…
Posted by GitBox <gi...@apache.org>.
chibenwa commented on a change in pull request #256:
URL: https://github.com/apache/james-project/pull/256#discussion_r517250434
##########
File path: server/protocols/webadmin/webadmin-mail-over-web/src/test/java/org/apache/james/routes/ReceiveMailOverWebRoutesTest.java
##########
@@ -0,0 +1,91 @@
+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 {
Review comment:
We only test the response code here.
Maybe testing the content of the mail queue can be valuable too?
##########
File path: server/container/guice/pom.xml
##########
@@ -189,6 +190,46 @@
<artifactId>james-server-guice-webadmin-jmap</artifactId>
<version>${project.version}</version>
</dependency>
+ <dependency>
+ <groupId>${james.groupId}</groupId>
+ <artifactId>james-server-guice-webadmin-mail-over-web</artifactId>
+ <version>${project.version}</version>
+ </dependency>
+ <dependency>
+ <groupId>${james.groupId}</groupId>
+ <artifactId>james-server-guice-webadmin-mail-over-web</artifactId>
+ <version>${project.version}</version>
+ </dependency>
+ <dependency>
+ <groupId>${james.groupId}</groupId>
+ <artifactId>james-server-guice-webadmin-mail-over-web</artifactId>
+ <version>${project.version}</version>
+ </dependency>
+ <dependency>
+ <groupId>${james.groupId}</groupId>
+ <artifactId>james-server-guice-webadmin-mail-over-web</artifactId>
+ <version>${project.version}</version>
+ </dependency>
+ <dependency>
+ <groupId>${james.groupId}</groupId>
+ <artifactId>james-server-guice-webadmin-mail-over-web</artifactId>
+ <version>${project.version}</version>
+ </dependency>
+ <dependency>
+ <groupId>${james.groupId}</groupId>
+ <artifactId>james-server-guice-webadmin-mail-over-web</artifactId>
+ <version>${project.version}</version>
+ </dependency>
+ <dependency>
+ <groupId>${james.groupId}</groupId>
+ <artifactId>james-server-guice-webadmin-mail-over-web</artifactId>
+ <version>${project.version}</version>
+ </dependency>
+ <dependency>
+ <groupId>${james.groupId}</groupId>
+ <artifactId>james-server-guice-webadmin-mail-over-web</artifactId>
+ <version>${project.version}</version>
+ </dependency>
Review comment:
Why adding the dependency 8 times?
##########
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,80 @@
+package org.apache.james.webadmin.routes;
+
+import java.io.ByteArrayInputStream;
+import java.util.UUID;
+
+import javax.inject.Inject;
+import javax.mail.Session;
+import javax.mail.internet.MimeMessage;
+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.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 {
+
+ public static final String BASE_URL = "/mail-transfer-service";
+
+ private MailQueue queue;
+
+ @Override
+ public String getBasePath() {
+ return BASE_URL;
+ }
+
+ @Inject
+ public ReceiveMailOverWebRoutes(MailQueueFactory<?> queueFactory) {
+ queue = queueFactory.createQueue(MailQueueFactory.SPOOL);
+ }
+
+ @Override
+ public void define(Service service) {
+ defineReceiveMailFromWebService(service);
+ }
+
+ @POST
+ @Path("/mail-transfer-service")
+ @ApiOperation(value = "Receiving a message/rfc822 over REST interface")
+ @ApiImplicitParams({
+ @ApiImplicitParam(required = true, dataType = "string", name = "username", paramType = "path")
+ })
+ @ApiResponses(value = {
+ @ApiResponse(code = HttpStatus.CREATED_201, message = ""),
+ @ApiResponse(code = HttpStatus.BAD_REQUEST_400, message = "Could not ceate mail from supplied body"),
+ @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) -> {
+ //parse MimeMessage from request body
+ MimeMessage mimeMessage = new MimeMessage(Session.getDefaultInstance(System.getProperties()), new ByteArrayInputStream(request.bodyAsBytes()));
+ //create MailImpl object from MimeMessage
+ MailImpl mail = MailImpl.fromMimeMessage(UUID.randomUUID().toString(), mimeMessage);
Review comment:
If we are fine about never having a different envelope that the mime headers, then that's fine with me!
##########
File path: server/protocols/webadmin/webadmin-mail-over-web/src/main/java/org/apache/james/webadmin/routes/ReceiveMailOverWebRoutes.java
##########
@@ -0,0 +1,80 @@
+package org.apache.james.webadmin.routes;
+
+import java.io.ByteArrayInputStream;
+import java.util.UUID;
+
+import javax.inject.Inject;
+import javax.mail.Session;
+import javax.mail.internet.MimeMessage;
+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.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)
Review comment:
I prefer a terminology enforcing the process (transfer) rather than on which side of the transfer we are (receive here)
Hence I propose to name this `TransferEmailRoutes`
----------------------------------------------------------------
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
[GitHub] [james-project] chibenwa commented on pull request #256: JAMES-3389, add api to webadmin server for receiving mail over rest i…
Posted by GitBox <gi...@apache.org>.
chibenwa commented on pull request #256:
URL: https://github.com/apache/james-project/pull/256#issuecomment-722771795
Hi @andrevka ,
I just merged this (great!) peace of work. This PR can thus be closed.
Would you find some energy to handle https://github.com/apache/james-project/pull/256#issuecomment-722141233 ?
Cheers,
Benoit
----------------------------------------------------------------
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
[GitHub] [james-project] andrevka commented on a change in pull request #256: JAMES-3389, add api to webadmin server for receiving mail over rest i…
Posted by GitBox <gi...@apache.org>.
andrevka commented on a change in pull request #256:
URL: https://github.com/apache/james-project/pull/256#discussion_r510685206
##########
File path: server/protocols/webadmin/webadmin-mail-over-web/src/test/resources/json/mail.json
##########
@@ -0,0 +1,13 @@
+{
Review comment:
Thanks for the pointers. I'll start with fixing the coding style issues right away, and moving the code to the mailqueue module. After that i will try to implement the request payload as the emailSubmission object.
----------------------------------------------------------------
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
[GitHub] [james-project] chibenwa commented on pull request #256: JAMES-3389, add api to webadmin server for receiving mail over rest i…
Posted by GitBox <gi...@apache.org>.
chibenwa commented on pull request #256:
URL: https://github.com/apache/james-project/pull/256#issuecomment-723007208
>I think i will find the energy
Many thanks!
----------------------------------------------------------------
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
[GitHub] [james-project] mbaechler commented on a change in pull request #256: JAMES-3389, add api to webadmin server for receiving mail over rest i…
Posted by GitBox <gi...@apache.org>.
mbaechler commented on a change in pull request #256:
URL: https://github.com/apache/james-project/pull/256#discussion_r512578027
##########
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:
if you think about this feature as a "put things in queue" you are right. I'm rather in favor of a functional "send a mail" view of it and then talking about mailqueue doesn't make sense.
----------------------------------------------------------------
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
[GitHub] [james-project] chibenwa commented on pull request #256: JAMES-3389, add api to webadmin server for receiving mail over rest i…
Posted by GitBox <gi...@apache.org>.
chibenwa commented on pull request #256:
URL: https://github.com/apache/james-project/pull/256#issuecomment-722069157
> @chibenwa : can you please check on Linagora's CI for this time ?
Was my plan ;-)
----------------------------------------------------------------
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
[GitHub] [james-project] andrevka commented on pull request #256: JAMES-3389, add api to webadmin server for receiving mail over rest i…
Posted by GitBox <gi...@apache.org>.
andrevka commented on pull request #256:
URL: https://github.com/apache/james-project/pull/256#issuecomment-722969520
> Thinking about it, we might want, within the mailet container, to be able to identify emails submitted by this API - in order for instance to allow their relay to remote mail server.
>
> The way we addressed this problem, for instance within the JMAP code base, is to position a mail attribute on the email, which can then be matched on.
>
> See for instance MailSpool::send method && SentByJmap matcher.
>
> I think this work would benefit to adapt a similar approach - this can of course be done as a separate PR.
> Hi @andrevka ,
>
> I just merged this (great!) peace of work. This PR can thus be closed.
>
> Would you find some energy to handle [#256 (comment)](https://github.com/apache/james-project/pull/256#issuecomment-722141233) ?
>
> Cheers,
>
> Benoit
I think i will find the energy
----------------------------------------------------------------
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
[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…
Posted by GitBox <gi...@apache.org>.
chibenwa commented on a change in pull request #256:
URL: https://github.com/apache/james-project/pull/256#discussion_r512818796
##########
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:
Fibe with me
----------------------------------------------------------------
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
[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…
Posted by GitBox <gi...@apache.org>.
chibenwa commented on a change in pull request #256:
URL: https://github.com/apache/james-project/pull/256#discussion_r512597233
##########
File path: pom.xml
##########
@@ -1860,6 +1860,11 @@
<artifactId>james-server-webadmin-jmap</artifactId>
<version>${project.version}</version>
</dependency>
+ <dependency>
+ <groupId>${james.groupId}</groupId>
+ <artifactId>james-server-webadmin-mail-over-web</artifactId>
Review comment:
What are the pros of this separate module?
I see it as closely related to the mail queue, hence I wonder what we gain from separating it.
----------------------------------------------------------------
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
[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…
Posted by GitBox <gi...@apache.org>.
chibenwa commented on a change in pull request #256:
URL: https://github.com/apache/james-project/pull/256#discussion_r510707090
##########
File path: server/protocols/webadmin/webadmin-mail-over-web/src/test/resources/json/mail.json
##########
@@ -0,0 +1,13 @@
+{
Review comment:
> After that i will try to implement the request payload as the emailSubmission object.
Don't. I did share them in order to highlight that this is a complex topic.
I would personnally prefer passing the mail envelope via URL query parameters, and having the request payload being the raw MIME message. (I think having the raw MIME EML submission format is really easy to use). The client system could be as simple as a curl command (good inter-operability, but client system have to generate mime messages itself).
If we want to support another format, JSON based, we could use the Accept header to specify the Mail to be sent, and could be friendlier to modern application (but we won't have a one format fit all use cases without some over-engineering - we should rather start with something simple). The client system would need to be adapted to this format (limited inter-operability).
In short `message/rfc822` => raw EML as a request body, envelope parameters in the URL - and `application/json` a JSON format, if we want to.
I propose you write an API proposition, with examples, in the JIRA ticket before implementing more things.
(You can of course take care of stylish and location issues).
----------------------------------------------------------------
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
[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…
Posted by GitBox <gi...@apache.org>.
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
[GitHub] [james-project] andrevka commented on a change in pull request #256: JAMES-3389, add api to webadmin server for receiving mail over rest i…
Posted by GitBox <gi...@apache.org>.
andrevka commented on a change in pull request #256:
URL: https://github.com/apache/james-project/pull/256#discussion_r517433488
##########
File path: server/protocols/webadmin/webadmin-mail-over-web/src/test/java/org/apache/james/routes/ReceiveMailOverWebRoutesTest.java
##########
@@ -0,0 +1,91 @@
+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 {
Review comment:
added checking mailqueue contents to the tests
----------------------------------------------------------------
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
[GitHub] [james-project] andrevka commented on a change in pull request #256: JAMES-3389, add api to webadmin server for receiving mail over rest i…
Posted by GitBox <gi...@apache.org>.
andrevka commented on a change in pull request #256:
URL: https://github.com/apache/james-project/pull/256#discussion_r517433681
##########
File path: server/protocols/webadmin/webadmin-mail-over-web/src/main/java/org/apache/james/webadmin/routes/ReceiveMailOverWebRoutes.java
##########
@@ -0,0 +1,80 @@
+package org.apache.james.webadmin.routes;
+
+import java.io.ByteArrayInputStream;
+import java.util.UUID;
+
+import javax.inject.Inject;
+import javax.mail.Session;
+import javax.mail.internet.MimeMessage;
+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.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)
Review comment:
Changed
----------------------------------------------------------------
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
[GitHub] [james-project] mbaechler commented on a change in pull request #256: JAMES-3389, add api to webadmin server for receiving mail over rest i…
Posted by GitBox <gi...@apache.org>.
mbaechler commented on a change in pull request #256:
URL: https://github.com/apache/james-project/pull/256#discussion_r512617244
##########
File path: pom.xml
##########
@@ -1860,6 +1860,11 @@
<artifactId>james-server-webadmin-jmap</artifactId>
<version>${project.version}</version>
</dependency>
+ <dependency>
+ <groupId>${james.groupId}</groupId>
+ <artifactId>james-server-webadmin-mail-over-web</artifactId>
Review comment:
From a user perspective, mailqueue is a technical detail, what matters to them/me is sending an email to some people.
So having a component that allow to send emails to people looks good.
Cons:
* more module
Pros:
* easier to understand / find / document
* well separated features
* easier to audit dependencies
----------------------------------------------------------------
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
[GitHub] [james-project] chibenwa commented on pull request #256: JAMES-3389, add api to webadmin server for receiving mail over rest i…
Posted by GitBox <gi...@apache.org>.
chibenwa commented on pull request #256:
URL: https://github.com/apache/james-project/pull/256#issuecomment-722141233
Thinking about it, we might want, within the mailet container, to be able to identify emails submitted by this API - in order for instance to allow their relay to remote mail server.
The way we addressed this problem, for instance within the JMAP code base, is to position a mail attribute on the email, which can then be matched on.
See for instance MailSpool::send method && SentByJmap matcher.
I think this work would benefit to adapt a similar approach - this can of course be done as a separate PR.
----------------------------------------------------------------
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
[GitHub] [james-project] andrevka commented on pull request #256: JAMES-3389, add api to webadmin server for receiving mail over rest i…
Posted by GitBox <gi...@apache.org>.
andrevka commented on pull request #256:
URL: https://github.com/apache/james-project/pull/256#issuecomment-721804207
Made the neccessary changes
----------------------------------------------------------------
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
[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…
Posted by GitBox <gi...@apache.org>.
chibenwa commented on a change in pull request #256:
URL: https://github.com/apache/james-project/pull/256#discussion_r510707090
##########
File path: server/protocols/webadmin/webadmin-mail-over-web/src/test/resources/json/mail.json
##########
@@ -0,0 +1,13 @@
+{
Review comment:
> After that i will try to implement the request payload as the emailSubmission object.
Don't. I points it to show you this is a complex topic.
I would prefer passing the mail envelope via URL query parameters, and having the request payload being the raw MIME message. (I think having the raw MIME EML submission format is really easy to use). The client system could be as simple as a curl command (good inter-operability, but client system have to generate mime messages itself).
If we want to support another format, JSON based, we could use the Accept header to specify the Mail to be sent, and could be friendlier to modern application (but we won't have a one format fit all use cases without some over-engineering - we should rather start with something simple). The client system would need to be adapted to this format (limited inter-operability).
I propose you write an API proposition, with examples, in the JIRA ticket before implementing more things.
(You can of course take care of stylish and location issues).
----------------------------------------------------------------
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