james-server-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Darrell DeBoer <darr...@apache.org>
Subject Re: Introduction (was Re: cvs commit: jakarta-james/proposals/imap/java/org/apache/james/imapserver/commands CommandTemplate.java)
Date Thu, 14 Nov 2002 06:29:32 GMT
Hi Peter,

It's great to see so much energy and enthusiasm in the James development team 
ATM. I'm looking forward to being a part of the v3 development effort.

Regarding the issues you enumerate:

On Tue, 12 Nov 2002 04:17, Peter M. Goldstein wrote:
>
> 1) Conceptual misunderstanding of the role of the message sequence
> numbers.  MSNs cannot be associated with a mailbox object, because they
> are a per connection, not per mailbox, concept.  It's more than possible
> for two different connections to have different MSN=>UID maps for a
> given mailbox.  Mailboxes should only know about UIDs.

You may be right, but can you give an example of where 2 different connections 
would have different MSN's for the same underlying mailbox. From my 
understanding, the rule for mapping MSN=>UID is strictly a contiguous 
sequence in ascending order of UIDs. This doesn't allow allowing different 
mappings. The MSN=>UID mapping may change between sessions when the 
underlying mailbox has changed, which may be what you meant by "connections".

That said, I agree that the mailbox shouldn't need to track MSN's. They can 
and do change whenever a mailbox message is expunged, and are easily 
generated by enumerating the messages in a mailbox.

>
> 2) Lack of handling for the "literal" string argument type.  Basically,
> the spec allows (and requires in the case of
> internationalization/localization support, since quoted strings are
> restricted to US-ASCII) the use of {<number of bytes>} as an argument to
> specify that the server should send a continuation response and take the
> next <number of bytes> as the string data.
>
> 3) Mailbox name encoding (as per spec) is not supported
>
> 4) The Search command is not implemented
>
> 5) Support for the special "INBOX" mailbox alias is not present

Yep, to all these.

>
> 6) Three of the commands, for no reason I can discern, are not in the
> commands package.  These commands also don't seem to take advantage of
> any of the encapsulated methods in SingleThreadedConnectionHandler.
> This led to at least one set of malformed NO responses, and a lot of
> duplicate code.  I'm assuming these classes predate the others, and
> simply need to be modified/moved.

Your assumption is correct. Originally every command seemed to be implemented 
via cut-and-paste of a single method. I refactored out the easier ones into 
the separate command classes you see today, using the Template Method pattern 
to handle to commonality between them. Some of the commands, I barely touched 
at all - namely the ones that weren't moved into the commands package.

The main culprits are BaseCommand and it's derivatives - CommandStore, 
SingleThreadedConnectionHandler, CopyCommand and CommandFetch. For some 
reason CopyCommand was moved into the commands package, but wasn't moved over 
to the new framework.

>
> 7) The SingleThreadedConnectionHandler class has bad code at the point
> where commands are read off the wire.  The whole
> "setCanParse/getCanParse" concept violates basic threading techniques
> (if the connection handling is single threaded it's superfluous, if
> multi-threaded it's a race condition) and there is a busy loop at this
> point.

I'm sure you're right - the whole handling connections bit needed an overhaul.

>
> 8) EXPUNGE responses are being sent after command completion responses,
> in violation of spec.

Looks like I refactored the existing implementation into a separate command, 
and left it at that. For all of the commands I actually worked on, I wrote a 
protocol test file. All the others should be assumed to be broken.

The commands that have tests are: AUTHENTICATE, CAPABILITY, CREATE, DELETE, 
EXAMINE, FETCH, LIST, LOGIN, LOGOUT, LSUB, SELECT,  SUBSCRIBE. I'm not saying 
that these all work, but anything else should be assume *not* to work. In 
fact FETCH needs wholesale refactoring, but at least some tests are present 
to aid the process.

>
> 9) MSN/UID set handling was incorrect - using a '-1' as an end delimiter
> in some cases as opposed to producing a correct range.  No real reason
> to do this - the comment about Microsoft Outlook's behavior is not
> correct, as Outlook was behaving according to spec.
>
I'm not surprised. Needs fixing.

> 10) Comments are almost wholly absent.
>

Guilty as charged, although I *did* add some tests, so I plead for leniency. 
;). 

> 11) Information is duplicated between the ImapRequest and ImapSession
> interfaces.

11a. ImapSession interface is a travesty. Originally, there was no interface 
between SingleThreadedConnectionHandler and the commands - at least this 
makes it clear what the commands are using (and not).

>
> 12) Despite not being ConnectionHandlers, many of the command objects
> extended BaseConnectionHandler (all functionality was unused).  The
> command hierarchy in general is a little confused, with both BaseCommand
> and CommandTemplate appearing to assert central roles.  Usage and
> argument validation mechanisms are not unified and, in at least a few
> cases, will need to be redesigned to account for point 2 above.

See 6) above. BaseConnection and BaseCommand are in a "pre-refactored" state.

>
> 13) Lots and lots of "magic strings".
>
> 14) The partial fetch (allowing one to specify octets to be fetched) is
> not implemented.
>
> 15) The highest UID stuff is not currently implemented for mailboxes -
> highest UID is null on start.

Yeah, yeah, yeah... to these. ;)

>
> This is the "off the top of my head" list - I think there were a few
> other issues I ran into.  I'll have to look at the code again to be
> sure.  Any other issues you're aware of?

a) Every command without an rfc protocol test doesn't work.
b) Some of the commands with rfc protocol tests may not work.
    - tests are incomplete
    - tests fail
c) Many browsers may not work, even when the rfc protocol tests do.
d) Licence headers need to be full Apache licence (I think).
e) Test suite should be updated to use regular expressions rather than dinky 
custom codes. (All my fault, I'm afraid).

Plenty to keep us busy, anyhow.
-- 
ciao,
Daz

--
To unsubscribe, e-mail:   <mailto:james-dev-unsubscribe@jakarta.apache.org>
For additional commands, e-mail: <mailto:james-dev-help@jakarta.apache.org>


Mime
View raw message