anton-vinogradov commented on code in PR #13461:
URL: https://github.com/apache/ignite/pull/13461#discussion_r3752986778
##########
modules/codegen/src/main/java/org/apache/ignite/internal/MessageMarshallerGenerator.java:
##########
@@ -1147,6 +1147,8 @@ private enum MarshalledKind {
if (ann == null)
return null;
+ ensureCorrectlyAnnotated(field, ann);
Review Comment:
The check runs before the kind is computed, so it judges the field type
instead of the companion and rejects a working pattern (Set<GridTopicMessage> +
GridTopicMessage[] companion) that the protocol writes itself. Please move the
check after the kind is known and apply it only to BLOB and ELEMENT_BLOBS.
##########
modules/codegen/src/main/java/org/apache/ignite/internal/MessageMarshallerGenerator.java:
##########
@@ -1170,6 +1172,41 @@ private enum MarshalledKind {
return MarshalledKind.ELEMENT_BLOBS;
}
+ /** */
+ private void ensureCorrectlyAnnotated(VariableElement field, Marshalled
ann) {
+ boolean msgToBytes = false;
+
+ if (assignableFrom(field.asType(), msgType)) {
+ msgToBytes = true;
+ }
+ else if (isCollection(field.asType())) {
+ DeclaredType type = (DeclaredType)field.asType();
+
+ List<? extends TypeMirror> typeArgs = type.getTypeArguments();
+
+ assert typeArgs.size() == 1;
+
+ msgToBytes = assignableFrom(typeArgs.get(0), msgType);
Review Comment:
Only one generic level is inspected, so Collection<List<Message>> and Map<K,
List<Message>> still reach the marshaller silently. Please walk the type
recursively, the way needsCtxType() above does.
##########
modules/codegen/src/main/java/org/apache/ignite/internal/MessageMarshallerGenerator.java:
##########
@@ -1170,6 +1172,41 @@ private enum MarshalledKind {
return MarshalledKind.ELEMENT_BLOBS;
}
+ /** */
+ private void ensureCorrectlyAnnotated(VariableElement field, Marshalled
ann) {
+ boolean msgToBytes = false;
+
+ if (assignableFrom(field.asType(), msgType)) {
+ msgToBytes = true;
+ }
+ else if (isCollection(field.asType())) {
+ DeclaredType type = (DeclaredType)field.asType();
+
+ List<? extends TypeMirror> typeArgs = type.getTypeArguments();
+
+ assert typeArgs.size() == 1;
Review Comment:
A raw Collection field has no type arguments, and assert is off in a normal
build while its Error escapes catch (Exception) in MessageProcessor. Please
replace both asserts (here and at line 1199) with a size check and report the
failure through the Messager.
##########
modules/core/src/test/java/org/apache/ignite/internal/codegen/MessageProcessorTest.java:
##########
@@ -683,6 +683,46 @@ public void testNonMarshallableSelfMarshallingFailed() {
"or SelfMarshallingMessage, nor declare @Marshalled fields");
}
+ /** Test that {@code @Marshalled} annotation on {@link Message} field will
fail generation. */
+ @Test
+ public void testMarshallableOnMessageFieldFailGeneration() {
+ Compilation compilation = compile("InternalMessage.java",
"InternalMessageContainer.java");
+
+ assertThat(compilation).failed();
+
+ assertThat(compilation).hadErrorContaining("Field \"msg\" is Message.
Must be written by communication protocol");
+ }
+
+ /** Test that {@code @Marshalled} annotation on {@link Message} collection
field will fail generation. */
+ @Test
+ public void testMarshallableOnMessageCollectionFieldFailGeneration() {
+ Compilation compilation = compile("InternalMessage.java",
"InternalMessageCollectionContainer.java");
+
+ assertThat(compilation).failed();
+
+ assertThat(compilation).hadErrorContaining("Field \"msgColl\" is
Message. Must be written by communication protocol");
+ }
+
+ /** Test that {@code @Marshalled} annotation on {@link Message} map field
will fail generation. */
+ @Test
+ public void testMarshallableOnMessageMapFieldFailGeneration() {
+ Compilation compilation = compile("InternalMessage.java",
"InternalMessageMapContainer.java");
+
+ assertThat(compilation).failed();
+
+ assertThat(compilation).hadErrorContaining("Field \"msgMap\" is
Message. Must be written by communication protocol");
+ }
+
+ /** Test that {@code @Marshalled} annotation on {@link Message} map field
will fail generation. */
Review Comment:
The javadoc says "map field" while the test covers the array case. Please
fix the copy-paste.
##########
modules/codegen/src/main/java/org/apache/ignite/internal/MessageMarshallerGenerator.java:
##########
@@ -1170,6 +1172,41 @@ private enum MarshalledKind {
return MarshalledKind.ELEMENT_BLOBS;
}
+ /** */
+ private void ensureCorrectlyAnnotated(VariableElement field, Marshalled
ann) {
+ boolean msgToBytes = false;
+
+ if (assignableFrom(field.asType(), msgType)) {
+ msgToBytes = true;
+ }
+ else if (isCollection(field.asType())) {
+ DeclaredType type = (DeclaredType)field.asType();
+
+ List<? extends TypeMirror> typeArgs = type.getTypeArguments();
+
+ assert typeArgs.size() == 1;
+
+ msgToBytes = assignableFrom(typeArgs.get(0), msgType);
+ }
+ else if (field.asType() instanceof ArrayType) {
Review Comment:
Line 1166 checks the same thing as getKind() == TypeKind.ARRAY. Please use
that form here too.
##########
modules/core/src/test/java/org/apache/ignite/internal/codegen/MessageProcessorTest.java:
##########
@@ -683,6 +683,46 @@ public void testNonMarshallableSelfMarshallingFailed() {
"or SelfMarshallingMessage, nor declare @Marshalled fields");
}
+ /** Test that {@code @Marshalled} annotation on {@link Message} field will
fail generation. */
+ @Test
+ public void testMarshallableOnMessageFieldFailGeneration() {
Review Comment:
"Marshallable" collides with MarshallableMessage, already covered by
testMarshallableMessage in this file. Please rename all four to
testMarshalledOn..., after the @Marshalled annotation.
##########
modules/core/src/test/resources/codegen/InternalMessageArrayContainer.java:
##########
@@ -0,0 +1,32 @@
+/*
+ * 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;
+
+import java.util.Collection;
Review Comment:
Unused import. Please remove it.
##########
modules/core/src/test/java/org/apache/ignite/internal/codegen/MessageProcessorTest.java:
##########
@@ -683,6 +683,46 @@ public void testNonMarshallableSelfMarshallingFailed() {
"or SelfMarshallingMessage, nor declare @Marshalled fields");
}
+ /** Test that {@code @Marshalled} annotation on {@link Message} field will
fail generation. */
+ @Test
+ public void testMarshallableOnMessageFieldFailGeneration() {
+ Compilation compilation = compile("InternalMessage.java",
"InternalMessageContainer.java");
+
+ assertThat(compilation).failed();
+
+ assertThat(compilation).hadErrorContaining("Field \"msg\" is Message.
Must be written by communication protocol");
+ }
+
+ /** Test that {@code @Marshalled} annotation on {@link Message} collection
field will fail generation. */
+ @Test
+ public void testMarshallableOnMessageCollectionFieldFailGeneration() {
+ Compilation compilation = compile("InternalMessage.java",
"InternalMessageCollectionContainer.java");
+
+ assertThat(compilation).failed();
+
+ assertThat(compilation).hadErrorContaining("Field \"msgColl\" is
Message. Must be written by communication protocol");
+ }
+
+ /** Test that {@code @Marshalled} annotation on {@link Message} map field
will fail generation. */
+ @Test
+ public void testMarshallableOnMessageMapFieldFailGeneration() {
+ Compilation compilation = compile("InternalMessage.java",
"InternalMessageMapContainer.java");
+
+ assertThat(compilation).failed();
+
+ assertThat(compilation).hadErrorContaining("Field \"msgMap\" is
Message. Must be written by communication protocol");
+ }
+
+ /** Test that {@code @Marshalled} annotation on {@link Message} map field
will fail generation. */
+ @Test
+ public void testMarshallableOnMessageArrayFieldFailGeneration() {
+ Compilation compilation = compile("InternalMessage.java",
"InternalMessageArrayContainer.java");
+
+ assertThat(compilation).failed();
+
+ assertThat(compilation).hadErrorContaining("Field \"msgArr\" is
Message. Must be written by communication protocol");
+ }
Review Comment:
ELEMENT_BLOBS is the second rejected kind and has no test. Please add one
for Collection<Message> with a Collection<byte[]> companion.
##########
modules/codegen/src/main/java/org/apache/ignite/internal/MessageMarshallerGenerator.java:
##########
@@ -1170,6 +1172,41 @@ private enum MarshalledKind {
return MarshalledKind.ELEMENT_BLOBS;
}
+ /** */
+ private void ensureCorrectlyAnnotated(VariableElement field, Marshalled
ann) {
+ boolean msgToBytes = false;
+
+ if (assignableFrom(field.asType(), msgType)) {
Review Comment:
Single-statement branch. Please drop the braces.
##########
modules/codegen/src/main/java/org/apache/ignite/internal/MessageMarshallerGenerator.java:
##########
@@ -1170,6 +1172,41 @@ private enum MarshalledKind {
return MarshalledKind.ELEMENT_BLOBS;
}
+ /** */
Review Comment:
The contract here is not obvious and the name promises a general check while
one rule is implemented. Please describe what is rejected and rename after that
rule.
##########
modules/codegen/src/main/java/org/apache/ignite/internal/MessageMarshallerGenerator.java:
##########
@@ -1170,6 +1172,41 @@ private enum MarshalledKind {
return MarshalledKind.ELEMENT_BLOBS;
}
+ /** */
+ private void ensureCorrectlyAnnotated(VariableElement field, Marshalled
ann) {
+ boolean msgToBytes = false;
+
+ if (assignableFrom(field.asType(), msgType)) {
+ msgToBytes = true;
+ }
+ else if (isCollection(field.asType())) {
+ DeclaredType type = (DeclaredType)field.asType();
+
+ List<? extends TypeMirror> typeArgs = type.getTypeArguments();
+
+ assert typeArgs.size() == 1;
+
+ msgToBytes = assignableFrom(typeArgs.get(0), msgType);
+ }
+ else if (field.asType() instanceof ArrayType) {
+ msgToBytes =
assignableFrom(((ArrayType)field.asType()).getComponentType(), msgType);
+ }
+ else if (isMap(field.asType()) && !ann.value().isEmpty()) {
+ DeclaredType type = (DeclaredType)field.asType();
+
+ List<? extends TypeMirror> typeArgs = type.getTypeArguments();
+
+ assert typeArgs.size() == 2;
+
+ msgToBytes = assignableFrom(typeArgs.get(0), msgType) ||
assignableFrom(typeArgs.get(1), msgType);
+ }
+
+ if (msgToBytes) {
+ throw new IllegalArgumentException("Field \"" + field + "\" is
Message. Must be written by communication protocol" +
Review Comment:
Throwing attaches the diagnostic to the class instead of the field, adds a
misleading "Failed to generate" prefix, and repeats per subclass for fields
from an abstract parent. Please report via
env.getMessager().printMessage(ERROR, msg, field), as this method already does
six lines above.
##########
modules/core/src/test/resources/codegen/InternalMessageContainer.java:
##########
@@ -0,0 +1,31 @@
+/*
+ * 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;
+
+import org.apache.ignite.plugin.extensions.communication.Message;
+
+/** */
+public class InternalMessageContainer implements Message {
+ /** */
+ @Marshalled("msgBytes")
+ InternalMessage msg;
+
+ /** */
+ @Order(value = 0)
Review Comment:
Three resources use @Order(value = 0) while InternalMessageMapContainer in
this same PR uses @Order(0). Please use the short form everywhere.
##########
modules/core/src/test/resources/codegen/InternalMessageMapContainer.java:
##########
@@ -0,0 +1,33 @@
+/*
+ * 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;
+
+import java.util.Map;
Review Comment:
Collection is unused and the two imports are out of alphabetical order.
Please remove it and swap the remaining ones.
##########
modules/codegen/src/main/java/org/apache/ignite/internal/MessageMarshallerGenerator.java:
##########
@@ -1170,6 +1172,41 @@ private enum MarshalledKind {
return MarshalledKind.ELEMENT_BLOBS;
}
+ /** */
+ private void ensureCorrectlyAnnotated(VariableElement field, Marshalled
ann) {
+ boolean msgToBytes = false;
+
+ if (assignableFrom(field.asType(), msgType)) {
+ msgToBytes = true;
+ }
+ else if (isCollection(field.asType())) {
+ DeclaredType type = (DeclaredType)field.asType();
+
+ List<? extends TypeMirror> typeArgs = type.getTypeArguments();
+
+ assert typeArgs.size() == 1;
+
+ msgToBytes = assignableFrom(typeArgs.get(0), msgType);
+ }
+ else if (field.asType() instanceof ArrayType) {
+ msgToBytes =
assignableFrom(((ArrayType)field.asType()).getComponentType(), msgType);
+ }
+ else if (isMap(field.asType()) && !ann.value().isEmpty()) {
+ DeclaredType type = (DeclaredType)field.asType();
+
+ List<? extends TypeMirror> typeArgs = type.getTypeArguments();
+
+ assert typeArgs.size() == 2;
+
+ msgToBytes = assignableFrom(typeArgs.get(0), msgType) ||
assignableFrom(typeArgs.get(1), msgType);
+ }
+
+ if (msgToBytes) {
+ throw new IllegalArgumentException("Field \"" + field + "\" is
Message. Must be written by communication protocol" +
+ ". Remove @" + Marshalled.class.getSimpleName() + " annotation
and remove corresponding byte[] field.");
Review Comment:
"Field X is Message" is inaccurate for collection, array and map fields, and
the companion is Collection<byte[]> for ELEMENT_BLOBS. Please reword to cover
the container cases and drop the byte[] wording.
##########
modules/core/src/test/resources/codegen/InternalMessage.java:
##########
@@ -0,0 +1,27 @@
+/*
+ * 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;
+
+import org.apache.ignite.plugin.extensions.communication.Message;
+
+/** */
+public class InternalMessage implements Message {
Review Comment:
Resources in this folder are named Test*Message / Wrong*Message, and these
read like production classes. Please rename to match.
--
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]
