weiqingy commented on code in PR #756:
URL: https://github.com/apache/flink-agents/pull/756#discussion_r3360986369
##########
plan/src/main/java/org/apache/flink/agents/plan/serializer/ActionJsonDeserializer.java:
##########
@@ -66,10 +66,15 @@ public Action deserialize(JsonParser jsonParser,
DeserializationContext deserial
throw new IOException("Unsupported function type: " + funcType);
}
- // Deserialize listenEventTypes
- List<String> listenEventTypes = new ArrayList<>();
- node.get("listen_event_types")
- .forEach(eventTypeNode ->
listenEventTypes.add(eventTypeNode.asText()));
+ // Deserialize trigger_conditions (fall back to legacy
listen_event_types for older JSONs)
+ List<String> triggerConditions = new ArrayList<>();
+ JsonNode triggerNode = node.get("trigger_conditions");
+ if (triggerNode == null) {
+ triggerNode = node.get("listen_event_types");
Review Comment:
This fallback to the legacy `listen_event_types` key is the load-bearing
guarantee that plan JSON persisted in older Flink state still deserializes
after the rename — but nothing exercises it. The cross-version snapshot
resources were all regenerated to `trigger_conditions` in this PR, so the
compat tests now compare new-format against new-format and this branch never
fires under test. A future refactor could delete the fallback and every test
would stay green.
Worth a small deserialize test that feeds a JSON blob carrying
`listen_event_types` and asserts it lands in `getTriggerConditions()` /
`getListenEventTypes()`? That pins the old-key path so it can't be silently
dropped.
##########
api/src/test/java/org/apache/flink/agents/api/EventTypeTest.java:
##########
@@ -0,0 +1,39 @@
+/*
+ * 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.flink.agents.api;
+
+import org.junit.jupiter.api.Test;
+
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+
+/** Tests for {@link EventType}. */
+class EventTypeTest {
+
+ @Test
+ void builtInConstantsAreNonNull() {
Review Comment:
`assertNotNull` on inlined `String` constants can't really fail — the
constants are non-null by construction — so this test passes regardless of what
each constant holds. A copy-paste slip like `OutputEvent =
InputEvent.EVENT_TYPE` would sail through.
Since the point of `EventType` is to be a single source of truth, would
asserting value-equality against each source (e.g.
`assertEquals(InputEvent.EVENT_TYPE, EventType.InputEvent)`) be a stronger
check? That actually guards the mapping. Same applies to the Python
`test_event_type.py` smoke test.
##########
python/flink_agents/api/tests/test_event_type.py:
##########
@@ -0,0 +1,38 @@
+################################################################################
+# 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.
+#################################################################################
+"""Smoke tests for :mod:`flink_agents.api.events.event_type`."""
+
+from __future__ import annotations
+
+from flink_agents.api.events.event_type import EventType
+
+
+def test_builtin_constants_are_non_empty_strings() -> None:
Review Comment:
This asserts each constant is a non-empty string, which holds for any string
value and so won't catch a wrong mapping (e.g. two constants pointing at the
same source `EVENT_TYPE`). Would asserting each constant against its source
event's `EVENT_TYPE` be a better guard on the single-source-of-truth contract?
Mirrors the suggestion on the Java `EventTypeTest`, so whatever shape you land
on there can carry over here.
##########
python/flink_agents/plan/actions/action.py:
##########
@@ -71,7 +81,13 @@ def __serialize_config(self, config: Dict[str, Any]) ->
Dict[str, Any] | None:
@model_validator(mode="before")
def __custom_deserialize(self) -> "Action":
- config = self["config"]
+ # Legacy fallback: listen_event_types → trigger_conditions
+ if "trigger_conditions" not in self or self.get("trigger_conditions")
is None:
+ if self.get("listen_event_types"):
+ self["trigger_conditions"] = list(self["listen_event_types"])
+ self.pop("listen_event_types", None)
Review Comment:
Same thought as the Java side: this `model_validator` is the only thing
keeping older persisted plans (with `listen_event_types`) deserializable, but
no test feeds it the old key — the regenerated snapshot fixtures all use
`trigger_conditions` now, so this branch is untested.
Would a small round-trip test help here — construct an `Action` from a dict
with `listen_event_types` and assert it surfaces as `trigger_conditions`? That
guards the fallback against a future cleanup.
--
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]
