-
Notifications
You must be signed in to change notification settings - Fork 63
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WIP MailboxManager::search #1008
Conversation
…filtering for now
setMailboxId is a Mailbox method already
- Allow missing namespace and username in mailboxQuery - Matching logic SHOULD be moved in MailboxQuery
List<MailboxMetaData> metaDatas = mailboxManager.search(new MailboxQuery(MailboxPath.forUser(USER_1, ""), "*", '.'), session); | ||
List<MailboxMetaData> metaDatas = mailboxManager.search( | ||
MailboxQuery.builder() | ||
.base(MailboxPath.forUser(USER_1, "")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should probably introduce something else than MailboxPath for something that can't be a mailbox path (empty string is probably not legal)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DONE ;-)
List<MailboxMetaData> metaDatas = mailboxManager.search( | ||
MailboxQuery.builder() | ||
.base(MailboxPath.forUser(USER_1, "")) | ||
.expression("*") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be matchesAll
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done :-)
@@ -503,6 +503,10 @@ public String serialize() { | |||
public String toString() { | |||
return serialize(); | |||
} | |||
|
|||
public boolean isUser() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there's already getNameType
public Map<String, Rfc4314Rights> usersACL() { | ||
return this.entries.entrySet().stream() | ||
.filter(entry -> entry.getKey().isUser()) | ||
.map(entry -> Pair.of(entry.getKey().getName(), entry.getValue())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Guavate.entriesToMap
@@ -878,4 +882,10 @@ public MailboxACL union(EntryKey key, Rfc4314Rights mailboxACLRights) throws Uns | |||
return union(new MailboxACL(new Entry(key, mailboxACLRights))); | |||
} | |||
|
|||
public Map<String, Rfc4314Rights> usersACL() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why don't we take NameType as argument for filtering ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also added "rejection of (unhandled) negative entries in ACL".
pattern = constructEscapedRegex(); | ||
} | ||
|
||
public MailboxPath getPathLike() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but it's not a MailboxPath
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A refactoring solved this.
|
||
return belongsToRequestedNamespaceAndUser(mailboxPath) | ||
&& mailboxName.startsWith(baseName) | ||
&& isExpressionMatch(mailboxName.substring(baseNameLength)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we are still doing substrings ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A refactoring solved this.
MailboxPath inbox1 = MailboxPath.inbox(session1); | ||
mailboxManager.createMailbox(inbox1, session1); | ||
mailboxManager.setRights(inbox1, | ||
new MailboxACL( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there's a great builder for that
@@ -218,12 +222,17 @@ public boolean hasChildren(Mailbox mailbox, char delimiter) { | |||
public void updateACL(Mailbox mailbox, MailboxACL.ACLCommand mailboxACLCommand) throws MailboxException { | |||
CassandraId cassandraId = (CassandraId) mailbox.getMailboxId(); | |||
cassandraACLMapper.updateACL(cassandraId, mailboxACLCommand); | |||
if (mailboxACLCommand.getEntryKey().isUser()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my opinion, we lack an ACLChangedEvent that is triggered on ACL changes. With that event, we could handle these ACL updates (and projections building) based on events
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO we don't need it now. Cassandra DAO consistency is very well handled without.
Thus I would advise to do it later to avoid unecessary big refactoring
rowOptional.map(Throwing.function(row -> Rfc4314Rights.fromSerializedRfc4314Rights(row.getString(RIGHTS))))); | ||
} | ||
|
||
public CompletableFuture<Map<MailboxId, Rfc4314Rights>> retrieveUser(String userName) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
listRightsForUser
See #1011 |
This commit also adds a minimal README.md to be improved with more detailed deployment instructions in the future.
No description provided.