Details

    • Type: Sub-task Sub-task
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: api
    • Labels:
      None

      Description

      Make the contract of org.apache.james.mailbox.model.MailboxPath explicit and documented.

      Points to consider:

      (1) Make all attributes immutable - can be done quite easily, no discussion expected.

      (2) Null values should be forbidden for at least for name and namespace attributes - NullPointerException will be thrown from constructors. A lot of client code uses e.g. getName().length(). Clients should be responsible for the proper values when creating new instances of MailboxPath.

      (3) Consider adding a delimiter field. Currently delimiter is implicitly present in the name value anyway. It would allow:

      (3.1) removing the delimiter parameter from getHierarchyLevels()

      (3.2) adding relativePath(relativeName) method (which is currently done by concatenating path segments with bona fide default delimiter in the client code).

      (3.3) name attribute could guarrantee that it does not start with delimiter (which is sometimes checked in the client code now). For names starting with delimiter, either

      (3.3.a) constructors should throw IllegalStateException or

      (3.3.b) the name attribute would be rewritten silently - should perhaps be prefered, because of the existing client code.

      (3.4) Currently, there is no delimiter escaping/unescaping (e.g. in parse() or getHierarchyLevels()). Should there be any?

        Activity

        Hide
        Jochen Gazda added a comment -

        Eric,

        > what does the rfc says regarding "" as namespace ?

        I have no exact reference in my head but Cyrus uses "" for shared namespace.

        > not sure, but is the caller gives a null, that's not good, and maybe we should runtimeexception

        Yes, for both name and namespace attributes, I propose NPE.

        > or maybe you have a usecase for null parameters?

        I do not have any. Client code should be fixed.

        > +1 for immutability.

        Show
        Jochen Gazda added a comment - Eric, > what does the rfc says regarding "" as namespace ? I have no exact reference in my head but Cyrus uses "" for shared namespace. > not sure, but is the caller gives a null, that's not good, and maybe we should runtimeexception Yes, for both name and namespace attributes, I propose NPE. > or maybe you have a usecase for null parameters? I do not have any. Client code should be fixed. > +1 for immutability.
        Hide
        Ioan Eugen Stan added a comment -

        Hi Gazda,

        Nice.Overall +1. See my comments:

        1. Please do, I think you can set them to final and remove setters. I don't know how things will impact the existing code but we should fix things if they are broken.
        2. From some time now I prefer to use guava's Preconditions. I find it makes the code clear and small. Guava also has some nice way of generating good equals and hashcode, etc.
        We should fail fast if something wrong.
        3. Agree, please consider guava Preconditions again.
        3.4 Don't know

        As a final note I like when I can count on certain things staying the same no matter what.

        Show
        Ioan Eugen Stan added a comment - Hi Gazda, Nice.Overall +1. See my comments: 1. Please do, I think you can set them to final and remove setters. I don't know how things will impact the existing code but we should fix things if they are broken. 2. From some time now I prefer to use guava's Preconditions. I find it makes the code clear and small. Guava also has some nice way of generating good equals and hashcode, etc. We should fail fast if something wrong. 3. Agree, please consider guava Preconditions again. 3.4 Don't know As a final note I like when I can count on certain things staying the same no matter what.
        Hide
        Jochen Gazda added a comment -

        Thanks for your comments, Ioan.

        I have completed (1) and (2).
        I have chosen not to fulfill (3) as it would result in adding delimiter attribute to org.apache.james.mailbox.store.mail.model.Mailbox (along with its persistent implementors) as well.

        MailboxPath immutability in combination with no delimiter attribute was the most difficult to solve in org.apache.james.mailbox.store.StoreMailboxManager.createMailbox(MailboxPath, MailboxSession) where the name-ends-with-delimiter check was done and when positive then setName() was called on a method-external instance of MailboxPath. I have chosen to throw a MailboxException in that case. There were too many callers of StoreMailboxManager.createMailbox() to find out in which ones it can happen.

        I will commit together with MAILBOX-167.

        Show
        Jochen Gazda added a comment - Thanks for your comments, Ioan. I have completed (1) and (2). I have chosen not to fulfill (3) as it would result in adding delimiter attribute to org.apache.james.mailbox.store.mail.model.Mailbox (along with its persistent implementors) as well. MailboxPath immutability in combination with no delimiter attribute was the most difficult to solve in org.apache.james.mailbox.store.StoreMailboxManager.createMailbox(MailboxPath, MailboxSession) where the name-ends-with-delimiter check was done and when positive then setName() was called on a method-external instance of MailboxPath. I have chosen to throw a MailboxException in that case. There were too many callers of StoreMailboxManager.createMailbox() to find out in which ones it can happen. I will commit together with MAILBOX-167 .
        Hide
        Eric Charles added a comment -

        >> what does the rfc says regarding "" as namespace ?
        > I have no exact reference in my head but Cyrus uses "" for shared namespace.
        OK

        >> not sure, but is the caller gives a null, that's not good, and maybe we should runtimeexception
        > Yes, for both name and namespace attributes, I propose NPE.

        I would have thrown IllegalArgumentException

        >> or maybe you have a usecase for null parameters?
        > I do not have any. Client code should be fixed.

        Will you fix it? (when you say 'client', I guess you mean the other james modules).

        Show
        Eric Charles added a comment - >> what does the rfc says regarding "" as namespace ? > I have no exact reference in my head but Cyrus uses "" for shared namespace. OK >> not sure, but is the caller gives a null, that's not good, and maybe we should runtimeexception > Yes, for both name and namespace attributes, I propose NPE. I would have thrown IllegalArgumentException >> or maybe you have a usecase for null parameters? > I do not have any. Client code should be fixed. Will you fix it? (when you say 'client', I guess you mean the other james modules).
        Hide
        Jochen Gazda added a comment -

        >>> not sure, but is the caller gives a null, that's not good, and maybe we should runtimeexception
        >>
        >> Yes, for both name and namespace attributes, I propose NPE.
        >
        > I would have thrown IllegalArgumentException

        OK, be it IllegalArgumentException.

        >>> or maybe you have a usecase for null parameters?
        >
        >> I do not have any. Client code should be fixed.
        >
        > Will you fix it? (when you say 'client', I guess you mean the other james modules).

        Yes, I am doing my best now to fix the callers.

        Show
        Jochen Gazda added a comment - >>> not sure, but is the caller gives a null, that's not good, and maybe we should runtimeexception >> >> Yes, for both name and namespace attributes, I propose NPE. > > I would have thrown IllegalArgumentException OK, be it IllegalArgumentException. >>> or maybe you have a usecase for null parameters? > >> I do not have any. Client code should be fixed. > > Will you fix it? (when you say 'client', I guess you mean the other james modules). Yes, I am doing my best now to fix the callers.
        Hide
        Jochen Gazda added a comment -

        >> what does the rfc says regarding "" as namespace ?

        >I have no exact reference in my head but Cyrus uses "" for shared namespace.

        A small addendum of mine:
        It is our own business, what we choose as namespace prefixes. Currently we always send

        NAMESPACE (("" ".")) NIL NIL

        to IMAP clients; see NamespaceProcessor.buildPersonalNamespaces(MailboxSession, ImapSession)
        That means "" is the prefix for the personal namespace and there are no other namespaces.
        I.e. we effectively do not support namespaces at the moment.

        I propose that we abandon sending "" as a namespace prefix to clients from now on.
        NettyImapSession.supportMultipleNamespaces() should return true.
        Then IMAP clients should also cease to send "" namespaces to us and we can internally declare "" namespace prefix as invalid.

        Show
        Jochen Gazda added a comment - >> what does the rfc says regarding "" as namespace ? >I have no exact reference in my head but Cyrus uses "" for shared namespace. A small addendum of mine: It is our own business, what we choose as namespace prefixes. Currently we always send NAMESPACE (("" ".")) NIL NIL to IMAP clients; see NamespaceProcessor.buildPersonalNamespaces(MailboxSession, ImapSession) That means "" is the prefix for the personal namespace and there are no other namespaces. I.e. we effectively do not support namespaces at the moment. I propose that we abandon sending "" as a namespace prefix to clients from now on. NettyImapSession.supportMultipleNamespaces() should return true. Then IMAP clients should also cease to send "" namespaces to us and we can internally declare "" namespace prefix as invalid.
        Hide
        Jochen Gazda added a comment -

        The ability of the current James code to handle multiple namespaces is much worse than I hoped initially. I carry on the discussion here as MailboxPath is the medium used to transport the namespace information throughout James from IMAP to store backends and back.

        What I say here for MailboxPath also holds mutatis mutandis for org.apache.james.mailbox.store.mail.model.Mailbox which mirrors MailboxPath attributes. I will abbreviate org.apache.james.mailbox.store.mail.model.Mailbox as model.Mailbox to avoid confusion with "mailbox" in its common sense.

        The undelying problem seems to be the following:
        The exact meaning of the given IMAP mailbox name may strongly depend on its contextual interpretation. This has to do with namespace prefixes and (ii) the special name "INBOX".
        Example: When an IMAP client sends e.g. LIST "" "#private.folder.%", the mailbox name "#private.folder" does not have any definitive meaning untill it is combined with the name of the current user. Similarly "INBOX" is "the primary mailbox for this user on this server".

        Not only that there is no central place where this interpretation happens, much worse, one could say that this interpretation does not really happen: namespace prefixes and INBOX names are forwarded to storage backends which blindly (with minor exceptions) take them as parts of the given mailbox'es primary key.

        Example: when user "u1" asks for folder "#private.folder.subfolder", all storage backends basically do what the JPA storage does: SELECT mailbox FROM Mailbox mailbox WHERE mailbox.name = :nameParam and mailbox.user= :userParam and mailbox.namespace= :namespaceParam

        Please note that this makes no harm at present as only the single personal namespace is in play.

        Now, if we turn multiple namespaces on, look what will happen:
        Say that other users' mailboxes will be available to the current user "u1" under the namespace "#otherUsers". Say further that the currently set ACLs grant the user "u1" full rights to all folders of the user "u2". Now, say "u1" wants to pass his folder "#private.sales.orders" to the user "u2" (e.g. because "u1" gets retired). To make it happen, "u1" moves (i.e. uses IMAP RENAME command) "#private.sales.orders" to "#otherUsers.u2.sales.orders". What will the JPA storage backend do? - rougly the following:

        UPDATE Mailbox mailbox SET
        mailbox.name = 'u2.sales.orders'
        mailbox.user = null
        mailbox.namespace = '#otherUsers'
        WHERE mailbox.name = 'sales.orders'
        and mailbox.user = 'u1'
        and mailbox.namespace = '#private'

        Clearly, after this update, "u2" will not see "sales.orders" between his folders.
        Similar could be demonstrated also for COPY, LIST and probably also other IMAP commands.

        Here I am trying to propose a strategy how this problem should be addressed. Please comment.

        First of all there should be a central place where this mailbox name interpretation takes place. As "INBOX" and namespaces are IMAP notions, they should be interpreted in the IMAP module. Further, MailboxPath should be made absolute in its nature. And finally, the storage backends should play only with such absolute MailboxPaths.

        What I propose for MailboxPath and model.Mailbox:

        I still stick with (1) and (2) from my above post.

        I have abandoned (3), I rather prefer (8).

        (4) MailboxPath should be absolute, i.e. independent from any contextual interpretation:
        (4.1) independent from any "current folder" notion
        (4.2) independent from any "current user" notion
        Any contextual interpretation should take place outside of MailboxPath. Currently I see only IMAP as a place where any interpretation should take place; see (5).

        (5) A consequence of (4) is that namespace attribute should be removed from MailboxPath.
        Why: Namespace prefixes may be context dependent. The notion of namespace comes from IMAP. Any context dependency of namespace prefixes should be resolved in the IMAP module.
        Example: the exact meaning of a MailboxPath located in "personal" (a.k.a "private") namespace depends on the current user. So if an IMAP client authenticated as user "u1" is asking for folder "#personal.folder.subfolder", the folder name should be resolved by the IMAP module as e.g. "users.u1.folder.subfolder".
        The opposite direction: if a store backend returns "users.u1.folder.subfolder" to the IMAP module, the IMAP module should transform it to "#personal.folder.subfolder" if the current user is "u1".

        (6) INBOX: The same holds for the special folder name "INBOX": If an IMAP client authenticated as user "u1" is asking for folder "INBOX", the folder name should be resolved by the IMAP module as e.g. "users.u1".

        (7) Another consequence of (4) is that the user attribute can be removed from MailboxPath. It is not necessary for the interpretation of the meaning of the given MailboxPath.
        (7.1) However, the information who is the owner of the given folder should be stored in model.Mailbox.
        The owner attribute of model.Mailbox should allow for storing both users and groups as owners.
        (7.2) A new boolean attribute ownerGroup should be added to model.Mailbox storing the information whether the owner is a group or an individual user. This is needed for the interpretation of the "owner" ACL key.

        (8) MailboxPath should be delimiter agnostic: its attributes must not contain path delimiters. Currently the name attribute may contain path delimiters. Name should be replaced with an immutable collection of the given path's segments; i.e. "folder.subfolder" would be stored e.g. as new String[]

        {"folder", "subfolder}

        . Every module which handles with MailboxPaths should be responsible for the selection of the appropriate delimiter and serialization of the path. The IMAP module may choose to use '.' and a filesystem-based storage may choose '/' as a hierarchy delimiter.

        (9) All data stored by MailboxPath should be unescaped. Every module which handles with MailboxPaths (IMAP, Storage, Sieve, ...) should be responsible for its own escaping/unescaping, e.g. to avoid conflicts with the hierarchy delimiter it has chosen.

        Show
        Jochen Gazda added a comment - The ability of the current James code to handle multiple namespaces is much worse than I hoped initially. I carry on the discussion here as MailboxPath is the medium used to transport the namespace information throughout James from IMAP to store backends and back. What I say here for MailboxPath also holds mutatis mutandis for org.apache.james.mailbox.store.mail.model.Mailbox which mirrors MailboxPath attributes. I will abbreviate org.apache.james.mailbox.store.mail.model.Mailbox as model.Mailbox to avoid confusion with "mailbox" in its common sense. The undelying problem seems to be the following: The exact meaning of the given IMAP mailbox name may strongly depend on its contextual interpretation. This has to do with namespace prefixes and (ii) the special name "INBOX". Example: When an IMAP client sends e.g. LIST "" "#private.folder.%", the mailbox name "#private.folder" does not have any definitive meaning untill it is combined with the name of the current user. Similarly "INBOX" is "the primary mailbox for this user on this server". Not only that there is no central place where this interpretation happens, much worse, one could say that this interpretation does not really happen: namespace prefixes and INBOX names are forwarded to storage backends which blindly (with minor exceptions) take them as parts of the given mailbox'es primary key. Example: when user "u1" asks for folder "#private.folder.subfolder", all storage backends basically do what the JPA storage does: SELECT mailbox FROM Mailbox mailbox WHERE mailbox.name = :nameParam and mailbox.user= :userParam and mailbox.namespace= :namespaceParam Please note that this makes no harm at present as only the single personal namespace is in play. Now, if we turn multiple namespaces on, look what will happen: Say that other users' mailboxes will be available to the current user "u1" under the namespace "#otherUsers". Say further that the currently set ACLs grant the user "u1" full rights to all folders of the user "u2". Now, say "u1" wants to pass his folder "#private.sales.orders" to the user "u2" (e.g. because "u1" gets retired). To make it happen, "u1" moves (i.e. uses IMAP RENAME command) "#private.sales.orders" to "#otherUsers.u2.sales.orders". What will the JPA storage backend do? - rougly the following: UPDATE Mailbox mailbox SET mailbox.name = 'u2.sales.orders' mailbox.user = null mailbox.namespace = '#otherUsers' WHERE mailbox.name = 'sales.orders' and mailbox.user = 'u1' and mailbox.namespace = '#private' Clearly, after this update, "u2" will not see "sales.orders" between his folders. Similar could be demonstrated also for COPY, LIST and probably also other IMAP commands. Here I am trying to propose a strategy how this problem should be addressed. Please comment. First of all there should be a central place where this mailbox name interpretation takes place. As "INBOX" and namespaces are IMAP notions, they should be interpreted in the IMAP module. Further, MailboxPath should be made absolute in its nature. And finally, the storage backends should play only with such absolute MailboxPaths. What I propose for MailboxPath and model.Mailbox: I still stick with (1) and (2) from my above post. I have abandoned (3), I rather prefer (8). (4) MailboxPath should be absolute, i.e. independent from any contextual interpretation: (4.1) independent from any "current folder" notion (4.2) independent from any "current user" notion Any contextual interpretation should take place outside of MailboxPath. Currently I see only IMAP as a place where any interpretation should take place; see (5). (5) A consequence of (4) is that namespace attribute should be removed from MailboxPath. Why: Namespace prefixes may be context dependent. The notion of namespace comes from IMAP. Any context dependency of namespace prefixes should be resolved in the IMAP module. Example: the exact meaning of a MailboxPath located in "personal" (a.k.a "private") namespace depends on the current user. So if an IMAP client authenticated as user "u1" is asking for folder "#personal.folder.subfolder", the folder name should be resolved by the IMAP module as e.g. "users.u1.folder.subfolder". The opposite direction: if a store backend returns "users.u1.folder.subfolder" to the IMAP module, the IMAP module should transform it to "#personal.folder.subfolder" if the current user is "u1". (6) INBOX: The same holds for the special folder name "INBOX": If an IMAP client authenticated as user "u1" is asking for folder "INBOX", the folder name should be resolved by the IMAP module as e.g. "users.u1". (7) Another consequence of (4) is that the user attribute can be removed from MailboxPath. It is not necessary for the interpretation of the meaning of the given MailboxPath. (7.1) However, the information who is the owner of the given folder should be stored in model.Mailbox. The owner attribute of model.Mailbox should allow for storing both users and groups as owners. (7.2) A new boolean attribute ownerGroup should be added to model.Mailbox storing the information whether the owner is a group or an individual user. This is needed for the interpretation of the "owner" ACL key. (8) MailboxPath should be delimiter agnostic: its attributes must not contain path delimiters. Currently the name attribute may contain path delimiters. Name should be replaced with an immutable collection of the given path's segments; i.e. "folder.subfolder" would be stored e.g. as new String[] {"folder", "subfolder} . Every module which handles with MailboxPaths should be responsible for the selection of the appropriate delimiter and serialization of the path. The IMAP module may choose to use '.' and a filesystem-based storage may choose '/' as a hierarchy delimiter. (9) All data stored by MailboxPath should be unescaped. Every module which handles with MailboxPaths (IMAP, Storage, Sieve, ...) should be responsible for its own escaping/unescaping, e.g. to avoid conflicts with the hierarchy delimiter it has chosen.
        Hide
        Ioan Eugen Stan added a comment -

        Wow, you definitely took some time to analyze this. I also found Mailnbox and MailboxPath handling a bit fishy but didn't do anything. I ill take some time to look into this and come back with feed-back and impressions. Please add the rfc's and pages used for documentation as reference so we will all follow.

        Show
        Ioan Eugen Stan added a comment - Wow, you definitely took some time to analyze this. I also found Mailnbox and MailboxPath handling a bit fishy but didn't do anything. I ill take some time to look into this and come back with feed-back and impressions. Please add the rfc's and pages used for documentation as reference so we will all follow.
        Show
        Jochen Gazda added a comment - Ioan, I see only http://www.apps.ietf.org/rfc/rfc2342.html and http://tools.ietf.org/html/rfc3501 esp. http://tools.ietf.org/html/rfc3501#section-5.1.1 as relevant for this discussion.

          People

          • Assignee:
            Jochen Gazda
            Reporter:
            Jochen Gazda
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:

              Development