chibenwa commented on code in PR #2386:
URL: https://github.com/apache/james-project/pull/2386#discussion_r1725676546
##########
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:
Maybe because this is not necessarily the right place for that log.
Few things first with this location we miss some answers:
```
17:14:05.272 [TRACE] o.a.j.i.m.r.UnpooledStatusResponseFactory - Created
IMAP response: MAILBOXID OK Ok
17:14:05.272 [TRACE] o.a.j.i.m.r.UnpooledStatusResponseFactory - Created
IMAP response: UIDVALIDITY OK UIDs valid
17:14:05.272 [TRACE] o.a.j.i.m.r.UnpooledStatusResponseFactory - Created
IMAP response: PERMANENTFLAGS OK Limited
17:14:05.272 [TRACE] o.a.j.i.m.r.UnpooledStatusResponseFactory - Created
IMAP response: HIGHESTMODSEQ OK Highest
17:14:05.272 [TRACE] o.a.j.i.m.r.UnpooledStatusResponseFactory - Created
IMAP response: UIDNEXT OK Predicted next UID
17:14:05.272 [TRACE] o.a.j.i.m.r.UnpooledStatusResponseFactory - Created
IMAP response: READ-WRITE OK b2 completed. SELECT
```
VS
```
17:14:05.272 [DEBUG] o.a.j.m.p.ProtocolSession - S: * OK [MAILBOXID (4)] Ok
17:14:05.272 [DEBUG] o.a.j.m.p.ProtocolSession - S: * FLAGS (\Answered
\Deleted \Draft \Flagged \Seen)
17:14:05.272 [DEBUG] o.a.j.m.p.ProtocolSession - S: * 0 EXISTS
17:14:05.272 [DEBUG] o.a.j.m.p.ProtocolSession - S: * 0 RECENT
17:14:05.272 [DEBUG] o.a.j.m.p.ProtocolSession - S: * OK [UIDVALIDITY
277573197] UIDs valid
17:14:05.273 [DEBUG] o.a.j.m.p.ProtocolSession - S: * OK [PERMANENTFLAGS
(\Answered \Deleted \Draft \Flagged \Seen \*)] Limited
17:14:05.273 [DEBUG] o.a.j.m.p.ProtocolSession - S: * OK [HIGHESTMODSEQ 0]
Highest
17:14:05.273 [DEBUG] o.a.j.m.p.ProtocolSession - S: * OK [UIDNEXT 1]
Predicted next UID
17:14:05.273 [DEBUG] o.a.j.m.p.ProtocolSession - S: b2 OK [READ-WRITE]
SELECT completed.
```
(it only includes responses with a status)
In practice putting the log in `ChannelImapResponseWriter::write(byte[])`
should yield very good results:
- You are guarantied to get the exact output sent
- You would get all responses but avoid expensive litterals - litterals
sending could be traced too btw but maybe without content
- We could pass the IMAP session as a parameter easily to that class and
access the username for structured logging
Though this hooks onto the TCP transport layer provided by
server/protocols/protocols-imap4 and won't be seen with MPT that exercise only
the underlying logic - not an issue as MPT itself allow to see/assert the
exchanges.
That is definitly the way I would undertook.
--
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]
