Vladsz83 commented on code in PR #13095:
URL: https://github.com/apache/ignite/pull/13095#discussion_r3657743349
##########
modules/core/src/main/java/org/apache/ignite/spi/communication/tcp/internal/GridNioServerWrapper.java:
##########
@@ -834,18 +851,18 @@ private MessageFactory get() {
};
GridNioMessageReaderFactory readerFactory = new
GridNioMessageReaderFactory() {
- private IgniteSpiContext context;
+ private IgniteSpiContext spiCtx;
private MessageFormatter formatter;
- @Override public MessageReader reader(GridNioSession ses,
MessageFactory msgFactory)
+ @Override public MessageReader reader(GridNioSession ses,
MessageFactory<? extends Message> msgFactory)
throws IgniteCheckedException {
final IgniteSpiContext ctx =
stateProvider.getSpiContextWithoutInitialLatch();
- if (formatter == null || context != ctx) {
- context = ctx;
+ if (formatter == null || this.spiCtx != ctx) {
Review Comment:
No need of `this`. Below too.
##########
modules/core/src/main/java/org/apache/ignite/internal/managers/communication/GridIoManager.java:
##########
@@ -460,6 +461,9 @@ public void resetMetrics() {
msg.getClass().getName() + ". Most likely
GridCommunicationSpi is being used directly, " +
"which is illegal - make sure to send messages
only via GridProjection API.");
}
+ catch (IgniteCheckedException e) {
Review Comment:
Why changed?
##########
modules/core/pom.xml:
##########
@@ -243,6 +243,13 @@
<scope>test</scope>
</dependency>
+ <dependency>
+ <groupId>com.tngtech.archunit</groupId>
+ <artifactId>archunit-junit4</artifactId>
Review Comment:
It is used only in one or a couple tests. Can we rewrite these test so that
they would not use this additional library?
##########
modules/core/src/test/java/org/apache/ignite/internal/codegen/MarshallerCacheFreeUnmarshalTest.java:
##########
@@ -0,0 +1,110 @@
+/*
+ * 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.codegen;
+
+import java.util.Collection;
+import java.util.List;
+import java.util.Map;
+import com.tngtech.archunit.base.DescribedPredicate;
+import com.tngtech.archunit.core.domain.JavaClass;
+import com.tngtech.archunit.core.domain.JavaClasses;
+import com.tngtech.archunit.core.domain.JavaCodeUnit;
+import com.tngtech.archunit.core.domain.JavaMethodCall;
+import com.tngtech.archunit.core.importer.ClassFileImporter;
+import com.tngtech.archunit.core.importer.ImportOption;
+import com.tngtech.archunit.lang.ArchRule;
+import org.apache.ignite.internal.GridKernalContext;
+import org.apache.ignite.plugin.extensions.communication.MessageMarshaller;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import static com.tngtech.archunit.lang.syntax.ArchRuleDefinition.noClasses;
+
+/**
+ * A message is finish-unmarshalled in two passes — cache-free (via {@code
GridIoManager}) and cache-aware (via a
+ * subsystem) — and the mode-aware no-double-unmarshal check ({@link
MessageMarshaller.Dedup}) deliberately allows
Review Comment:
There is not `MessageMarshaller.Dedup`
##########
modules/core/src/main/java/org/apache/ignite/internal/managers/communication/GridIoMessage.java:
##########
@@ -140,6 +145,16 @@ public boolean skipOnTimeout() {
return skipOnTimeout;
}
+ /** Marks this message as marshalled. */
+ void markMarshalled() {
+ marshalled = true;
+ }
+
+ /** @return {@code true} if this message has been marshalled. */
+ boolean marshalled() {
Review Comment:
Is used only with asserts. Let's remove. We have the double0marshalling
check mechanics
##########
modules/core/src/main/java/org/apache/ignite/internal/plugin/AbstractMarshallableMessageFactoryProvider.java:
##########
@@ -18,78 +18,175 @@
package org.apache.ignite.internal.plugin;
import java.lang.reflect.Constructor;
+import java.util.function.Supplier;
import org.apache.ignite.IgniteException;
-import org.apache.ignite.configuration.IgniteConfiguration;
import org.apache.ignite.internal.MarshallableMessage;
import org.apache.ignite.internal.binary.BinaryMarshaller;
-import org.apache.ignite.internal.util.typedef.internal.U;
+import org.apache.ignite.internal.managers.communication.IgniteMessageFactory;
+import org.apache.ignite.internal.processors.cache.GridCacheMessage;
+import org.apache.ignite.internal.processors.cache.GridCacheMessageDeployer;
import org.apache.ignite.marshaller.Marshaller;
import org.apache.ignite.marshaller.jdk.JdkMarshaller;
import org.apache.ignite.plugin.extensions.communication.Message;
-import org.apache.ignite.plugin.extensions.communication.MessageFactory;
import
org.apache.ignite.plugin.extensions.communication.MessageFactoryProvider;
+import org.apache.ignite.plugin.extensions.communication.MessageMarshaller;
import org.apache.ignite.plugin.extensions.communication.MessageSerializer;
+import
org.apache.ignite.plugin.extensions.communication.NonMarshallableMessage;
+import org.jetbrains.annotations.Nullable;
/**
* An extension of {@link MessageFactoryProvider} allowing to use provided
schema-aware marshaller
- * and resolved class loader to register {@link MarshallableMessage}.
+ * to register {@link MarshallableMessage}.
*/
public abstract class AbstractMarshallableMessageFactoryProvider implements
MessageFactoryProvider {
+ /** Generated-companion constructors per message class, including cached
negative lookups. */
+ private static final ClassValue<Companions> COMPANIONS = new
ClassValue<>() {
+ @Override protected Companions computeValue(Class<?> cls) {
+ return new Companions(companionCtor(cls, "Serializer"),
companionCtor(cls, "Marshaller"), companionCtor(cls, "Deployer"));
+ }
+ };
+
/** Default schema-less marshaller. */
protected Marshaller dfltMarsh;
- /** Default class loader. */
- protected final ClassLoader dftlClsLdr = U.gridClassLoader();
-
/** Schema-aware marshaller like {@link BinaryMarshaller}. */
protected Marshaller schemaAwareMarsh;
- /** Resolved (configured) class loader like {@link
IgniteConfiguration#setClassLoader(ClassLoader)}. */
- protected ClassLoader resolvedClsLdr;
-
/**
* @param dfltMarsh Default schema-less marshaller like {@link
JdkMarshaller}.
* @param schemaAwareMarsh Schema-aware marshaller like {@link
BinaryMarshaller}.
- * @param resolvedClsLdr Resolved (configured) class loader like {@link
IgniteConfiguration#setClassLoader(ClassLoader)}.
*/
- public void init(Marshaller dfltMarsh, Marshaller schemaAwareMarsh,
ClassLoader resolvedClsLdr) {
+ public void init(Marshaller dfltMarsh, Marshaller schemaAwareMarsh) {
this.dfltMarsh = dfltMarsh;
this.schemaAwareMarsh = schemaAwareMarsh;
- this.resolvedClsLdr = resolvedClsLdr;
}
- /** Registers message automatically generating message supplier and
serializer. */
- protected static <T extends Message> void register(MessageFactory factory,
Class<T> cls, short id, Marshaller marsh,
- ClassLoader clsLrd) {
+ /** Registers a message with its generated serializer, marshaller (if
marshallable), and deployer (if any). */
+ protected static <T extends Message> void register(IgniteMessageFactory
factory, Class<T> cls, short id, Marshaller marsh) {
Constructor<T> ctor;
- MessageSerializer<T> serializer;
try {
ctor = cls.getConstructor();
+ }
+ catch (NoSuchMethodException e) {
+ throw new IgniteException("Failed to register message of type " +
cls.getSimpleName(), e);
+ }
+
+ register(factory, cls, id, () -> {
+ try {
+ return ctor.newInstance();
+ }
+ catch (Exception e) {
+ throw new IgniteException("Failed to create message of type "
+ cls.getSimpleName(), e);
+ }
+ }, marsh);
+ }
+
+ /**
+ * Registers a message with a caller-provided {@code supplier} and its
generated serializer, marshaller (if
+ * marshallable), and deployer (if any). Use this overload when {@code
cls} is package-private and so cannot be
+ * instantiated by reflection from this package — pass an in-package
{@code ::new} reference as {@code supplier}.
+ */
+ protected static <T extends Message> void register(IgniteMessageFactory
factory, Class<T> cls, short id,
+ Supplier<Message> supplier, Marshaller marsh) {
+ MessageSerializer<T> serializer = requireGenerated(cls, "Serializer",
marsh);
+
+ // A MarshallableMessage always gets a generated marshaller (the hook
call alone is a statement), so its
+ // absence is a build problem. For the rest the generator skips
statement-free marshallers, so absence
+ // legitimately means "nothing to marshal"; the message and its
companions ship in the same jar, hence
+ // a missing class cannot be a packaging accident that spares the
(required) serializer.
+ MessageMarshaller<T> marshaller =
NonMarshallableMessage.class.isAssignableFrom(cls)
+ ? null
+ : MarshallableMessage.class.isAssignableFrom(cls)
+ ? requireGenerated(cls, "Marshaller", marsh)
+ : loadGenerated(cls, "Marshaller", marsh);
+
+ // Deployers are generated for GridCacheMessage subclasses only, so
the class lookup is skipped for the rest;
+ // a DeployableMessage left without a deployer is then rejected at
registration.
+ GridCacheMessageDeployer deployer =
GridCacheMessage.class.isAssignableFrom(cls)
Review Comment:
Might be `GridCacheMessageDeployer<?>`
##########
modules/core/src/main/java/org/apache/ignite/internal/plugin/AbstractMarshallableMessageFactoryProvider.java:
##########
@@ -18,78 +18,175 @@
package org.apache.ignite.internal.plugin;
import java.lang.reflect.Constructor;
+import java.util.function.Supplier;
import org.apache.ignite.IgniteException;
-import org.apache.ignite.configuration.IgniteConfiguration;
import org.apache.ignite.internal.MarshallableMessage;
import org.apache.ignite.internal.binary.BinaryMarshaller;
-import org.apache.ignite.internal.util.typedef.internal.U;
+import org.apache.ignite.internal.managers.communication.IgniteMessageFactory;
+import org.apache.ignite.internal.processors.cache.GridCacheMessage;
+import org.apache.ignite.internal.processors.cache.GridCacheMessageDeployer;
import org.apache.ignite.marshaller.Marshaller;
import org.apache.ignite.marshaller.jdk.JdkMarshaller;
import org.apache.ignite.plugin.extensions.communication.Message;
-import org.apache.ignite.plugin.extensions.communication.MessageFactory;
import
org.apache.ignite.plugin.extensions.communication.MessageFactoryProvider;
+import org.apache.ignite.plugin.extensions.communication.MessageMarshaller;
import org.apache.ignite.plugin.extensions.communication.MessageSerializer;
+import
org.apache.ignite.plugin.extensions.communication.NonMarshallableMessage;
+import org.jetbrains.annotations.Nullable;
/**
* An extension of {@link MessageFactoryProvider} allowing to use provided
schema-aware marshaller
- * and resolved class loader to register {@link MarshallableMessage}.
+ * to register {@link MarshallableMessage}.
*/
public abstract class AbstractMarshallableMessageFactoryProvider implements
MessageFactoryProvider {
+ /** Generated-companion constructors per message class, including cached
negative lookups. */
+ private static final ClassValue<Companions> COMPANIONS = new
ClassValue<>() {
+ @Override protected Companions computeValue(Class<?> cls) {
+ return new Companions(companionCtor(cls, "Serializer"),
companionCtor(cls, "Marshaller"), companionCtor(cls, "Deployer"));
+ }
+ };
+
/** Default schema-less marshaller. */
protected Marshaller dfltMarsh;
- /** Default class loader. */
- protected final ClassLoader dftlClsLdr = U.gridClassLoader();
-
/** Schema-aware marshaller like {@link BinaryMarshaller}. */
protected Marshaller schemaAwareMarsh;
- /** Resolved (configured) class loader like {@link
IgniteConfiguration#setClassLoader(ClassLoader)}. */
- protected ClassLoader resolvedClsLdr;
-
/**
* @param dfltMarsh Default schema-less marshaller like {@link
JdkMarshaller}.
* @param schemaAwareMarsh Schema-aware marshaller like {@link
BinaryMarshaller}.
- * @param resolvedClsLdr Resolved (configured) class loader like {@link
IgniteConfiguration#setClassLoader(ClassLoader)}.
*/
- public void init(Marshaller dfltMarsh, Marshaller schemaAwareMarsh,
ClassLoader resolvedClsLdr) {
+ public void init(Marshaller dfltMarsh, Marshaller schemaAwareMarsh) {
this.dfltMarsh = dfltMarsh;
this.schemaAwareMarsh = schemaAwareMarsh;
- this.resolvedClsLdr = resolvedClsLdr;
}
- /** Registers message automatically generating message supplier and
serializer. */
- protected static <T extends Message> void register(MessageFactory factory,
Class<T> cls, short id, Marshaller marsh,
- ClassLoader clsLrd) {
+ /** Registers a message with its generated serializer, marshaller (if
marshallable), and deployer (if any). */
+ protected static <T extends Message> void register(IgniteMessageFactory
factory, Class<T> cls, short id, Marshaller marsh) {
Constructor<T> ctor;
- MessageSerializer<T> serializer;
try {
ctor = cls.getConstructor();
+ }
+ catch (NoSuchMethodException e) {
+ throw new IgniteException("Failed to register message of type " +
cls.getSimpleName(), e);
+ }
+
+ register(factory, cls, id, () -> {
+ try {
+ return ctor.newInstance();
+ }
+ catch (Exception e) {
+ throw new IgniteException("Failed to create message of type "
+ cls.getSimpleName(), e);
+ }
+ }, marsh);
+ }
+
+ /**
+ * Registers a message with a caller-provided {@code supplier} and its
generated serializer, marshaller (if
+ * marshallable), and deployer (if any). Use this overload when {@code
cls} is package-private and so cannot be
+ * instantiated by reflection from this package — pass an in-package
{@code ::new} reference as {@code supplier}.
+ */
+ protected static <T extends Message> void register(IgniteMessageFactory
factory, Class<T> cls, short id,
+ Supplier<Message> supplier, Marshaller marsh) {
+ MessageSerializer<T> serializer = requireGenerated(cls, "Serializer",
marsh);
+
+ // A MarshallableMessage always gets a generated marshaller (the hook
call alone is a statement), so its
+ // absence is a build problem. For the rest the generator skips
statement-free marshallers, so absence
+ // legitimately means "nothing to marshal"; the message and its
companions ship in the same jar, hence
+ // a missing class cannot be a packaging accident that spares the
(required) serializer.
+ MessageMarshaller<T> marshaller =
NonMarshallableMessage.class.isAssignableFrom(cls)
+ ? null
Review Comment:
Lets rewrite, remove double `? :`
##########
modules/core/src/test/java/org/apache/ignite/internal/codegen/MarshallerCacheFreeUnmarshalTest.java:
##########
@@ -0,0 +1,110 @@
+/*
+ * 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.codegen;
+
+import java.util.Collection;
+import java.util.List;
+import java.util.Map;
+import com.tngtech.archunit.base.DescribedPredicate;
+import com.tngtech.archunit.core.domain.JavaClass;
+import com.tngtech.archunit.core.domain.JavaClasses;
+import com.tngtech.archunit.core.domain.JavaCodeUnit;
+import com.tngtech.archunit.core.domain.JavaMethodCall;
+import com.tngtech.archunit.core.importer.ClassFileImporter;
+import com.tngtech.archunit.core.importer.ImportOption;
+import com.tngtech.archunit.lang.ArchRule;
+import org.apache.ignite.internal.GridKernalContext;
+import org.apache.ignite.plugin.extensions.communication.MessageMarshaller;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import static com.tngtech.archunit.lang.syntax.ArchRuleDefinition.noClasses;
+
+/**
+ * A message is finish-unmarshalled in two passes — cache-free (via {@code
GridIoManager}) and cache-aware (via a
+ * subsystem) — and the mode-aware no-double-unmarshal check ({@link
MessageMarshaller.Dedup}) deliberately allows
+ * both. So whatever the cache-free pass does must stay correct when the
cache-aware pass runs it again.
+ *
+ * <p>The distinction is the side effect. A plain {@code @Marshalled} field
unmarshals by <b>assignment</b>
Review Comment:
Suggestion: Lets simplify and shorten this doc.
##########
modules/core/src/test/java/org/apache/ignite/internal/codegen/MarshallerCacheFreeUnmarshalTest.java:
##########
@@ -0,0 +1,110 @@
+/*
+ * 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.codegen;
+
+import java.util.Collection;
+import java.util.List;
+import java.util.Map;
+import com.tngtech.archunit.base.DescribedPredicate;
+import com.tngtech.archunit.core.domain.JavaClass;
+import com.tngtech.archunit.core.domain.JavaClasses;
+import com.tngtech.archunit.core.domain.JavaCodeUnit;
+import com.tngtech.archunit.core.domain.JavaMethodCall;
+import com.tngtech.archunit.core.importer.ClassFileImporter;
+import com.tngtech.archunit.core.importer.ImportOption;
+import com.tngtech.archunit.lang.ArchRule;
+import org.apache.ignite.internal.GridKernalContext;
+import org.apache.ignite.plugin.extensions.communication.MessageMarshaller;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import static com.tngtech.archunit.lang.syntax.ArchRuleDefinition.noClasses;
+
+/**
+ * A message is finish-unmarshalled in two passes — cache-free (via {@code
GridIoManager}) and cache-aware (via a
+ * subsystem) — and the mode-aware no-double-unmarshal check ({@link
MessageMarshaller.Dedup}) deliberately allows
+ * both. So whatever the cache-free pass does must stay correct when the
cache-aware pass runs it again.
+ *
+ * <p>The distinction is the side effect. A plain {@code @Marshalled} field
unmarshals by <b>assignment</b>
+ * ({@code msg.x = ...}); assigning the same value twice is a no-op, so it is
idempotent and needs no guarding. A
+ * {@code @Marshalled} / {@code @Marshalled} field unmarshals by
<b>mutation</b> ({@code collection.add} /
+ * {@code map.put}); running that twice appends the elements twice — a
doubled, corrupt collection. So a collection/map
+ * mutation is the one thing that must not run in both passes, and the
generator keeps it in the cache-aware pass only.
+ *
+ * <p>That is exactly what this rule checks — purely the mutating side effect,
not the fields: the cache-free
+ * {@code unmarshal(msg, kctx)} overload of every generated marshaller must
not call {@link Collection#add} or
+ * {@link Map#put}. Assignments are left unchecked (they can't break); a
generator regression that moved an append into
+ * the cache-free pass — a real double-add the runtime check can't see, since
it allows both passes — fails here.
+ */
+public class MarshallerCacheFreeUnmarshalTest {
+ /**
+ * The two-arg, cache-free {@code unmarshal} overload. The cache-aware
overload takes a cache context and a
+ * class loader (four args); {@code unmarshalNio} shares the two-arg
shape, so the name is matched too.
+ */
+ private static final DescribedPredicate<JavaCodeUnit> CACHE_FREE_UNMARSHAL
=
+ new DescribedPredicate<>("cache-free unmarshal(msg, kctx)") {
+ @Override public boolean test(JavaCodeUnit unit) {
+ List<JavaClass> params = unit.getRawParameterTypes();
+
+ return "unmarshal".equals(unit.getName())
+ && params.size() == 2
+ && params.get(1).isEquivalentTo(GridKernalContext.class);
+ }
+ };
+
+ /** A {@link Collection#add} or {@link Map#put} append made from within
the cache-free {@code unmarshal}. */
+ private static final DescribedPredicate<JavaMethodCall>
CACHE_FREE_UNMARSHAL_APPEND =
+ new DescribedPredicate<>("Collection.add / Map.put from the cache-free
unmarshal pass") {
+ @Override public boolean test(JavaMethodCall call) {
+ if (!CACHE_FREE_UNMARSHAL.test(call.getOrigin()))
+ return false;
+
+ JavaClass owner = call.getTarget().getOwner();
+ String mtd = call.getTarget().getName();
+
+ return owner.isAssignableTo(Collection.class) &&
"add".equals(mtd)
+ || owner.isAssignableTo(Map.class) && "put".equals(mtd);
+ }
+ };
+
+ /** All production classes on the classpath (the generated marshallers
among them), excluding JARs. */
+ private static JavaClasses classes;
+
+ /** */
+ @BeforeClass
+ public static void importClasses() {
+ classes = new ClassFileImporter()
+ .withImportOption(ImportOption.Predefined.DO_NOT_INCLUDE_JARS)
+ .importPackages("org.apache.ignite");
+ }
+
+ /** The cache-free {@code unmarshal} overload must not append to
collections/maps; those are cache-pass only. */
+ @Test
+ public void cacheFreeFinishDoesNotAppendToCollections() {
+ ArchRule rule = noClasses()
+ .that()
+ .areAssignableTo(MessageMarshaller.class)
+ .should()
+ .callMethodWhere(CACHE_FREE_UNMARSHAL_APPEND)
+ .because("@Marshalled/@Marshalled appends are non-idempotent and
run only in the cache-aware " +
Review Comment:
Same: `@Marshalled/@Marshalled`
##########
modules/core/src/test/java/org/apache/ignite/internal/codegen/MarshallerCacheFreeUnmarshalTest.java:
##########
@@ -0,0 +1,110 @@
+/*
+ * 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.codegen;
+
+import java.util.Collection;
+import java.util.List;
+import java.util.Map;
+import com.tngtech.archunit.base.DescribedPredicate;
+import com.tngtech.archunit.core.domain.JavaClass;
+import com.tngtech.archunit.core.domain.JavaClasses;
+import com.tngtech.archunit.core.domain.JavaCodeUnit;
+import com.tngtech.archunit.core.domain.JavaMethodCall;
+import com.tngtech.archunit.core.importer.ClassFileImporter;
+import com.tngtech.archunit.core.importer.ImportOption;
+import com.tngtech.archunit.lang.ArchRule;
+import org.apache.ignite.internal.GridKernalContext;
+import org.apache.ignite.plugin.extensions.communication.MessageMarshaller;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import static com.tngtech.archunit.lang.syntax.ArchRuleDefinition.noClasses;
+
+/**
+ * A message is finish-unmarshalled in two passes — cache-free (via {@code
GridIoManager}) and cache-aware (via a
+ * subsystem) — and the mode-aware no-double-unmarshal check ({@link
MessageMarshaller.Dedup}) deliberately allows
+ * both. So whatever the cache-free pass does must stay correct when the
cache-aware pass runs it again.
+ *
+ * <p>The distinction is the side effect. A plain {@code @Marshalled} field
unmarshals by <b>assignment</b>
+ * ({@code msg.x = ...}); assigning the same value twice is a no-op, so it is
idempotent and needs no guarding. A
+ * {@code @Marshalled} / {@code @Marshalled} field unmarshals by
<b>mutation</b> ({@code collection.add} /
Review Comment:
`{@code @Marshalled} / {@code @Marshalled}` ?
##########
modules/core/src/test/java/org/apache/ignite/internal/codegen/MessageProcessorTest.java:
##########
@@ -352,6 +401,154 @@ public void
testCompressAnnotationFailsForUnsupportedTypes() {
assertThat(compilation).hadErrorContaining("Compress annotation is
used for an unsupported type: java.util.List");
}
+ /** Verifies that {@code @NioField} on a non-{@link Message}-typed field
is a compilation error. */
+ @Test
+ public void testNioFieldOnNonMessageTypeFails() {
+ Compilation compilation = compile("NioFieldOnNonMessageMessage.java");
+
+ assertThat(compilation).failed();
+ assertThat(compilation).hadErrorContaining("@NioField has no effect on
non-Message field");
+ }
+
+ /** Verifies that {@code @NioField} on a message needing a cache object
context is a compilation error. */
+ @Test
+ public void testNioFieldNeedingCacheContextFails() {
+ Compilation compilation = compile("NioFieldNeedsCtxMessage.java");
+
+ assertThat(compilation).failed();
+ assertThat(compilation).hadErrorContaining("needs a cache object
context to unmarshal");
+ }
+
+ /** Verifies that {@code @Marshalled} generates {@code U.unmarshal} with a
blank line before the null-out. */
+ @Test
+ public void testMarshalledMessage() {
+ Compilation compilation = compile("TestMarshalledMessage.java");
+
+ assertThat(compilation).succeeded();
+
+ assertEquals(2, compilation.generatedSourceFiles().size());
+
+ assertThat(compilation)
+
.generatedSourceFile("org.apache.ignite.internal.TestMarshalledMessageSerializer")
+
.hasSourceEquivalentTo(javaFile("TestMarshalledMessageSerializer.java"));
+
+ assertThat(compilation)
+
.generatedSourceFile("org.apache.ignite.internal.TestMarshalledMessageMarshaller")
+
.hasSourceEquivalentTo(javaFile("TestMarshalledMessageMarshaller.java"));
+ }
+
+ /** Verifies that {@code @Marshalled} generates Set reconstruction in
FINISH_CACHE mode. */
+ @Test
+ public void testMarshalledCollectionMessage() {
+ Compilation compilation =
compile("TestMarshalledCollectionMessage.java");
+
+ assertThat(compilation).succeeded();
+
+ assertEquals(2, compilation.generatedSourceFiles().size());
+
+ assertThat(compilation)
+
.generatedSourceFile("org.apache.ignite.internal.TestMarshalledCollectionMessageSerializer")
+
.hasSourceEquivalentTo(javaFile("TestMarshalledCollectionMessageSerializer.java"));
+
+ assertThat(compilation)
+
.generatedSourceFile("org.apache.ignite.internal.TestMarshalledCollectionMessageMarshaller")
+
.hasSourceEquivalentTo(javaFile("TestMarshalledCollectionMessageMarshaller.java"));
+ }
+
+ /** Verifies that {@code @Marshalled} generates Map reconstruction in
FINISH_CACHE mode. */
Review Comment:
Where does it check `reconstruction in FINISH_CACHE mode.` ?
##########
modules/core/src/test/java/org/apache/ignite/internal/codegen/MessageSerializationArchitectureTest.java:
##########
@@ -0,0 +1,149 @@
+/*
+ * 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.codegen;
+
+import com.tngtech.archunit.base.DescribedPredicate;
+import com.tngtech.archunit.core.domain.JavaClasses;
+import com.tngtech.archunit.core.domain.JavaMethodCall;
+import com.tngtech.archunit.core.domain.JavaModifier;
+import com.tngtech.archunit.core.domain.properties.HasOwner;
+import com.tngtech.archunit.core.importer.ClassFileImporter;
+import com.tngtech.archunit.core.importer.ImportOption;
+import com.tngtech.archunit.lang.ArchRule;
+import org.apache.ignite.internal.managers.communication.MessageMarshalling;
+import org.apache.ignite.internal.processors.cache.GridCacheMessageDeployer;
+import org.apache.ignite.internal.util.nio.MessageSerialization;
+import org.apache.ignite.plugin.extensions.communication.MessageMarshaller;
+import org.apache.ignite.plugin.extensions.communication.MessageSerializer;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import static com.tngtech.archunit.core.domain.JavaCall.Predicates.target;
+import static
com.tngtech.archunit.core.domain.JavaClass.Predicates.assignableTo;
+import static com.tngtech.archunit.lang.syntax.ArchRuleDefinition.noClasses;
+
+/**
+ * Verifies that instance methods of {@link MessageSerializer}, {@link
MessageMarshaller} and
+ * {@link GridCacheMessageDeployer} are only called from classes that
implement these interfaces (i.e. generated
+ * serializers/marshallers/deployers and their hand-written wrappers). All
other code must use the static
+ * convenience methods:
+ * <ul>
+ * <li>{@link MessageSerialization#writeTo}</li>
+ * <li>{@link MessageSerialization#readFrom}</li>
+ * <li>{@link MessageMarshalling#marshal}</li>
+ * <li>{@code MessageMarshalling.unmarshal}</li>
+ * <li>static {@code GridCacheMessageDeployer.deploy(factory, msg,
ctx)}</li>
+ * </ul>
+ *
+ * <p>The rules key on whether the called method is {@code static}, not on its
name — so any instance method added
+ * to these interfaces is covered automatically.
+ */
+public class MessageSerializationArchitectureTest {
+ /** Matches method calls that resolve to a non-static (instance) method. */
+ private static final DescribedPredicate<JavaMethodCall> TO_INSTANCE_METHOD
=
+ new DescribedPredicate<>("to instance method") {
+ @Override public boolean test(JavaMethodCall call) {
+ return call.getTarget().resolveMember()
+ .map(m -> !m.getModifiers().contains(JavaModifier.STATIC))
+ .orElse(false);
+ }
+ };
+
+ /** Classes under analysis: all production + test sources on the
classpath, excluding JARs. */
+ private static JavaClasses classes;
+
+ /** */
+ @BeforeClass
+ public static void importClasses() {
+ classes = new ClassFileImporter()
+ .withImportOption(ImportOption.Predefined.DO_NOT_INCLUDE_JARS)
+ .importPackages("org.apache.ignite");
+ }
+
+ /**
+ * Instance methods of {@link MessageSerializer} ({@code writeTo}, {@code
readFrom}) must only be
+ * called from within classes that themselves implement {@link
MessageSerializer} — i.e. generated
+ * serializers and hand-written wrappers that delegate to the underlying
serializer — or the
+ * {@link MessageSerialization} dispatcher that resolves and calls them.
+ *
+ * Everyone else must use the static {@link MessageSerialization} entry
points.
+ */
+ @Test
+ public void serializerInstanceMethodsOnlyCalledFromImplementations() {
+ ArchRule rule = noClasses()
+ .that()
+ // Exclude MessageSerializer implementations (generated +
wrappers) and the dispatch facade.
+ .areNotAssignableTo(MessageSerializer.class)
+ .and().areNotAssignableTo(MessageSerialization.class)
+ .should()
+ .callMethodWhere(TO_INSTANCE_METHOD
+
.and(target(HasOwner.Predicates.With.owner(assignableTo(MessageSerializer.class))))
+ )
+ .because("Use static MessageSerialization.writeTo(factory, msg,
writer) and " +
+ "MessageSerialization.readFrom(factory, msg, reader) instead
of calling instance methods directly.");
+
+ rule.check(classes);
+ }
+
+ /**
+ * Instance methods of {@link MessageMarshaller} ({@code marshal}, {@code
unmarshal}) must
+ * only be called from within classes that themselves implement {@link
MessageMarshaller} — i.e. generated
+ * marshallers and hand-written wrappers that delegate to the underlying
marshaller — or the
+ * {@link MessageMarshalling} dispatcher that resolves and calls them.
+ *
Review Comment:
`<p>` for empty lines, next lines?
##########
modules/core/src/test/java/org/apache/ignite/internal/codegen/MessageSerializationArchitectureTest.java:
##########
@@ -0,0 +1,149 @@
+/*
+ * 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.codegen;
+
+import com.tngtech.archunit.base.DescribedPredicate;
+import com.tngtech.archunit.core.domain.JavaClasses;
+import com.tngtech.archunit.core.domain.JavaMethodCall;
+import com.tngtech.archunit.core.domain.JavaModifier;
+import com.tngtech.archunit.core.domain.properties.HasOwner;
+import com.tngtech.archunit.core.importer.ClassFileImporter;
+import com.tngtech.archunit.core.importer.ImportOption;
+import com.tngtech.archunit.lang.ArchRule;
+import org.apache.ignite.internal.managers.communication.MessageMarshalling;
+import org.apache.ignite.internal.processors.cache.GridCacheMessageDeployer;
+import org.apache.ignite.internal.util.nio.MessageSerialization;
+import org.apache.ignite.plugin.extensions.communication.MessageMarshaller;
+import org.apache.ignite.plugin.extensions.communication.MessageSerializer;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import static com.tngtech.archunit.core.domain.JavaCall.Predicates.target;
+import static
com.tngtech.archunit.core.domain.JavaClass.Predicates.assignableTo;
+import static com.tngtech.archunit.lang.syntax.ArchRuleDefinition.noClasses;
+
+/**
+ * Verifies that instance methods of {@link MessageSerializer}, {@link
MessageMarshaller} and
+ * {@link GridCacheMessageDeployer} are only called from classes that
implement these interfaces (i.e. generated
+ * serializers/marshallers/deployers and their hand-written wrappers). All
other code must use the static
+ * convenience methods:
+ * <ul>
+ * <li>{@link MessageSerialization#writeTo}</li>
+ * <li>{@link MessageSerialization#readFrom}</li>
+ * <li>{@link MessageMarshalling#marshal}</li>
+ * <li>{@code MessageMarshalling.unmarshal}</li>
+ * <li>static {@code GridCacheMessageDeployer.deploy(factory, msg,
ctx)}</li>
+ * </ul>
+ *
+ * <p>The rules key on whether the called method is {@code static}, not on its
name — so any instance method added
+ * to these interfaces is covered automatically.
+ */
+public class MessageSerializationArchitectureTest {
Review Comment:
Can we join it with `MarshallerCacheFreeUnmarshallTest`?
##########
modules/core/src/test/java/org/apache/ignite/internal/codegen/MessageSerializationArchitectureTest.java:
##########
@@ -0,0 +1,149 @@
+/*
+ * 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.codegen;
+
+import com.tngtech.archunit.base.DescribedPredicate;
+import com.tngtech.archunit.core.domain.JavaClasses;
+import com.tngtech.archunit.core.domain.JavaMethodCall;
+import com.tngtech.archunit.core.domain.JavaModifier;
Review Comment:
We bring new dependency/library only for tests, for 2 tests and only to
check where we call own methods. I doubt it worths. @shishkovilja WDYT? If we
aren'tsure how we call methods, probably it is design-related and should be
reconsidered in general.
--
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]
