You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@james.apache.org by "Arsnael (via GitHub)" <gi...@apache.org> on 2023/03/23 07:29:23 UTC
[GitHub] [james-project] Arsnael commented on a diff in pull request #1501: [wip] JAMES-4865 Add FUTURERELEASEParameters
Arsnael commented on code in PR #1501:
URL: https://github.com/apache/james-project/pull/1501#discussion_r1145774707
##########
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.");
Review Comment:
Not working at the moment?
##########
server/protocols/protocols-smtp/src/main/java/org/apache/james/smtpserver/futurerelease/FutureReleaseEHLOHook.java:
##########
@@ -0,0 +1,21 @@
+package org.apache.james.smtpserver.futurerelease;
Review Comment:
Apache License missing !
##########
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:
I actually see the same constants declared in `FUTURERELEASEParameters` and I think it's enough (no need to have them duplicated)
Just remove those from `DsnParameters` class :)
##########
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:
@thanhbv200585 don't hesitate to ask the team about checkstyle, I'm not certain you are familiar with it!
##########
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)){
Review Comment:
Careful with the syntaxt of your if {} else {} structures.
As you saw quickly in some comments we have some checkstyle plugin in place to enforce some coding rules to guarantee a minimum of readability and quality in our code (and when those rules are not respected, it breaks).
Give more space in your structures. For example instead of:
```
if(CONDITION){
// if body
}else(CONDITION){
// else body
}
```
prefer giving more space for a better readability :
```
if (CONDITION) {
// if body
} else (CONDITION) {
// else body
}
```
--
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