IGNITE-28767 Use Message DTO to transfer QueryEntity#13236
Conversation
f70aad6 to
f3ea189
Compare
9a62645 to
4595d90
Compare
TCBot Test Analysis
Possible Blockers (0)No blockers found. New Tests (0)No new tests found. |
anton-vinogradov
left a comment
There was a problem hiding this comment.
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:
- Tests. There is currently no coverage for the new serialization path. A round-trip test (
QueryEntity/QueryEntityEx→*Message→ marshal → unmarshal →toEntity(), assertingequals) with non-emptydefaultFieldValueswould guard the core of this change. - A reflection parity-guard so a field added to
QueryEntity/QueryEntityExlater can't be silently dropped from the hand-written*Messagetwins.
A few smaller points inline (one unrelated change to AuthentificationDataBagItem, a no-op withSchema→withNoSchema flip, and a stale comment).
| /** | ||
| * Message for {@link QueryEntity} transfer. | ||
| */ | ||
| public class QueryEntityMessage implements MarshallableMessage { |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
This change looks out of scope for IGNITE-28767 — AuthentificationDataBagItem 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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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. */ |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Good point, copy paste artifacts.
| /** Serialized form of query entities. */ | ||
| public class SchemaAddQueryEntityOperation extends SchemaAbstractOperation { | ||
| /** Query entities. */ | ||
| @Order(0) |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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()); |
There was a problem hiding this comment.
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.
|
Round-trip serialization test for the new DTOs (follow-up to my "add tests" review comment). It exercises the full direct-marshalling path ( To add it from the browser: Add file → Create new file, set the path to
|
anton-vinogradov
left a comment
There was a problem hiding this comment.
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.
| QueryEntityEx qryEntity = new QueryEntityEx(super.toEntity()); | ||
|
|
||
| qryEntity.setNotNullFields(notNullFields); |
There was a problem hiding this comment.
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.
| 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. */ |
There was a problem hiding this comment.
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.
| /** 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); |
There was a problem hiding this comment.
This withSchema→withNoSchema 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).

Added duplicate messages because classes belong to the public API:
QueryEntity->QueryEntityMessageQueryEntityEx->QueryEntityExMessageRemoved
SerializableandMarshallableMessagecontracts from schema operation messages: