chibenwa commented on code in PR #2928: URL: https://github.com/apache/james-project/pull/2928#discussion_r2753006185
########## mailbox/api/src/main/java/org/apache/james/mailbox/model/SearchOptions.java: ########## @@ -0,0 +1,41 @@ +/**************************************************************** + * Licensed to the Apache Software Foundation (ASF) under one * + * or more contributor license agreements. See the NOTICE file * + * distributed with this work for additional information * + * regarding copyright ownership. The ASF licenses this file * + * to you under the Apache License, Version 2.0 (the * + * "License"); you may not use this file except in compliance * + * with the License. You may obtain a copy of the License at * + * * + * http://www.apache.org/licenses/LICENSE-2.0 * + * * + * Unless required by applicable law or agreed to in writing, * + * software distributed under the License is distributed on an * + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY * + * KIND, either express or implied. See the License for the * + * specific language governing permissions and limitations * + * under the License. * + ****************************************************************/ + +package org.apache.james.mailbox.model; + +import org.apache.james.util.streams.Limit; +import org.apache.james.util.streams.Offset; + +import com.google.common.base.Preconditions; + +public record SearchOptions(Offset offset, Limit limit) { + public static SearchOptions limit(Limit limit) { + return SearchOptions.of(Offset.none(), limit); + } + + public static SearchOptions of(Offset offset, Limit limit) { + return new SearchOptions(offset, limit); + } + + public SearchOptions { + Preconditions.checkNotNull(offset, "'offset' is mandatory"); + Preconditions.checkNotNull(limit, "'limit' is mandatory"); + Preconditions.checkArgument(!limit.isUnlimited(), "'limit' cannot be unlimited"); Review Comment: I can agree with that as of now but there's no reason for the POJO to actually enforce that It prevents reuse eg in the IMAP layer (search by mailbox) where we actually have unlimited search. A trick could be: ``` public record SearchOptions(Offset offset, Limit limit) { // base POJO accepts unlimited values public record Boulded(Offset offset, Limit limit) extends SearchOptions { // Here we validate that limit is not unlimited } Bounded bounded() { return new Bounded(offset, limit); } } ``` JMAP layer would construct and pass Bounded, the IMAP layer would pass directly SearchOptions. I do not think this code change is needed as of now, please do not need to commit this now, but it is meant as an example of how we can tweak the object modal around such limitations. ########## server/protocols/jmap-rfc-8621/src/main/java/org/apache/james/jmap/mailet/ExtractMDNOriginalJMAPMessageId.java: ########## @@ -105,7 +107,7 @@ private Optional<MessageId> findMessageIdForRFC822MessageId(String messageId, Ma MultimailboxesSearchQuery searchByRFC822MessageId = MultimailboxesSearchQuery .from(SearchQuery.of(SearchQuery.mimeMessageID(messageId))) .build(); - return Flux.from(mailboxManager.search(searchByRFC822MessageId, session, limit)).toStream().findFirst(); + return Flux.from(mailboxManager.search(searchByRFC822MessageId, session, SearchOptions.limit(Limit.limit(limit)))).toStream().findFirst(); Review Comment: How about making this a default constant: ``` SearchOptions.FIRST ``` (Directly in the `SearchOptions` class) ########## mailbox/lucene/src/test/java/org/apache/james/mailbox/lucene/search/LuceneMailboxMessageSearchIndexTest.java: ########## @@ -324,7 +326,7 @@ void searchAllShouldMatchAllMailboxEmails() throws Exception { void searchBodyInAllMailboxesShouldMatch() throws Exception { SearchQuery query = SearchQuery.of(SearchQuery.bodyContains("My Body")); - List<MessageId> result = index.search(session, ImmutableList.of(mailbox.getMailboxId(), mailbox2.getMailboxId(), mailbox3.getMailboxId()), query, LIMIT) + List<MessageId> result = index.search(session, ImmutableList.of(mailbox.getMailboxId(), mailbox2.getMailboxId(), mailbox3.getMailboxId()), query, SearchOptions.limit(Limit.limit(LIMIT))) Review Comment: Make `SearchOptions.limit(Limit.limit(LIMIT))` the constant to limit the boiler plate? ########## mailbox/store/src/test/java/org/apache/james/mailbox/store/search/AbstractMessageSearchIndexTest.java: ########## @@ -372,7 +374,7 @@ void searchingMessageInMultipleMailboxShouldNotReturnLessMessageThanLimitArgumen List<MessageId> result = messageSearchIndex.search(session, ImmutableList.of(mailbox2.getMailboxId(), mailbox.getMailboxId()), searchQuery, - limit) + SearchOptions.limit(Limit.limit(limit))) Review Comment: ```suggestion LIMIT) ``` Here and below ? ########## server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/method/MDNParseMethod.scala: ########## @@ -132,7 +133,7 @@ case class MDNEmailIdResolver @Inject()(mailboxManager: MailboxManager) { def resolveForEmailId(originalMessageId: Option[OriginalMessageId], session: MailboxSession): SMono[Option[MessageId]] = originalMessageId.map(originalMsg => { val searchByRFC822MessageId: MultimailboxesSearchQuery = MultimailboxesSearchQuery.from(SearchQuery.of(SearchQuery.mimeMessageID(originalMsg.getOriginalMessageId))).build - SFlux.fromPublisher(mailboxManager.search(searchByRFC822MessageId, session, NUMBER_OF_ORIGINAL_MESSAGE_ID_VALID + 1)).collectSeq().map { + SFlux.fromPublisher(mailboxManager.search(searchByRFC822MessageId, session, SearchOptions.limit(JavaLimit.limit(NUMBER_OF_ORIGINAL_MESSAGE_ID_VALID + 1)))).collectSeq().map { Review Comment: IMO we shall open another issue to actually fix this unmonitored use of search to resolve MessageID (booouh) My suggestion is actually to rely on the `ThreadIdGuessingAlgorithm` in order to get the email thread and from there list thread headers in order to identify the corresponding email. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
