Re: [PR] JAMES-3946 add a memory implementation of the DropList [james-project]
Arsnael merged PR #2197: URL: https://github.com/apache/james-project/pull/2197 -- 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: notifications-unsubscr...@james.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@james.apache.org For additional commands, e-mail: notifications-h...@james.apache.org
Re: [PR] JAMES-3946 add a memory implementation of the DropList [james-project]
Maxxx873 commented on PR #2197: URL: https://github.com/apache/james-project/pull/2197#issuecomment-2057818132 > Looks good to me. Tiny cosmetic fixes. > > Can you confirm the test suite establishes a complete test coverage?  -- 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: notifications-unsubscr...@james.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@james.apache.org For additional commands, e-mail: notifications-h...@james.apache.org
Re: [PR] JAMES-3946 add a memory implementation of the DropList [james-project]
Maxxx873 commented on PR #2197: URL: https://github.com/apache/james-project/pull/2197#issuecomment-2057753241 > Read it, nothing to add. > > Thank you for the work, appreciated as always :) Thank you too, I'm really glad to be involved in this project and follow its development. -- 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: notifications-unsubscr...@james.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@james.apache.org For additional commands, e-mail: notifications-h...@james.apache.org
Re: [PR] JAMES-3946 add a memory implementation of the DropList [james-project]
chibenwa commented on code in PR #2197:
URL: https://github.com/apache/james-project/pull/2197#discussion_r1565497055
##
server/data/data-api/src/test/java/org/apache/james/droplists/api/DropListContract.java:
##
@@ -0,0 +1,142 @@
+/
+ * 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.droplists.api;
+
+import static org.apache.james.droplists.api.OwnerScope.GLOBAL;
+import static org.assertj.core.api.AssertionsForClassTypes.assertThat;
+
+import java.util.Objects;
+import java.util.stream.Stream;
+
+import jakarta.mail.internet.AddressException;
+
+import org.apache.james.core.Domain;
+import org.apache.james.core.MailAddress;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
+
+import reactor.core.publisher.Flux;
+import reactor.core.publisher.Mono;
+
+public interface DropListContract {
+
+DropList dropList();
+
+@Test
+default void shouldAddEntry() throws AddressException {
+DropListEntry dropListEntry =
getDropListTestEntries().toList().getFirst();
+
+Mono result = dropList().add(dropListEntry);
+
+assertThat(Objects.requireNonNull(dropList().list(GLOBAL,
dropListEntry.getOwner()).collectList().block()).size()).isEqualTo(1);
Review Comment:
I think Objects.requireNonNull is overkill here.
-> A NPE thrown would still be understandable for a dev debugging this
-> Reading is harder and we do not have such a convention at all through
the code base.
Suggestion: relax your IDE warning or ignore those.
--
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: notifications-unsubscr...@james.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: notifications-unsubscr...@james.apache.org
For additional commands, e-mail: notifications-h...@james.apache.org
Re: [PR] JAMES-3946 add a memory implementation of the DropList [james-project]
chibenwa commented on code in PR #2197:
URL: https://github.com/apache/james-project/pull/2197#discussion_r1565494926
##
server/data/data-api/src/test/java/org/apache/james/droplists/api/DropListContract.java:
##
@@ -0,0 +1,142 @@
+/
+ * 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.droplists.api;
+
+import static org.apache.james.droplists.api.OwnerScope.GLOBAL;
+import static org.assertj.core.api.AssertionsForClassTypes.assertThat;
+
+import java.util.Objects;
+import java.util.stream.Stream;
+
+import jakarta.mail.internet.AddressException;
+
+import org.apache.james.core.Domain;
+import org.apache.james.core.MailAddress;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
+
+import reactor.core.publisher.Flux;
+import reactor.core.publisher.Mono;
+
+public interface DropListContract {
+
+DropList dropList();
+
+@Test
+default void shouldAddEntry() throws AddressException {
+DropListEntry dropListEntry =
getDropListTestEntries().toList().getFirst();
Review Comment:
That's quite complex for just getting one item.
Calling the builder here without hiding things behind a method call would
seem more straight forward to me...
--
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: notifications-unsubscr...@james.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: notifications-unsubscr...@james.apache.org
For additional commands, e-mail: notifications-h...@james.apache.org
Re: [PR] JAMES-3946 add a memory implementation of the DropList [james-project]
Maxxx873 commented on PR #2197: URL: https://github.com/apache/james-project/pull/2197#issuecomment-2052139758 > Hello, > > Thanks to put this together! > > While the datastructure for the memory structure is not optimal (`O(N)` scans) And while we could easily do better (one multimap for global scope, one table for domain scope, one table for user scope) this does not matter much performance wise in a testing environment. > > Other remarks however likely deserve attention. Thanks for the excellent code review! I will try to correct everything that was pointed out to me in the remarks. -- 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: notifications-unsubscr...@james.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@james.apache.org For additional commands, e-mail: notifications-h...@james.apache.org
Re: [PR] JAMES-3946 add a memory implementation of the DropList [james-project]
chibenwa commented on code in PR #2197:
URL: https://github.com/apache/james-project/pull/2197#discussion_r1562094408
##
server/data/data-api/src/main/java/org/apache/james/droplists/api/DropListEntry.java:
##
@@ -62,7 +67,7 @@ public Builder deniedEntity(String deniedEntity) {
return this;
}
-public DropListEntry build() {
+public DropListEntry build() throws AddressException {
Review Comment:
With below suggestion no parsing would be needed wich means this method
would never throw.
##
server/data/data-api/src/test/java/org/apache/james/droplists/api/DropListContract.java:
##
@@ -0,0 +1,139 @@
+/
+ * 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.droplists.api;
+
+import static org.apache.james.droplists.api.OwnerScope.GLOBAL;
+import static org.assertj.core.api.AssertionsForClassTypes.assertThat;
+
+import java.util.Objects;
+
+import jakarta.mail.internet.AddressException;
+
+import org.apache.james.core.MailAddress;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.CsvSource;
+
+import reactor.core.publisher.Flux;
+import reactor.core.publisher.Mono;
+
+public interface DropListContract {
+
+DropList dropList();
+
+@Test
+default void shouldAddEntry() throws AddressException {
+DropListEntry dropListEntry = DropListEntry.builder()
+.owner("global_sc...@example.com")
+.deniedEntity("den...@denied.com")
+.deniedEntityType(DeniedEntityType.ADDRESS)
+.build();
+
+Mono result = dropList().add(dropListEntry);
+
+assertThat(1).isEqualTo(Objects.requireNonNull(dropList().list(GLOBAL,
dropListEntry.getOwner()).collectList().block()).size());
+assertThat(Mono.empty()).isEqualTo(result);
+}
+
+@Test
+default void shouldRemoveEntry() throws AddressException {
+DropListEntry dropListEntry = DropListEntry.builder()
+.owner("global_sc...@example.com")
+.deniedEntity("den...@denied.com")
+.deniedEntityType(DeniedEntityType.ADDRESS)
+.build();
+
+dropList().add(dropListEntry);
+
+Mono result = dropList().remove(dropListEntry);
+
+assertThat(0).isEqualTo(Objects.requireNonNull(dropList().list(GLOBAL,
dropListEntry.getOwner()).collectList().block()).size());
+assertThat(Mono.empty()).isEqualTo(result);
+}
+
+@ParameterizedTest(name = "{index} ownerScope: {0}, owner: {1},")
+@CsvSource(value = {
+"GLOBAL, global_sc...@example.com",
+"DOMAIN, domain_sc...@example.com",
Review Comment:
I would expect the value to be
```suggestion
"DOMAIN, example.com",
```
##
server/data/data-memory/src/main/java/org/apache/james/droplists/memory/MemoryDropList.java:
##
@@ -0,0 +1,87 @@
+/
+ * 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,
Re: [PR] JAMES-3946 add a memory implementation of the DropList [james-project]
chibenwa commented on code in PR #2197:
URL: https://github.com/apache/james-project/pull/2197#discussion_r1562092627
##
server/data/data-api/src/main/java/org/apache/james/droplists/api/DropListEntry.java:
##
@@ -71,6 +76,11 @@ public DropListEntry build() {
Preconditions.checkArgument(deniedEntity != null &&
!deniedEntity.isBlank(), "`deniedEntity` must not be null, empty, or blank");
Preconditions.checkArgument(deniedEntity.length() <=
MAXIMUM_DOMAIN_LENGTH,
"deniedEntity length should not be longer than %s characters",
MAXIMUM_DOMAIN_LENGTH);
+if (deniedEntityType.equals(DeniedEntityType.DOMAIN)) {
+deniedEntity = Domain.of(deniedEntity).asString();
+} else {
+deniedEntity = new MailAddress(deniedEntity).toString();
+}
Review Comment:
Generally speaking we can avoid variable reallocation by method extraction.
This is more elegant and less complex.
--
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: notifications-unsubscr...@james.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: notifications-unsubscr...@james.apache.org
For additional commands, e-mail: notifications-h...@james.apache.org
[PR] JAMES-3946 add a memory implementation of the DropList [james-project]
Maxxx873 opened a new pull request, #2197: URL: https://github.com/apache/james-project/pull/2197 [https://issues.apache.org/jira/projects/JAMES/issues/JAMES-3946](https://issues.apache.org/jira/projects/JAMES/issues/JAMES-3946?filter=allopenissues) -- 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: notifications-unsubscr...@james.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@james.apache.org For additional commands, e-mail: notifications-h...@james.apache.org
