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.

Sign up for GitHub

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

Jump to bottom

fix(cts): add tests for HTML error #4097

Merged
merged 10 commits into from
Nov 19, 2024
Merged

fix(cts): add tests for HTML error #4097

merged 10 commits into from
Nov 19, 2024

Conversation

millotp
Copy link
Collaborator

@millotp millotp commented Nov 13, 2024

🧭 What and Why

Same as go, we need to properly handle html error, currently it's less than ideal:

 "error": {
    "csharp": "<html><body>429 too many requests</body></html>",
    "go": "API error [429] Too Many Requests",
    "java": "Status Code: 429 - <html><body>429 Too Many Requests</body></html>",
    "javascript": "<html><body>429 Too Many Requests</body></html>",
    "kotlin": "Client request(GET http://%localhost%:6676/1/html-error) invalid: 429 Too Many Requests. Text: \"<html><body>429 Too Many Requests</body></html>\"",
    "php": "json_decode error: Syntax error",
    "python": "Too Many Requests",
    "ruby": "unexpected token at '<html><body>429 Too Many Requests</body></html>'",
    "scala": "<html><body>429 Too Many Requests</body></html>",
    "swift": "HTTP error: Status code: 429 Message: No message"
  }

@millotp millotp self-assigned this Nov 13, 2024
@algolia-bot
Copy link
Collaborator

algolia-bot commented Nov 13, 2024 •
edited
Loading

✔️ Code generated!

Name Link
🪓 Triggered by 624bcab6a91a009d432274fe3b368b4b6385da4b
🍃 Generated commit 0364ecf79e762f5bd9b67223b811cbe3dc56eb86
🌲 Generated branch generated/fix/all-html-error
📊 Benchmark results

Benchmarks performed on the method using a mock server, the results might not reflect the real-world performance.

Language Req/s
go 1579
javascript 1572
php 1489
csharp 1257
java 1135
python 1098
ruby 871
swift 737

@millotp millotp marked this pull request as ready for review November 18, 2024 22:08
@millotp millotp requested a review from a team as a code owner November 18, 2024 22:08
shortcuts
shortcuts approved these changes Nov 19, 2024
View reviewed changes
Copy link
Member

@shortcuts shortcuts left a comment

Choose a reason for hiding this comment

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

efficace

...client-java/algoliasearch/src/main/java/com/algolia/internal/interceptors/RetryStrategy.java
@@ -84,6 +84,9 @@ private Response handleResponse(StatefulHost host, @Nonnull Response response) t

try {
String message = response.body() != null ? response.body().string() : response.message();
if (response.header("Content-Type", "application/json").contains("text/html")) {
Copy link
Member

Choose a reason for hiding this comment

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

can it have both? or do I misunderstand the header method?

Copy link
Member

Choose a reason for hiding this comment

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

ahhh it's a default?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes it's the default, I'm not sure why I put json, I could have used empty string

Copy link
Member

Choose a reason for hiding this comment

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

might speed up things my .2ns

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it's only in the rate limiting case, it will rate limit them even more

Copy link
Member

Choose a reason for hiding this comment

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

yes that would make a difference for sure

Copy link
Member

Choose a reason for hiding this comment

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

let's benchmark it

@millotp millotp merged commit f97e44c into main Nov 19, 2024
28 checks passed
@millotp millotp deleted the fix/all-html-error branch November 19, 2024 10:39
algolia-bot added a commit that referenced this pull request Nov 19, 2024
@algolia-bot @millotp
fix(cts): add tests for HTML error (#4097) (generated) [skip ci]
f8a1da7 
Co-authored-by: Pierre Millot <pierre.millot@algolia.com>
algolia-bot added a commit to algolia/algoliasearch-client-java that referenced this pull request Nov 19, 2024
@algolia-bot @millotp
fix(cts): add tests for HTML error (generated)
900f2ee 
algolia/api-clients-automation#4097

Co-authored-by: algolia-bot <accounts+algolia-api-client-bot@algolia.com>
Co-authored-by: Pierre Millot <pierre.millot@algolia.com>
algolia-bot added a commit to algolia/algoliasearch-client-javascript that referenced this pull request Nov 19, 2024
@algolia-bot @millotp
fix(cts): add tests for HTML error (generated)
49fc82b 
algolia/api-clients-automation#4097

Co-authored-by: algolia-bot <accounts+algolia-api-client-bot@algolia.com>
Co-authored-by: Pierre Millot <pierre.millot@algolia.com>
algolia-bot added a commit to algolia/algoliasearch-client-php that referenced this pull request Nov 19, 2024
@algolia-bot @millotp
fix(cts): add tests for HTML error (generated)
a595930 
algolia/api-clients-automation#4097

Co-authored-by: algolia-bot <accounts+algolia-api-client-bot@algolia.com>
Co-authored-by: Pierre Millot <pierre.millot@algolia.com>
algolia-bot added a commit to algolia/algoliasearch-client-ruby that referenced this pull request Nov 19, 2024
@algolia-bot @millotp
fix(cts): add tests for HTML error (generated)
aad41b4 
algolia/api-clients-automation#4097

Co-authored-by: algolia-bot <accounts+algolia-api-client-bot@algolia.com>
Co-authored-by: Pierre Millot <pierre.millot@algolia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shortcuts shortcuts shortcuts approved these changes

@morganleroi morganleroi Awaiting requested review from morganleroi morganleroi is a code owner automatically assigned from algolia/api-clients-automation

Assignees

@millotp millotp

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@millotp @algolia-bot @shortcuts