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

Handling for non-json response #881

Merged

Conversation

AyushSawant18588
Copy link
Contributor

Describe the change
Since the original error response is included in the body field of openai.RequestError, even when the response is not in JSON format, it would be more appropriate to return openai.RequestError directly, rather than returning an errorString. This approach ensures that the original error details are preserved within the body field.

Describe your solution
Removed the condition that checked if the response was in a non-JSON format and returned an errorString with the original error response. Now, the original error response will be directly included in the body field of openai.RequestError.

Copy link

codecov bot commented Oct 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.85%. Comparing base (774fc9d) to head (380a62d).
Report is 65 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #881      +/-   ##
==========================================
+ Coverage   98.46%   98.85%   +0.39%     
==========================================
  Files          24       26       +2     
  Lines        1364     1749     +385     
==========================================
+ Hits         1343     1729     +386     
+ Misses         15       14       -1     
  Partials        6        6              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@AyushSawant18588 AyushSawant18588 changed the title Removed handling for non-json response Handling for non-json response Oct 16, 2024
@AyushSawant18588
Copy link
Contributor Author

@sashabaranov Can you please look into this?

@sashabaranov
Copy link
Owner

Thank you for the PR!

client_test.go Outdated
<hr><center>nginx</center>
</body>
</html>`,
expected: `error, status code: 413, status: , message: invalid character '<' looking for beginning of value`,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This expected error text is a bit misleading. Code 413 indeed points out to "Content Too Large" (generic HTTP error), but the text afterward applies to json de-serialization.

I agree that this approach is better in terms of preserving the code, but in this case we actually don't preserve data

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if Error() for RequestError (https://github.com/sashabaranov/go-openai/blob/22cf414b4be89a38eb702b13c27be1de6bcef74d/error.go#L106C1-L108C2) is updated to return body also like:
fmt.Sprintf("error, status code: %d, status: %s, message: %s, body: %s", e.HTTPStatusCode, e.HTTPStatus, e.Err, e.Body)

Should I make this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sashabaranov I have updated this PR with the above mentioned change, now in error response the original error data is also present. Can you please review it?

@johnugeorge
Copy link

Thanks @sashabaranov. Can you please review this again?

Copy link
Owner

@sashabaranov sashabaranov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work, thank you!

@sashabaranov sashabaranov merged commit fb15ff9 into sashabaranov:master Oct 21, 2024
3 checks passed
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