Opened 8 years ago

Closed 7 years ago

#4069 defect closed fixed (fixed)

FLAGS response with only one flag(i.e. "FLAGS (\Seen)") is not taken in IMAP4Client.__cbSelect

Reported by: natta Owned by:
Priority: normal Milestone:
Component: mail Keywords:
Cc: Jean-Paul Calderone, khorn Branch: branches/imap4-select-tests-4069
branch-diff, diff-cov, branch-cov, buildbot
Author: exarkun

Description

The prolem is about this part of IMAP4Client.cbSelect

def __cbSelect(self, (lines, tagline), rw):
        # In the absense of specification, we are free to assume:
        #   READ-WRITE access
        datum = {'READ-WRITE': rw}
        lines.append(tagline)
        for parts in lines:
            split = parts.split()
            if len(split) == 2:
                if split[1].upper().strip() == 'EXISTS':
                    try:
                        datum['EXISTS'] = int(split[0])
                    except ValueError:
                        raise IllegalServerResponse(parts)
                elif split[1].upper().strip() == 'RECENT':
                    try:
                        datum['RECENT'] = int(split[0])
                    except ValueError:
                        raise IllegalServerResponse(parts)
                else:
                    log.err('Unhandled SELECT response (1): ' + parts)
            elif split[0].upper().strip() == 'FLAGS':

etc. When parts variable is equal to "FLAGS (\Seen)" for example ( for * FLAGS (\Seen) response to SELECT command),the length of split list will be 2, but we check only for RECENT or EXISTS responses in case of length equal to 2, so our FLAGS response will be not taken into account.

The solution for this problem is very simple: the branch

 elif split[0].upper().strip() == 'FLAGS':
     split = parts.split(None, 1)
     datum['FLAGS'] = tuple(parseNestedParens(split[1])[0])

should be also added to the if operator in the length = 2 part

Change History (17)

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

Cc: Jean-Paul Calderone added
Owner: changed from Jean-Paul Calderone to natta

Hi, thanks for reporting this. I wonder if you could help out by adding a unit test which fails because of this problem? That would make it much easier to understand the case that's not handled properly.

Thanks again.

comment:2 Changed 8 years ago by khorn

Cc: khorn added

comment:3 Changed 8 years ago by natta

Resolution: fixed
Status: newclosed

In new version (9.0.0) this bug is already fixed

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

This can't really be fixed, since there's still no unit test for the behavior. Like as not, the next release of Twisted will re-introduce whatever problem this ticket is supposed to be describing. That kind of accidental change is what happens when there's no unit tests for correct behavior.

So it'd be really great if you could provide a unit test for this anyway.

comment:5 Changed 8 years ago by natta

OK, I will try to do it asap

comment:6 Changed 8 years ago by khorn

Resolution: fixed
Status: closedreopened

If it's not fixed it should be an open ticket, yes?

We can close it again after an appropriate test is applied.

comment:7 in reply to:  6 ; Changed 7 years ago by natta

Replying to khorn:

If it's not fixed it should be an open ticket, yes?

We can close it again after an appropriate test is applied.

Ok, so I suggest to add the following test in the test_imap.py in IMAP4ServerTestCase:

def testSelectOneFlag(self):
        SimpleServer.theAccount.addMailbox('test-mailbox')
        mbox = SimpleServer.theAccount.mailboxes['TEST-MAILBOX']
        mbox.flags  = ('\Flag1')
        self.selectedArgs = None
        def login():
            return self.client.login('testuser', 'password-test')
        def select():
            def selected(args):
                self.selectedArgs = args
                self._cbStopClient(None)
            d = self.client.select('test-mailbox')
            d.addCallback(selected)
            return d

        d1 = self.connected.addCallback(strip(login))
        d1.addCallback(strip(select))
        d1.addErrback(self._ebGeneral)
        d2 = self.loopback()
        return defer.gatherResults([d1, d2]).addCallback(self._cbTestSelectOneFlag)

    def _cbTestSelectOneFlag(self, ignored):
        self.assertTrue('FLAGS' in self.selectedArgs.keys())

comment:8 in reply to:  7 Changed 7 years ago by natta

Replying to natta:

Replying to khorn:

If it's not fixed it should be an open ticket, yes?

We can close it again after an appropriate test is applied.

Ok, so I suggest to add the following test in the test_imap.py in IMAP4ServerTestCase:

def testSelectOneFlag(self):
        SimpleServer.theAccount.addMailbox('test-mailbox')
        mbox = SimpleServer.theAccount.mailboxes['TEST-MAILBOX']
        mbox.flags  = ('\Flag1')
        self.selectedArgs = None
        def login():
            return self.client.login('testuser', 'password-test')
        def select():
            def selected(args):
                self.selectedArgs = args
                self._cbStopClient(None)
            d = self.client.select('test-mailbox')
            d.addCallback(selected)
            return d

        d1 = self.connected.addCallback(strip(login))
        d1.addCallback(strip(select))
        d1.addErrback(self._ebGeneral)
        d2 = self.loopback()
        return defer.gatherResults([d1, d2]).addCallback(self._cbTestSelectOneFlag)

    def _cbTestSelectOneFlag(self, ignored):
        self.assertTrue('FLAGS' in self.selectedArgs.keys())

Sorry, not ('\Flag1'), but ('\Flag1',). Failed for that Python feature again.

comment:9 Changed 7 years ago by Jean-Paul Calderone

Keywords: review added
Owner: natta deleted
Status: reopenednew

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

The branch for #1105 is what fixed the behavior, it seems.

comment:11 Changed 7 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to Jean-Paul Calderone
Status: newassigned

Thanks for supplying the test. Once I tracked down the revision that fixed the problem it made it clear how the bug actually got fixed. Since there are a bunch of examine tests, I think it makes sense to apply all of these to select. I'm going to do that and put it in a branch for review.

comment:12 Changed 7 years ago by Jean-Paul Calderone

Author: exarkun
Branch: branches/imap4-select-tests-4069

(In [28762]) Branching to 'imap4-select-tests-4069'

comment:13 Changed 7 years ago by Jean-Paul Calderone

(In [28763]) Put the existing IMAP4Client.examine tests into a mixin and apply them to both IMAP4Client.examine and IMAP4Client.select

refs #4069

comment:14 Changed 7 years ago by Jean-Paul Calderone

Keywords: review added
Owner: Jean-Paul Calderone deleted
Status: assignednew

comment:15 Changed 7 years ago by therve

Keywords: review removed
Owner: set to Jean-Paul Calderone

Just one comment:

  • the _examine method should be renamed, and the docstring fixed.

Please merge once fixed!

comment:16 Changed 7 years ago by Jean-Paul Calderone

Resolution: fixed
Status: newclosed

(In [28802]) Merge imap4-select-tests-4069

Author: exarkun Reviewer: therve Fixes: #4069

Apply an existing test case full of IMAP4Client.examine tests to IMAP4Client.select as well.

comment:17 Changed 7 years ago by <automation>

Owner: Jean-Paul Calderone deleted
Note: See TracTickets for help on using tickets.