-
-
Notifications
You must be signed in to change notification settings - Fork 590
fix: add throwIfNotSuccessfulStatusCode method to validate HTTP statu… #553
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
base: main
Are you sure you want to change the base?
fix: add throwIfNotSuccessfulStatusCode method to validate HTTP statu… #553
Conversation
cc @bytestream who has some ideas on Exception improvement. As I look at this - I'm wondering if we ever get a 200 back from OpenAI that is not valid JSON. There is a common complaint that we don't bubble (in the exceptions) anything about the raw response so folks can't triage the exact reasoning why. If we were to introduce a new Exception I think it should be flexible enough to capture the mixed raw response (could be null or string I'm guessing) so userland can use it - if needed. However, thats where I was wondering if a HTTP 200 with invalid JSON would arrive, which would throw Minor @sinashakouri - you just have some CI failures. |
@iBotPeaches based on what you said, it sounds like this bits the bill a little better. It just needs to include the request and response objects in the exception so that userland can act accordingly.
Anything is possible when interacting with third party services. In my mind, it's better to be type certain - PHPStan would not be happy at the moment ;) I'm just not sure whether it's deliberately very loosely typed to allow for:
I personally thought the clue was in the name |
Like would be easier if that was the case for sure lol. Half the merges in past week are just fixes for non-openai models. |
Thank you so much for your insightful feedback and suggestions. I really appreciate the time you’ve taken to review the code and provide your valuable insights, which come from your extensive experience. |
438b45a
to
2193390
Compare
Hey @iBotPeaches, thanks again for the earlier feedback! I’ve made the suggested changes:
CI is green now as well 🎉 Would appreciate it if you could take another look when you get a chance 🙏 |
Thanks! I've got my head down trying to finish this Responses API, but will visit this for sure soon |
Summary
This PR introduces stricter HTTP status code validation to the
HttpTransporter
by adding a new method:throwIfNotSuccessfulStatusCode
. The method ensures that only successful HTTP responses (status codes 200–299) are allowed to proceed. Any other status code results in a thrownUnexpectedStatusCodeException
.Changes
throwIfNotSuccessfulStatusCode
method to validate response status codes.requestObject
,requestContent
, andrequestStream
methods to call this validation method.Expected Outcome
With this improvement, the client will now provide immediate and clearer feedback when encountering invalid or unexpected HTTP responses, preventing potentially misleading or silent failures.
Looking forward to your feedback!