DanielLeens commented on PR #11557: URL: https://github.com/apache/seatunnel/pull/11557#issuecomment-5696058780
@SEZ9 Good push-back — I went and read the actual Hazelcast 5.1 `readObject(ObjectDataInput, Class)` implementation rather than assume the javadoc's intent. Findings, with exact locations (from `hazelcast-5.1-sources.jar`): - `AbstractSerializationService#readObject(ObjectDataInput, Class)` (`AbstractSerializationService.java:360-378`) still reads the wire's `typeId` first and resolves the `SerializerAdapter` from it exactly like the untyped path — the `Class` argument is only forwarded into `serializer.read(in, aClass)`. So the outer typeId-based serializer dispatch is unaffected by the `Class` hint. - `TaskLocation` is `IdentifiedDataSerializable`, so the adapter in play wraps Hazelcast's built-in `DataSerializableSerializer`, which does implement `TypedStreamDeserializer` (`DataSerializableSerializer.java:52`) — I confirmed SeaTunnel doesn't register any custom serializer for `TaskLocation` that would change this. - The actual behavior is in `DataSerializableSerializer#readInternal(ObjectDataInput, Class)` (`DataSerializableSerializer.java:115-163`). When `aClass != null`, it does **not** compare the wire-embedded `factoryId`/`classId` against `TaskLocation`'s own. It eagerly does `ds = (DataSerializable) aClass.newInstance()` *before* even reading the header, then still reads `factoryId`/`id` off the wire (so a registered-factory check still applies) but **discards** them instead of calling `dsf.create(id)`, and finally calls `ds.readData(in)` on the already-constructed `TaskLocation` instance. So the mechanism is subtly different from "enforce and reject on mismatch" — it's an override, not a validator: 1. The concrete F1 risk — the wire's `factoryId`/`classId` choosing *some other registered class* to instantiate, which then blows up as a CCE wherever `readerLocation` is later used as `TaskLocation` — is structurally closed, because we no longer go through `dsf.create(id)` at all once a `Class` is supplied. The returned object can never be anything but a `TaskLocation`. 2. What it does *not* do is validate that the sender actually meant to send a `TaskLocation`. If the payload was actually written for a different `IdentifiedDataSerializable`, `readData(in)` still runs on a fresh `TaskLocation`, consuming whatever bytes follow positionally. Depending on field layout that surfaces as a downstream exception inside `TaskLocation#readData` (a nested `readObject()`/`readString()` call hitting garbage length/type bytes), or in the worst case could populate `TaskLocation`'s three primitive fields with garbage without throwing at all. It will not surface as one clean "type mismatch" `HazelcastSerializationException`. Net: the concrete complaint F1 raised (CCE surfacing far from the read site, on first use) is fixed, and it matches `TaskOperation`'s existing pattern as I said before — but "enforce" was the wrong word on my part. It's "the class you ask for is what you get, with the stream read into it positionally," not "the class on the wire is validated against what you asked for." I'll take option 1: add a unit test that feeds `ManagedSourceRegisterOperation#readInternal` a payload serialized for a different `IdentifiedDataSerializable` (e.g. `TaskGroupLocation`, which this class already depends on) and assert on the *actual* observed outcome, rather than assert "decoding fails" from source-reading alone — given the mechanism above I'd expect it to throw downstream in `TaskLocation#readData` for that particular field-layout mismatch, but I want the test to pin that down rather than leave it as an unverified claim. Will push that as a follow-up commit. -- 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]
