Skip to content

fix:toolcall return string #2885

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.

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

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

Conversation

Cui-xf
Copy link

@Cui-xf Cui-xf commented Apr 25, 2025

Signed-off-by: Cui-xf cui_xf293@qq.com

Thank you for taking time to contribute this pull request!
You might have already read the [contributor guide][1], but as a reminder, please make sure to:

  • Sign the contributor license agreement
  • Rebase your changes on the latest main branch and squash your commits
  • Add/Update unit tests as needed
  • Run a build and make sure all tests pass prior to submission

Signed-off-by: Cui-xf <cui_xf293@qq.com>
@@ -58,6 +58,10 @@ public String convert(@Nullable Object result, @Nullable Type returnType) {
final var imgB64 = Base64.getEncoder().encodeToString(buf.toByteArray());
return JsonParser.toJson(Map.of("mimeType", "image/png", "data", imgB64));
}
else if (result instanceof String stringResult) {
Copy link
Member

@markpollack markpollack Apr 29, 2025

Choose a reason for hiding this comment

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

the current assumption is that if is not a rendered image, we assume it is json (also a string) in this current PR, the json conversion in the else statement will never be accessed. Can you explain more of the surrounding use case that motivated you to make this change? adding to DefaultToolCallResultConverterTests?

Copy link
Member

Choose a reason for hiding this comment

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

Also wondering why JsonParser.toJson(result); doesn't use Type returnType

Copy link
Member

Choose a reason for hiding this comment

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

any thoughts @tzolov ?

Copy link
Author

Choose a reason for hiding this comment

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

the current assumption is that if is not a rendered image, we assume it is json (also a string) in this current PR, the json conversion in the else statement will never be accessed. Can you explain more of the surrounding use case that motivated you to make this change? adding to DefaultToolCallResultConverterTests?

Hey, thanks for the review!
I noticed that when I use the org.springframework.ai.tool.annotation.Tool annotation and the method returns a String, the result gets escaped (see screenshot below). I’m not really sure if this is the expected behavior, or if there’s any doc about it.
If this is actually, I will to close the PR.
Thanks!

image image

@markpollack markpollack self-assigned this May 6, 2025
@markpollack markpollack added the bug Something isn't working label May 9, 2025
@markpollack markpollack added this to the 1.0.0 milestone May 9, 2025
@markpollack markpollack assigned tzolov and unassigned markpollack May 9, 2025
@tzolov
Copy link
Contributor

tzolov commented May 16, 2025

@Cui-xf this is an interesting case:

Currently the DefaultToolCallResultConverter assumes that all string values are of type JSON (including) the string literals.
Note that "Foo Bar" is not JSON but toJson("Foo Bar") converts it into a valid json: ""Foo Bar"".

Here is an example how the DefaultToolCallResultConverter behvaves:

public class Main {

    private static final ObjectMapper OBJECT_MAPPER = JsonMapper.builder().build();

    public static void main(String[] args) {

        var value1 = "Foo Bar";
        System.out.println("Value 1: " + value1);
        System.out.println(String.format("- raw value: %s, is JSON: %s", value1, isValidJSONJackson(value1)));
        System.out.println(String.format("- toJson(value): %s, is JSON: %s", toJson(value1),
                isValidJSONJackson(toJson(value1))));

        var value2 = "{\"hello\": \"world\"}";
        System.out.println("Value 2: " + value2);
        System.out.println(String.format("- raw value: %s, is JSON? %s", value1, isValidJSONJackson(value2)));
        System.out.println(String.format("- toJson(value): %s, is JSON? %s", toJson(value2),
                isValidJSONJackson(toJson(value2))));
    }

    public static boolean isValidJSONJackson(String jsonString) {
        try {
            OBJECT_MAPPER.readTree(jsonString);
            return true;
        } catch (Exception e) {
            return false;
        }
    }

    	public static String toJson(@Nullable Object object) {
		try {			
			return OBJECT_MAPPER.writeValueAsString(object);
		}
		catch (JsonProcessingException ex) {
			throw new IllegalStateException("Conversion from Object to JSON failed", ex);
		}
	}
}

Would generate:

Value 1: Foo Bar
- raw value: Foo Bar, is JSON: false
- toJson(value): "Foo Bar", is JSON: true

Value 2: {"hello": "world"}
- raw value: Foo Bar, is JSON? true
- toJson(value): "{\"hello\": \"world\"}", is JSON? true

Before we try to change the default behavior perhaps you can try to implement a custom ToolCallResultConverter implementation?
https://docs.spring.io/spring-ai/reference/api/tools.html#_result_conversion

@tzolov tzolov modified the milestones: 1.0.0, 1.0.x May 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working image models
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants