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

Depending on older Jackson version causes deserialization to fail with extraneous fields #301

Closed
zach-lf opened this issue Mar 11, 2025 · 27 comments

Comments

@zach-lf
Copy link

zach-lf commented Mar 11, 2025

This was working today but started breaking every time around 1pm est.

Attempting to use ChatCompletionResults.choice.message() always throws OpenAIInvalidDataException stating that message is invalid. Here's an example:

com.openai.errors.OpenAIInvalidDataException: message is invalid, received {role=assistant, content={"results":[{"score":100,"languageCode":"en"}]}, refusal=null, annotations=[]}

(is refusal meant to be nullable?)

@RobertCraigie
Copy link
Collaborator

Thanks for the report, would you be able to share a full example snippet?

@zach-lf
Copy link
Author

zach-lf commented Mar 11, 2025

Here's a mostly complete snippet:

ChatCompletionCreateParams.Builder builder = ChatCompletionCreateParams.builder();

builder.addSystemMessage("You will be given text, you must identify languages in the text by their two digit ISO 639-1 language code and give them a presence score from 0 - 100 based on how much they are used in the text.");
builder.addUserMessage("Identify the languages in the following text: " + text);

ResponseFormatJsonSchema.JsonSchema.Schema schema =
    ResponseFormatJsonSchema.JsonSchema.Schema.builder()
        .putAdditionalProperty("type", JsonValue.from("object"))
        .putAdditionalProperty(
            "properties",
            JsonValue.from(
                Map.of(
                    "results", Map.of(
                        "type", "array",
                        "items", Map.of(
                            "type", "object",
                            "properties", Map.of(
                                "languageCode", Map.of("type", "string"),
                                "score", Map.of("type", "number")
                            )
                        )
                    )
                )
            )
        )
        .build();

builder
    .model("gpt-4-omni")
    .temperature(0.0d)
    .topP(1.0d)
    .n(1)
    .logprobs(false)
    .maxCompletionTokens(2048)
    .presencePenalty(0.0d)
    .frequencyPenalty(0.0d)
    .responseFormat(ResponseFormatJsonSchema.builder()
        .jsonSchema(ResponseFormatJsonSchema.JsonSchema.builder()
            .name("language-detection-results")
            .schema(schema)
            .build())
        .build())
    .user("assistant");

ChatCompletion chatCompletionResult = openAiClient.chat().completions().create(builder.build());

chatCompletionResult.choice().message(); // this kills the java

@RobertCraigie
Copy link
Collaborator

Thanks! We're investigating.

@izian
Copy link

izian commented Mar 12, 2025

We are also seeing this today. We were just doing a POC demo to the company for an OpenAI integration and it failed. The exception was
Caused by: com.openai.errors.OpenAIInvalidDataException: 'message' is invalid, received {role=assistant, content={***REDACTED OUR JSON RESPONSE VALID PER SCHEMA***}, refusal=null, annotations=[]} at com.openai.core.JsonField.getRequired$openai_java_core(Values.kt:101) at com.openai.models.ChatCompletion$Choice.message(ChatCompletion.kt:316) at com.openai.models.ChatCompletion$Choice.validate(ChatCompletion.kt:354) at com.openai.models.ChatCompletion.validate(ChatCompletion.kt:121)

it appears annotations=[] is new and because it's unexpected, it's invalid

@izian
Copy link

izian commented Mar 12, 2025

Appears that my issue was as a result of missing updates to the version, I will try again with latest but I can see it fixed here
3216ffa#diff-1fc18ba9e933f721de439529dfd69eebe583c0caeebef374792fcbd3772a17cc

@TomerAberbach
Copy link
Collaborator

First of all, the code snippet from this comment seems not to compile. Did you typo @zach-lf?

I modified the example like so for testing:

OpenAIClient client = OpenAIOkHttpClient.fromEnv();

ChatCompletionCreateParams.Builder builder = ChatCompletionCreateParams.builder();

builder.addSystemMessage(
        "You will be given text, you must identify languages in the text by their two digit ISO 639-1 language code and give them a presence score from 0 - 100 based on how much they are used in the text.");
builder.addUserMessage("Identify the languages in the following text: " + "Hello World");

ResponseFormatJsonSchema.JsonSchema.Schema schema = ResponseFormatJsonSchema.JsonSchema.Schema.builder()
        .putAdditionalProperty("type", JsonValue.from("object"))
        .putAdditionalProperty(
                "properties",
                JsonValue.from(Map.of(
                        "results",
                        Map.of(
                                "type",
                                "array",
                                "items",
                                Map.of(
                                        "type",
                                        "object",
                                        "properties",
                                        Map.of(
                                                "languageCode", Map.of("type", "string"),
                                                "score", Map.of("type", "number")))))))
        .build();

builder.model(ChatModel.GPT_4O_MINI)
        .temperature(0.0d)
        .topP(1.0d)
        .n(1)
        .logprobs(false)
        .maxCompletionTokens(2048)
        .presencePenalty(0.0d)
        .frequencyPenalty(0.0d)
        .responseFormat(ResponseFormatJsonSchema.builder()
                .jsonSchema(ResponseFormatJsonSchema.JsonSchema.builder()
                        .name("language-detection-results")
                        .schema(schema)
                        .build())
                .build())
        .user("assistant");

ChatCompletion chatCompletionResult = client.chat().completions().create(builder.build());

System.out.println(chatCompletionResult.choices().stream()
        .map(ChatCompletion.Choice::message)
        .collect(toList()));

After doing that, the example works fine. I'm not able to reproduce any issue. We actually didn't make any changes to the SDK at 1pm EST. The Responses API was released at around 3pm EST. So maybe there was an ongoing issue with the API, not the SDK.

@izian
Copy link

izian commented Mar 12, 2025

After doing that, the example works fine. I'm not able to reproduce any issue. We actually didn't make any changes to the SDK at 1pm EST. The Responses API was released at around 3pm EST. So maybe there was an ongoing issue with the API, not the SDK.

From my inspection, the SDK changed recently to add the annotations field to the message response. In response to or in anticipation of the API change. It seems that on the latest latest release we might not be seeing this issue anymore.

It didn't seem to matter what the request was, all requests were failing to validate the response due to the empty new field

The validation is strict in this SDK, so the presence of extraneous data causes it to fail. I think it would be cool to have a forward compatible validation mode.

@zach-lf
Copy link
Author

zach-lf commented Mar 12, 2025

From my inspection, the SDK changed recently to add the annotations field to the message response. In response to or in anticipation of the API change. It seems that on the latest latest release we might not be seeing this issue anymore.

This was the issue. And I would agree that the validation is way too strict.

@TomerAberbach
Copy link
Collaborator

TomerAberbach commented Mar 12, 2025

After doing that, the example works fine. I'm not able to reproduce any issue. We actually didn't make any changes to the SDK at 1pm EST. The Responses API was released at around 3pm EST. So maybe there was an ongoing issue with the API, not the SDK.

From my inspection, the SDK changed recently to add the annotations field to the message response. In response to or in anticipation of the API change. It seems that on the latest latest release we might not be seeing this issue anymore.

I don't think I've seen evidence that the SDK broke due to a change in the SDK.

If you still believe this is the case, then please provide a full reproducible example that compiles as well as the version of the SDK you're using with it.

It didn't seem to matter what the request was, all requests were failing to validate the response due to the empty new field

The validation is strict in this SDK, so the presence of extraneous data causes it to fail. I think it would be cool to have a forward compatible validation mode.

The SDK is already forwards compatible with non-breaking changes to the API. It does not throw for extraneous fields. They will simply go into the additionalProperties bucket.

It will also not complain that the response mismatches the SDK's expectations unless you try to access the specific field that mismatches (or unless you call validate()). See here for more info: https://github.com/openai/openai-java#response-validation

@izian
Copy link

izian commented Mar 12, 2025

I don't think I've seen evidence that the SDK broke due to a change in the SDK.

I'm saying a change to the SDK has fixed the issue. The API changed, and the SDK validates strict, and the field was missing in the SDK, so it failed validation, but the SDK just udpated (yesterday? depends on time zones) and the field annotations is now in the SDK so the validation passes

@TomerAberbach
Copy link
Collaborator

I don't think I've seen evidence that the SDK broke due to a change in the SDK.

I'm saying a change to the SDK has fixed the issue. The API changed, and the SDK validates strict, and the field was missing in the SDK, so it failed validation, but the SDK just udpated (yesterday? depends on time zones) and the field annotations is now in the SDK so the validation passes

If that's true, then we should be able write an example snippet that uses an older SDK version and fails because of the new annotations field, but I ran this snippet on an old SDK version and I get this output:

[ChatCompletionMessage{content={"results":[{"languageCode":"en","score":100}]}, refusal=null, role=assistant, audio=, functionCall=, toolCalls=, additionalProperties={annotations=[]}}]

As you can see, the annotations field, which is unknown to this older version of the SDK, is gracefully handled by the additionalProperties map.


To be clear, I appreciate the issue report, but this is why I still don't think the issue is what you say it is. I do really want to understand what happened here, which is why I'm asking for a reproducible example.

@TomerAberbach
Copy link
Collaborator

I think I might know what happened!

The SDK is compiled against Jackson 2.18, and if I run my code snippet with that version, then annotations ends up in the additionalProperties bucket, but if I downgrade to Jackson 2.17, then the same code snippet throws that error.

It seems like maybe the folks on this issue are:

  • Depending on the SDK
  • Depending specifically on an older Jackson version, which seems to not handle the @JsonAnySetter on the additionalProperties correctly

I guess the older Jackson version ends up getting chosen at runtime for some reason.

@TomerAberbach
Copy link
Collaborator

TomerAberbach commented Mar 12, 2025

Yeah, I think we're depending on this feature, which was released in 2.18.1

@TomerAberbach
Copy link
Collaborator

Can folks who encountered this issue:

  • Confirm they are depending on a version of Jackson older than 2.18.1
  • Let me which dependency management tool they are using (e.g. Gradle, Maven, etc.)

@zach-lf
Copy link
Author

zach-lf commented Mar 12, 2025

Yup I'm on an older version of jackson (2.14.2), and using Maven.
Also thank you for looking into this so thoroughly!

@TomerAberbach
Copy link
Collaborator

@zach-lf if you have a moment, it would be great if you could confirm the error happens for you on an older SDK version and that it gets resolved by upgrading Jackson

@izian
Copy link

izian commented Mar 12, 2025

  • Confirm they are depending on a version of Jackson older than 2.18.1
  • Let me which dependency management tool they are using (e.g. Gradle, Maven, etc.)

We were/are on 2.18.0 which might explain things... Gradle, but we specify jackson as we use it for something else

@izian
Copy link

izian commented Mar 12, 2025

@TomerAberbach I do apologise, I'm a but confused.
In the link i sent earlier today, there was a change to this SDK that introduced

     @JsonProperty("annotations")
     @ExcludeMissing
     private val annotations: JsonField<List<Annotation>> = JsonMissing.of(),

That appeared to resolve the issue for us
Now I'm confused, if we update Jackson, will it move the annotations in to the additionalProperties ? because they're their own field in the latest 0.34.1 SDK... I think

@TomerAberbach
Copy link
Collaborator

  • Confirm they are depending on a version of Jackson older than 2.18.1
  • Let me which dependency management tool they are using (e.g. Gradle, Maven, etc.)

We were/are on 2.18.0 which might explain things... Gradle, but we specify jackson as we use it for something else

Interesting. My understanding was that Gradle chooses the latest version of all instances of the dependency in your dependency tree if there are duplicates (see here). Any idea if you have some configuration that would make it choose 2.18.0 instead?

@TomerAberbach I do apologise, I'm a but confused. In the link i sent earlier today, there was a change to this SDK that introduced

     @JsonProperty("annotations")
     @ExcludeMissing
     private val annotations: JsonField<List<Annotation>> = JsonMissing.of(),

That appeared to resolve the issue for us Now I'm confused, if we update Jackson, will it move the annotations in to the additionalProperties ? because they're their own field in the latest 0.34.1 SDK... I think

Sorry, I think I was unclear. Let me try to explain again.

Yes, in the newest version, annotations will be deserialized into the annotations field, regardless of Jackson version.

However, if there's a new field again, then it will throw an error again if you do not update your Jackson version. The reason is that we are using @JsonAnySetter on the additionalProperties field (so that any unknown fields get put in there), but it seems that this way of using @JsonAnySetter is only supported in Jackson 2.181.

So what I was asking (if you have bandwidth/time) is if you can confirm/test my hypothesis like so:

  1. Downgrade the SDK version you are using to v0.33.0 (before annotations field was added as "known")
  2. Confirm the code snippet fails with the error in this issue report
  3. Upgrade your Jackson version to 2.18.1
  4. Confirm the code snippet passes (with annotations ending up in additionalProperties)

I'm going to see if there's anything we can on our end to make sure that users of the SDK are using a compatible Jackson version. As far as I can tell so far, it's hard for a Java library to enforce that on its users

@zach-lf
Copy link
Author

zach-lf commented Mar 12, 2025

@TomerAberbach Upgrading jackson to 2.18.1 resolved the issue for the release prior to the addition of annotations.

I downgraded the sdk to 0.33.0 and confirmed again that the validation throws an error and then upgraded jackson to 2.18.1 and it no longer threw the error.

@TomerAberbach
Copy link
Collaborator

Amazing! Thanks for confirming :)

Alright, next I will continue digging for what we can do to make sure people depending on the SDK don't accidentally depend on an incompatible Jackson version

@TomerAberbach TomerAberbach changed the title Retrieving chat completion message always throws an exception Depending on older Jackson version causes deserialization to fail with extraneous fields Mar 12, 2025
@izian
Copy link

izian commented Mar 12, 2025

I think I now understand. So we had an older SDK and an older Jackson.

We are in POC and we just fly tipped an app a month or so ago to a sandbox container, and the product team went to demo the POC today and it failed. It worked yesterday. Hilarity ensued.

I only chimed in here because I thought I’d identified it being the new field.

Great work to everyone. I can see why the Jackson version helps and will help.

I’d still consider an option to use a forward compatible validation, unless that’s what you’d effectively get with newer Jackson anyway.
Should the API add a bunch of new stuff I guess it would be painful for apps to go dark over strict validation issues.

It’s almost sleep time here. Many thanks, even though I didn’t raise the issue, appreciate the time

@TomerAberbach
Copy link
Collaborator

I’d still consider an option to use a forward compatible validation, unless that’s what you’d effectively get with newer Jackson anyway. Should the API add a bunch of new stuff I guess it would be painful for apps to go dark over strict validation issues.

Yup, that's the intended/actual behavior when the correct version of Jackson is used.

To be clear, it's not that the new Jackson version automatically makes the SDK forwards compatible. We explicitly designed the SDK to be forwards compatible (see here) and in our implementation of that we rely on a Jackson feature that's only available in Jackson v2.18.1.

The problem here is not that we didn't make the SDK forwards compatible. We did make it forwards compatible! The problem is that folks are inadvertently using an older Jackson version despite the fact that the SDK explicitly depends on v2.18.1:

api("com.fasterxml.jackson.core:jackson-databind:2.18.1")

Unfortunately, it's quite difficult as a library author to ensure folks are not accidentally using older versions. Maven and Gradle can be finicky here. Still, we will try to come up with some ideas.

@TomerAberbach
Copy link
Collaborator

Still, we will try to come up with some ideas.

disabling DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES would make it work fine on older versions of jackson.

We already don't fail on unknown properties at the Jackson layer. The exception reported in this issue is unrelated to this.

@noboomu
Copy link

noboomu commented Mar 13, 2025

Given the importance of your organization and it's services, please consider the fact that many of us are dealing with large pre-existing codebases. Jackson v2.18.1 is pretty young, seems recent for a breaking change. I understand y'all can't consider everyone's dependencies but I'd guess the majority use Jackson. I don't mean to complain, I love what you do, I just wanted to communicate the point.

@TomerAberbach
Copy link
Collaborator

I'm not sure I follow the "seems recent for a breaking change" part, but we are looking into our options here and one of the potential options is making sure we only use features that are available up to some older Jackson version. I think this fits your feedback if I understand correctly

@TomerAberbach
Copy link
Collaborator

Hey folks! Just wanted to give an update here. We thought long and hard about this problem and made the following changes (as of v0.37.0):

  1. We now compile and test the SDK against Jackson version v2.13.4 to ensure we support older Jackson versions. This required some changes to how we use Jackson internally, but there should be no difference in serialization/deserialization behavior.

  2. We still declare Jackson version v2.18.1 as the one we depend on in our published POM. This ensures that if a user depends on the SDK without also depending on Jackson on their end, then they'll get this modern version by default (instead of v2.13.4, which is what would happen if we put that in our POM). Putting v2.18.1 in our POM does not prevent end users from using lower versions.

  3. When the client is instantiated, we now check if the runtime Jackson version is incompatible with our minimum supported Jackson version (v2.13.4). If the runtime version is incompatible, then we will throw. This ensures that you find out about a bad Jackson version during development; not in production.

Definitely let me know if you encounter any issues going forward. Thanks everyone for the reports :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants