Hi again, answers inlined. --
Best regards, Benoit TELLIER General manager of Linagora VIETNAM. Product owner for Team-Mail product. Chairman of the Apache James project. Mail: btell...@linagora.com Tel: (0033) 6 77 26 04 58 (WhatsApp, Signal) On Aug 21, 2024 5:29 PM, from Wojtek Hi, On 21/08/2024 10:09, Benoit TELLIER wrote: > Hello Wojtek, > I would expect UID FETCH 1:* XXX on an empty mailbox to return OK > > Kinda: > > C: A0 UID FETCH 1:* UID > S: A0 OK FETCH completed. Thank you > The way to go regarding non compliance to IMAP syntax/grammar/logic would be: > > - Try to get the exact IMAP exchange. Evolution is especially handy > regarding this as allows turning on advanced IMAP debugging that actually > drops the IMAP conversation to a log file. Indeed: gitlab.gnome.org/GNOME/evolution/-/wikis/Debugging: `CAMEL_DEBUG=imapx:io evolution >& logfile` > - That exchange can actually be reused in order to write a MPT test that > actually acts as an "integration test" This is veeeery conveniet > - Then we need to address the faulty logic in protocols/imap . Unit tests > may be written if relevant. > - Open a JIRA ticket to track the defect issues.apache.org/jira/browse/JAMES-4060 > - Open a PR referencing the JIRA issue in the title + commit messages > (starts with JAMES-XXXX) for automatic linking > > Here, it seems rather straightforward, here is the MPT output I came up with > that reproduces the issue (sorry I was curious!) : > github.com/apache/james-project/pull/2385 A followup PR build upon yours due to GH limitations: github.com/apache/james-project/pull/2386 > Regarding the fix, I believe it is rather the way FetchProcessor handles an > empty range. > - currently it considers empty ranges as bad input. While it MAY be > legitimate with commands that alter state (COPY MOVE STORE EXPUNGE REPLACE) > it is definitly inappropriate for FETCH. > - It likely rather should skip the given range rather than throwing an > exception in FetchProcessor line 205. > - (in the recent IMAP work, we mostly ported the existing code without > questionning its actual logic and we likely missed that one-line test > coverage to uncover this issue). > > Do you want to get a look at the issue? Tried my best, let's see how you deem the result :) > Ps: SELECT is mandatory to work with a folder content. TB is necessarily > doing it, though it may be clever enough to infer from the SELECT command (or > the STATUS counters) that the folder is empty and thus that opening it may > not be needed. Indeed TB is smarter and after select and empty mailbox it doesn't issue FETCH (it's also possible to debug TB in the same way as Evolution it turns out). >> it would be handy if the log would contain complete received command, >> even for learning/understanding better what's going on/what was received >> (for example UID fetch lacks actual parameters) > Regarding actual IMAP logging... > > The MDC logging context is set with precious debugging information including > user supplied parameters. You may need to configure your logging output to > include MDC to benefit from it. Would it fit your needs? Hmm... it's not set everywhere, and especially missing for IMAP commands IMHO (but can't see nice way to improve it) That's a reactive programming issue: MDC is thread local and thus do not play well with asynchronous code. One would need to manually cary over and set the context, which should mostly be done in IMAP processors ( .doOnEach(logOnError(MailboxException.class, e -> LOGGER.error(...))) stuff...) > I agree the "GOT Tag" messages are useless and may be removed. I made a couple of tweaks to the commands output and removed individual ones in favour of the "final" one which IMHO has a bit more sense and is more informative. 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... I will comment on GitHub too ^^ > AbstractProcessor (org.apache.james.imap.processor.base.AbstractProcessor) > have a debug log logging the command along side the associated MDC, which > shall be enough... Though this is the interpretation of IMAP exchanges by > James and not the actual IMAP exchanges. > > However that is true that I did often ended up tweaking the IMAP network > layer to extract the IMAP exchange. Having a standard ready to use way to do > this would be handy I believe. > The IMAP parsers are read ahead (reads data as needed) thus getting the exact > user input is tricky, but if needed we could add traces onto > NettyImapRequestLineReader and drop a copy of the buffer as a log. For what > james sends we sould put a trace into ChannelImapResponseWriter:write. In oth > case we shall beware of eager evaluation of the logging input as computing > the traces content may be expensive. > Do you want to get a shot at this too?-- > Best regards, > Benoit TELLIER Yeah, outputting socket data in case of IMAP seems quite daunting as each command/processor can read more data as it needs. Though for my needs and fixing this issue the exchange I got seems enough. And there is still possibility to get the complete exchange from the client perspective True with exchange, not true with outlook and friends. And this also means you can control client side which in production isn't necessary true. wk. --------------------------------------------------------------------- To unsubscribe, e-mail: server-dev-unsubscr...@james.apache.org For additional commands, e-mail: server-dev-h...@james.apache.org
