Bug #68310 for Mail-IMAPClient: exists() does a STATUS than can go to a disconnexion or wrong folders() reply

Thu May 19 07:40:38 2011 gilles.lamiral [...] laposte.net - Ticket created

Subject:

exists() does a STATUS than can go to a disconnexion or wrong folders() reply

Hello Phil, 1) exists() calls STATUS. Some IMAP servers like Exchange disconnect after ten STATUS fails with NO. 2) folders( $folder ) calls the good LIST but it also calls exists( $folder ) so STATUS is called the response of folders( $folder ) is void if STATUS is NO. Example with folders( 'DTU' ) a STATUS is called and fails even if \Noselect is given previously by the server. Sending: 7 LIST "" "DTU/*" Sent 19 bytes Read: * LIST (\Noselect \HasChildren) "/" "DTU/" * LIST (\NoInferiors \UnMarked) "/" "DTU/Vestervang" ... Example with folders( 'DTU/*' ) could be good (no STATUS called) but folders( 'DTU/*' ) replies an empty list that should not be an empty (many subfolders here). Even a folders( 'DTU/*' ) doesn't work. Sending: 6 LIST "" "DTU/*" Sent 19 bytes Read: * LIST (\Noselect \HasChildren) "/" "DTU/" * LIST (\NoInferiors \UnMarked) "/" "DTU/Vestervang" Some quotes from the IMAP RFC 3501 http://www.faqs.org/rfcs/rfc3501.html suggesting STATUS is not adequate for exists(). "the STATUS command SHOULD NOT be used on the currently selected mailbox." "Because the STATUS command is not guaranteed to be fast in its results, clients SHOULD NOT expect to be able to issue many consecutive STATUS commands and obtain reasonable performance." "Unlike the LIST command, the STATUS command is not guaranteed to be fast in its response." I suggest to use LIST instead of STATUS in the exists() implementation and correct folders() to permit top level folders that are not selectable but have children. Thanks in advance Phil.

Thu May 26 02:15:04 2011 PLOBBES [...] cpan.org - Correspondence added

Hi Gilles, thanks for the bug report. I'm catching up from vacation so it may take some time before I get some free time to look into this further. At a quick glance, here are my thoughts (I reserve the right to change opinions once I get to think about and research more): - calling exists() in list seems unnecessary + I need to research history here to see if there is a reason for this - regardless of exists() usage in list, we should probably be trying to ensure we do not call exists on something that returned a \Noselect in the LIST results - if we call exists() in folders() use the results from the LIST call to get the mailbox name(s) and not the argument to folders() I believe we're dealing with some history here and I don't want to rush to make changes without trying to understand why that old code is the way it is. I know I refactored code, but I believe the logic/operations are essentially the same from old 3.x code I inherited.

Thu May 26 02:15:07 2011 The RT System itself - Status changed from 'new' to 'open'

Thu May 26 02:15:07 2011 PLOBBES [...] cpan.org - Given to PLOBBES

Sun Jun 05 00:13:35 2011 gilles.lamiral [...] laposte.net - Correspondence added

Subject:	Re: [rt.cpan.org #68310] exists() does a STATUS than can go to a disconnexion or wrong folders() reply
Date:	Sun, 05 Jun 2011 06:13:17 +0200
To:	bug-Mail-IMAPClient [...] rt.cpan.org
From:	Gilles LAMIRAL <gilles.lamiral [...] laposte.net>

Hi Phil, Show quoted text

> <URL: https://rt.cpan.org/Ticket/Display.html?id=68310 > > > Hi Gilles, thanks for the bug report. I'm catching up from vacation so it > may take some time before I get some free time to look into this further.

No problem. Show quoted text

> At a quick glance, here are my thoughts (I reserve the right to change > opinions once I get to think about and research more): > - calling exists() in list seems unnecessary > + I need to research history here to see if there is a reason for this

Ok. Show quoted text

> - regardless of exists() usage in list, we should probably be trying to > ensure we do not call exists on something that returned a \Noselect in the > LIST results

So never calling exists() would be the conclusion. Show quoted text

> - if we call exists() in folders() use the results from the LIST call to > get the mailbox name(s) and not the argument to folders()

Surely better. Show quoted text

> I believe we're dealing with some history here and I don't want to rush to > make changes without trying to understand why that old code is the way it > is. I know I refactored code, but I believe the logic/operations are > essentially the same from old 3.x code I inherited.

Do as you think the better is. Good criteria are the documented API and RFCs. -- Au revoir, 09 51 84 42 42 Gilles Lamiral. France, Baulon (35580) 06 20 79 76 06

Mon Aug 08 22:51:23 2011 phil [...] perkpartners.com - Correspondence added

Subject:	Re: [rt.cpan.org #68310] exists() does a STATUS than can go to a disconnexion or wrong folders() reply
Date:	Mon, 08 Aug 2011 22:49:53 -0400
To:	bug-Mail-IMAPClient [...] rt.cpan.org
From:	Phil Pearl (Lobbes) <phil [...] perkpartners.com>

Hi again Gilles, Show quoted text

> Queue: Mail-IMAPClient > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=68310 >

... Show quoted text

> > I believe we're dealing with some history here and I don't want to > > rush to make changes without trying to understand why that old code > > is the way it is. I know I refactored code, but I believe the > > logic/operations are essentially the same from old 3.x code I > > inherited.

> > Do as you think the better is. Good criteria are the documented API > and RFCs.

I have updated the folders() documentation with this caveat: --- Please note that documentation previously suggested that if you just want to list a folder's subfolders (and not the folder itself), then you need to include the hierarchy separator character (as returned by the L</separator> method). However, this does not match the behavior of the existing implementation, so you will need to manually exclude the parent folder from the results. --- Below is a candidate patch for this bug, please let me know your thoughts and/or concerns about this proposed change! $ git diff diff --git a/lib/Mail/IMAPClient.pm b/lib/Mail/IMAPClient.pm index 9134ef0..2a9d860 100644 --- a/lib/Mail/IMAPClient.pm +++ b/lib/Mail/IMAPClient.pm @@ -562,7 +562,7 @@ sub _folders_or_subscribed { { my @list; if ($what) { - my $sep = $self->separator($what); + my $sep = $self->separator($what) || $self->separator(undef); last unless defined $sep; my $whatsub = $what =~ m/\Q${sep}\E$/ ? "$what*" : "$what$sep*"; @@ -571,8 +571,10 @@ sub _folders_or_subscribed { shift @$tref; # remove command push @list, @$tref; - my $exists = $self->exists($what) or last; - if ($exists) { + # BUG?: this behavior has been around since 2.x, why? + my $cansel = $self->selectable($what); + last unless defined $cansel; + if ($cansel) { $tref = $self->$method( undef, $what ) or last; shift @$tref; # remove command push @list, @$tref; @@ -2715,8 +2717,8 @@ sub is_parent { sub selectable { my ( $self, $f ) = @_; - my $info = $self->list( "", $f ); - defined $info ? not( grep /NoSelect/i, @$info ) : undef; + my $info = $self->list( "", $f ) or return undef; + return not( grep /\\Noselect/i, @$info ); }

Fri Aug 12 21:44:18 2011 PLOBBES [...] cpan.org - Fixed in 3.29 added

Fri Aug 12 21:44:18 2011 PLOBBES [...] cpan.org - Correspondence added

Subject:

exists() does a STATUS than can go to a disconnection or wrong folders() reply

Mail::IMAPClient 3.29 was released today with the patch for this bug, please feel free to reopen if you find the problem still exists in this release.

Fri Aug 12 21:44:19 2011 PLOBBES [...] cpan.org - Status changed from 'open' to 'resolved'

Fri Aug 12 23:27:23 2011 gilles.lamiral [...] laposte.net - Correspondence added

Subject:	Re: [rt.cpan.org #68310] exists() does a STATUS than can go to a disconnexion or wrong folders() reply
Date:	Sat, 13 Aug 2011 05:17:53 +0200
To:	bug-Mail-IMAPClient [...] rt.cpan.org
From:	Gilles LAMIRAL <gilles.lamiral [...] laposte.net>

Hi Phil, I didn't check it. No problem, I will. -- Au revoir, 09 51 84 42 42 Gilles Lamiral. France, Baulon (35580) 06 20 79 76 06

Fri Aug 12 23:27:24 2011 The RT System itself - Status changed from 'resolved' to 'open'

Tue Oct 04 14:19:26 2011 PLOBBES [...] cpan.org - Status changed from 'open' to 'resolved'