Skip to content

Conversation

JM-Lab
Copy link
Contributor

@JM-Lab JM-Lab commented Jul 7, 2025

Return String tool results directly (or "null" if null) when returnType is String.class, instead of serializing them to JSON. This removes unnecessary formatting while preserving existing behavior for other types.

import java.lang.reflect.Type;
import java.util.Base64;
import java.util.Map;
import java.util.Optional;
Copy link

@valery1707 valery1707 Jul 10, 2025

Choose a reason for hiding this comment

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

This import is needed now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review! I've squashed the optimization commits into one and pushed the update.

@JM-Lab JM-Lab force-pushed the fix-string-returntype-no-json-conversion branch from 77ecb6c to e01be9e Compare July 11, 2025 07:10
import org.springframework.ai.util.json.JsonParser;
import org.springframework.lang.Nullable;

import javax.imageio.ImageIO;

Choose a reason for hiding this comment

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

As far as I can see in neighboring PRs (1, 2, 3), it is recommended to maintain the order of imports in a certain structure:

  1. java.*
  2. other imports
  3. org.springframework.*
  4. static imports

Also, please avoid using wildcard imports .*

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed imports, squashed commits, and pushed. Thanks for the review!

@JM-Lab JM-Lab force-pushed the fix-string-returntype-no-json-conversion branch from e01be9e to 5864958 Compare July 11, 2025 10:10
@markpollack
Copy link
Member

What is wrong serializing it to JSON? This could maybe go into the 1.1 main branch, but I suspect would be considered a breaking change for 1.0.1

Thoughts @JM-Lab ?

@JM-Lab
Copy link
Contributor Author

JM-Lab commented Jul 18, 2025

The change addresses an issue where return values like ""Done"" or ""test"" cause unnecessary escape handling on the consumer side.
I encountered this while working on a project based on version 1.0.0 and applied a workaround to simplify processing.

I agree this is a breaking change and should be included in version 1.1 to give users a chance to adapt.

Thanks, @markpollack — let me know if there's anything else.

@ilayaperumalg ilayaperumalg modified the milestones: 1.1.0.M1, 1.1.0.M2 Sep 18, 2025
@ilayaperumalg ilayaperumalg removed this from the 1.1.0.M2 milestone Sep 19, 2025
@ilayaperumalg
Copy link
Member

@JM-Lab Thanks for the PR. Unfortunately, returning the String value for the "String" type response has the following downsides:

  1. It is by design that the return value of the tool response is expected to be a valid JSON. This sets the contract for the tool response which gets returned to LLM or the caller (when the tool definition enforces "returnDirect").
  2. This change breaks the current experience even if we get this in just for 1.1.x.

While this fix is definitely a convenience, it's better to keep the interchange contract as is.

I am closing this PR without merging. Please feel free to re-open if you still want to continue the discussion. Thanks once again.

@JM-Lab
Copy link
Contributor Author

JM-Lab commented Sep 22, 2025

@ilayaperumalg Got it, thanks for the clarification! Makes sense to keep the JSON contract consistent.
Appreciate the review and your time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants