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 Sun, 15 Jul 2012 09:33:53 GMT
Hi,

Quick fact: you can also do review with inline comments on Github by
clicking on the plus icon on the left while viewing the commit.

I think comments are more valuable there because you can put them next
to the code and add context to the words.
This is old, but I just found out and figured it might help.

Cheers,

2012/7/10 Ioan Eugen Stan <stan.ieugen@gmail.com>:
> 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



-- 
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