chibenwa commented on code in PR #3236:
URL: https://github.com/apache/james-project/pull/3236#discussion_r4192276356
##########
protocols/smtp/src/main/java/org/apache/james/protocols/smtp/core/esmtp/EhloCmdHandler.java:
##########
@@ -96,7 +96,7 @@ private Response doEHLO(SMTPSession session, String argument)
{
SMTPResponse resp = new SMTPResponse(SMTPRetCode.MAIL_OK, new
StringBuilder(session.getConfiguration().getHelloName()).append(" Hello
").append(argument)
.append(" [")
-
.append(session.getRemoteAddress().getAddress().getHostAddress()).append("])"));
+
.append(session.getRemoteAddress().getAddress().getHostAddress()).append("]"));
Review Comment:
his isn't an RFC 5321 compliance issue: the text after the domain in an EHLO
reply is free-form. It's a cosmetic fix, and still worth doing.
HeloCmdHandler has the same "])" at
protocols/smtp/src/main/java/org/apache/james/protocols/smtp/core/HeloCmdHandler.java:85.
Please fix it too.
##########
server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/method/EmailSetUpdatePerformer.scala:
##########
@@ -230,30 +230,34 @@ class EmailSetUpdatePerformer @Inject() (serializer:
EmailSetSerializer,
.doOnSuccess(_ => auditMove(Seq(messageId), mailboxIds.value,
targetIds.value, session))
.onErrorResume(e =>
SMono.just[EmailUpdateResult](EmailUpdateFailure(EmailSet.asUnparsed(messageId),
e)))
.switchIfEmpty(SMono.just[EmailUpdateResult](EmailUpdateSuccess(messageId)))
- } else {
Review Comment:
the race described doesn't exist
- The combined case is already handled. When both mailboxes and flags
change, the code already calls the atomic updateEmail (JAMES-3728).
- The else branch only ever does one real operation. It's reached only
when at most one of the two changes. One of updateFlags/updateMailboxIds then
returns EmailUpdateSuccess
without touching the store. There was never a "partial update window"
between two writes.
- The refactor is equivalent but brings no benefit. It is a bit more
explicit, at the cost of copy-pasting the flag computation (the fold over
originalFlags), which now appears
twice.
- Verdict: a cosmetic refactor sold as a race-condition fix. Drop it.
##########
server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/mail/EmailSet.scala:
##########
@@ -412,7 +412,7 @@ case class EmailCreationRequest(mailboxIds: MailboxIds,
private def validateTextBody(blobResolvers: BlobResolvers, mailboxSession:
MailboxSession): SMono[ClientBodyPart] =
textBody match {
case None => SMono.empty
- case Some(text :: Nil) if !text.`type`.value.equals("text/plain") =>
SMono.error(new IllegalArgumentException("Expecting htmlBody type to be
text/html"))
+ case Some(text :: Nil) if !text.`type`.value.equals("text/plain") =>
SMono.error(new IllegalArgumentException("Expecting textBody type to be
text/plain"))
Review Comment:
Ok ;-)
##########
server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/mail/Email.scala:
##########
@@ -76,6 +76,10 @@ object Email {
.map(list => list.reduce(_ ++ _))
}
+ // RFC 8621 sec.4.1: Deterministically pick representative message copy
across multiple mailboxes
Review Comment:
We aggregate flag and the rest of the message is immutable. Please drop this
unneeded commit.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
