[Tickets #13153] Re: Bugs in the current implementation for PARTIAL limiting (RFC 5267 [4.4]) in Horde's Imap_Client library

noreply at bugs.horde.org noreply at bugs.horde.org
Sat May 3 04:53:48 UTC 2014


DO NOT REPLY TO THIS MESSAGE. THIS EMAIL ADDRESS IS NOT MONITORED.

Ticket URL: http://bugs.horde.org/ticket/13153
------------------------------------------------------------------------------
  Ticket             | 13153
  Updated By         | Michael Slusarz <slusarz at horde.org>
  Summary            | Bugs in the current implementation for PARTIAL limiting
                     | (RFC 5267 [4.4]) in Horde's Imap_Client library
  Queue              | Horde Framework Packages
  Version            | Git master
  Type               | Bug
  State              | Feedback
-Priority           | 2. Medium
+Priority           | 1. Low
  Milestone          |
-Patch              | 1
+Patch              |
  Owners             | Michael Slusarz
------------------------------------------------------------------------------


Michael Slusarz <slusarz at horde.org> (2014-05-02 22:53) wrote:

>>> To fix this it should just enforce the usage of the provided partial
>>> if it is a valid sequence range ('range_start:range_end'), as
>>> specified in RFC 5267 [4.4].
>>
>> This won't work, since 'partial' can be, for example, array(1, 2, 3).
>>  So we do need to convert to an Ids object first.  We just need to
>> catch the single case where the range contains a single ID
>
> Why would you even want to allow the ability to specify partial  
> ranges as anything else but the actual literal representation?

Because that's what the API (2.0) requires.  You can't take that  
ability away from someone using the code ... that's the definition of  
breaking backward compatibility.  Whether its a good idea or not is  
completely irrelevant - we're stuck with it until Horde_Imap_Client 3.0.

And from an API perspective, by requiring someone to provide a full  
partial string you are requiring them to do 2 things: 1.) Have a  
detailed knowledge of RFC 3501 (number sequence formatting) and 2.)  
Have a detailed knowledge of RFC 5267.

The brilliance/advantage of allowing someone to pass an array in is  
that all of these details are abstracted.  The library does all the  
IMAP formatting details for you.

> Partial ranges have nothing to do with message UIDs and I don't  
> think that trying to handle them the same way just for consistency  
> is the best idea.

Why not?  From RFC 3501 [9]:

seq-number      = nz-number / "*"
uniqueid        = nz-number

Outside of the '*', they *are* identicial from a syntax level.  So we  
can (and should) treat them the same internally.   
Horde_Imap_Client_Ids works on both equally well.  We surely don't  
want to have two libraries that duplicate the exact same code.

> In case of specifying a set of message UIDs

This isn't allowed.  So not sure what you are saying here.

> Also you did not commit any changes to Horde_Imap_Client_Ids so at  
> the moment your current fix will always set the partials range to  
> ':' which is clearly invalid:

How?  Are you passing in an empty array?  Don't do that.  (I can add  
code to catch that, but the result's going to be the same ... an  
invalid search.  GIGO).

I haven't gone through and been 100% thorough in type-checking,  
sanity-checking, etc. especially for parameters that are very liberal  
about their data input.  Trying to do that right now is simply going  
to add more bloat to the library - Socket.php is already too big.   
Once the commands are broken up into separate classes (coming in 3.0),  
this will probably happen since the code will be more maintainable  
even with all the sanity checks.

> C: 4 UID SORT RETURN (PARTIAL :) (DATE) US-ASCII UNDELETED
> S: 4 BAD Error in IMAP command UID SORT: PARTIAL range broken.
>
> And last but not least, if you're hell bent on allowing this array option
> then you need even more additional processing, to make sure that  
> you're actually sending a proper partial range. For example  
> something like array(1,2,3,5,6,7), while valid as far as Ids class  
> is concerned, would still break your current fix because it does not  
> have a single sequence-set rage representation and after the  
> conversion it would get translated from '1:3,5:7'*, which is  
> obviously still not a valid partial range:

Incorrect.  Directly from the documentation:

      *   - partial: (mixed) The range of results to return (message sequence
      *              numbers) Only a single range is supported (represented by
      *              the minimum and maximum values contained in the range
      *              given).
      *              DEFAULT: All messages are returned.

This code:

$pids = $this->getIdsOb(array(1,2,3,5,6,7))->range_string;

returns:

1:7

> C: 4 UID SORT RETURN (PARTIAL 1:3,5:7) (DATE) US-ASCII UNDELETED
> S: 4 BAD Error in IMAP command UID SORT: PARTIAL range broken.

That's flat out impossible with the current code (at least for the  
PARTIAL return).  $pids, used as the argument to PARTIAL, can NEVER  
contain a comma (,) for example.  You either copy/pasted this example  
or you are not running an unmodified copy of the current code.

> However the reverse is neither implied nor required for either  
> cases. For example you can have a server that supports ESORT but not  
> CONTEXT=SORT, which is exactly the case on my test server:

**EXACTLY**.  And a server that supports ESORT but not CONTEXT=SORT  
can't use PARTIAL (or CONTEXT or UPDATE) with ESORT.

Would you happen to be using Dovecot?  Dovecot does NOT support  
CONTEXT=SORT.  See, e.g., http://wiki2.dovecot.org/Roadmap

Now what you are seeing is probably the following: internally,  
Dovecot's SEARCH and SORT code is the same.  So limiting via PARTIAL  
will work on SORT simply because the code being used is identical and  
limitations on PARTIAL with SORT aren't being enforced.  Which is  
perfectly legal: there's nothing in IMAP spec that prevents commands  
from executing if not listed in CAPABILITY.

But the more difficult part in implementing CONTEXT=SORT actually  
deals with CONTEXT & UPDATE, which hasn't been written yet in Dovecot.  
  Without those two, CONTEXT=SORT can't be listed as supported since  
there is no way to indicate which of the 3 return values are supported.

Fortuitously this works on your system.  But there is absolutely no  
way of knowing this from the CAPABILITY string and no guarantee this  
works on other systems.  Per the RFC, the only way PARTIAL is  
supported for SORTs is if CONTEXT=SORT is available

RFC 5267 [4.1]: if CONTEXT=SORT is advertised, ESORT accepts 3  
additional return arguments: UPDATE, CONTEXT, and PARTIAL.  Without  
CONTEXT=SORT, these 3 RETURN arguments aren't available.  That's all  
that section says.

The RFC does not say that PARTIAL searching is supported for SORTs if  
CONTEXT=SEARCH is available.  (Which makes logical sense: there's no  
reason to support CONTEXT=SOMETHING type capability strings unless  
they refer to a specific subset of behavior modifications.  Otherwise  
a single CONTEXT capability would have been defined handling both  
SEARCH and SORT.)

What really should be done, and would be a more productive use of our  
time, is to abstract partial support to the client also (i.e.  
client-side sorting if SORT is not available).  Meaning a user should  
be able to pass-in the partial argument and get the partial list of  
responses, regardless if CONTEXT is available on the server.   
Obviously CONTEXT would make this a much less expensive operation, but  
that should be irrelevant for the purposes of someone using the API.





More information about the bugs mailing list