gnodet commented on code in PR #27656:
URL: https://github.com/apache/camel/pull/27656#discussion_r4237407774
##########
components/camel-ai/camel-semantic/src/main/java/org/apache/camel/semantic/internal/SemanticAuditService.java:
##########
@@ -361,10 +381,50 @@ public final class Invocation {
private final List<SemanticObserver.Observation> handles = new
ArrayList<>();
// Confined to the invocation thread; cancellation emits a separate
request record.
private boolean completed;
+ private final Map<String, Object> inputFields = new LinkedHashMap<>();
private Invocation(Map<String, Object> fields, boolean enabled) {
this.fields = fields;
this.enabled = enabled;
+ if (enabled && configuration.getInput((String)
fields.get("expert")).isEnabled()) {
+ inputFields.put("inputOmitted", "unavailable");
+ }
+ }
+
+ /** Snapshot the effective selected state before a provider can mutate
it. Observers never receive this data. */
+ public void captureInput(Object input) {
+ if (fields == null || completed || inputFields.isEmpty()) {
Review Comment:
⚠️ **Idempotency:** `inputFields.isEmpty()` is the wrong guard for
double-call protection. On the first call, `inputFields.clear()` is called
(line 401), then new entries are put in — so on a second invocation
`inputFields.isEmpty()` is `false` and the method re-captures. If
`captureInput()` is only supposed to run once, set a flag (or set `completed =
true`) after capturing and check that instead.
##########
components/camel-ai/camel-semantic/src/main/java/org/apache/camel/semantic/internal/SemanticAuditService.java:
##########
@@ -64,6 +66,7 @@ public final class SemanticAuditService extends
ServiceSupport {
private final AtomicLong observerErrors = new AtomicLong();
private final Map<String, AtomicLong> sinkErrors = new LinkedHashMap<>();
private final Map<String, SemanticAuditSink> sinks = new LinkedHashMap<>();
+ private final Map<String, SemanticAuditInputRedactor> redactors = new
LinkedHashMap<>();
Review Comment:
⚠️ **Concurrency:** `redactors` is a plain `LinkedHashMap` populated during
`doStart()` (lifecycle thread) and read in `captureInput()`
(exchange-evaluation threads). Without a memory-visibility guarantee, threads
calling `captureInput()` may observe a stale or partially-constructed map.
Consider:
- Replacing with `Collections.unmodifiableMap(new LinkedHashMap<>(...))`
built once in `doStart()` after all puts, and publishing it via a `volatile`
field, or
- Using a `ConcurrentHashMap` directly.
##########
components/camel-ai/camel-semantic/src/main/java/org/apache/camel/semantic/internal/SemanticAuditInputSnapshot.java:
##########
@@ -0,0 +1,88 @@
+/*
+ * 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.camel.semantic.internal;
+
+import java.math.BigDecimal;
+import java.math.BigInteger;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Map;
+
+/** Bounded JSON values only: never converts arbitrary objects or consumes
streams. */
+public final class SemanticAuditInputSnapshot {
+ private int remaining;
+ private int nodes = 1000;
+
+ private SemanticAuditInputSnapshot(int maxChars) {
+ remaining = maxChars;
+ }
+
+ public static Object copy(Object input, int maxChars) {
+ return new SemanticAuditInputSnapshot(maxChars).value(input, 0);
+ }
+
+ private Object value(Object value, int depth) {
+ if (--nodes < 0 || depth > 8) {
+ throw new IllegalArgumentException("Input snapshot limit");
+ }
+ if (value == null || value instanceof Boolean) {
+ text(String.valueOf(value));
+ return value;
+ }
+ if (value instanceof String text) {
+ return text(text);
+ }
+ if (value instanceof Byte || value instanceof Short || value
instanceof Integer || value instanceof Long
+ || value instanceof Float || value instanceof Double || value
instanceof BigDecimal
+ || value instanceof BigInteger) {
+ if (value instanceof Double d && !Double.isFinite(d) || value
instanceof Float f && !Float.isFinite(f)
+ || value instanceof BigInteger integer &&
integer.bitLength() > remaining * 4
+ || value instanceof BigDecimal decimal &&
decimal.precision() > remaining) {
+ throw new IllegalArgumentException("Input snapshot limit");
+ }
+ text(value.toString());
+ return value;
+ }
+ if (value instanceof List<?> list && list.size() <= nodes) {
Review Comment:
💡 **Note:** `list.size() <= nodes` only checks the top-level element count.
A list of `N` elements, each a deeply-nested structure, can still consume many
more than `nodes` total nodes before the `--nodes < 0` hard limit triggers.
This is fine for correctness (the hard limit holds), but callers calling
`copy()` with a specific budget may get a snapshot larger than expected. A
comment here clarifying that the pre-check is an optimistic fast-path (not a
strict budget) would help future readers.
##########
components/camel-ai/camel-semantic/src/main/java/org/apache/camel/semantic/internal/SemanticAuditService.java:
##########
@@ -361,10 +381,50 @@ public final class Invocation {
private final List<SemanticObserver.Observation> handles = new
ArrayList<>();
// Confined to the invocation thread; cancellation emits a separate
request record.
private boolean completed;
+ private final Map<String, Object> inputFields = new LinkedHashMap<>();
private Invocation(Map<String, Object> fields, boolean enabled) {
this.fields = fields;
this.enabled = enabled;
+ if (enabled && configuration.getInput((String)
fields.get("expert")).isEnabled()) {
+ inputFields.put("inputOmitted", "unavailable");
+ }
+ }
+
+ /** Snapshot the effective selected state before a provider can mutate
it. Observers never receive this data. */
+ public void captureInput(Object input) {
+ if (fields == null || completed || inputFields.isEmpty()) {
+ return;
+ }
+ SemanticAuditInputConfiguration policy =
configuration.getInput((String) fields.get("expert"));
+ SemanticAuditInputRedactor redactor =
redactors.get(fields.get("expert"));
+ inputFields.clear();
+ if (policy.getRedactor() != null && redactor == null) {
+ inputFields.put("inputOmitted", "redaction_failed");
+ return;
+ }
+ Object snapshot;
+ try {
+ snapshot = SemanticAuditInputSnapshot.copy(input,
policy.getMaxChars());
+ } catch (Exception | AssertionError | LinkageError failure) {
+ inputFields.put("inputOmitted",
"snapshot_limit_or_unsupported_type");
+ return;
+ }
+ if (redactor != null) {
+ try {
+ snapshot = redactor.redact(snapshot);
+ if (snapshot == null) {
+ inputFields.put("inputOmitted", "redacted");
+ return;
+ }
+ snapshot = SemanticAuditInputSnapshot.copy(snapshot,
policy.getMaxChars());
+ } catch (Exception | AssertionError | LinkageError failure) {
Review Comment:
💡 Same broad catch as above, but more justified here since `redactor` is
user-supplied code. Still worth a comment: `// user-supplied redactor may throw
anything`.
##########
components/camel-ai/camel-semantic/src/main/java/org/apache/camel/semantic/internal/SemanticAuditService.java:
##########
@@ -361,10 +381,50 @@ public final class Invocation {
private final List<SemanticObserver.Observation> handles = new
ArrayList<>();
// Confined to the invocation thread; cancellation emits a separate
request record.
private boolean completed;
+ private final Map<String, Object> inputFields = new LinkedHashMap<>();
private Invocation(Map<String, Object> fields, boolean enabled) {
this.fields = fields;
this.enabled = enabled;
+ if (enabled && configuration.getInput((String)
fields.get("expert")).isEnabled()) {
+ inputFields.put("inputOmitted", "unavailable");
+ }
+ }
+
+ /** Snapshot the effective selected state before a provider can mutate
it. Observers never receive this data. */
+ public void captureInput(Object input) {
+ if (fields == null || completed || inputFields.isEmpty()) {
+ return;
+ }
+ SemanticAuditInputConfiguration policy =
configuration.getInput((String) fields.get("expert"));
+ SemanticAuditInputRedactor redactor =
redactors.get(fields.get("expert"));
+ inputFields.clear();
+ if (policy.getRedactor() != null && redactor == null) {
+ inputFields.put("inputOmitted", "redaction_failed");
+ return;
+ }
+ Object snapshot;
+ try {
+ snapshot = SemanticAuditInputSnapshot.copy(input,
policy.getMaxChars());
+ } catch (Exception | AssertionError | LinkageError failure) {
Review Comment:
💡 `AssertionError` is a JVM assertion failure (`-ea` flag) and
`LinkageError` is a class-loading failure — neither is something
`SemanticAuditInputSnapshot.copy()` would normally throw. Catching them
silently makes sense from a "never break the route" perspective, but a brief
inline comment explaining the rationale would make this intentional choice
clear to future readers.
--
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]
