Opened 10 years ago

Closed 7 years ago

Last modified 6 months ago

#2278 defect closed fixed (fixed)

IMAP4 server fails for commands like "11 search uid 2:*"

Reported by: Jean-Paul Calderone Owned by:
Priority: normal Milestone:
Component: mail Keywords:
Cc: jesstess Branch: branches/imap-wildcard-2278
branch-diff, diff-cov, branch-cov, buildbot
Author: jesstess

Description

The wildcard endpoint for the UID range to search causes the MessageSet created in search_UID to have an unset last attribute which makes iteration over it invalid, which makes the contains implementation fail.

It looks like it would probably be okay to not raise TypeError in that case, though -- if an endpoint is wildcard, half of the range check should suffice.

Attachments (4)

patch-twisted-2278.patch (13.8 KB) - added by Jan Urbański 7 years ago.
patch-twisted-2278-v2.patch (17.2 KB) - added by Jan Urbański 7 years ago.
patch-twisted-2278-v3.patch (16.2 KB) - added by Jan Urbański 7 years ago.
patch against the review branch
2278.bugfix (88 bytes) - added by Jan Urbański 7 years ago.
topfile for the fix

Download all attachments as: .zip

Change History (20)

comment:1 Changed 10 years ago by Jean-Paul Calderone

Milestone: Mail-0.4

comment:2 Changed 8 years ago by Jean-Paul Calderone

Milestone: Twisted-8.2+1

I can't see any reason this needs to be targeted at 9.0 (8.2+1).

Changed 7 years ago by Jan Urbański

Attachment: patch-twisted-2278.patch added

Changed 7 years ago by Jan Urbański

Attachment: patch-twisted-2278-v2.patch added

comment:3 Changed 7 years ago by Jan Urbański

Keywords: review added
Owner: Jean-Paul Calderone deleted

Whoops, updated patch attached.

It fixes this bug as well as:

  • adds tests for invalid search terms
  • adds tests for the string representation of a MessageSet
  • adds a TODO test for searches with 0 as the message UID (not sure if they should be rejected, can't find definite answer in RFC)
  • adds tests for cases where the UIDs are in reverse order (4:2 vs 2:4) or for a single star as the search term
  • makes parseListId take an optional argument that is the highest value of the UID or the sequence id in a mailbox, depending on the context in which you are parsing a search term. It only matters if there are wildcards in the search term.

comment:4 Changed 7 years ago by jesstess

Author: jesstess
Branch: branches/imap-wildcard-2278

(In [28786]) Branching to 'imap-wildcard-2278'

comment:5 Changed 7 years ago by jesstess

(In [28787]) Apply patch-twisted-2278-v2.patch

refs #2278

comment:6 Changed 7 years ago by jesstess

Cc: jesstess added
Keywords: review removed
Owner: set to Jan Urbański

Thanks for all the work on this ticket, in particular the new test coverage and improved documentation, wulczer! Some review feedback:

test_imap.py

  • class IMAP4HelperTestCase needs a docstring
  • The tests you've added in IMAP4HelperTestCase should use the test_testName convention described in the test standard even though the surrounding tests don't (if you feel like renaming the other tests in that class that would be great too).
  • testMessageSetStringRepresentation, testInvalidIdListParser, testInvalidIdListParserTodo, and testIdListParser need docstrings
  • Can you clarify testInvalidIdListParserTodo.todo to indicate that there is ambiguity in the RFC (is this 3501?), and about what exactly?
  • there should be two blank lines between tests, as described in the coding standard, even though the surrounding code isn't being good about this (and again, if you're interested in adding appropriate spacing in the rest of the classes affected by your changes that's great)
  • The "it"s in the docstrings for test_searchMessageSetWithStarFirst and test_searchMessageSetUIDWithStarFirst are ambiguous. Can you be more explicit about what the "it"s are?

imap4.py

  • even though it's private, can you give __cbManualSearch a docstring with epytext markup for it's parameters?
  • the _searchFilter and _singleSearchStep parameters need @type markup
  • #messageSet.last = lastSequenceId can just go away instead of being commented out
  • search_NOT, search_OR, and search_UID should have docstrings, but I've thrown a lot of comment requests already at you in this review. If you're up for it, great, otherwise maybe it'll happen down the review road.
  • In parseIdList, the @param lastMessageId should indent lines after the first line, like in search_SENTSINCE, for example.
  • There's a corerct ==> correct typo in parseIdList
  • The TODO in parseIdList needs its own ticket (which should reference the skip in test_imap.py)
  • In __cbCopy, addedIDs and failures are assigned to but never used

builds look good. Please make subsequent diffs against the branch. Thanks again!

comment:7 in reply to:  6 Changed 7 years ago by Jan Urbański

Replying to jesstess:

Thanks for all the work on this ticket, in particular the new test coverage and improved documentation, wulczer! Some review feedback:

Thanks for the review!

test_imap.py

  • class IMAP4HelperTestCase needs a docstring

Done

  • The tests you've added in IMAP4HelperTestCase should use the test_testName convention described in the test standard even though the surrounding tests don't (if you feel like renaming the other tests in that class that would be great too).

Renamed, and renamed the rest of the tests in that class.

  • testMessageSetStringRepresentation, testInvalidIdListParser, testInvalidIdListParserTodo, and testIdListParser need docstrings

Done

  • Can you clarify testInvalidIdListParserTodo.todo to indicate that there is ambiguity in the RFC (is this 3501?), and about what exactly?

Hehe, in a classic example of explainin your problem to a teddy bear and finding the answer as you try to express it, I have actually found a grammar production in RFC 3501 that says that the numbers should be non-negative. I've added tests for 0 and negatives and removed the todo.

  • there should be two blank lines between tests, as described in the coding standard, even though the surrounding code isn't being good about this (and again, if you're interested in adding appropriate spacing in the rest of the classes affected by your changes that's great)

Was simple enough, added blank lines to the classes I was touching.

  • The "it"s in the docstrings for test_searchMessageSetWithStarFirst and test_searchMessageSetUIDWithStarFirst are ambiguous. Can you be more explicit about what the "it"s are?

Done

imap4.py

  • even though it's private, can you give __cbManualSearch a docstring with epytext markup for it's parameters?

Yes, and in another example of how docstrings are useful, while looking at the code of that method I found #4385... the fix is on that ticket, in here I just added the docstring.

  • the _searchFilter and _singleSearchStep parameters need @type markup

Done

  • #messageSet.last = lastSequenceId can just go away instead of being commented out

Done

  • search_NOT, search_OR, and search_UID should have docstrings, but I've thrown a lot of comment requests already at you in this review. If you're up for it, great, otherwise maybe it'll happen down the review road.

Added docstrings and started adding them to the rest of search_* methods, but then ran out of steam...

  • In parseIdList, the @param lastMessageId should indent lines after the first line, like in search_SENTSINCE, for example.

Done

  • There's a corerct ==> correct typo in parseIdList

Done

  • The TODO in parseIdList needs its own ticket (which should reference the skip in test_imap.py)

ENOMORETODO :o)

  • In __cbCopy, addedIDs and failures are assigned to but never used

There's a XXX in that method though, that would probably use them... I haven't touched that code, so I'd rather leave these alone (or they can go away in an different commit, I guess). I'm sure pychecker would find a lot of things to complain about in imap4.py ;)

builds look good. Please make subsequent diffs against the branch. Thanks again!

Patch against that branch attached, thanks for a thorough review!

Changed 7 years ago by Jan Urbański

Attachment: patch-twisted-2278-v3.patch added

patch against the review branch

comment:8 Changed 7 years ago by Jan Urbański

Owner: Jan Urbański deleted

comment:9 Changed 7 years ago by Jan Urbański

Keywords: review added

comment:10 Changed 7 years ago by jesstess

(In [28790]) Apply patch-twisted-2278-v3.patch

refs #2278

comment:11 Changed 7 years ago by jesstess

Keywords: review removed
Owner: set to Jan Urbański

wulczer: the branch looks great. One small note: in the future, omit "Test that" preludes in test docstrings and get straight to the meat of the description :) (these have been removed in the branch).

Can you attach a topfile as described in ReviewProcess? After that I think this is ready to merge!

Changed 7 years ago by Jan Urbański

Attachment: 2278.bugfix added

topfile for the fix

comment:12 Changed 7 years ago by Jan Urbański

Keywords: review added
Owner: Jan Urbański deleted

Topfile attached

comment:13 Changed 7 years ago by jesstess

Owner: set to jesstess

comment:14 Changed 7 years ago by jesstess

Resolution: fixed
Status: newclosed

(In [28795]) Merge imap-wildcard-2278

Author: wulczer Reviewer: jesstess Fixes: #2278

twisted.mail.imap4.IMAP4Server no longer fails on search queries that contain wildcards. Additionally, this commit adds unit tests for the string representation of a MessageSet and several search edge cases, as well as vastly improved documentation in imap4.py.

comment:15 Changed 6 years ago by <automation>

Owner: jesstess deleted

comment:16 Changed 6 months ago by hawkowl

Keywords: review removed

[mass edit] Removing review from closed tickets.

Note: See TracTickets for help on using tickets.