MAILBOX-310 MailboxManager::search and delegation #1011

chibenwa · 2017-10-04T09:35:54Z

This PR includes:

Fix of Cassandra UID and MODSEQ (faster tests, less changes when modifying Mappers)
Heavy refactoring of MailboxQuery
Some ACL filtering
Wiring and logic for delegated search.

Enjoy.

…tructor

chibenwa · 2017-10-04T11:43:50Z

test this please

strokyl · 2017-10-04T14:03:19Z

mailbox/api/src/test/java/org/apache/james/mailbox/model/MailboxACLTest.java

 /**
 * @author Peter Palaga
 */
 public class MailboxACLTest {

    private static final String USER_1 = "user1";
    private static final String USER_2 = "user2";
+    private static final boolean NEGATIVE = true;


NEGATIVE = true this is weird

You're peaky. what I am supposed to do with this?

You can let it like that, I just wanted to mention it's a bit weird but I don't see any overkill solution to it

rouazana · 2017-10-04T15:28:54Z

mailbox/api/src/main/java/org/apache/james/mailbox/acl/ACLChanged.java

+
+import org.apache.james.mailbox.model.MailboxACL;
+
+public class ACLChanged {


it is not necessary to specify that this class only handles user positive entries?

rouazana · 2017-10-04T16:58:12Z

...dra/src/main/java/org/apache/james/mailbox/cassandra/mail/CassandraUserMailboxRightsDAO.java

+            .and(eq(MAILBOX_ID, bindMarker(MAILBOX_ID))));
+    }
+
+    private PreparedStatement prepareSelectUser(Session session) {


SelectAllForUser?

rouazana · 2017-10-04T16:58:57Z

...x/cassandra/src/main/java/org/apache/james/mailbox/cassandra/modules/CassandraAclModule.java

+                        .compactionOptions(SchemaBuilder.leveledStrategy())
+                        .caching(SchemaBuilder.KeyCaching.ALL,
+                            SchemaBuilder.rows(CassandraConstants.DEFAULT_CACHED_ROW_PER_PARTITION))
+                        .comment("Denormalisation table. Allow to retrieve tnon personnal mailboxId a user has right on")));


s/tnon/non/
s/mailboxId/mailboxIds

rouazana · 2017-10-04T17:00:42Z

...src/test/java/org/apache/james/mailbox/cassandra/mail/CassandraUserMailboxRightsDAOTest.java

+    }
+
+    @Test
+    public void retrieveUserShouldReturnEmptyWhenEmptyData() throws Exception {


s/retrieveUser/listRightsForUser/

rouazana · 2017-10-04T17:06:15Z

mailbox/cassandra/src/main/java/org/apache/james/mailbox/cassandra/mail/CassandraACLMapper.java

-                            .map(aclTransformation)
-                            .map(aclWithVersion -> updateStoredACL(cassandraId, aclWithVersion))
-                            .orElseGet(() -> insertACL(cassandraId, replacement));
+                            .map(value ->


value? aclWithVersion was a better naming, no?

This value was already used before I refactored. Changed.

rouazana · 2017-10-04T17:45:55Z

mailbox/store/src/main/java/org/apache/james/mailbox/store/StoreMailboxManager.java

@@ -100,6 +101,7 @@
 */
 public class StoreMailboxManager implements MailboxManager {
    private static final Logger LOGGER = LoggerFactory.getLogger(StoreMailboxManager.class);
+    public static final char SQL_WILDCARD_CHAR = '%';


public before private :)

but honestly, I'm wondering if we should not authorize LOGGER to be always the first line, even if private

Me I prefer Logger to be in the first line, as it's pretty generic.

rouazana · 2017-10-04T17:54:11Z

mailbox/api/src/test/java/org/apache/james/mailbox/model/PrefixedRegexTest.java

+    }
+
+    @Test
+    public void IsWildShouldReturnTrueWhenLocalWildcardAtBeginning() throws Exception {


s/Is/is/ everywhere in this file

rouazana · 2017-10-04T17:55:16Z

mailbox/api/src/test/java/org/apache/james/mailbox/model/PrefixedRegexTest.java

+    }
+
+    @Test
+    public void getCombinedNameShouldWork() throws Exception {


bad test name, but it's copy / pasted from the refactoring, so I won't ask it to be changed for now

Thanks, that's kind. I got the impression I did the refactoring quota :-)

rouazana · 2017-10-04T17:56:04Z

mailbox/api/src/test/java/org/apache/james/mailbox/model/PrefixedRegexTest.java

+        boolean actual = testee.isExpressionMatch("folder\\123");
+
+        assertThat(actual).isTrue();
+    }


I did not check each test, I assume it's just a copy paster from previous regexp tests

That's exactly this ;-)

rouazana · 2017-10-04T18:03:29Z

mailbox/store/src/test/java/org/apache/james/mailbox/store/StoreMailboxManagerTest.java

@@ -176,8 +177,7 @@ public void getPathLikeShouldReturnUserPathLikeWhenNoBaseDefined() throws Except
        //Given
        MailboxSession session = new MockMailboxSession("user");
        MailboxQuery.Builder testee = MailboxQuery.builder()
-            .expression("abc")
-            .mailboxSession(session);
+            .expression(new PrefixedRegex("", "abc", session.getPathDelimiter()));


what is this empty string? can you name it?

No prefix, regex only ;-)

I guess the exact one might be used...

…ible (non personnal) mailboxes

- Allow missing namespace and username in mailboxQuery - Matching logic SHOULD be moved in MailboxQuery

…mailboxes

…nymore on MailboxPath for its internal representation

…come after static definitions

…atch

…ompulsory to build MailboxQuery

…xternal class hierarchy

…Mailbox search related classes

…ailboxNameExpression

…ilboxes

…ace search filtering on the manager layer

mbaechler · 2017-10-05T08:20:14Z

mailbox/store/src/main/java/org/apache/james/mailbox/store/StoreMailboxManager.java

+    }
+
+    private boolean isReadable(MailboxSession session, Mailbox mailbox) throws MailboxException {
+        return (Objects.equals(mailbox.getUser(), session.getUser().getUserName())


too many conditions here IMO

mbaechler · 2017-10-05T08:26:17Z

mailbox/api/src/main/java/org/apache/james/mailbox/acl/ACLChanged.java

+    }
+
+    public Stream<MailboxACL.Entry> addedEntries() {
+        Map<MailboxACL.EntryKey, MailboxACL.Rfc4314Rights> oldEntries = oldACL.ofPositiveNameType(MailboxACL.NameType.user);


diff feature should not belong to this class because we will need it really often

mbaechler · 2017-10-05T08:31:40Z

mailbox/api/src/main/java/org/apache/james/mailbox/model/search/MailboxQuery.java

-            this.pathDelimiter = Optional.of(pathDelimiter);
+        
+        public Builder matchesAllMailboxNames() {
+            this.mailboxNameExpression = Optional.of(new Wildcard());


Wildcard should be a Singleton

mbaechler · 2017-10-05T08:33:12Z

...webadmin-mailbox/src/test/java/org/apache/james/webadmin/routes/UserMailboxesRoutesTest.java

@@ -349,7 +349,8 @@ public void putShouldReturnOkWhenIssuedTwoTimes() {
        @Test
        public void putShouldAddAMailbox() {
            with()
-                .put(MAILBOX_NAME);
+                .put(MAILBOX_NAME)
+            .prettyPeek();


sysout spotted

…ly on MailboxNameExpression

chibenwa

👍 for @aduprat changes

strokyl · 2017-10-04T14:44:04Z

mailbox/api/src/main/java/org/apache/james/mailbox/model/MailboxQuery.java

-    private final char pathDelimiter;
-
-    private final Pattern pattern;
+    public static final char SQL_WILDCARD_CHAR = '%';


Why do we have sql logic here ?

this part of the code was not changed, just moved

rouazana

did not check the rebase though

mbaechler · 2017-10-05T09:44:19Z

mailbox/api/src/main/java/org/apache/james/mailbox/model/search/MailboxQuery.java

@@ -95,7 +96,7 @@ public Builder expression(MailboxNameExpression expression) {
        }

        public Builder matchesAllMailboxNames() {
-            this.mailboxNameExpression = Optional.of(new Wildcard());
+            this.mailboxNameExpression = Optional.of(WILDCARD);


it's not what I meant, Wildcard whould not have a public constructor as it's always the exact same object.

Could you handle this?

…ly on MailboxNameExpression

mbaechler · 2017-10-05T18:03:25Z

merged

aduprat added 2 commits October 4, 2017 16:29

MAILBOX-310 Mailbox query refactoring: Don't expose MailboxQuery cons…

434e040

…tructor

MAILBOX-310 MailboxACL should present only users ACL

929f31d

This was referenced Oct 4, 2017

WIP MailboxManager::search #1008

Closed

WIP : try to define a consistent SearchQuery.Builder that make sense #1009

Closed

strokyl reviewed Oct 4, 2017

View reviewed changes

rouazana reviewed Oct 4, 2017

View reviewed changes

quynhn approved these changes Oct 5, 2017

View reviewed changes

rouazana approved these changes Oct 5, 2017

View reviewed changes

aduprat approved these changes Oct 5, 2017

View reviewed changes

chibenwa and others added 20 commits October 5, 2017 09:48

MAILBOX-310 ACLChanged should represent the diff between two ACLs

3192535

MAILBOX-310 CassandraUserMailboxRightsDAO should store user -> access…

e55b371

…ible (non personnal) mailboxes

MAILBOX-310 Mapper should store the ACL by user/mailbox

78f208c

MAILBOX-310 Refactor mailbox search

42e200d

- Allow missing namespace and username in mailboxQuery - Matching logic SHOULD be moved in MailboxQuery

MAILBOX-310 MailboxManager::search should allow retrieving delegated …

04d3388

…mailboxes

MAILBOX-310 Introduce ACL capability

d19f88f

MAILBOX-310 Cassandra modseq and uid test don't need mailbox mapper

fa04dab

MAILBOX-310 Mailbox query refactoring: MailboxQuery should not rely a…

8c0edd7

…nymore on MailboxPath for its internal representation

MAILBOX-310 Mailbox query refactoring: Reorder fields. Fields should …

3986c33

…come after static definitions

MAILBOX-310 Mailbox query refactoring: Slightly improve isExpressionM…

1a7bcf2

…atch

MAILBOX-310 Mailbox query refactoring: MailboxSession should not be c…

e8b5c8a

…ompulsory to build MailboxQuery

MAILBOX-310 Mailbox query refactoring: Reindent isWild

087bed4

MAILBOX-310 Mailbox query refactoring: Use matchesAll more

fa43bc1

MAILBOX-310 Mailbox query refactoring: Remove javadoc for toString

99dcb1e

MAILBOX-310 Mailbox query refactoring: Copy mailboxPath matching in e…

9b38fc3

…xternal class hierarchy

MAILBOX-310 Mailbox query refactoring: Create dedicated packages for …

080d04b

…Mailbox search related classes

MAILBOX-310 Mailbox query refactoring: Mailbox query should rely on M…

200d8cd

…ailboxNameExpression

MAILBOX-310 Propose a more explicit wording for MailboxMapper::findMa…

5bff630

…ilboxes

MAILBOX-310 Correct WebAdmin mailbox handling

7cdc63f

MAILBOX-310 Adding additional tests demonstrating per username/namesp…

b70167d

…ace search filtering on the manager layer

mbaechler force-pushed the MAILBOX-310 branch from 7504b93 to b70167d Compare October 5, 2017 08:37

mbaechler reviewed Oct 5, 2017

View reviewed changes

aduprat added 3 commits October 5, 2017 10:57

fixup! MAILBOX-310 Refactor mailbox search

50b6c2f

fixup! MAILBOX-310 Mailbox query refactoring: Mailbox query should re…

d50df3b

…ly on MailboxNameExpression

fixup! MAILBOX-310 Correct WebAdmin mailbox handling

5227fdb

chibenwa commented Oct 5, 2017

View reviewed changes

strokyl reviewed Oct 5, 2017

View reviewed changes

rouazana approved these changes Oct 5, 2017

View reviewed changes

mbaechler reviewed Oct 5, 2017

View reviewed changes

fixup! MAILBOX-310 Mailbox query refactoring: Mailbox query should re…

9c19ead

…ly on MailboxNameExpression

mbaechler approved these changes Oct 5, 2017

View reviewed changes

fixup! MAILBOX-310 Mailbox query refactoring: Mailbox query should re…

6e24197

…ly on MailboxNameExpression

rouazana approved these changes Oct 5, 2017

View reviewed changes

mbaechler closed this Oct 5, 2017

chibenwa added a commit to chibenwa/james-project that referenced this pull request May 25, 2022

[NO_REVIEW] linagora#1011 JAMES-3737 Reactive IMAP CREATE

be27828

chibenwa added a commit to chibenwa/james-project that referenced this pull request May 26, 2022

[NO_REVIEW] linagora#1011 JAMES-3737 Reactive IMAP CREATE

a8faeaf

chibenwa added a commit to chibenwa/james-project that referenced this pull request May 26, 2022

JAMES-3737 Reactive IMAP CREATE (linagora#1011)

e031f46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MAILBOX-310 MailboxManager::search and delegation #1011

MAILBOX-310 MailboxManager::search and delegation #1011

chibenwa commented Oct 4, 2017

chibenwa commented Oct 4, 2017

strokyl Oct 4, 2017

chibenwa Oct 4, 2017

strokyl Oct 5, 2017

rouazana Oct 4, 2017

rouazana Oct 4, 2017

rouazana Oct 4, 2017

rouazana Oct 4, 2017

rouazana Oct 4, 2017

chibenwa Oct 5, 2017

rouazana Oct 4, 2017

rouazana Oct 4, 2017

chibenwa Oct 5, 2017

rouazana Oct 4, 2017

rouazana Oct 4, 2017

chibenwa Oct 5, 2017

rouazana Oct 4, 2017

chibenwa Oct 5, 2017

rouazana Oct 4, 2017

chibenwa Oct 5, 2017 •

edited

mbaechler Oct 5, 2017

mbaechler Oct 5, 2017

chibenwa Oct 5, 2017

mbaechler Oct 5, 2017

mbaechler Oct 5, 2017

chibenwa left a comment

strokyl Oct 4, 2017

rouazana Oct 5, 2017

rouazana left a comment

mbaechler Oct 5, 2017

chibenwa Oct 5, 2017

mbaechler commented Oct 5, 2017


		import org.apache.james.mailbox.model.MailboxACL;

		public class ACLChanged {

MAILBOX-310 MailboxManager::search and delegation #1011

MAILBOX-310 MailboxManager::search and delegation #1011

Conversation

chibenwa commented Oct 4, 2017

chibenwa commented Oct 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chibenwa Oct 5, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chibenwa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rouazana left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbaechler commented Oct 5, 2017

chibenwa Oct 5, 2017 •

edited