Refactor JSON utilities#6135
Conversation
|
I added a few minor suggestions, otherwise LGTM! |
|
I am going to turn |
2959868 to
dfc467b
Compare
dfc467b to
9db80b9
Compare
|
Done, and I also added documentation on how to customize the default |
9db80b9 to
0d22388
Compare
| * @param type the target type | ||
| * @return the converted typed object | ||
| */ | ||
| public Object convertToTypedObject(Object value, Class<?> type) { |
There was a problem hiding this comment.
@tzolov I did my best to improve this typed object conversion without breaking anything, but I would welcome some context on what this is supposed to be.
I was able to replace the manual typed conversion found in JsonParser by JsonMapper#convertValue which is an improvement, but the serialization + deserialization thing at the end, also found in AbstractMcpToolMethodCallback#buildTypedArgument and MethodToolCallback#buildTypedArgument looks very suspicious. Could you please described what it is supposed to do ?
|
I like the refactoring! |
|
@sdeleuze, great improvement. Thanks!
Not sure if this is still an LLM issue (e.g. empty string finish_reason). Maybe for streaming mode we can get a partially initialized json snippets that we map to the same Java classes we use for non-streaming mode. Looks like Jackson3 uses jsonMapper = JsonMapper.builder()
.disable(DeserializationFeature.FAIL_ON_TRAILING_TOKENS)
.withCoercionConfig(LogicalType.Enum, cfg ->
cfg.setCoercion(CoercionInputShape.EmptyString, CoercionAction.AsNull))
.addModules(JacksonUtils.instantiateAvailableModules())
.build();
Perhaps, with jackson3 we can do something like this insetead: // JsonHelper.java
public Object convertToTypedObject(Object value, Type type) {
if (type instanceof Class<?> clazz) {
return convertToTypedObject(value, clazz);
}
return this.jsonMapper.convertValue(value, this.jsonMapper.constructType(type));
}
// MethodToolCallback.java
private Object buildTypedArgument(@Nullable Object value, Type type) {
if (value == null) { return null;}
try {
return jsonHelper.convertToTypedObject(value, type);
}
catch (Exception ex) {
logger.warn("Conversion from JSON failed", ex);
Throwable cause = (ex.getCause() instanceof JacksonException) ? ex.getCause() : ex;
throw new ToolExecutionException(this.getToolDefinition(), cause);
}
}But, lets tackle this in a follow up PR? @sdeleuze if you want i can handle (1 and 2) while merging the PR and then address 3 in a separate PR? |
This commit introduces JsonHelper and updates JacksonUtils in spring-ai-commons in order to: - Allow customizing the underlying Jackson JsonMapper used - Stop providing JSON writing methods in JsonParser and McpJsonParser (not consistent with the names). - Stop exposing Jackson types on public APIs outside of JacksonUtils and JsonHelper constructor It replaces the implementation of JsonParser#toTypedObject by a refined JsonHelper#convertToTypedObject one that leverages Jackson conversion capabilities instead of the previous hand-crafted code. It stops providing JSON related methods in ModelOptionsUtils since model options do not use Jackson anymore, and it updates the codebase to use Spring Frameworks's ParameterizedTypeReference instead of Jackson's TypeReference. In order to avoid too much breaking changes for the ecosystem, it keeps JsonParser deprecated which now leverages JsonUtils for its implementation. It removes McpJsonMapperUtils and McpJsonParser classes. A getDefaultJsonMapper() method is added to JacksonUtils with related Javadoc explaining how to customize the returned JsonMapper. Closes spring-projects#6135 Signed-off-by: Sébastien Deleuze <sdeleuze@users.noreply.github.com>
0d22388 to
ee3ec7f
Compare
|
Glad you like it @ThomasVitale @tzolov ! @tzolov Good catch for the empty string to enum, I was able to add a test to reproduce and fix it with I will let you take care of 2 (document the breaking changes) and merge this PR, thanks for taking care of that. |
This PR is an important refactoring of the JSON utilities and some related public APIs that was not possible before since depending on previous refactoring. I have spend quite some time to make it as safe as possible, and it should be merged as part of the upcoming 2.0.0-RC1. I am pretty happy about the result as it solves many issues and pave the way for more consistent and well designed APIs. I would welcome your review @ThomasVitale @nicolaskrier in addition to @tzolov one.
For the detail, it introduces
JsonHelperand updatesJacksonUtilsinspring-ai-commonsin order to:JsonMapperusedJsonParserandMcpJsonParser(not consistent with the names).JacksonUtilsandJsonHelperconstructorIt replaces the implementation of
JsonParser#toTypedObjectby a refinedJsonHelper#convertToTypedObjectone that leverages Jackson conversion capabilities instead of the previous hand-crafted code.It stops providing JSON related methods in
ModelOptionsUtilssince model options do not use Jackson anymore, and it updates the codebase to use Spring Frameworks'sParameterizedTypeReferenceinstead of Jackson'sTypeReference.In order to avoid too much breaking changes for the ecosystem, it keeps
JsonParserdeprecated which now leveragesJsonUtilsfor its implementation.It removes
McpJsonMapperUtilsandMcpJsonParserclasses.A
getDefaultJsonMapper()method is added toJacksonUtilswith related Javadoc explaining how to customize the returnedJsonMapper.