IGNITE-28767 Use Message DTO to transfer QueryEntity by shishkovilja · Pull Request #13236 · apache/ignite · GitHub
Skip to content

IGNITE-28767 Use Message DTO to transfer QueryEntity#13236

Open
shishkovilja wants to merge 1 commit into
apache:masterfrom
shishkovilja:ignite-28767
Open

IGNITE-28767 Use Message DTO to transfer QueryEntity#13236
shishkovilja wants to merge 1 commit into
apache:masterfrom
shishkovilja:ignite-28767

Conversation

@shishkovilja

@shishkovilja shishkovilja commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

Added duplicate messages because classes belong to the public API:
QueryEntity -> QueryEntityMessage
QueryEntityEx -> QueryEntityExMessage

Removed Serializable and MarshallableMessage contracts from schema operation messages:

  • SchemaAbstractOperation
  • SchemaAddQueryEntityOperation
  • SchemaAlterTableAddColumnOperation
  • SchemaAlterTableDropColumnOperation
  • SchemaIndexCreateOperation
  • SchemaIndexDropOperation
  • QueryField

@ignitetcbot

Copy link
Copy Markdown
Contributor

@ignitetcbot

Copy link
Copy Markdown
Contributor

TCBot Test Analysis

Possible Blockers (0)

No blockers found.

New Tests (0)

No new tests found.

@anton-vinogradov anton-vinogradov left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the migration. The bulk of it cleanly follows the IEP-132 "format-preserving" pattern (dropping Serializable/serialVersionUID without changing the @Order layout), and I verified the field mapping is complete — QueryEntity (13 fields), QueryEntityEx (7), QueryIndex (4) are all carried, and the QueryEntityEx#notNullFields getter/setter override is handled correctly by the explicit setNotNullFields(...) in QueryEntityExMessage#toEntity().

Two things I'd like to see before merge:

  1. Tests. There is currently no coverage for the new serialization path. A round-trip test (QueryEntity/QueryEntityEx*Message → marshal → unmarshal → toEntity(), asserting equals) with non-empty defaultFieldValues would guard the core of this change.
  2. A reflection parity-guard so a field added to QueryEntity/QueryEntityEx later can't be silently dropped from the hand-written *Message twins.

A few smaller points inline (one unrelated change to AuthentificationDataBagItem, a no-op withSchemawithNoSchema flip, and a stale comment).

/**
* Message for {@link QueryEntity} transfer.
*/
public class QueryEntityMessage implements MarshallableMessage {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing tests (blocker, imo). There's no test exercising this serialization path. Could you add a round-trip test: build a QueryEntity and a QueryEntityEx with all fields populated (incl. a non-empty defaultFieldValues of a non-trivial type), convert to *Message, run prepare/finish marshal, toEntity() back, and assert equals? QueryEntity/QueryEntityEx already have correct equals/hashCode, so this is cheap and would protect the whole change.

/**
* @param qryEntity Original {@link QueryEntity}.
*/
public QueryEntityMessage(QueryEntity qryEntity) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Guard the hand-written clone against drift. QueryEntityMessage/QueryEntityExMessage duplicate QueryEntity/QueryEntityEx field-by-field in two directions, with no compile-time link to the source classes. When someone later adds a field to the public QueryEntity, it's easy to miss the twin and silently drop it on the wire. The mapping is complete today — to keep it complete, consider a small reflection-based parity test asserting every data field of QueryEntity/QueryEntityEx is represented here.

withNoSchema(UserAuthenticateResponseMessage.class);
withNoSchema(TcpDiscoveryAuthFailedMessage.class);
withSchema(AuthentificationDataBagItem.class);
withNoSchema(AuthentificationDataBagItem.class);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change looks out of scope for IGNITE-28767AuthentificationDataBagItem is auth/users, not QueryEntity. It also appears to be a no-op: it's a plain (non-MarshallableMessage) Message, so the registered marshaller is never used — nested messages resolve their own marshaller by directType, and the generated marshaller only takes a Marshaller when marshallable || hasMarshalled. Could we drop it from this PR?

withSchema(CacheContinuousQueryEntry.class);
withNoSchema(QueryInlineSizesDataBagItem.class);
withSchema(QueryProposalsDataBagItem.class);
withNoSchema(QueryProposalsDataBagItem.class);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same reasoning as the AuthentificationDataBagItem flip below: QueryProposalsDataBagItem is a plain Message, so withSchema vs withNoSchema has no functional effect here (its registered marshaller is unused). Harmless, but it's diff noise — either drop it or move both flips to a separate cleanup commit with a one-line rationale.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, in fact it is a tech debt of: 13157

@Order(8)
String tableName;

/** Fields that must have non-null value. NB: DO NOT remove underscore to avoid clashes with QueryEntityEx. */

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: this comment was copied from QueryEntity, but here the field is notNullFields (no underscore) and QueryEntityExMessage doesn't redeclare it, so the "DO NOT remove underscore" note no longer applies and is misleading. Suggest dropping it or replacing with a plain description.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, copy paste artifacts.

/** Serialized form of query entities. */
public class SchemaAddQueryEntityOperation extends SchemaAbstractOperation {
/** Query entities. */
@Order(0)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just flagging (not blocking): this is the only change in the PR that alters the on-wire representation of @Order(0) (previously a Java-serialized byte[], now a Collection<Message>). It's safe for OSS, where mixed-version discovery isn't supported (old nodes are refused at join), but it diverges from the IEP-132 "binary representation unchanged" property the rest of the series relies on. Worth a note in the PR description as an intentional format-breaking change so the rolling-upgrade track is aware.

/** {@inheritDoc} */
@Override public void prepareMarshal(Marshaller marsh) throws IgniteCheckedException {
if (!F.isEmpty(dfltFieldValues))
dfltFieldValuesBytes = U.marshal(marsh, dfltFieldValues);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth a line in the PR description: defaultFieldValues are arbitrary user objects that can't be a Message, so they still go through U.marshal (Java serialization). That's unavoidable, but it means this particular payload isn't actually moved off Java serialization by the change — and it's exactly the part the round-trip test should cover with a non-trivial value.

/** */
private static final long serialVersionUID = 0L;

public abstract class SchemaAbstractOperation implements Message {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow-up, non-blocking: now that the operation isn't Serializable, its discovery container (SchemaAbstractDiscoveryMessage, SchemaProposeDiscoveryMessage) still declares implements Serializable / serialVersionUID. AFAICT it's vestigial — both TCP and ZK discovery marshal custom messages via the Message framework, not JdkMarshaller, so nothing Java-serializes the op. For consistency it'd be nice to drop the dead Serializable from those containers too, in a separate cleanup.

.setKeyFieldName(keyFieldName)
.setValueFieldName(valFieldName)
.setFields(fields)
.setKeyFields(!F.isEmpty(keyFields) ? new LinkedHashSet<>(List.of(keyFields)) : null)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: a non-null-but-empty keyFields/idxs round-trips to null (empty-check in, null out). Almost certainly harmless since the defaults are null, but worth being aware of if an equals-based test is added.


/** {@inheritDoc} */
@Override public QueryEntity toEntity() {
QueryEntityEx qryEntity = new QueryEntityEx(super.toEntity());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow-up to my earlier review (the "add tests" comment): while writing a round-trip test I hit a real asymmetry here that I think is worth a look.

QueryEntityEx overrides getNotNullFields()/setNotNullFields() to use its own notNullFields field, which shadows the base QueryEntity._notNullFields. super.toEntity() (line 74) populates the base _notNullFields from the message, and line 76 then sets the Ex field — so after a round-trip a QueryEntityEx always has base._notNullFields == ex.notNullFields.

But a normally-built QueryEntityEx has base._notNullFields == null (the Ex setter only touches the Ex field) while ex.notNullFields is set. The old Java-serialized path preserved both fields independently; this DTO path normalizes the base field. Net effect: a normally-built QueryEntityEx is no longer .equals() to its round-tripped copy, because QueryEntity.equals()/hashCode() compare the shadowed base _notNullFields.

It's easy to miss because QueryEntityEx.toString() doesn't print that field — the two objects render identically while being unequal.

Verified with a round-trip test: consistent base == ex notNullFields passes; ex.setNotNullFields(...) only (the natural case) fails assertEquals(orig, roundTripped) with identical toString.

Impact depends on whether anything compares a QueryEntityEx via equals()/hashCode() across (de)serialization (e.g. QueryEntity/schema conflict checks on join or cache start). Two ways out: don't populate the round-tripped Ex's base _notNullFields from the Ex value (keep it consistent with how the original was built), or stop letting the shadowed base field participate in equality. Happy to add a regression test once you pick a direction.

@anton-vinogradov

anton-vinogradov commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Round-trip serialization test for the new DTOs (follow-up to my "add tests" review comment). It exercises the full direct-marshalling path (QueryEntity/QueryEntityEx*MessageDirectMessageWriter/DirectMessageReadertoEntity()) through the real CoreMessagesProvider factory, with non-empty defaultFieldValues.

To add it from the browser: Add file → Create new file, set the path to
modules/core/src/test/java/org/apache/ignite/internal/processors/query/schema/message/QueryEntityMessageSerializationTest.java
then paste the code below and commit to this branch.

QueryEntityMessageSerializationTest.java
/*
 * 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.processors.query.schema.message;

import java.math.BigDecimal;
import java.nio.ByteBuffer;
import java.util.Arrays;
import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
import java.util.LinkedHashMap;
import java.util.LinkedHashSet;
import java.util.Map;
import org.apache.ignite.IgniteCheckedException;
import org.apache.ignite.cache.QueryEntity;
import org.apache.ignite.cache.QueryIndex;
import org.apache.ignite.cache.QueryIndexType;
import org.apache.ignite.internal.CoreMessagesProvider;
import org.apache.ignite.internal.direct.DirectMessageReader;
import org.apache.ignite.internal.direct.DirectMessageWriter;
import org.apache.ignite.internal.managers.communication.IgniteMessageFactoryImpl;
import org.apache.ignite.internal.processors.query.QueryEntityEx;
import org.apache.ignite.internal.util.typedef.internal.U;
import org.apache.ignite.marshaller.Marshaller;
import org.apache.ignite.plugin.extensions.communication.MessageFactory;
import org.apache.ignite.plugin.extensions.communication.MessageFactoryProvider;
import org.apache.ignite.testframework.junits.common.GridCommonAbstractTest;
import org.junit.Test;

import static org.apache.ignite.marshaller.Marshallers.jdk;

/** Serialization round-trip of {@link QueryEntityMessage} and {@link QueryEntityExMessage}. */
public class QueryEntityMessageSerializationTest extends GridCommonAbstractTest {
    /** Marshaller for the {@code dfltFieldValues} payload (matches the {@code withNoSchema} registration). */
    private final Marshaller marsh = jdk();

    /** Factory aware of {@link QueryEntityMessage} and its nested messages. */
    private final MessageFactory msgFactory = new IgniteMessageFactoryImpl(
        new MessageFactoryProvider[] {new CoreMessagesProvider(marsh, marsh, U.gridClassLoader())});

    /** */
    @Test
    public void testQueryEntity() throws Exception {
        QueryEntity entity = queryEntity();

        assertEquals(entity, roundTrip(entity));
    }

    /** */
    @Test
    public void testQueryEntityEx() throws Exception {
        QueryEntityEx entity = queryEntityEx();

        QueryEntity res = roundTrip(entity);

        assertTrue(res instanceof QueryEntityEx);
        assertEquals(entity, res);

        // Not part of QueryEntityEx.equals(), so assert it explicitly.
        assertEquals(entity.fillAbsentPKsWithDefaults(), ((QueryEntityEx)res).fillAbsentPKsWithDefaults());
    }

    /**
     * @param src Source entity.
     * @return Entity rebuilt after message conversion and a full direct-marshalling round-trip.
     */
    private QueryEntity roundTrip(QueryEntity src) throws IgniteCheckedException {
        QueryEntityMessage msg = src instanceof QueryEntityEx
            ? new QueryEntityExMessage((QueryEntityEx)src) : new QueryEntityMessage(src);

        msg.prepareMarshal(marsh);

        QueryEntityMessage res = doMarshalUnmarshal(msg);

        res.finishUnmarshal(marsh, U.gridClassLoader());

        return res.toEntity();
    }

    /**
     * @param msg Message to write and read back through {@link DirectMessageWriter}/{@link DirectMessageReader}.
     * @return Restored message.
     */
    private QueryEntityMessage doMarshalUnmarshal(QueryEntityMessage msg) {
        ByteBuffer buf = ByteBuffer.allocate(64 * 1024);

        DirectMessageWriter writer = new DirectMessageWriter(msgFactory);
        writer.setBuffer(buf);

        assertTrue(writer.writeMessage(msg, false));

        buf.flip();

        DirectMessageReader reader = new DirectMessageReader(msgFactory, null);
        reader.setBuffer(buf);

        QueryEntityMessage res = (QueryEntityMessage)reader.readMessage(false);

        assertNotNull(res);

        return res;
    }

    /** @return Query entity with every field populated, including non-empty default field values. */
    private QueryEntity queryEntity() {
        LinkedHashMap<String, String> fields = new LinkedHashMap<>();
        fields.put("id", Integer.class.getName());
        fields.put("name", String.class.getName());
        fields.put("price", BigDecimal.class.getName());

        Map<String, Object> dfltVals = new HashMap<>();
        dfltVals.put("name", "unknown");
        dfltVals.put("price", new BigDecimal("9.99"));
        dfltVals.put("id", 42);

        Map<String, Integer> precision = new HashMap<>();
        precision.put("name", 64);

        Map<String, Integer> scale = new HashMap<>();
        scale.put("price", 2);

        Map<String, String> aliases = new HashMap<>();
        aliases.put("name", "NAME_ALIAS");

        return new QueryEntity()
            .setKeyType(Integer.class.getName())
            .setValueType("org.apache.ignite.Person")
            .setKeyFieldName("id")
            .setValueFieldName("name")
            .setTableName("PERSON")
            .setFields(fields)
            .setKeyFields(new LinkedHashSet<>(Arrays.asList("id")))
            .setAliases(aliases)
            .setIndexes(Collections.singletonList(new QueryIndex("name", QueryIndexType.SORTED).setInlineSize(32)))
            .setNotNullFields(new HashSet<>(Arrays.asList("id", "name")))
            .setDefaultFieldValues(dfltVals)
            .setFieldsPrecision(precision)
            .setFieldsScale(scale);
    }

    /** @return Extended query entity with all base and extended fields populated. */
    private QueryEntityEx queryEntityEx() {
        QueryEntity base = queryEntity();
        base.setNotNullFields(null);

        QueryEntityEx entity = new QueryEntityEx(base);

        // Natural QueryEntityEx: notNullFields lives only in the Ex field, base shadow stays null.
        entity.setNotNullFields(new HashSet<>(Arrays.asList("id", "name")));
        entity.setPreserveKeysOrder(true);
        entity.implicitPk(true);
        entity.fillAbsentPKsWithDefaults(true);
        entity.sql(true);
        entity.setPrimaryKeyInlineSize(16);
        entity.setAffinityKeyInlineSize(24);

        return entity;
    }
}

Note: testQueryEntityEx uses a naturally-built QueryEntityEx (notNullFields set only via the Ex setter), so it pairs with the _notNullFields fix suggestion in the review above — without that fix it fails (demonstrating the bug), with it it passes (verified, 2/2). When running from the CLI, surefire's forkCount=0 means the pom argLine --add-opens aren't applied — pass them via MAVEN_OPTS (the IDE works as-is).

@anton-vinogradov anton-vinogradov left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concrete fixes as applyable suggestions for three points from my review: the _notNullFields round-trip bug (verified against a round-trip test), the stale comment, and the unrelated AuthentificationDataBagItem change.

Comment on lines +74 to +76
QueryEntityEx qryEntity = new QueryEntityEx(super.toEntity());

qryEntity.setNotNullFields(notNullFields);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concrete fix for the _notNullFields round-trip asymmetry I flagged. QueryEntityEx keeps notNullFields in its own field and leaves the inherited QueryEntity._notNullFields as a null shadow. super.toEntity() populates that base shadow, so a deserialized Ex ends up with base._notNullFields == ex.notNullFields, while a normally-built one has base._notNullFields == null — hence the two aren't .equals(). Nulling the base copy makes a deserialized entity structurally identical to a freshly-built one.

Verified: with this change a round-trip test on a naturally-built QueryEntityEx (notNullFields set only via the Ex setter) passes assertEquals(orig, roundTripped); without it, it fails.

Suggested change
QueryEntityEx qryEntity = new QueryEntityEx(super.toEntity());
qryEntity.setNotNullFields(notNullFields);
QueryEntity base = super.toEntity();
// QueryEntityEx keeps notNullFields in its own field; null the shadowed base copy so a
// deserialized entity equals a normally-built one (where base _notNullFields is null).
base.setNotNullFields(null);
QueryEntityEx qryEntity = new QueryEntityEx(base);
qryEntity.setNotNullFields(notNullFields);

@Order(8)
String tableName;

/** Fields that must have non-null value. NB: DO NOT remove underscore to avoid clashes with QueryEntityEx. */

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: this comment was copied from QueryEntity (where the field is _notNullFields); here the field is notNullFields and QueryEntityExMessage doesn't redeclare it, so the "DO NOT remove underscore" note no longer applies.

Suggested change
/** Fields that must have non-null value. NB: DO NOT remove underscore to avoid clashes with QueryEntityEx. */
/** Fields that must have non-null value. */

withNoSchema(UserAuthenticateResponseMessage.class);
withNoSchema(TcpDiscoveryAuthFailedMessage.class);
withSchema(AuthentificationDataBagItem.class);
withNoSchema(AuthentificationDataBagItem.class);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This withSchemawithNoSchema change is unrelated to QueryEntity (it's auth/users) and is a no-op anyway — AuthentificationDataBagItem is a plain Message, so its registered marshaller is never used. Suggest reverting it to keep the PR focused (if the flip is intentional cleanup, a separate commit would be clearer).

Suggested change

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants