rpuch commented on code in PR #2040:
URL: https://github.com/apache/ignite-3/pull/2040#discussion_r1190972813
##########
modules/network-annotation-processor/src/main/java/org/apache/ignite/internal/network/processor/messages/MessageImplGenerator.java:
##########
@@ -688,26 +721,52 @@ private static TypeSpec generateBuilderImpl(
.addFields(fields)
.addMethods(setters)
.addMethods(getters)
- .addMethod(buildMethod(message, messageImplClass, fields))
+ .addMethod(buildMethod(message, messageImplClass, fields,
notNullFieldNames, marshallableFieldNames))
.build();
}
/**
* Generates the {@code build()} method for the Builder interface
implementation.
*/
- private static MethodSpec buildMethod(MessageClass message, ClassName
messageImplClass, List<FieldSpec> fields) {
- String constructorParameters = fields.stream()
- .map(field -> field.name)
- .collect(Collectors.joining(", ", "(", ")"));
+ private static MethodSpec buildMethod(MessageClass message, ClassName
messageImplClass, List<FieldSpec> fields,
+ Set<String> notNullFieldNames, Set<String> marshallableFieldNames)
{
+
+ CodeBlock.Builder methodBodyBuilder = CodeBlock.builder();
+
+ methodBodyBuilder.add("return new $T", messageImplClass);
+
+ CodeBlock constructorArgs = fields.stream()
+ .map(field -> {
+ String fieldName = field.name;
+
+ if (notNullFieldNames.contains(fieldName) &&
!marshallableFieldNames.contains(fieldName)) {
Review Comment:
Why is it only for non-marshallable fields?
##########
modules/network-annotation-processor/src/test/java/org/apache/ignite/internal/network/processor/tests/NullabilityFieldsTest.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,
+ * 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.ignite.internal.network.processor.tests;
+
+import static org.junit.jupiter.api.Assertions.assertThrows;
+
+import java.util.UUID;
+import org.junit.jupiter.api.Test;
+
+/** Tests for messages with {@link org.jetbrains.annotations.NotNull} fields.
*/
Review Comment:
`@NotNull` does not seem to be mentioned in the code, so the javadoc is
innacurate
##########
modules/network-annotation-processor/src/main/java/org/apache/ignite/internal/network/processor/messages/MessageImplGenerator.java:
##########
@@ -688,26 +721,52 @@ private static TypeSpec generateBuilderImpl(
.addFields(fields)
.addMethods(setters)
.addMethods(getters)
- .addMethod(buildMethod(message, messageImplClass, fields))
+ .addMethod(buildMethod(message, messageImplClass, fields,
notNullFieldNames, marshallableFieldNames))
.build();
}
/**
* Generates the {@code build()} method for the Builder interface
implementation.
*/
- private static MethodSpec buildMethod(MessageClass message, ClassName
messageImplClass, List<FieldSpec> fields) {
- String constructorParameters = fields.stream()
- .map(field -> field.name)
- .collect(Collectors.joining(", ", "(", ")"));
+ private static MethodSpec buildMethod(MessageClass message, ClassName
messageImplClass, List<FieldSpec> fields,
+ Set<String> notNullFieldNames, Set<String> marshallableFieldNames)
{
+
+ CodeBlock.Builder methodBodyBuilder = CodeBlock.builder();
+
+ methodBodyBuilder.add("return new $T", messageImplClass);
+
+ CodeBlock constructorArgs = fields.stream()
+ .map(field -> {
+ String fieldName = field.name;
+
+ if (notNullFieldNames.contains(fieldName) &&
!marshallableFieldNames.contains(fieldName)) {
+ return CodeBlock.of("$T.requireNonNull($L, $S)",
Objects.class, fieldName, fieldName + " is not marked @Nullable");
+ }
+
+ return CodeBlock.of("$L", fieldName);
+ })
+ .collect(CodeBlock.joining(", ", "(", ")"));
+
+ methodBodyBuilder.add(constructorArgs);
return MethodSpec.methodBuilder("build")
.addAnnotation(Override.class)
.addModifiers(Modifier.PUBLIC)
.returns(message.className())
- .addStatement("return new $T$L", messageImplClass,
constructorParameters)
+ .addStatement(methodBodyBuilder.build())
.build();
}
+ private static boolean isNotNull(ExecutableElement el) {
+ TypeKind kind = el.getReturnType().getKind();
+
+ if (kind == TypeKind.ARRAY) {
+ return el.getReturnType().getAnnotation(Nullable.class) != null;
Review Comment:
This doesn't seem right
##########
modules/network-annotation-processor/src/test/java/org/apache/ignite/internal/network/processor/tests/NullabilityFieldsTest.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,
+ * 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.ignite.internal.network.processor.tests;
+
+import static org.junit.jupiter.api.Assertions.assertThrows;
+
+import java.util.UUID;
+import org.junit.jupiter.api.Test;
+
+/** Tests for messages with {@link org.jetbrains.annotations.NotNull} fields.
*/
+public class NullabilityFieldsTest {
+ private final TestMessagesFactory factory = new TestMessagesFactory();
+
+ @Test
+ public void testNotNullArbitraryFieldBuildWithNull() {
+ assertThrows(NullPointerException.class, () -> {
+ // Should throw NPE from setter.
+ factory.notNullArbitraryFieldMessage().value(null);
+ });
+ assertThrows(NullPointerException.class, () -> {
+ // Should throw NPE from build method.
+ factory.notNullArbitraryFieldMessage().build();
+ });
+ }
+
+ @Test
+ public void testNotNullArbitraryFieldBuild() {
+ // Build with value.
+
factory.notNullArbitraryFieldMessage().value(UUID.randomUUID()).build();
+ }
+
+ @Test
+ public void testNotNullMarshallableFieldBuildWithNull() {
+ assertThrows(NullPointerException.class, () -> {
+ // Should throw NPE from constructor.
+ factory.notNullMarshallableFieldMessage().build();
+ });
+ }
+
+ @Test
+ public void testNotNullMarshallableFieldBuild() {
+ // Build with value.
+ factory.notNullMarshallableFieldMessage().value(new Object()).build();
+
+ // Build with byte array representation of a value.
+ factory.notNullMarshallableFieldMessage().valueByteArray(new
byte[0]).build();
+ }
+
+ @Test
+ public void testNotNullNetworkMessageFieldBuild() {
+ // Build with value.
+
factory.notNullNetworkMessageFieldMessage().value(factory.notNullArbitraryFieldMessage().value(UUID.randomUUID()).build()).build();
Review Comment:
Same thing about assertDoesNotThrow()
##########
modules/network-annotation-processor/src/test/java/org/apache/ignite/internal/network/processor/tests/NullabilityFieldsTest.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,
+ * 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.ignite.internal.network.processor.tests;
+
+import static org.junit.jupiter.api.Assertions.assertThrows;
+
+import java.util.UUID;
+import org.junit.jupiter.api.Test;
+
+/** Tests for messages with {@link org.jetbrains.annotations.NotNull} fields.
*/
+public class NullabilityFieldsTest {
+ private final TestMessagesFactory factory = new TestMessagesFactory();
+
+ @Test
+ public void testNotNullArbitraryFieldBuildWithNull() {
+ assertThrows(NullPointerException.class, () -> {
+ // Should throw NPE from setter.
+ factory.notNullArbitraryFieldMessage().value(null);
+ });
+ assertThrows(NullPointerException.class, () -> {
+ // Should throw NPE from build method.
+ factory.notNullArbitraryFieldMessage().build();
+ });
+ }
+
+ @Test
+ public void testNotNullArbitraryFieldBuild() {
+ // Build with value.
+
factory.notNullArbitraryFieldMessage().value(UUID.randomUUID()).build();
+ }
+
+ @Test
+ public void testNotNullMarshallableFieldBuildWithNull() {
+ assertThrows(NullPointerException.class, () -> {
+ // Should throw NPE from constructor.
+ factory.notNullMarshallableFieldMessage().build();
+ });
+ }
+
+ @Test
+ public void testNotNullMarshallableFieldBuild() {
+ // Build with value.
+ factory.notNullMarshallableFieldMessage().value(new Object()).build();
+
+ // Build with byte array representation of a value.
+ factory.notNullMarshallableFieldMessage().valueByteArray(new
byte[0]).build();
+ }
+
+ @Test
+ public void testNotNullNetworkMessageFieldBuild() {
+ // Build with value.
+
factory.notNullNetworkMessageFieldMessage().value(factory.notNullArbitraryFieldMessage().value(UUID.randomUUID()).build()).build();
+ }
+
Review Comment:
Same thing about `assertDoesNotThrow()` for the rest of the tests
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/replication/request/ReadWriteReplicaRequest.java:
##########
@@ -21,11 +21,13 @@
import org.apache.ignite.internal.replicator.message.PrimaryReplicaRequest;
import org.apache.ignite.internal.replicator.message.TimestampAware;
import org.apache.ignite.network.annotations.Marshallable;
+import org.jetbrains.annotations.Nullable;
/**
* Read-write replica request.
*/
public interface ReadWriteReplicaRequest extends PrimaryReplicaRequest,
TimestampAware {
+ @Nullable
Review Comment:
An RW replica request is inside an RW transaction which has transaction ID,
so this does not seem right
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/replication/request/ReadWriteSingleRowReplicaRequest.java:
##########
@@ -32,6 +33,7 @@ public interface ReadWriteSingleRowReplicaRequest extends
SingleRowReplicaReques
*
* @return Table partition id.
*/
+ @Nullable
Review Comment:
Is it actually nullable?
##########
modules/network-annotation-processor/src/test/java/org/apache/ignite/internal/network/processor/tests/NullabilityFieldsTest.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,
+ * 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.ignite.internal.network.processor.tests;
+
+import static org.junit.jupiter.api.Assertions.assertThrows;
+
+import java.util.UUID;
+import org.junit.jupiter.api.Test;
+
+/** Tests for messages with {@link org.jetbrains.annotations.NotNull} fields.
*/
+public class NullabilityFieldsTest {
+ private final TestMessagesFactory factory = new TestMessagesFactory();
+
+ @Test
+ public void testNotNullArbitraryFieldBuildWithNull() {
+ assertThrows(NullPointerException.class, () -> {
+ // Should throw NPE from setter.
+ factory.notNullArbitraryFieldMessage().value(null);
+ });
+ assertThrows(NullPointerException.class, () -> {
+ // Should throw NPE from build method.
+ factory.notNullArbitraryFieldMessage().build();
+ });
+ }
+
+ @Test
+ public void testNotNullArbitraryFieldBuild() {
+ // Build with value.
+
factory.notNullArbitraryFieldMessage().value(UUID.randomUUID()).build();
+ }
+
+ @Test
+ public void testNotNullMarshallableFieldBuildWithNull() {
+ assertThrows(NullPointerException.class, () -> {
+ // Should throw NPE from constructor.
+ factory.notNullMarshallableFieldMessage().build();
+ });
+ }
+
+ @Test
+ public void testNotNullMarshallableFieldBuild() {
+ // Build with value.
+ factory.notNullMarshallableFieldMessage().value(new Object()).build();
Review Comment:
Same thing about `assertDoesNotThrow()`
##########
modules/network-annotation-processor/src/main/java/org/apache/ignite/internal/network/processor/messages/MessageImplGenerator.java:
##########
@@ -596,15 +610,27 @@ private static void
generateEqualsAndHashCode(TypeSpec.Builder messageImplBuilde
/**
* Creates a constructor for the current Network Message implementation.
*/
- private static MethodSpec constructor(List<FieldSpec> fields) {
+ private static MethodSpec constructor(List<FieldSpec> fields, Set<String>
notNullFieldNames, Set<String> marshallableFieldNames) {
MethodSpec.Builder constructor = MethodSpec.constructorBuilder()
.addModifiers(Modifier.PRIVATE);
- fields.forEach(field ->
- constructor
- .addParameter(field.type, field.name)
- .addStatement("this.$N = $N", field, field)
- );
+ fields.forEach(field -> {
+ String fieldName = field.name;
+
+ if (notNullFieldNames.contains(fieldName) &&
marshallableFieldNames.contains(fieldName)) {
Review Comment:
Why do we only check nulls for marshallable fields?
##########
modules/network-annotation-processor/src/test/java/org/apache/ignite/internal/network/processor/tests/NullabilityFieldsTest.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,
+ * 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.ignite.internal.network.processor.tests;
+
+import static org.junit.jupiter.api.Assertions.assertThrows;
+
+import java.util.UUID;
+import org.junit.jupiter.api.Test;
+
+/** Tests for messages with {@link org.jetbrains.annotations.NotNull} fields.
*/
+public class NullabilityFieldsTest {
+ private final TestMessagesFactory factory = new TestMessagesFactory();
+
+ @Test
+ public void testNotNullArbitraryFieldBuildWithNull() {
+ assertThrows(NullPointerException.class, () -> {
+ // Should throw NPE from setter.
+ factory.notNullArbitraryFieldMessage().value(null);
+ });
+ assertThrows(NullPointerException.class, () -> {
+ // Should throw NPE from build method.
+ factory.notNullArbitraryFieldMessage().build();
+ });
+ }
+
+ @Test
+ public void testNotNullArbitraryFieldBuild() {
+ // Build with value.
+
factory.notNullArbitraryFieldMessage().value(UUID.randomUUID()).build();
+ }
+
+ @Test
+ public void testNotNullMarshallableFieldBuildWithNull() {
+ assertThrows(NullPointerException.class, () -> {
+ // Should throw NPE from constructor.
+ factory.notNullMarshallableFieldMessage().build();
+ });
+ }
+
+ @Test
+ public void testNotNullMarshallableFieldBuild() {
+ // Build with value.
+ factory.notNullMarshallableFieldMessage().value(new Object()).build();
+
+ // Build with byte array representation of a value.
+ factory.notNullMarshallableFieldMessage().valueByteArray(new
byte[0]).build();
Review Comment:
And here
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/raft/snapshot/message/SnapshotRequestMessage.java:
##########
@@ -19,11 +19,13 @@
import java.util.UUID;
import org.apache.ignite.network.NetworkMessage;
+import org.jetbrains.annotations.Nullable;
/**
* Base interface for snapshot request messages.
*/
public interface SnapshotRequestMessage extends NetworkMessage {
/** Snapshot id. */
+ @Nullable
Review Comment:
Why is it nullable? It doesn't look right because a snapshot request must
reference a snapshot by ID. If something fails due to it, maybe a ticket has to
be filed and linked here with a TODO to resolve the issue.
##########
modules/network-annotation-processor/src/test/java/org/apache/ignite/internal/network/processor/tests/NullabilityFieldsTest.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,
+ * 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.ignite.internal.network.processor.tests;
+
+import static org.junit.jupiter.api.Assertions.assertThrows;
+
+import java.util.UUID;
+import org.junit.jupiter.api.Test;
+
+/** Tests for messages with {@link org.jetbrains.annotations.NotNull} fields.
*/
+public class NullabilityFieldsTest {
+ private final TestMessagesFactory factory = new TestMessagesFactory();
+
+ @Test
+ public void testNotNullArbitraryFieldBuildWithNull() {
+ assertThrows(NullPointerException.class, () -> {
+ // Should throw NPE from setter.
+ factory.notNullArbitraryFieldMessage().value(null);
+ });
+ assertThrows(NullPointerException.class, () -> {
+ // Should throw NPE from build method.
+ factory.notNullArbitraryFieldMessage().build();
+ });
+ }
+
+ @Test
+ public void testNotNullArbitraryFieldBuild() {
+ // Build with value.
+
factory.notNullArbitraryFieldMessage().value(UUID.randomUUID()).build();
Review Comment:
Let's put the statement inside of an `assertDoesNotThrow()` call to make it
clear that we are ensuring that nothing gets thrown here
##########
modules/placement-driver-api/src/main/java/org/apache/ignite/internal/placementdriver/message/PlacementDriverMessage.java:
##########
@@ -30,6 +31,7 @@ public interface PlacementDriverMessage extends
NetworkMessage {
*
* @return Replication group id.
*/
+ @Nullable
Review Comment:
I wonder why can it be null? On the 'consuming' side, it looks as if it was
non-nullable.
##########
modules/replicator/src/main/java/org/apache/ignite/internal/replicator/message/ReplicaRequest.java:
##########
@@ -30,6 +31,7 @@ public interface ReplicaRequest extends NetworkMessage {
*
* @return Replication group id.
*/
+ @Nullable
Review Comment:
I wonder when replicaId can be null... It looks strange
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/replication/request/ReadWriteMultiRowReplicaRequest.java:
##########
@@ -32,6 +33,7 @@ public interface ReadWriteMultiRowReplicaRequest extends
MultipleRowReplicaReque
*
* @return Table partition id.
*/
+ @Nullable
Review Comment:
Is it actually nullable?
--
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]
