Skip to content

fix: Allow injecting ObjectMapper in FunctionTool, default to ObjectMapper (re. #473) #487

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

fix: Allow injecting ObjectMapper in FunctionTool, default to ObjectMapper (re. #473) #487

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 18 additions & 14 deletions core/src/main/java/com/google/adk/tools/FunctionTool.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@

import com.fasterxml.jackson.core.type.TypeReference;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.fasterxml.jackson.datatype.jdk8.Jdk8Module;
import com.google.adk.JsonBaseModel;
import com.google.adk.agents.InvocationContext;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
Expand All @@ -42,13 +42,13 @@

/** FunctionTool implements a customized function calling tool. */
public class FunctionTool extends BaseTool {
private static final ObjectMapper OBJECT_MAPPER =
new ObjectMapper().registerModule(new Jdk8Module());

private static final Logger logger = LoggerFactory.getLogger(FunctionTool.class);

@Nullable private final Object instance;
private final Method func;
private final FunctionDeclaration funcDeclaration;
private final ObjectMapper objectMapper;

public static FunctionTool create(Object instance, Method func) {
if (!areParametersAnnotatedWithSchema(func) && wasCompiledWithDefaultParameterNames(func)) {
Expand Down Expand Up @@ -123,6 +123,11 @@ private static boolean wasCompiledWithDefaultParameterNames(Method func) {
}

protected FunctionTool(@Nullable Object instance, Method func, boolean isLongRunning) {
this(instance, func, isLongRunning, JsonBaseModel.getMapper());
}

protected FunctionTool(
@Nullable Object instance, Method func, boolean isLongRunning, ObjectMapper objectMapper) {
super(
func.isAnnotationPresent(Annotations.Schema.class)
&& !func.getAnnotation(Annotations.Schema.class).name().isEmpty()
Expand All @@ -144,6 +149,7 @@ protected FunctionTool(@Nullable Object instance, Method func, boolean isLongRun
this.funcDeclaration =
FunctionCallingUtils.buildFunctionDeclaration(
this.func, ImmutableList.of("toolContext", "inputStream"));
this.objectMapper = objectMapper;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

While the runtime argument conversion now uses the injected objectMapper, the function declaration schema is built using FunctionCallingUtils, which has its own static, non-configurable ObjectMapper. This can lead to inconsistencies if the injected objectMapper has custom serializers, as the generated schema will not reflect them. This could cause mismatches between what the LLM expects and what the tool can handle at runtime. To ensure consistency, FunctionCallingUtils should also be updated to use the same ObjectMapper instance.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TBD...

}

@Override
Expand Down Expand Up @@ -225,7 +231,7 @@ private Maybe<Map<String, Object>> call(Map<String, Object> args, ToolContext to
continue;
}
} else if (argValue instanceof Map) {
arguments[i] = OBJECT_MAPPER.convertValue(argValue, paramType);
arguments[i] = objectMapper.convertValue(argValue, paramType);
continue;
}
arguments[i] = castValue(argValue, paramType);
Expand All @@ -236,16 +242,14 @@ private Maybe<Map<String, Object>> call(Map<String, Object> args, ToolContext to
} else if (result instanceof Maybe) {
return ((Maybe<?>) result)
.map(
data ->
OBJECT_MAPPER.convertValue(data, new TypeReference<Map<String, Object>>() {}));
data -> objectMapper.convertValue(data, new TypeReference<Map<String, Object>>() {}));
} else if (result instanceof Single) {
return ((Single<?>) result)
.map(
data -> OBJECT_MAPPER.convertValue(data, new TypeReference<Map<String, Object>>() {}))
.map(data -> objectMapper.convertValue(data, new TypeReference<Map<String, Object>>() {}))
.toMaybe();
} else {
return Maybe.just(
OBJECT_MAPPER.convertValue(result, new TypeReference<Map<String, Object>>() {}));
objectMapper.convertValue(result, new TypeReference<Map<String, Object>>() {}));
}
}

Expand Down Expand Up @@ -291,7 +295,7 @@ public Flowable<Map<String, Object>> callLive(
continue;
}
} else if (argValue instanceof Map) {
arguments[i] = OBJECT_MAPPER.convertValue(argValue, paramType);
arguments[i] = objectMapper.convertValue(argValue, paramType);
continue;
}
arguments[i] = castValue(argValue, paramType);
Expand All @@ -305,7 +309,7 @@ public Flowable<Map<String, Object>> callLive(
}
}

private static List<Object> createList(List<Object> values, Class<?> type) {
private List<Object> createList(List<Object> values, Class<?> type) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The callLive method, a caller of this function, has a potential bug. It obtains a java.lang.reflect.Type and casts it directly to Class before passing it here. This is unsafe and will cause a ClassCastException if the type is a ParameterizedType (e.g., for a List<Map<String, String>>). The call method contains the correct, more robust logic for handling this, which should be replicated in callLive to prevent runtime errors.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

==> #489

List<Object> list = new ArrayList<>();
// List of parameterized type is not supported.
if (type == null) {
Expand All @@ -321,13 +325,13 @@ private static List<Object> createList(List<Object> values, Class<?> type) {
|| cls == String.class) {
list.add(castValue(value, cls));
} else {
list.add(OBJECT_MAPPER.convertValue(value, type));
list.add(objectMapper.convertValue(value, type));
}
}
return list;
}

private static Object castValue(Object value, Class<?> type) {
private Object castValue(Object value, Class<?> type) {
if (type.equals(Integer.class) || type.equals(int.class)) {
if (value instanceof Integer) {
return value;
Expand Down Expand Up @@ -372,6 +376,6 @@ private static Object castValue(Object value, Class<?> type) {
return value;
}
}
return OBJECT_MAPPER.convertValue(value, type);
return objectMapper.convertValue(value, type);
}
}