anton-vinogradov commented on code in PR #13095: URL: https://github.com/apache/ignite/pull/13095#discussion_r3664295618
########## 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: The three rules pin the central invariant of this change: nobody outside the generated companions and the static facades may call `MessageSerializer.writeTo/readFrom`, `MessageMarshaller.marshal/unmarshal` or `GridCacheMessageDeployer.deploy` directly. That is not a stylistic wish — a direct call bypasses the dispatcher and, for the marshalling side, silently moves payload (de)serialization back onto the NIO thread and around the marshal-once contract. The rules have already earned their keep on this branch: every master merge that touched `GridNioServer` or the Calcite tests brought back a direct `msgSer.writeTo(...)` call, and the test caught it locally instead of in a suite run. There is no design-level way to enforce it with the compiler: `MessageSerializer` is a public SPI, its implementations are generated into other packages, and `factory.serializer(...)` is called from `GridNioServerWrapper` (core) and `MessageSerialization` (nio), so the instance methods cannot be package-private or hidden. Writing it without the library means bytecode analysis by hand (reflection cannot see call sites) — a home-grown ArchUnit of a couple hundred lines to maintain. The dependency is `test`-scoped and never ships; `modules/core/pom.xml` already carries `com.google.testing.compile` for exactly the same reason — a test-only library for one narrow check, and that one comes from master. If "no new dependencies" is the priority, I'd rather file a follow-up to rewrite these three rules without the library than drop the check — losing it means the next such regression lands in master unnoticed. WDYT? ########## modules/core/pom.xml: ########## @@ -243,6 +243,13 @@ <scope>test</scope> </dependency> + <dependency> + <groupId>com.tngtech.archunit</groupId> + <artifactId>archunit-junit4</artifactId> Review Comment: Same topic as the thread in MessageSerializationArchitectureTest — answered there. Short version: it is `test`-scoped, it guards the invariant that everything goes through the static dispatchers (a direct instance call silently returns payload (de)serialization to the NIO thread), and it has already caught that regression on every master merge that touched GridNioServer. `modules/core/pom.xml` already has `com.google.testing.compile` as a test-only library for a similarly narrow check. Happy to file a follow-up to reimplement the rules without ArchUnit if you prefer, but I would not drop the check itself. ########## 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: They check different things: `MarshallerCacheFreeUnmarshalTest` verifies runtime behaviour of one path (a cache-free unmarshal leaves `@Marshalled` payload untouched and the cache-aware pass finishes it), while this class states three architectural rules over the whole `org.apache.ignite` class graph. Merging them would put a class-graph scan into a behavioural test and drag the ArchUnit import into it — the opposite of the concern in the other thread. Kept separate. -- 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]
