You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@james.apache.org by "woj-tek (via GitHub)" <gi...@apache.org> on 2024/08/21 15:17:20 UTC
[PR] JAMES-4060: Fix UID FETCH command failing on empty mailbox [james-project]
woj-tek opened a new pull request, #2386:
URL: https://github.com/apache/james-project/pull/2386
* extending testcase to include `UID FETCH`
* fix the issue by ignoring range retrieval when the inbox is empty
* improvements to debug logs (compacting DefaultImapDecoder logs to single entry; add missing `toString()` and simplify toString in `ImmutableStatusResponse`):
Log afterwards looks nicer/cleaner IMHO (it's not perfect as it's mostly parsed data but gives more information about request-response chatter):
```
17:14:05.268 [DEBUG] o.a.j.m.p.ProtocolSession - C: b1 CREATE box
17:14:05.268 [TRACE] o.a.j.i.d.m.DefaultImapDecoder - Processing b1 CREATE CreateRequest{mailboxName=box}
17:14:05.268 [DEBUG] o.a.j.m.s.StoreMailboxManager - createMailbox #private:imapuser:box
17:14:05.269 [TRACE] o.a.j.i.m.r.UnpooledStatusResponseFactory - Created IMAP response: MAILBOXID OK b1 completed. CREATE
17:14:05.269 [DEBUG] o.a.j.m.p.ProtocolSession - S: b1 OK [MAILBOXID (4)] CREATE completed.
17:14:05.269 [DEBUG] o.a.j.m.p.ProtocolSession - C: b2 SELECT box
17:14:05.270 [TRACE] o.a.j.i.d.m.DefaultImapDecoder - Processing b2 SELECT SelectRequest{mailboxName=box, condstore=false, lastKnownUidValidity=UidValidity{UNKNOWN}, knownModSeq=null, uidSet=null, knownUidSet=null, knownSequenceSet=null}
17:14:05.270 [DEBUG] o.a.j.m.s.StoreMailboxManager - Loaded mailbox #private:imapuser:box
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
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.
17:14:05.273 [DEBUG] o.a.j.m.p.ProtocolSession - C: b3 FETCH 1:* (UID)
17:14:05.273 [TRACE] o.a.j.i.d.m.DefaultImapDecoder - Processing b3 FETCH FetchRequest{useUids=false, idSet=[IdRange ( 1->9223372036854775807 )], fetch=FetchData{items=[UID], setSeen=false, bodyElements=[], changedSince=-1, vanished=false}}
17:14:05.273 [DEBUG] o.a.j.m.s.StoreMailboxManager - Loaded mailbox 4 #private:imapuser:box
17:14:05.273 [TRACE] o.a.j.i.m.r.UnpooledStatusResponseFactory - Created IMAP response: OK b3 completed. FETCH
17:14:05.273 [DEBUG] o.a.j.m.p.ProtocolSession - S: b3 OK FETCH completed.
17:14:05.273 [DEBUG] o.a.j.m.p.ProtocolSession - C: b4 UID FETCH 1:* (UID)
17:14:05.274 [TRACE] o.a.j.i.d.m.DefaultImapDecoder - Processing b4 UID FetchRequest{useUids=true, idSet=[IdRange ( 1->9223372036854775807 )], fetch=FetchData{items=[UID], setSeen=false, bodyElements=[], changedSince=-1, vanished=false}}
17:14:05.274 [DEBUG] o.a.j.m.s.StoreMailboxManager - Loaded mailbox 4 #private:imapuser:box
17:14:05.274 [TRACE] o.a.j.i.m.r.UnpooledStatusResponseFactory - Created IMAP response: OK b4 completed. FETCH
17:14:05.274 [DEBUG] o.a.j.m.p.ProtocolSession - S: b4 OK FETCH completed.
```
--
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
Re: [PR] JAMES-4060: Fix UID FETCH command failing on empty mailbox [james-project]
Posted by "woj-tek (via GitHub)" <gi...@apache.org>.
woj-tek commented on code in PR #2386:
URL: https://github.com/apache/james-project/pull/2386#discussion_r1726559162
##########
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:
Yeah, reactor gets a bit in the way here (though I'm mildly optimistic about loom/virtualthreads :) ) and `MDCStructuredLogger` is a bit "mouthful". Updated the code though.
--
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
Re: [PR] JAMES-4060: Fix UID FETCH command failing on empty mailbox [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on code in PR #2386:
URL: https://github.com/apache/james-project/pull/2386#discussion_r1725661398
##########
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:
> .trace() do the check itself - should we check if the logger is enabled still?
Without explicit calls to 'toString' of course we can aggressively call `.trace()`
> I wonder - shouldn't this be handled by MDC (and would be more general)?
As explained on the mailing list, MDC needs to be explicitly carried other accross threds in reactor wich could be expensive.
In 'classic' log statements not integrated with reactor syntax you can assume the log would not be here.
One other alternative could be to rely explicitly on MDCStructuredLogger:
```
// Avoid cost of initializing MDC locally
if (LOGGER.isTraceEnabled()) {
new MDCStructuredLogger(LOGGER)
.field("username", username)
...
.log(logger -> logger.trace("My trace"));
}
```
A bit boiler plate but this may be better than seriously messing up reactor syntax to cary other the MDC in this case, and cleaner than just adding the username in the message......
--
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
Re: [PR] JAMES-4060: Fix UID FETCH command failing on empty mailbox [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on code in PR #2386:
URL: https://github.com/apache/james-project/pull/2386#discussion_r1726620585
##########
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:
> should we use asString() for custom string output?
+1
> which (in case of logging) may require additional guards for log-levels?
That's true!
In non critical code portions or non costly operations we don't really care but for big string concatenations this might be a concern.
I'm sorry to be peacky, this kind of stuff in IMAP code tend to have a non neglictible impact on CPU (mdc context handling) and memory allocations in what I could see with flame graphs (cf async-profiler) and being careful here do not cost much.
If wrapping things in ifs is for you also not your cup of tea we apparently can use suppliers in the logging event builder (I just found this out!)
```
LOGGER.makeLoggingEventBuilder(Level.DEBUG)
.setMessage("My message {}")
.addArgument(() -> "I'm costly and I know it!")
.log();
```
And under the hood (log4j but other log provider would do the same):
```
logBuilder.log(this.message, this.arguments.toArray());
```
Could be nice to defer expensive argument evaluation, eg for custom displays?
--
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
Re: [PR] JAMES-4060: Fix UID FETCH command failing on empty mailbox [james-project]
Posted by "woj-tek (via GitHub)" <gi...@apache.org>.
woj-tek commented on code in PR #2386:
URL: https://github.com/apache/james-project/pull/2386#discussion_r1725320053
##########
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:
Cleaner look in the logs:
```
17:40:06.015 [TRACE] o.a.j.i.m.r.UnpooledStatusResponseFactory - Created IMAP response: OK a001 completed. LOGIN
17:40:06.016 [TRACE] o.a.j.i.m.r.UnpooledStatusResponseFactory - Created IMAP response (MO) : ImmutableStatusResponse{responseCode=null, serverResponseType=OK, tag=a001, textKey=completed., command=LOGIN}
```
But it's personal taste and can revert those :)
--
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
Re: [PR] JAMES-4060: Fix UID FETCH command failing on empty mailbox [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on code in PR #2386:
URL: https://github.com/apache/james-project/pull/2386#discussion_r1725683721
##########
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:
Sorry for getting chatty it is very interesting technical discussions ^^
Given https://github.com/apache/james-project/pull/2386#discussion_r1725676546 first this change is no longer needed.
Keeping "toString" standard definitly is valuable: apply the IDE template, get the same everywhere.
This do not mean we should blindly apply toString to our log statements. We can apply alternative formats. Here in this case something that kinda duplicates StatusResponseEncoder behaviour.
--
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
Re: [PR] JAMES-4060: Fix UID FETCH command failing on empty mailbox [james-project]
Posted by "woj-tek (via GitHub)" <gi...@apache.org>.
woj-tek commented on code in PR #2386:
URL: https://github.com/apache/james-project/pull/2386#discussion_r1726692863
##########
protocols/imap/src/main/java/org/apache/james/imap/processor/AbstractMailboxProcessor.java:
##########
@@ -444,14 +444,10 @@ protected Optional<MessageRange> messageRange(SelectedMailbox selected, IdRange
private MessageRange msnRangeToMessageRange(SelectedMailbox selected, long lowVal, long highVal)
throws MessageRangeException {
- // Take care of "*" and "*:*" values by return the last message in
+ // Take care of "*", "1:*" and "*:*" values by return the last message in
// the mailbox. See IMAP-289
- if (lowVal == Long.MAX_VALUE && highVal == Long.MAX_VALUE) {
- Optional<MessageUid> last = selected.getLastUid();
- if (!last.isPresent()) {
- throw new MessageRangeException("Mailbox is empty");
- }
- return last.get().toRange();
+ if ((lowVal == Long.MAX_VALUE || lowVal == 1) && highVal == Long.MAX_VALUE) {
+ return MessageRange.all();
}
Review Comment:
I actually added all those variants (i.e. `1:*`, `*:*` and `*`): https://github.com/apache/james-project/pull/2386/commits/8283f2ed9b2f0f2d46d1728600fc8920037f27f3#diff-bccae2dfc450f23b2e358900c6953df48b210c9ca79d08e8b4a9749684ce7c1fR104
but... only to empty mailbox test :D
I'll try extending the tests.
--
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
Re: [PR] JAMES-4060: Fix UID FETCH command failing on empty mailbox [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on code in PR #2386:
URL: https://github.com/apache/james-project/pull/2386#discussion_r1725685987
##########
mpt/impl/imap-mailbox/core/src/main/resources/org/apache/james/imap/scripts/Uid.test:
##########
@@ -32,6 +32,12 @@ S: \* 2 FETCH \(UID 3\)
S: \* 3 FETCH \(UID 4\)
S: a OK FETCH completed.
+C: a2 FETCH 1:* (UID)
Review Comment:
In a file name `UID` I would expect tests about `UID ...` not about Fetch used in MSN mode.
Those tests are valuable, maybe they can be move in an existing `fetch...` script?
--
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
Re: [PR] JAMES-4060: Fix UID FETCH command failing on empty mailbox [james-project]
Posted by "woj-tek (via GitHub)" <gi...@apache.org>.
woj-tek commented on code in PR #2386:
URL: https://github.com/apache/james-project/pull/2386#discussion_r1725322913
##########
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:
Is it even possible to get username here? Again, maybe using MDC would be more convenient somehow?
--
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
Re: [PR] JAMES-4060: Fix UID FETCH command failing on empty mailbox [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on code in PR #2386:
URL: https://github.com/apache/james-project/pull/2386#discussion_r1726646284
##########
server/protocols/protocols-imap4/src/main/java/org/apache/james/imapserver/netty/ChannelImapResponseWriter.java:
##########
@@ -68,6 +83,15 @@ public void setFlushCallback(FlushCallback flushCallback) {
@Override
public void write(byte[] buffer) {
+ if (LOGGER.isTraceEnabled()) {
+ if (ctx != null) {
+ ImapSession imapSession = ctx.channel().attr(IMAP_SESSION_ATTRIBUTE_KEY).get();
+ String username = retrieveUsername(imapSession);
Review Comment:
I'm sorry I am definitly picky here but this means a map read every time we send an IMAP response.
This is likely unecessary are the code that creates ChannelImapResponseWriter already have an ImapSession at hand.
I would add the ImapSession as a field, then pass it as an argument and avoid resolve it here.
(Sorry that's a critical section so I'm picky!)
--
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
Re: [PR] JAMES-4060: Fix UID FETCH command failing on empty mailbox [james-project]
Posted by "woj-tek (via GitHub)" <gi...@apache.org>.
woj-tek commented on PR #2386:
URL: https://github.com/apache/james-project/pull/2386#issuecomment-2304026413
> I did not comment on the PR, but I belive we could split the debug log improvment in a standalone PR (at least for follow up work 1 PR 1 topic is better IMO) + provide a sample "trace" log output...
Should I split this PR into two (logging and fixing UID issue)? Or this was more of a comment for future/followup improvements?
--
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
Re: [PR] JAMES-4060: Fix UID FETCH command failing on empty mailbox [james-project]
Posted by "woj-tek (via GitHub)" <gi...@apache.org>.
woj-tek commented on code in PR #2386:
URL: https://github.com/apache/james-project/pull/2386#discussion_r1726565857
##########
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:
I'm trying to be more open and adjust `toString` to make sense (and I'm quite often weary of just dumping everything). While IDE template is nice one has to remember it either way (same like with more custom one).
As for `StatusResponseEncoder` - should we use `asString()` for custom string output? But in that case it requires explicitly calling such method, which (in case of logging) may require additional guards for log-levels?
Btw. I'm all for technical discussions as I see it as learning experience :)
--
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
Re: [PR] JAMES-4060: Fix UID FETCH command failing on empty mailbox [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on code in PR #2386:
URL: https://github.com/apache/james-project/pull/2386#discussion_r1726620585
##########
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:
> should we use asString() for custom string output?
+1
>
That's true!
In non critical code portions or non costly operations we don't really care but for big string concatenations this might be a concern.
I'm sorry to be peacky, this kind of stuff in IMAP code tend to have a non neglictible impact on CPU (mdc context handling) and memory allocations in what I could see with flame graphs (cf async-profiler) and being careful here do not cost much.
If wrapping things in ifs is for you also not your cup of tea we apparently can use suppliers in the logging event builder (I just found this out!)
```
LOGGER.makeLoggingEventBuilder(Level.DEBUG)
.setMessage("My message {}")
.addArgument(() -> "I'm costly and I know it!")
.log();
```
And under the hood (log4j but other log provider would do the same):
```
logBuilder.log(this.message, this.arguments.toArray());
```
Could be nice to defer expensive argument evaluation, eg for custom displays?
--
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
Re: [PR] JAMES-4060: Fix UID FETCH command failing on empty mailbox [james-project]
Posted by "woj-tek (via GitHub)" <gi...@apache.org>.
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: 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
Re: [PR] JAMES-4060: Fix UID FETCH command failing on empty mailbox [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on code in PR #2386:
URL: https://github.com/apache/james-project/pull/2386#discussion_r1725677990
##########
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:
By logging the exact IMAP chatter output isn't this limitation lifted?
--
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
Re: [PR] JAMES-4060: Fix UID FETCH command failing on empty mailbox [james-project]
Posted by "woj-tek (via GitHub)" <gi...@apache.org>.
woj-tek commented on code in PR #2386:
URL: https://github.com/apache/james-project/pull/2386#discussion_r1726683857
##########
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:
> In non critical code portions or non costly operations we don't really care but for big string concatenations this might be a concern.
Indeed, but this is not-user facing and mostly for debugging so it shouldn't be that visible in production?
But I agree that making it more optimal is always better.
> If wrapping things in ifs is for you also not your cup of tea we apparently can use suppliers in the logging event builder (I just found this out!)
Yeah, slf4j has awesome fluent API since 2.x.
Though in this case we are not using logger but having/discussing actual (`.toString()`) method which this doesn't apply directly. And if/when called we don't really need to use fluent API because object would be passed directly to slf4j and if logging is not enabled then `toString()` would not be called (beauty of slf4j api, even not fluent -- and the optimizations for 1/2/3 arguments instead of using varargs always which yields pesky array allocation under the hood :) ).
Also, JID should be smart enough to optimise those pesky ifs IMHO.
On the other hand, looking at `MoreObjects` - it does have interesing effect of allocating new `ValueHolder` for each added field... and using `StringBuilder` and null-check-with-ifs in it's `toString()`. While allocation is cheap it affects overall GC. And considering `MoreObjects.ToStringHelper#toString` implementation one could argue that using simpler, hand-crafted toString that performs less checks could be more efficient ;)
At any rate - I reverted the `toString` to original flavour and moved custom one to `asString`, though considering removal of logging in `UnpooledStatusResponseFactory` it doesn't matter that much (at least for me, logging in `ChannelImapResponseWriter` is perfect!)
--
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
Re: [PR] JAMES-4060: Fix UID FETCH command failing on empty mailbox [james-project]
Posted by "woj-tek (via GitHub)" <gi...@apache.org>.
woj-tek commented on code in PR #2386:
URL: https://github.com/apache/james-project/pull/2386#discussion_r1726640567
##########
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:
Yeah, removed.
--
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
Re: [PR] JAMES-4060: Fix UID FETCH command failing on empty mailbox [james-project]
Posted by "woj-tek (via GitHub)" <gi...@apache.org>.
woj-tek commented on code in PR #2386:
URL: https://github.com/apache/james-project/pull/2386#discussion_r1726705276
##########
server/protocols/protocols-imap4/src/main/java/org/apache/james/imapserver/netty/ChannelImapResponseWriter.java:
##########
@@ -68,6 +83,15 @@ public void setFlushCallback(FlushCallback flushCallback) {
@Override
public void write(byte[] buffer) {
+ if (LOGGER.isTraceEnabled()) {
+ if (ctx != null) {
+ ImapSession imapSession = ctx.channel().attr(IMAP_SESSION_ATTRIBUTE_KEY).get();
+ String username = retrieveUsername(imapSession);
Review Comment:
> I'm sorry I am definitly picky
Please, do be! :)
> I would add the ImapSession as a field, then pass it as an argument and avoid resolve it here.
Changed the code but I think, to make it better it would be good to deprecate (and make private) other constructors (`org.apache.james.imapserver.netty.ChannelImapResponseWriter#ChannelImapResponseWriter(io.netty.channel.Channel)` and `org.apache.james.imapserver.netty.ChannelImapResponseWriter#ChannelImapResponseWriter(io.netty.channel.Channel, boolean)`)
--
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
Re: [PR] JAMES-4060: Fix UID FETCH command failing on empty mailbox [james-project]
Posted by "woj-tek (via GitHub)" <gi...@apache.org>.
woj-tek commented on code in PR #2386:
URL: https://github.com/apache/james-project/pull/2386#discussion_r1725349088
##########
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:
So basically:
```java
private MessageRange msnRangeToMessageRange(SelectedMailbox selected, long lowVal, long highVal)
throws MessageRangeException {
// Take care of "*", "1:*" and "*:*" values by return the last message in
// the mailbox. See IMAP-289
if ((lowVal == Long.MAX_VALUE || lowVal == 1) && highVal == Long.MAX_VALUE) {
return MessageRange.all();
}
```
?
(ran the MPT tests and it does work but not sure if that was your suggestion)
--
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
Re: [PR] JAMES-4060: Fix UID FETCH command failing on empty mailbox [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
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: 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
Re: [PR] JAMES-4060: Fix UID FETCH command failing on empty mailbox [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on code in PR #2386:
URL: https://github.com/apache/james-project/pull/2386#discussion_r1725686575
##########
protocols/imap/src/main/java/org/apache/james/imap/api/Tag.java:
##########
@@ -53,8 +52,6 @@ public final int hashCode() {
@Override
public String toString() {
- return MoreObjects.toStringHelper(this)
- .add("value", value)
- .toString();
+ return value;
Review Comment:
+1 here MoreObject definitly is overkill
--
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
Re: [PR] JAMES-4060: Fix UID FETCH command failing on empty mailbox [james-project]
Posted by "woj-tek (via GitHub)" <gi...@apache.org>.
woj-tek commented on code in PR #2386:
URL: https://github.com/apache/james-project/pull/2386#discussion_r1726658865
##########
mpt/impl/imap-mailbox/core/src/main/resources/org/apache/james/imap/scripts/Uid.test:
##########
@@ -32,6 +32,12 @@ S: \* 2 FETCH \(UID 3\)
S: \* 3 FETCH \(UID 4\)
S: a OK FETCH completed.
+C: a2 FETCH 1:* (UID)
Review Comment:
Extracted this to dedicated test/script.
--
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
Re: [PR] JAMES-4060: Fix UID FETCH command failing on empty mailbox [james-project]
Posted by "Arsnael (via GitHub)" <gi...@apache.org>.
Arsnael merged PR #2386:
URL: https://github.com/apache/james-project/pull/2386
--
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
Re: [PR] JAMES-4060: Fix UID FETCH command failing on empty mailbox [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on code in PR #2386:
URL: https://github.com/apache/james-project/pull/2386#discussion_r1726641331
##########
protocols/imap/src/main/java/org/apache/james/imap/processor/AbstractMailboxProcessor.java:
##########
@@ -444,14 +444,10 @@ protected Optional<MessageRange> messageRange(SelectedMailbox selected, IdRange
private MessageRange msnRangeToMessageRange(SelectedMailbox selected, long lowVal, long highVal)
throws MessageRangeException {
- // Take care of "*" and "*:*" values by return the last message in
+ // Take care of "*", "1:*" and "*:*" values by return the last message in
// the mailbox. See IMAP-289
- if (lowVal == Long.MAX_VALUE && highVal == Long.MAX_VALUE) {
- Optional<MessageUid> last = selected.getLastUid();
- if (!last.isPresent()) {
- throw new MessageRangeException("Mailbox is empty");
- }
- return last.get().toRange();
+ if ((lowVal == Long.MAX_VALUE || lowVal == 1) && highVal == Long.MAX_VALUE) {
+ return MessageRange.all();
}
Review Comment:
We got a new bug here!
CF https://datatracker.ietf.org/doc/html/rfc3501 ABNF
```
seq-number = nz-number / "*"
; message sequence number (COPY, FETCH, STORE
; commands) or unique identifier (UID COPY,
; UID FETCH, UID STORE commands).
; * represents the largest number in use. In
; the case of message sequence numbers, it is
; the number of messages in a non-empty mailbox.
; In the case of unique identifiers, it is the
; unique identifier of the last message in the
; mailbox or, if the mailbox is empty, the
; mailbox's current UIDNEXT value.
; The server should respond with a tagged BAD
; response to a command that uses a message
; sequence number greater than the number of
; messages in the selected mailbox. This
; includes "*" if the selected mailbox is empty.
```
`1:*` -> Messages from first to last IE `MessageRange.all() (we are good)
But `*:*` means range from last message to last message IE `last.get().toRange()`
So we actually need two separate ifs: one handling `1:*` and one handling `*:*`
Also we MAY which:
- [ ] To ensure MPT tests `C: A0 FETCH *:*`
- [ ] To test the behaviour of `C: A0 UID FETCH *:*`
- [ ] Technically speacking a lone `seq-number` is a valid `sequence-set` so we may which to ensure `C: A0 FETCH *` and `C: A0 UID FETCH *` are nicely handled - and tested.
--
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
Re: [PR] JAMES-4060: Fix UID FETCH command failing on empty mailbox [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
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: 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
Re: [PR] JAMES-4060: Fix UID FETCH command failing on empty mailbox [james-project]
Posted by "woj-tek (via GitHub)" <gi...@apache.org>.
woj-tek commented on code in PR #2386:
URL: https://github.com/apache/james-project/pull/2386#discussion_r1725325113
##########
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:
Not exactly. It gives more information about the mailbox, which is not present in UnpooledStatusResponseFactory but yeah, can be removed.
--
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
Re: [PR] JAMES-4060: Fix UID FETCH command failing on empty mailbox [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on PR #2386:
URL: https://github.com/apache/james-project/pull/2386#issuecomment-2304088469
> Or this was more of a comment for future/followup improvements?
No need to bother, this was more a comment for the future.
--
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
Re: [PR] JAMES-4060: Fix UID FETCH command failing on empty mailbox [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on code in PR #2386:
URL: https://github.com/apache/james-project/pull/2386#discussion_r1726620585
##########
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:
> should we use asString() for custom string output?
+1
> which (in case of logging) may require additional guards for log-levels?
That's true!
In non critical code portions or non costly operations we don't really care but for big string concatenations this might be a concern.
I'm sorry to be peacky, this kind of stuff in IMAP code tend to have a non neglictible impact on CPU (mdc context handling) and memory allocations in what I could see with flame graphs (cf async-profiler) and being careful here do not cost much.
If wrapping things in ifs is for you also not your cup of tea we apparently can use suppliers in the logging event builder (I just found this out!)
```
LOGGER.makeLoggingEventBuilder(Level.DEBUG)
.setMessage("My message: {}")
.addArgument(() -> "I'm costly and I know it!")
.log();
```
And under the hood (log4j but other log provider would do the same):
```
logBuilder.log(this.message, this.arguments.toArray());
```
Could be nice to defer expensive argument evaluation, eg for custom displays?
--
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
Re: [PR] JAMES-4060: Fix UID FETCH command failing on empty mailbox [james-project]
Posted by "woj-tek (via GitHub)" <gi...@apache.org>.
woj-tek commented on code in PR #2386:
URL: https://github.com/apache/james-project/pull/2386#discussion_r1726750795
##########
protocols/imap/src/main/java/org/apache/james/imap/processor/AbstractMailboxProcessor.java:
##########
@@ -444,14 +444,10 @@ protected Optional<MessageRange> messageRange(SelectedMailbox selected, IdRange
private MessageRange msnRangeToMessageRange(SelectedMailbox selected, long lowVal, long highVal)
throws MessageRangeException {
- // Take care of "*" and "*:*" values by return the last message in
+ // Take care of "*", "1:*" and "*:*" values by return the last message in
// the mailbox. See IMAP-289
- if (lowVal == Long.MAX_VALUE && highVal == Long.MAX_VALUE) {
- Optional<MessageUid> last = selected.getLastUid();
- if (!last.isPresent()) {
- throw new MessageRangeException("Mailbox is empty");
- }
- return last.get().toRange();
+ if ((lowVal == Long.MAX_VALUE || lowVal == 1) && highVal == Long.MAX_VALUE) {
+ return MessageRange.all();
}
Review Comment:
Added missing tests to the non-empty mailbox and fixed the logic for range retrieval.
--
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
Re: [PR] JAMES-4060: Fix UID FETCH command failing on empty mailbox [james-project]
Posted by "woj-tek (via GitHub)" <gi...@apache.org>.
woj-tek commented on code in PR #2386:
URL: https://github.com/apache/james-project/pull/2386#discussion_r1725316119
##########
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
Yeah, my bad from quickly testing things.
> Wrap it in a if (LOGGER.isTraceEnabled() ?
`.trace()` do the check itself - should we check if the logger is enabled still?
> Please put the username from the session in there.
I wonder - shouldn't this be handled by MDC (and would be more general)?
--
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
Re: [PR] JAMES-4060: Fix UID FETCH command failing on empty mailbox [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on code in PR #2386:
URL: https://github.com/apache/james-project/pull/2386#discussion_r1725678677
##########
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:
Yes this was.
--
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