woj-tek commented on code in PR #2386:
URL: https://github.com/apache/james-project/pull/2386#discussion_r1726639773
##########
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:
I agree it may not be the best, but I'm (still!) not very familiar with
James codebase and navigating it's intricacies I found this place and adding
this log entry (at least temporarily) solved issue I had at hand.
Indeed, I was mildly annoyed that not all info was present but deemed it
"good enough".
---
> In practice putting the log in `ChannelImapResponseWriter::write(byte[])`
should yield very good results:
Thanks for pointing me in the right direction! Indeed, this gives perfect
and complete output!
As for the username - should that be additional constructor (to avoid
breaking API)? Though I wonder shouldn't we just pass complete context here?
Looking at the calls there is a bunch of possibly convenient data there that
would be re-used (e.g.
`ctx.channel().attr(IMAP_SESSION_ATTRIBUTE_KEY).set(imapsession);`)
I pushed my idea about it. It stores whole context there but as it's not
explicitly mandatory in other constructors it's nullable (not liking it, maybe
remove/deprecate other constructors going forward? Use optional for now?)
--
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]
