Skip to content

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

sinashakouri
Copy link

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 thrown UnexpectedStatusCodeException.

Changes

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!

@iBotPeaches
Copy link
Collaborator

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 UnserializableResponse that would be missing the raw response content.

Minor @sinashakouri - you just have some CI failures.

@bytestream
Copy link

@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.

if a HTTP 200 with invalid JSON would arrive

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:

this library is used for so many more services than just OpenAI

I personally thought the clue was in the name openai-php/client and it was exclusive to OpenAI 😅

@iBotPeaches
Copy link
Collaborator

I personally thought the clue was in the name openai-php/client and it was exclusive to OpenAI 😅

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.

@sinashakouri
Copy link
Author

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.
I’ll definitely take the points you mentioned into consideration and will work on addressing and refining them as suggested.

sinashakouri added a commit to sinashakouri/openai-php-client that referenced this pull request Apr 17, 2025
@sinashakouri sinashakouri force-pushed the improvement/http-status-check branch from 438b45a to 2193390 Compare April 17, 2025 18:20
@sinashakouri
Copy link
Author

sinashakouri commented Apr 17, 2025

Hey @iBotPeaches, thanks again for the earlier feedback! I’ve made the suggested changes:

  • the exception now includes the request and response, and tests have been updated accordingly.

CI is green now as well 🎉

Would appreciate it if you could take another look when you get a chance 🙏

@iBotPeaches
Copy link
Collaborator

Thanks! I've got my head down trying to finish this Responses API, but will visit this for sure soon

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

Successfully merging this pull request may close these issues.

3 participants