chibenwa commented on code in PR #2386:
URL: https://github.com/apache/james-project/pull/2386#discussion_r1725304398
##########
protocols/imap/src/main/java/org/apache/james/imap/processor/StatusProcessor.java:
##########
@@ -128,6 +128,7 @@ Mono<MailboxStatusResponse> sendStatus(MessageManager
mailbox, StatusDataItems s
if (response.getHighestModSeq() != null) {
condstoreEnablingCommand(session, responder, metaData,
false);
}
+ LOGGER.trace("Status on mailbox named {}, response: {}",
mailbox, response);
Review Comment:
Doesn;t it duplicates the log in UnpooledStatusResponseFactory ?
##########
protocols/imap/src/main/java/org/apache/james/imap/message/response/UnpooledStatusResponseFactory.java:
##########
@@ -26,12 +26,18 @@
import org.apache.james.imap.api.message.response.StatusResponse.ResponseCode;
import org.apache.james.imap.api.message.response.StatusResponse.Type;
import org.apache.james.imap.api.message.response.StatusResponseFactory;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
public class UnpooledStatusResponseFactory extends
AbstractStatusResponseFactory implements StatusResponseFactory {
+ private static final Logger LOGGER =
LoggerFactory.getLogger(UnpooledStatusResponseFactory.class);
+
@Override
protected StatusResponse createResponse(Type type, Tag tag, ImapCommand
command, HumanReadableText displayTextKey, ResponseCode code) {
- return new ImmutableStatusResponse(type, tag, command, displayTextKey,
code);
+ var response = new ImmutableStatusResponse(type, tag, command,
displayTextKey, code);
+ LOGGER.trace("Created IMAP response: {}", response);
Review Comment:
I'm missing a username in this log...
##########
protocols/imap/src/main/java/org/apache/james/imap/decode/main/DefaultImapDecoder.java:
##########
@@ -121,6 +119,7 @@ private ImapMessage
decodeCommandNamed(ImapRequestLineReader request, Tag tag, S
if (count == null || (int) count > 0) {
session.setAttribute(INVALID_COMMAND_COUNT, 0);
}
+ LOGGER.trace("Processing {} {} {}", tag.asString(), commandName,
message.toString());
Review Comment:
Please put the username from the session in there.
##########
protocols/imap/src/main/java/org/apache/james/imap/decode/main/DefaultImapDecoder.java:
##########
@@ -121,6 +119,7 @@ private ImapMessage
decodeCommandNamed(ImapRequestLineReader request, Tag tag, S
if (count == null || (int) count > 0) {
session.setAttribute(INVALID_COMMAND_COUNT, 0);
}
+ LOGGER.trace("Processing {} {} {}", tag.asString(), commandName,
message.toString());
Review Comment:
Red flag: tostring is eagerly instanciated here
Wrap it in a if (LOGGER.isTraceEnabled() ?
##########
protocols/imap/src/main/java/org/apache/james/imap/processor/fetch/FetchProcessor.java:
##########
@@ -201,10 +201,12 @@ private Mono<Void> doFetch(SelectedMailbox selected,
FetchRequest request, Respo
List<MessageRange> ranges = new ArrayList<>();
for (IdRange range : request.getIdSet()) {
- MessageRange messageSet = messageRange(session.getSelected(),
range, request.isUseUids())
- .orElseThrow(() -> new
MessageRangeException(range.getFormattedString() + " is an invalid range"));
- if (messageSet != null) {
- MessageRange normalizedMessageSet =
normalizeMessageRange(selected, messageSet);
+ if (session.getSelected().getLastUid().isEmpty()) {
+ continue;
+ }
Review Comment:
See https://github.com/apache/james-project/pull/2385#issuecomment-2302370425
I am not fan of basing a decision (skip the data) on a projection (the state
of the local in-memory data structure "Selected Mailbox" may be temporarilry
out-of-date especially in a distributed setup)
On the example provided the MSN id set conversion `1:*` bear the meaning
*all messages* and it would make sense to me to convert it into
`MessageRange.all()`. (right now we are translating `1:*` into
`MessageRange.of(firstUid, MessageUid.MAX)`.
This would even make sens regarding performances as downstream code akka the
mailbox is optimized to handle `MessageRange.all()`.
Thoughts?
##########
protocols/imap/src/main/java/org/apache/james/imap/message/response/ImmutableStatusResponse.java:
##########
@@ -72,12 +70,22 @@ public ImapCommand getCommand() {
@Override
public String toString() {
- return MoreObjects.toStringHelper(this)
- .add("responseCode", responseCode)
- .add("serverResponseType", serverResponseType)
- .add("tag", tag)
- .add("textKey", textKey)
- .add("command", command)
- .toString();
+ final StringBuilder sb = new StringBuilder();
+ if (getResponseCode() != null) {
+ sb.append(' ').append(getResponseCode());
+ }
+ if (getServerResponseType() != null) {
+ sb.append(' ').append(getServerResponseType());
+ }
+ if (getTag() != null) {
+ sb.append(' ').append(getTag());
+ }
+ if (getTextKey() != null) {
+ sb.append(' ').append(getTextKey());
+ }
+ if (getCommand() != null) {
+ sb.append(' ').append(getCommand());
+ }
+ return sb.toString();
Review Comment:
Why? You do not like MoreObjects.toStringHelper ?
--
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]
