Skip to content
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

Support for using external Json ObjectMapper #77

Closed
the-gigi opened this issue Oct 31, 2024 · 0 comments · Fixed by #78
Closed

Support for using external Json ObjectMapper #77

the-gigi opened this issue Oct 31, 2024 · 0 comments · Fixed by #78
Assignees
Labels
enhancement New feature or request

Comments

@the-gigi
Copy link
Contributor

the-gigi commented Oct 31, 2024

Here is the issue (coming from simple-openai). This specific instance happened during tool calls.

Oct 30 18:01:37.882
i-0c37647db12b51de5
invisible
Caused by: com.fasterxml.jackson.databind.exc.InvalidDefinitionException: Java 8 date/time type `java.time.LocalDate` not supported by default: add Module "com.fasterxml.jackson.datatype:jackson-datatype-jsr310" to enable handling

Oct 30 18:01:37.882
i-0c37647db12b51de5
invisible
... 51 more

Oct 30 18:01:37.882
i-0c37647db12b51de5
invisible
at io.github.sashirestela.openai.common.function.FunctionExecutor.execute(FunctionExecutor.java:81)

In our code we always do this when we need to handle dates

import com.fasterxml.jackson.datatype.jsr310.JavaTimeModule;
...
  private static final ObjectMapper mapper =
      new ObjectMapper()
          .registerModule(new JavaTimeModule())
          .configure(FAIL_ON_UNKNOWN_PROPERTIES, false);

Cleverclient shouldn't do that, because it may not always be needed and other use cases may require other module.
Instead it should be possible to pass an external ObjectMapper that will be passed to JsonUtilobjectToMap()

I suggest to add another objectToMap() method that takes an external ObjectMapper like so:

    public static <T> Map<String, Object> objectToMap(T object) {
        try {
            return objectMapperStrict.convertValue(object, new TypeReference<>() {
            });
        } catch (IllegalArgumentException e) {
            throw new CleverClientException("Cannot convert object {0} to Map.", object, e);
        }
    }

    public static <T> Map<String, Object> objectToMap(T object, ObjectMapper objectMapper) {
        try {
            return objectMapper.convertValue(object, new TypeReference<>() {
            });
        } catch (IllegalArgumentException e) {
            throw new CleverClientException("Cannot convert object {0} to Map.", object, e);
        }
    }    

The original objectToMap() can just call it then with the static object mapper like so:

    public static <T> Map<String, Object> objectToMap(T object) {
        return objectToMap(object, objectMapperStrict);
    }

    public static <T> Map<String, Object> objectToMap(T object, ObjectMapper objectMapper) {
        try {
            return objectMapper.convertValue(object, new TypeReference<>() {
            });
        } catch (IllegalArgumentException e) {
            throw new CleverClientException("Cannot convert object {0} to Map.", object, e);
        }
    }   

There is a similar situation with jsonToXXX() methods, which use internally the objectReaderIgnoringUnknown.

    private static final ObjectReader objectReaderIgnoringUnknown = objectMapperStrict.reader()
            .without(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES);

They need to support an external ObjectReader too. For example:

    public static <T> T jsonToObject(String json, Class<T> clazz, ObjectReader objectReader) {
        try {
            return objectReader.readValue(json, clazz);
        } catch (IOException e) {
            throw new CleverClientException("Cannot convert the Json {0} to class {1}.", json, clazz.getName(), e);
        }
    }

I opened a corresponding issue in simple-openai, becuase once this is implemented, so simple-openai should be able to be configured with an external object mapper and/or object reader and use it when calling JsonUtil methods.

sashirestela/simple-openai#205

@the-gigi the-gigi changed the title Cleverclient uses an internal ObjectMapper in JsonUtils that fails to handle dates Cleverclient uses an internal ObjectMapper in JsonUtils that fails to deserialize dates Oct 31, 2024
@sashirestela sashirestela added the enhancement New feature or request label Nov 4, 2024
@sashirestela sashirestela changed the title Cleverclient uses an internal ObjectMapper in JsonUtils that fails to deserialize dates Support for using external Json ObjectMapper Nov 18, 2024
@sashirestela sashirestela self-assigned this Nov 18, 2024
@sashirestela sashirestela linked a pull request Nov 19, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants