Tickets :: Comment on [#13153] Bugs in the current implementation for PARTIAL limiting (RFC 5267 [4.4]) in Horde's Imap

* Your Email Address
* Spam protection	Enter the letters below: \ /. .. . __.. . >< \|_/ \| \|(__ \|\/\| / \\| \\|/\\|.__)\| \|
Comment	>>>> 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.
Attachment
Watch this ticket

Saved Queries