You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@james.apache.org by "chibenwa (via GitHub)" <gi...@apache.org> on 2023/03/23 06:41:24 UTC
[GitHub] [james-project] chibenwa commented on a diff in pull request #1501: [wip] JAMES-4865 Add FUTURERELEASEParameters
chibenwa commented on code in PR #1501:
URL: https://github.com/apache/james-project/pull/1501#discussion_r1145738093
##########
mailet/api/src/main/java/org/apache/mailet/FUTURERELEASEParameters.java:
##########
@@ -0,0 +1,126 @@
+/****************************************************************
+ * 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.mailet;
+
+import java.util.Map;
+import java.util.Objects;
+import java.util.Optional;
+
+import com.google.common.base.MoreObjects;
+import com.google.common.base.Preconditions;
+
+public class FUTURERELEASEParameters {
Review Comment:
CAN WE DISABLE CAPS LOCK AndUseSnakeCase as it is more readable?
##########
mailet/api/src/main/java/org/apache/mailet/DsnParameters.java:
##########
@@ -53,7 +53,8 @@ public class DsnParameters {
public static final String ORCPT_PARAMETER = "ORCPT";
public static final String ENVID_PARAMETER = "ENVID";
public static final String RET_PARAMETER = "RET";
-
+ public static final String HOLDFOR_PARAMETER = "HOLDFOR";
+ public static final String HOLDUNITL_PARAMETER = "HOLDUNITL";
Review Comment:
Future release parameters are not DSN parameters. Please move them to a dedicated file.
##########
mailet/api/src/main/java/org/apache/mailet/DsnParameters.java:
##########
@@ -239,6 +240,8 @@ public String toString() {
}
}
+
+
Review Comment:
Please remove these unrelated changes.
##########
mailet/api/src/main/java/org/apache/mailet/FUTURERELEASEParameters.java:
##########
@@ -0,0 +1,126 @@
+/****************************************************************
+ * 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.mailet;
+
+import java.util.Map;
+import java.util.Objects;
+import java.util.Optional;
+
+import com.google.common.base.MoreObjects;
+import com.google.common.base.Preconditions;
+
+public class FUTURERELEASEParameters {
+ public static final String HOLDFOR_PARAMETER = "HOLDFOR";
+ public static final String HOLDUNITL_PARAMETER = "HOLDUNITL";
+
+ public static class Holdfor{
Review Comment:
Please use equalsverifier to test the behaviour of this POJO...
##########
server/protocols/protocols-smtp/src/main/java/org/apache/james/smtpserver/futurerelease/FutureReleaseEHLOHook.java:
##########
@@ -0,0 +1,21 @@
+package org.apache.james.smtpserver.futurerelease;
+
+import org.apache.james.protocols.smtp.SMTPSession;
+import org.apache.james.protocols.smtp.hook.HeloHook;
+import org.apache.james.protocols.smtp.hook.HookResult;
+
+import java.util.Set;
+
+import com.google.common.collect.ImmutableSet;
+
+public class FutureReleaseEHLOHook implements HeloHook {
+
+ @Override
+ public Set<String> implementedEsmtpFeatures() {
+ return ImmutableSet.of("FUTURERELEASE");
+ }
+
+ @Override
+ public HookResult doHelo(SMTPSession session, String helo) {return HookResult.DECLINED;}
Review Comment:
```suggestion
public HookResult doHelo(SMTPSession session, String helo) {
return HookResult.DECLINED;
}
```
##########
server/protocols/protocols-smtp/src/main/java/org/apache/james/smtpserver/futurerelease/FutureReleaseMailParameterHook.java:
##########
@@ -0,0 +1,61 @@
+/****************************************************************
+ * 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.smtpserver.futurerelease;
+
+import static org.apache.mailet.DsnParameters.HOLDFOR_PARAMETER;
+import static org.apache.mailet.DsnParameters.HOLDUNITL_PARAMETER;
+
+import org.apache.james.protocols.api.ProtocolSession;
+import static org.apache.james.protocols.api.ProtocolSession.State.Transaction;
+
+
+import org.apache.james.protocols.smtp.SMTPSession;
+import org.apache.james.protocols.smtp.hook.HookResult;
+import org.apache.james.protocols.smtp.hook.MailParametersHook;
+import org.apache.mailet.DsnParameters;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class FutureReleaseMailParameterHook implements MailParametersHook {
+
+ private static final Logger logger = LoggerFactory.getLogger(FutureReleaseMailParameterHook.class);
+
+ public static final ProtocolSession.AttachmentKey<DsnParameters.Holdfor> FUTURERELEASE_HOLDFOR = ProtocolSession.AttachmentKey.of("FUTURERELEASE_HOLDFOR", DsnParameters.Holdfor.class);
+ public static final ProtocolSession.AttachmentKey<DsnParameters.Holduntil> FUTURERELEASE_HOLDUNTIL = ProtocolSession.AttachmentKey.of("FUTURERELEASE_HOLDUNTIL", DsnParameters.Holduntil.class);
+ @Override
+ public HookResult doMailParameter(SMTPSession session, String paramName, String paramValue) {
+ if(paramName.equals(HOLDFOR_PARAMETER)){
+ logger.debug("HoldFor parameter is set to {}", paramValue);
+ Integer value = Integer.parseInt(paramValue);
+ DsnParameters.Holdfor holdfor = DsnParameters.Holdfor.of(value);
+ session.setAttachment(FUTURERELEASE_HOLDFOR, holdfor, Transaction);
+ }else if(paramName.equals(HOLDUNITL_PARAMETER)){
+ logger.debug("HoldUntil parameter is set to {}", paramValue);
+ DsnParameters.Holduntil holduntil = DsnParameters.Holduntil.of(paramValue);
+ session.setAttachment(FUTURERELEASE_HOLDUNTIL, holduntil, Transaction);
+ }
+ return null;
Review Comment:
IMO we should convert Hold-Until into Hold-For in order to get a signle parameter
##########
server/protocols/protocols-smtp/src/test/java/org/apache/james/smtpserver/FutureReleaseEHLOHookTest.java:
##########
@@ -0,0 +1,17 @@
+package org.apache.james.smtpserver;
Review Comment:
Missing license
##########
.gitignore:
##########
@@ -6,4 +6,19 @@ dockerfiles/run/**/keystore
dockerfiles/run/**/glowroot
.m2
test-run.log
-build
\ No newline at end of file
+build
+*.iml
+.idea
+*.lib
+*.jar
+.floo
+.flooignore
+.m2
+_site/
+keystore
+derby.log
+src/homepage/Gemfile.lock
+mailbox/var/
+src/homepage/.sass-cache/
+KahaDB/
+.attach_pid*
Review Comment:
Please use git rebase to move changes of `.gitignore` in a distinct commit.
##########
mailet/api/src/main/java/org/apache/mailet/FUTURERELEASEParameters.java:
##########
@@ -0,0 +1,126 @@
+/****************************************************************
+ * 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.mailet;
+
+import java.util.Map;
+import java.util.Objects;
+import java.util.Optional;
+
+import com.google.common.base.MoreObjects;
+import com.google.common.base.Preconditions;
+
+public class FUTURERELEASEParameters {
+ public static final String HOLDFOR_PARAMETER = "HOLDFOR";
+ public static final String HOLDUNITL_PARAMETER = "HOLDUNITL";
+
+ public static class Holdfor{
Review Comment:
```suggestion
public static class Holdfor {
```
checkstyle
##########
mailet/api/src/main/java/org/apache/mailet/Mail.java:
##########
@@ -451,4 +451,6 @@ default void setDsnParameters(DsnParameters dsnParameters) {
.asAttributes()
.forEach(this::setAttribute);
}
+
+
Review Comment:
Unrelated change
##########
server/protocols/protocols-smtp/src/main/java/org/apache/james/smtpserver/futurerelease/FutureReleaseMailParameterHook.java:
##########
@@ -0,0 +1,61 @@
+/****************************************************************
+ * 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.smtpserver.futurerelease;
+
+import static org.apache.mailet.DsnParameters.HOLDFOR_PARAMETER;
+import static org.apache.mailet.DsnParameters.HOLDUNITL_PARAMETER;
+
+import org.apache.james.protocols.api.ProtocolSession;
+import static org.apache.james.protocols.api.ProtocolSession.State.Transaction;
+
+
+import org.apache.james.protocols.smtp.SMTPSession;
+import org.apache.james.protocols.smtp.hook.HookResult;
+import org.apache.james.protocols.smtp.hook.MailParametersHook;
+import org.apache.mailet.DsnParameters;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class FutureReleaseMailParameterHook implements MailParametersHook {
+
+ private static final Logger logger = LoggerFactory.getLogger(FutureReleaseMailParameterHook.class);
+
+ public static final ProtocolSession.AttachmentKey<DsnParameters.Holdfor> FUTURERELEASE_HOLDFOR = ProtocolSession.AttachmentKey.of("FUTURERELEASE_HOLDFOR", DsnParameters.Holdfor.class);
+ public static final ProtocolSession.AttachmentKey<DsnParameters.Holduntil> FUTURERELEASE_HOLDUNTIL = ProtocolSession.AttachmentKey.of("FUTURERELEASE_HOLDUNTIL", DsnParameters.Holduntil.class);
+ @Override
+ public HookResult doMailParameter(SMTPSession session, String paramName, String paramValue) {
Review Comment:
```suggestion
public static final ProtocolSession.AttachmentKey<DsnParameters.Holduntil> FUTURERELEASE_HOLDUNTIL = ProtocolSession.AttachmentKey.of("FUTURERELEASE_HOLDUNTIL", DsnParameters.Holduntil.class);
@Override
public HookResult doMailParameter(SMTPSession session, String paramName, String paramValue) {
```
##########
mailet/api/src/main/java/org/apache/mailet/FUTURERELEASEParameters.java:
##########
@@ -0,0 +1,126 @@
+/****************************************************************
+ * 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.mailet;
+
+import java.util.Map;
+import java.util.Objects;
+import java.util.Optional;
+
+import com.google.common.base.MoreObjects;
+import com.google.common.base.Preconditions;
+
+public class FUTURERELEASEParameters {
+ public static final String HOLDFOR_PARAMETER = "HOLDFOR";
+ public static final String HOLDUNITL_PARAMETER = "HOLDUNITL";
+
+ public static class Holdfor{
+ public static Optional<Holdfor> fromSMTPArgLine(Map<String, Integer> mailFromArgLine) {
+ return Optional.ofNullable(mailFromArgLine.get(HOLDFOR_PARAMETER))
+ .map(Holdfor::of);
+ }
+
+ public static Holdfor fromAttributeValue(AttributeValue<Integer> attributeValue) {
+ return of(attributeValue.value());
+ }
+
+ private final Integer value;
+
+ public static Holdfor of (Integer value) {
+ Preconditions.checkNotNull(value);
+// Preconditions.checkArgument(XText.isValid(value), "According to RFC-4865 Holdfor should be a valid xtext" +
+// ", thus composed of CHARs between \"!\" (33) and \"~\" (126) inclusive, except for \"+\" and \"=\" or follow the hexadecimal escape sequence.");
+
+ return new Holdfor(value);
+ }
+
+ private Holdfor(Integer value) {
+ this.value = value;
+ }
+
+ public Integer asString() {
+ return value;
+ }
+
+ public AttributeValue<Integer> toAttributeValue() {
+ return AttributeValue.of(value);
+ }
+
+ @Override
+ public final boolean equals(Object o) {
+ if (o instanceof Holdfor) {
+ Holdfor that = (Holdfor) o;
+
+ return Objects.equals(this.value, that.value);
+ }
+ return false;
+ }
+
+ @Override
+ public String toString() {
+ return MoreObjects.toStringHelper(this)
+ .add("value", value)
+ .toString();
+ }
+ }
+
+ public static class Holduntil{
Review Comment:
```suggestion
public static class Holduntil {
```
+ test it with equals verifier
##########
server/apps/distributed-app/docs/modules/ROOT/pages/extending/smtp-hooks.adoc:
##########
@@ -1,7 +1,7 @@
= Distributed James Server — Custom SMTP hooks
:navtitle: Custom SMTP hooks
-SMTP hooks enable extening capabilities of the SMTP server and are run synchronously upon email reception, before the email is
+SMTP hooks enable extending capabilities of the SMTP server and are run synchronously upon email reception, before the email is
Review Comment:
Please split this typo fixing to its own commit
##########
server/protocols/protocols-smtp/src/test/java/org/apache/james/smtpserver/DSNTest.java:
##########
@@ -228,7 +228,8 @@ void dsnParametersShouldBeSetOnTheFinalEmail() throws Exception {
.addRcptParameter(new MailAddress("rcpt@localhost"), DsnParameters.RecipientDsnParameters.of(
EnumSet.of(SUCCESS, FAILURE, DELAY),
new MailAddress("orcpt@localhost")
- )).build().get());
+ )).build().
+ get());
}
Review Comment:
Unrelated
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
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