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

OpenAIInvalidDataException should include the complete server response body "as is" #86

Open
stargazer33 opened this issue Jan 6, 2025 · 8 comments

Comments

@stargazer33
Copy link

stargazer33 commented Jan 6, 2025

At the moment I get only this:

Exception in thread "main" com.openai.errors.OpenAIInvalidDataException: 'choices' is not set
	at com.openai.core.JsonField.getRequired$openai_java_core(Values.kt:99)
	at com.openai.models.ChatCompletion.choices(ChatCompletion.kt:43)
	...

It is clear: something went wrong.

The question is: WHAT went wrong: it means: what was the actual server response? The library knows it!

So, just add it to exception as an additional field, something like "rawResponseBody".
Additionally: print in in the "toString()".

I would expect something like this:

Exception in thread "main" com.openai.errors.OpenAIInvalidDataException: 'choices' is not set. rawResponseBody: {"error":{"message":"Provider returned error","code":429,"metadata":{"raw":"{\"error\":{\"code\":null,\"message\":\"Rate limit exceeded\",\"...

Having this info direct in exception simplifies troubleshooting a lot!

Thanks in advance ))

@TomerAberbach
Copy link
Collaborator

Good feedback! We'll look into including more information directly in the exception message

@stargazer33
Copy link
Author

looking a bit deeper into exceptions I see this:

class OpenAIServiceException{
   ...
    public final int statusCode() 

    public final com.openai.core.http.Headers headers()

    public final java.lang.String body()   
   ...
}

well, this is fine for OpenAIServiceException but this info is missing in other subclasses on OpenAIException.

Just move statusCode(), headers(), body() up in the class hierarchy and thats it.

All HTTP responses have HTTP status code, headers, body (ok, in some cases body is empty, but this is OK)

@TomerAberbach
Copy link
Collaborator

Not all OpenAIException subclasses happen as a result of API calls, which is why status code, headers, and body are not at the level of OpenAIException

@TomerAberbach
Copy link
Collaborator

Looking at this again, was this error actually a result of a bad status code? Or did the server return a success status code, but the response body was simply erroneously missing this field?

Cause if you got to the point of having a ChatCompletion object, then I think that implies there was a successful response. Since we should throw before that point in the case of a bad status code:

@JvmSynthetic
internal fun <T> Handler<T>.withErrorHandler(errorHandler: Handler<OpenAIError>): Handler<T> =
object : Handler<T> {
override fun handle(response: HttpResponse): T {
when (val statusCode = response.statusCode()) {
in 200..299 -> {
return this@withErrorHandler.handle(response)
}
400 -> {
val buffered = response.buffered()
throw BadRequestException(
buffered.headers(),
stringHandler().handle(buffered),
errorHandler.handle(buffered),
)
}
401 -> {
val buffered = response.buffered()
throw UnauthorizedException(
buffered.headers(),
stringHandler().handle(buffered),
errorHandler.handle(buffered),
)
}
403 -> {
val buffered = response.buffered()
throw PermissionDeniedException(
buffered.headers(),
stringHandler().handle(buffered),
errorHandler.handle(buffered),
)
}
404 -> {
val buffered = response.buffered()
throw NotFoundException(
buffered.headers(),
stringHandler().handle(buffered),
errorHandler.handle(buffered),
)
}
422 -> {
val buffered = response.buffered()
throw UnprocessableEntityException(
buffered.headers(),
stringHandler().handle(buffered),
errorHandler.handle(buffered),
)
}
429 -> {
val buffered = response.buffered()
throw RateLimitException(
buffered.headers(),
stringHandler().handle(buffered),
errorHandler.handle(buffered),
)
}
in 500..599 -> {
val buffered = response.buffered()
throw InternalServerException(
statusCode,
buffered.headers(),
stringHandler().handle(buffered),
errorHandler.handle(buffered),
)
}
else -> {
val buffered = response.buffered()
throw UnexpectedStatusCodeException(
statusCode,
buffered.headers(),
stringHandler().handle(buffered),
errorHandler.handle(buffered),
)
}
}
}
}

@stargazer33
Copy link
Author

stargazer33 commented Jan 17, 2025

did the server return a success status code, but the response body was simply erroneously missing this field?

My point is:

  1. This API is used by tens of servers (may be even hundreds!). This is de-facto standard.
  2. Some servers implements it wrong: sometimes they return HTTP 200 and and response body contains some JSON with error description - completely NON-STANDARD, I saw multiple servers doing this!
  3. We can not fix these servers.
  4. Just implement it - in case this exception thrown - the USER of this library should have access to HTTP raw response body.
  5. You do not know how users use your library. Make it generic enough - in this case - provide access to HTTP raw response body.

@TomerAberbach
Copy link
Collaborator

I don't disagree raw response access should be available in general, but as I mentioned OpenAIInvalidDataException is not only thrown as a result of server responses so it simply does not make sense for those getters to be on that exception type.

It would be great if you could answer my question

@stargazer33
Copy link
Author

OpenAIInvalidDataException is not only thrown as a result of server responses so it simply does not make sense for those getters to be on that exception type.

I do not see a problem here.
Just re-structure/refactor these exceptions.
In case of this non-standard server response (HTTP 200 with error description in response body) I should be able to see this the actual server response without debugger.

@TomerAberbach
Copy link
Collaborator

I literally said the following in my first reply:

Good feedback! We'll look into including more information directly in the exception message

Please answer my question from above if you'd like to help get this issue resolved:

did the server return a success status code, but the response body was simply erroneously missing this field?

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

2 participants