[api][java][python] Introduce EventType constants and unify Action trigger entry#756
Conversation
| # TODO: Raise a warning when the action has a return value, as it will be ignored. | ||
| exec: PythonFunction | JavaFunction | ||
| listen_event_types: List[str] | ||
| trigger_conditions: List[str] |
There was a problem hiding this comment.
Python Action model lacks backward compat for old JSON key
Java ActionJsonDeserializer (L71-73) falls back to listen_event_types when trigger_conditions is absent. The Python pydantic Action model only declares trigger_conditions: List[str] with no alias or model_validator fallback — deserializing old-format plan JSON from persisted Flink state will raise ValidationError.
There was a problem hiding this comment.
Fixed in lines 84-88. Added a model_validator fallback that mirrors the Java ActionJsonDeserializer behavior:
# 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)
Old-format plan JSON with listen_event_types will now deserialize without ValidationError.
| private final String name; | ||
| private final Function exec; | ||
| private final List<String> listenEventTypes; | ||
| private final List<String> triggerConditions; |
There was a problem hiding this comment.
Discuss:Keep listen_event_types, add trigger_conditions as a new field
Rather than renaming listen_event_types → trigger_conditions, I'd suggest keeping both:
listen_event_types: which events route to this action (static dispatch)trigger_conditions: CEL expression filter for whether to process a matched event (dynamic filtering)
These are orthogonal concerns — routing vs. filtering. Keeping listen_event_types also avoids JSON backward-compat issues entirely (the Python pydantic model currently lacks the fallback that the Java deserializer has for the old key).
There was a problem hiding this comment.
Thanks for the comment — this was actually my first instinct too: a clean routing-vs-filtering separation. The more I worked through the problem though, the more I came to think it doesn't quite map onto what users actually want to express.
Event filtering is just event routing at a finer granularity, not a separate concern. Each entry in trigger_conditions answers exactly one question — "should this action fire for this event?" — and a bare event type is simply the most common (and most degenerate) form of that condition.
The case that really pins this down: for a single action, users often want per-event-type conditions, with different conditions for different types — and sometimes multiple conditions on the same type. With a unified list this falls out naturally. #726 shows the canonical shape:
// Mixed triggers. Entries are OR'ed.
@Action({
EventType.InputEvent, // direct type
"type == EventType.ChatResponseEvent && retryCount > 0", // metadata condition
"type == EventType.ToolResponseEvent && response.success == false", // payload condition
"type == EventType.ChatResponseEvent && response.plantype.contains('Day')" // metadata + payload
})
Split into listen_event_types + trigger_conditions, there's no clean way to express any of this. The two fields are parallel lists with no shared index, so "this condition belongs to that type" has no representation — and "ChatResponseEvent has two distinct conditions" is even less expressible. You'd end up either forcing one CEL expression to gate all listened types (loses per-type granularity), or inventing a multi-map like {ChatResponseEvent: [cond1, cond2], ToolResponseEvent: [cond3]} — which is just the unified list with extra ceremony, plus an awkward way to represent the "no condition" case for direct-type entries. The unified field is what naturally fits the actual shape of user intent.
Curious if you have a concrete scenario in mind where the two-field split would be cleaner — happy to revisit if there's a use case I'm missing.
| * | ||
| * <p>Resolution via {@link #lookupOrSelf}: built-in → user-registered → passthrough. | ||
| */ | ||
| public final class EventType { |
There was a problem hiding this comment.
Reconsider the EventType aggregation class
EventType.InputEvent duplicates InputEvent.EVENT_TYPE — two sources of truth for the same value. The register() / lookup() / lookupOrSelf() registry has zero callers in production code today.
InputEvent.EVENT_TYPE is already self-documenting and lives in its natural namespace. Suggest deferring the registry until the CEL PR actually needs it, and just using the event class constants directly.
There was a problem hiding this comment.
Agreed. I've refactored EventType to be a pure namespace of constants — each one simply re-exports the value from the corresponding event class (e.g., EventType.InputEvent = _InputEvent.EVENT_TYPE), so there's a single source of truth. The register() / lookup() / lookupOrSelf() registry has been removed from this PR and will be introduced later when the CEL PR actually requires dynamic resolution.
Meanwhile, I've also slimmed down the EventType class by removing the internal utility functions — their concrete implementations are deferred to a follow-up PR.
There was a problem hiding this comment.
Thanks for taking this on — the rename is clean, and I appreciate that you kept a backward-compat fallback for the old JSON key on both sides (the listen_event_types branch in ActionJsonDeserializer and the model_validator in action.py), so plan JSON persisted in older Flink state still deserializes. That's an easy thing to miss in a rename PR. A few questions inline.
| List<String> triggerConditions = new ArrayList<>(); | ||
| JsonNode triggerNode = node.get("trigger_conditions"); | ||
| if (triggerNode == null) { | ||
| triggerNode = node.get("listen_event_types"); |
There was a problem hiding this 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.
There was a problem hiding this comment.
Good catch — you're right that the regenerated snapshot fixtures mean the legacy branch is never exercised under test. I'll add a focused deserialization test that feeds a JSON blob with the old listen_event_types key and asserts it maps correctly to getTriggerConditions().
| 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) |
There was a problem hiding this 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.
There was a problem hiding this comment.
Same idea on the Python side. While adding the test I found that the custom init was rejecting the old key before model_validator(mode="before") had a chance to rename it — so the fallback was silently broken. Fixed init to accept **kwargs and handle the legacy key, and added a matching deserialization test.
| class EventTypeTest { | ||
|
|
||
| @Test | ||
| void builtInConstantsAreNonNull() { |
There was a problem hiding this 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.
There was a problem hiding this comment.
You're right — assertNotNull on inlined constants is vacuous. Originally addressed in a follow-up PR; now pulled forward into this commit with assertEquals(InputEvent.EVENT_TYPE, EventType.InputEvent) and similar for all 8 built-ins.
| from flink_agents.api.events.event_type import EventType | ||
|
|
||
|
|
||
| def test_builtin_constants_are_non_empty_strings() -> None: |
There was a problem hiding this 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.
There was a problem hiding this comment.
Mirrors the Java fix — the Python test now asserts EventType.InputEvent == InputEvent.EVENT_TYPE directly for all 8 constants. Consistent shape across both languages.
41c17f2 to
af7b6a3
Compare
weiqingy
left a comment
There was a problem hiding this comment.
Thanks for the quick turnaround — all four threads are addressed and CI is green. The legacy-key fallbacks now have focused tests on both sides, the EventType tests assert value-equality against each source, and the __init__ fix closes a real gap, not just a coverage one. One forward-looking question inline.
|
|
||
| // Add to actionsByEvent map | ||
| for (String eventTypeName : eventTypeNames) { | ||
| for (String eventTypeName : triggerConditions) { |
There was a problem hiding this comment.
This site builds the routing map from the raw triggerConditions, but the built-in path just below (addBuiltAction, L217) builds it through action.getListenEventTypes() — and that accessor's javadoc says a follow-up overrides it to "filter out non-type entries" once CEL lands. Python's agent_plan.py:148 also reads the raw field. So of the three routing sites, only the built-in one — which never carries CEL — goes through the filtering seam; the two that handle user-supplied conditions bypass it.
Today they're equivalent, since every entry is a plain event-type name. But once CEL expressions enter trigger_conditions, these two sites would register strings like "type == EventType.ChatResponseEvent && retryCount > 0" as literal keys in actionsByEvent — exactly what the accessor exists to strip. Would it make sense to route all three through the accessor now, or to defer the accessor until the CEL PR and read the raw field everywhere in the meantime? Either way the three sites agreeing seems worth locking in. Not a blocker for this PR — flagging while the context is fresh.
Linked issue: #754
Purpose of change
This PR introduces
EventTypeconstants and a registry for built-in and user-defined event types, and unifies the Action trigger entry point across Java and Python:@Action(value = ...)@action(*trigger_conditions)This is a preparatory API and plan change for the CEL Action Condition filtering feature tracked in #754. Follow-up PRs will add Java runtime CEL evaluation, Python runtime CEL evaluation, and documentation.
Scope
Since #709,
@Action/@actioncarry two orthogonal concerns:value()*trigger_conditionstarget()target=This PR only changes the trigger side:
listenEventTypesis renamed tovalue.*listen_eventsis renamed to*trigger_conditions.The dispatch side is unchanged.
target,PythonFunction, and cross-language dispatch semantics are reused as-is.Tests
EventTypeTest,AgentPlan*Test,Action*SerializerTest— all greenapi/tests+plan/tests— 218 passed, 11 skippedtargetdispatch still works after the trigger renameAPI
This is a source-level API rename on the trigger side.
Java:
@Action(listenEventTypes = {X.EVENT_TYPE})→@Action(EventType.X)@Action(listenEventTypes =..., target = @PythonFunction(...))→@Action(value = ..., target = @PythonFunction(...))Python:
@action(*listen_events, target=None)→@action(*trigger_conditions, target=None)target=remains unchanged.All in-tree Java and Python callers have been migrated.
doc-includedUpdated 10 markdown files under
docs/content/docs/to use the newEventType.X/@Action(EventType.X)form while preserving the existing cross-languagetargetexamples.