wenjin272 commented on code in PR #1060:
URL: https://github.com/apache/flink-agents/pull/1060#discussion_r4059144252
##########
python/flink_agents/api/chat_message.py:
##########
@@ -43,6 +44,136 @@ class MessageRole(str, Enum):
TOOL = "tool"
+class TextBlock(BaseModel):
+ """A plain-text, immutable part of a ChatMessage."""
+
+ model_config = ConfigDict(frozen=True)
+
+ type: Literal["text"] = "text"
+ text: str = ""
+
+ def __str__(self) -> str:
+ return self.text
+
+
+class Base64Source(BaseModel):
+ """An inline media payload, carried as base64 text."""
+
+ model_config = ConfigDict(frozen=True)
Review Comment:
Could we configure the nested block and source models with `extra="forbid"`
as well?
`ChatMessage` rejects unknown fields, but Pydantic configuration does not
propagate into nested models. For example, Python currently accepts a
`Base64Source` containing both `data` and an unexpected `url` field, then
silently drops `url`, while Java rejects the same wire payload. The same issue
applies to unknown fields on `TextBlock`, `UrlSource`, and `MediaBlock`.
Besides creating a Java/Python contract mismatch, this weakens the
structural source invariant because malformed source objects can be accepted
and normalized silently. Could we set `extra="forbid"` on these nested models
and add mirrored negative wire-format tests?
##########
api/src/main/java/org/apache/flink/agents/api/chat/messages/TextBlock.java:
##########
@@ -0,0 +1,75 @@
+/*
+ * 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.chat.messages;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonProperty;
+
+import java.util.LinkedHashMap;
+import java.util.Map;
+import java.util.Objects;
+
+/** A plain-text, immutable part of a {@link ChatMessage}. */
+public final class TextBlock extends ContentBlock {
+
+ private final String text;
+
+ @JsonCreator
+ public TextBlock(@JsonProperty("text") String text) {
+ this.text = text != null ? text : "";
Review Comment:
Could we align the handling of an explicit `null` text value between Java
and Python?
The Java constructor currently normalizes `"text": null` to an empty string,
while the Python `TextBlock` rejects `None`. As a result, the same public wire
payload is accepted and rewritten by Java but rejected by Python.
Since `TextBlock` is being introduced in this PR, I would prefer rejecting
explicit `null` in Java as well, while keeping the omitted-field/default
behavior consistent if needed. Either contract is possible, but both languages
should make the same decision and cover it with mirrored tests.
--
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]
