james-server-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ioan Eugen Stan <stan.ieu...@gmail.com>
Subject Re: Improved MailboxPath and how to handle the resulting change set
Date Mon, 09 Jul 2012 22:15:26 GMT
Hello Gazda,

I promised I'll do a review but unfortunately I don't have the time so
no use on waiting on me.

Cheers,

2012/6/29 Eric Charles <eric@apache.org>:
> comments inline, Eric
>
>
> On 06/28/2012 11:02 PM, Jochen Gazda wrote:
>>
>> Hi Eric,
>>
>> see inline.
>>
>> Best,
>>
>> gazda
>>
>> On Thu, Jun 28, 2012 at 4:57 PM, Eric Charles<eric@apache.org>  wrote:
>>>
>>> On 06/28/2012 04:00 PM, Jochen Gazda wrote:
>>>>>
>>>>>
>>>>> 1.- profile noTest on mailbox-integration-tester: ok
>>>>>
>>>>> 2.- LocalAndVirtualMailboxLocatorChain: what's the goal?
>>>>
>>>>
>>>>
>>>> MailDir is a file system based storage. There is the MaildirLocator
>>>> interface for mapping of MailboxNames to file system directories and
>>>> back. There are two basic MaildirLocator implementations:
>>>> LocalSystemMaildirLocator (maps to /home/<user>/MailDir) and
>>>> VirtualMailboxLocator (maps to<virtualRoot>/<domain>/<user>).
The
>>>> third one, LocalAndVirtualMailboxLocatorChain, cobines the two.
>>>>
>>>
>>> Ok, got it.
>>> (naming is just not consistent across LocalSystemMaildirLocator /
>>> VirtualMailboxLocator / LocalAndVirtualMailboxLocatorChain (you have
>>> Maildir
>>> in the first, not in the others).
>>>
>>>
>>>
>>>>> 3.- MailboxPath is now MailboxName: Path sounded more like
>>>>> folder/subfolder/subfolder.
>>>>
>>>>
>>>>
>>>> Yes, I agree, that is the case for common English. But there is no
>>>> single occurence of 'path' in RFC3501. It speaks only about
>>>> (hierarchical) names. On the other hand, MailboxPath/MailboxName is
>>>> our internal term which does not map 1:1 to the IMAP hierachical name.
>>>> We can name it however we want. I am not against going back to
>>>> MailboxPath.
>>>>
>>>
>>> The key is 'hierarchical'. I find MailboxPath correctly reflect this
>>> point.
>>> With MailboxName, you loose that, and MailboxHierarchicalName is just too
>>> long.
>>>
>>> At first sight, I would prefer to revert to MailboxPath.
>>
>>
>> There are several MailboxName* classes. I will have a look if renaming
>> them all to MailboxPath* sounds plausible to me.
>>
>>>>> 4.- MailboxNameResolver on MailboxManager
>>>>
>>>>
>>>> You mean that the methods from MailboxNameResolver should better move
>>>> to MailboxManager? - Well, MailboxManager is defines storage
>>>> operations, but MailboxNameResolver interprets names. That is
>>>> something different. Two distinct MailboxNameResolvers are conceivable
>>>> for a single MailboxManager; see (5) in my previous post (
>>>> /users/<username>    vs. /users/<username>/INBOX ).
>>>>
>>>
>>> I was just pointing an additional method MailboxNameResolver
>>> getMailboxNameResolver(); on the MailboxManager interface.
>>>
>>> So good as is.
>>>
>>>
>>>>> 5.- MailboxSession has no more PersonalSpace nor UserSpace but a
>>>>> MailboxOwner.
>>>>
>>>>
>>>>
>>>> The capability to list namespace prefixes has moved from
>>>> MailboxSession to MailboxNameResolver, which is now a member of
>>>> MailboxSession. Listing namespace prefixes belongs to the mailbox
>>>> hierarchy definition entailed in MailboxNameResolver.
>>>>
>>>
>>> OK
>>>
>>>
>>>> MailboxOwner is needed to distinguish groups from persons and 'normal'
>>>> (domain-less) users from virtual users.
>>>>
>>>
>>> So you assume a mailbox has always an owner.
>>> Sounds logical, but is it sustained by the RFC?
>>
>>
>> I have done it that way without thinking of RFC. It is quite practical
>> to have it like that because one can set reasonable default ACLs which
>> rely on the notion of owner. Dovecot and Cyrus do that. From
>> http://wiki.dovecot.org/ACL :
>>
>>> The default ACL for mailboxes is to give the mailbox
>>> owner all permissions and other users none. Mailboxes
>>> in public namespaces don't have owners, so by default
>>> no-one can access them.
>>
>>
>> To have owners on mailboxes is surely not against the spirit of RFC
>> 4314, which names owner in one of its examples:
>>
>>> For example,
>>> in an implementation that uses UNIX mode bits, the rights "swite" are
>>> tied, the "a" right is always granted to the owner of a mailbox and
>>> is never granted to another user.
>>
>>
>> It would probably be possible to do without mailbox owners but it
>> seems really impractical to me.
>>
>
> From your previous explanation, it sounds like MailboxOvwner is a good fit
> for us (specifically for ACL and potentially for the rest).
>
>
>>>>> 8. mmh, MailboxPath is still there.
>>>>
>>>>
>>>>
>>>> Yes, it is still there because there are still references to it from
>>>> mailets project which I was not able to fix.
>>>> Otherwise there are only refs in comments which can generally be
>>>> replaced by MailboxName.
>>>>
>>>
>>> Would it be solved if we revert from MailboxName to MailboxPath?
>>
>>
>> Yes, perhaps.
>>
>>>>> 9. MailboxNameSerializer, MailboxNameBuilder, MailboxNameCodec,
>>>>> MailboxNamespaceType, MailboxNameEscaper
>>>>
>>>>
>>>>
>>>> MailboxNameSerializer belongs to my first attempts. It could be made
>>>> parent of MailboxNameCodec or replaced altogether. As for
>>>> MailboxNameCodec cf. (6) of my previous post.
>>>> MailboxNameBuilder - allows for saving some memory and string-copying
>>>> when creating a new MailboxName.
>>>>
>>>> MailboxNameEscaper - ancillary interface for MailboxNameCodec.
>>>> MailboxNameCodec implementations mostly differ only in the
>>>> MailboxNameEscaper they use.
>>>>
>>>
>>> I need to read back all this.
>>> Is the goal of all these classes to better support mailbox name charsets
>>> and
>>> to build the hierachical mailbox name structure in a efficient way?
>>
>>
>> No, currently it is not about mailbox name charsets, though
>> MailboxNameCodec could serve also that purpose.
>> MailboxNameCodec (and its usual member MailboxNameEscaper) are about
>> parsing raw string names like "INBOX/asf" to segment collections like
>> {"INBOX", "asf"} and serializing those collections back to strings.
>> Their central task (when decoding) is to recognoze segment boundaries.
>> For that purpose they define the delimiter (e.g. '/') that will be
>> used for the given purpose.
>>
>
> So MaiboxNameCodec has two responsibilities:
> - Tokenize a 'Path' to an ordered list of 'PathSegment' (or 'PathToken')
> - Build a 'Path' from an ordered list of 'PathSegment' (or 'PathToken')
>
> But we need to take car to not over-engineer, after all, PathSegment/Token
> are just String.
>
> Still the fact is that it didn't popup to me reading the name
> MailboxNameCodec. I also like the mantra 'one class, one responsibility', so
> having two different classes coulb be an option (MailboxPathTokenizer and
> MailboxPathBuilder). I am also fine leaving as such the MailboxNameCodec (or
> MailboxPathCodec depending on your decision to revert on not to
> MailboxPath).
>
>
>>>>> 10. LikeSearchPatternEscaper: why deal with JCR, SQL in the API?
>>>>
>>>>
>>>>
>>>> It is not API, but common to many API consumers. Can you see a better
>>>> place for it?
>>>>
>>>
>>> I would prefer not to have them in the API, but we can leave it as such
>>> for
>>> now.
>>>
>>>
>>>>> 11. More and more unit tests... :)
>>>>
>>>>
>>>>
>>>> Finite verb?
>>>>
>>>
>>> ... is good :)
>>>
>>>
>>>>
>>>> Well, yes, definitely more typing - that is con.
>>>> Pros are:
>>>>   - Handling mailbox names more reliably
>>>>   - Mailbox names are interpreted on one central place.
>>>>   - Namespace prefixes other than personal are now possible, esp. group
>>>> folders are possible
>>>>
>>>
>>> As con, you will end with a few more object to instantiate, mainly on
>>> mailbox selection, but IMHO I this won't be an issue at all
>>>
>>>
>>>>> To be honest, I didn't see well if and how the ACL are applied... :)
>>>>
>>>>
>>>>
>>>> New in my proposal is only that the ACLs can now be stored in JPA, JCR
>>>> and HBase in addition to MailDir. They are still not enforced though.
>>>> https://issues.apache.org/jira/browse/IMAP-358 is still open for that
>>>> matter.
>>>>
>>>
>>> OK, with these changes, you are now in a better place to implement the
>>> ACL.
>>>
>>>
>>>>> [1]
>>>>>
>>>>>
>>>>> https://github.com/gazdahimself/current/commit/ae75a54400bc7aa93763657a48596d09ec357f98,
>>>>
>>>>
>>>>
>>>> [1] is now outdated, one can use
>>>> https://github.com/gazdahimself/current/compare/master...MAILBOX-175
>>>> to see the relevant diffs.
>>>>
>>>>
>>>>>
>>>>> On 06/27/2012 06:55 PM, Jochen Gazda wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> Gentlemen,
>>>>>>
>>>>>> I have finally managed it to publish my changeset on GitHub:
>>>>>> https://github.com/gazdahimself/current/tree/MAILBOX-175
>>>>>> The state of my brach MAILBOX-175 is in sync with the currently latest
>>>>>> svn revision 1354581. My brach MAILBOX-175 can also be diffed against
>>>>>> svn revision 1354581 directly on GitHub.
>>>>>>
>>>>>> Sorry for the delay, I am new to git and I have a new job.
>>>>>>
>>>>>> Please comment.
>>>>>>
>>>>>> Best,
>>>>>>
>>>>>> gazda
>>>>>>
>>>>>> On Wed, Jun 13, 2012 at 1:51 PM, Ioan Eugen
>>>>>> Stan<stan.ieugen@gmail.com>
>>>>>>   wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> HI Gazda,
>>>>>>>
>>>>>>> Git is great.
>>>>>>>
>>>>>>> Cheers,
>>>>>>>
>>>>>>> 2012/6/13 Eric Charles<eric@apache.org>:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Hi Gazda,
>>>>>>>>
>>>>>>>> I'm fine with the push to your personal github. It offers
nice ui to
>>>>>>>> show
>>>>>>>> diffs.
>>>>>>>>
>>>>>>>> Thx, Eric
>>>>>>>>
>>>>>>>>
>>>>>>>> On 06/13/2012 10:32 AM, Jochen Gazda wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Gentlemen,
>>>>>>>>>
>>>>>>>>> I could invest about 6 weeks of my time into solving
>>>>>>>>> https://issues.apache.org/jira/browse/MAILBOX-175 and
>>>>>>>>> https://issues.apache.org/jira/browse/MAILBOX-167.
>>>>>>>>> The result is quite a huge and deep changeset. There
is a de facto
>>>>>>>>> replacement for MailboxPath - so you can imagine, how
many classes
>>>>>>>>> were changed.
>>>>>>>>>
>>>>>>>>> Before going too much into details I wanted to agree
on a way how
>>>>>>>>> my
>>>>>>>>> changeset could find its way into SVN.
>>>>>>>>>
>>>>>>>>> The changes should be discussed before they are committed
to trunk.
>>>>>>>>> But I am not sure what is the best way to share my changes
before
>>>>>>>>> they
>>>>>>>>> are committed to trunk.
>>>>>>>>> Would cloning from http://git.apache.org/ and pushing
changes to my
>>>>>>>>> personal GitHub repo be a viable solution?
>>>>>>>>> I am also ready to send my zipped workspace (~30MB) to
anybody who
>>>>>>>>> wants to have a quick look.
>>>>>>>>>
>>>>>>>>> Best,
>>>>>>>>>
>>>>>>>>> gazda
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> ---------------------------------------------------------------------
>>>>>>>>> To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
>>>>>>>>> For additional commands, e-mail: server-dev-help@james.apache.org
>>>>>>>>>
>>>>>>>>
>>>>>>>> --
>>>>>>>> eric | http://about.echarles.net | @echarles
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> ---------------------------------------------------------------------
>>>>>>>> To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
>>>>>>>> For additional commands, e-mail: server-dev-help@james.apache.org
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> --
>>>>>>> Ioan Eugen Stan / http://axemblr.com / Tools for Clouds
>>>>>>>
>>>>>>> ---------------------------------------------------------------------
>>>>>>> To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
>>>>>>> For additional commands, e-mail: server-dev-help@james.apache.org
>>>>>>>
>>>>>>
>>>>>> ---------------------------------------------------------------------
>>>>>> To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
>>>>>> For additional commands, e-mail: server-dev-help@james.apache.org
>>>>>>
>>>>>
>>>>> --
>>>>> eric | http://about.echarles.net | @echarles
>>>>>
>>>>> ---------------------------------------------------------------------
>>>>> To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
>>>>> For additional commands, e-mail: server-dev-help@james.apache.org
>>>>>
>>>>
>>>> ---------------------------------------------------------------------
>>>> To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
>>>> For additional commands, e-mail: server-dev-help@james.apache.org
>>>>
>>>
>>> --
>>> eric | http://about.echarles.net | @echarles
>>>
>>> ---------------------------------------------------------------------
>>> To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
>>> For additional commands, e-mail: server-dev-help@james.apache.org
>>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
>> For additional commands, e-mail: server-dev-help@james.apache.org
>>
>
> --
> eric | http://about.echarles.net | @echarles
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
> For additional commands, e-mail: server-dev-help@james.apache.org
>



-- 
Ioan Eugen Stan / CTO / http://axemblr.com

---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org


Mime
View raw message