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

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

Merged
merged 10 commits into from
Nov 19, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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

message = response.message();
}
throw isRetryable(response)
? new AlgoliaRequestException(message, response.code())
: new AlgoliaApiException(message, response.code());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,29 @@ export function deserializeSuccess<TObject>(response: Response): TObject {
}
}

const httpMessages: Record<number, string> = {
400: 'Bad Request',
401: 'Unauthorized',
402: 'Payment Required',
403: 'Forbidden',
404: 'Not Found',
405: 'Method Not Allowed',
406: 'Not Acceptable',
407: 'Proxy Authentication Required',
408: 'Request Timeout',
409: 'Conflict',
410: 'Gone',
411: 'Length Required',
412: 'Precondition Required',
413: 'Request Entry Too Large',
414: 'Request-URI Too Long',
415: 'Unsupported Media Type',
416: 'Requested Range Not Satisfiable',
417: 'Expectation Failed',
418: 'I\'m a teapot',
429: 'Too Many Requests',
}

export function deserializeFailure({ content, status }: Response, stackFrame: StackFrame[]): Error {
try {
const parsed = JSON.parse(content);
Expand All @@ -92,5 +115,5 @@ export function deserializeFailure({ content, status }: Response, stackFrame: St
} catch {
// ..
}
return new ApiError(content, status, stackFrame);
return new ApiError(status in httpMessages ? httpMessages[status] : content, status, stackFrame);
}
Original file line number Diff line number Diff line change
Expand Up @@ -116,11 +116,12 @@ public function sendRequest(
$statusCode = (int) curl_getinfo($curlHandle, CURLINFO_HTTP_CODE);
$responseBody = curl_multi_getcontent($curlHandle);
$error = curl_error($curlHandle);
$contentType = curl_getinfo($curlHandle, CURLINFO_CONTENT_TYPE);

$this->releaseMHandle($curlHandle);
curl_close($curlHandle);

return new Response($statusCode, [], $responseBody, '1.1', $error);
return new Response($statusCode, ['Content-Type' => $contentType], $responseBody, '1.1', $error);
}

private function getMHandle($curlHandle)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,11 @@ private function handleResponse(
throw new RetriableException('Retriable failure on '.$request->getUri()->getHost().': '.$reason, $statusCode);
}

// handle HTML error responses
if (false !== strpos($response->getHeaderLine('Content-Type'), 'text/html')) {
throw new AlgoliaException($statusCode.': '.$response->getReasonPhrase(), $statusCode);
}

$responseArray = Helpers::json_decode($body, true);

if (404 === $statusCode) {
Expand Down
6 changes: 3 additions & 3 deletions clients/algoliasearch-client-ruby/lib/algolia/error.rb
Original file line number Diff line number Diff line change
Expand Up @@ -27,12 +27,12 @@ def initialize(message, errors = [])
# which is also included in the response attribute.
#
class AlgoliaHttpError < AlgoliaError
attr_accessor :code, :message
attr_accessor :code, :http_message

def initialize(code, message)
self.code = code
self.message = message
super("#{self.code()}: #{self.message()}")
self.http_message = message
super("#{code}: #{message}")
end
end
end
Original file line number Diff line number Diff line change
Expand Up @@ -39,14 +39,14 @@ def send_request(host, method, path, body, query_params, headers, timeout, conne
@logger.info("Request succeeded. Response status: #{response.status}, body: #{response.body}")
end

return Http::Response.new(status: response.status, body: response.body, headers: response.headers)
return Http::Response.new(status: response.status, reason_phrase: response.reason_phrase, body: response.body, headers: response.headers)
end

if ENV["ALGOLIA_DEBUG"]
@logger.info("Request failed. Response status: #{response.status}, error: #{response.body}")
end

Http::Response.new(status: response.status, error: response.body, headers: response.headers)
Http::Response.new(status: response.status, reason_phrase: response.reason_phrase, error: response.body, headers: response.headers)
rescue Faraday::TimeoutError => e
@logger.info("Request timed out. Error: #{e.message}") if ENV["ALGOLIA_DEBUG"]
Http::Response.new(error: e.message, has_timed_out: true)
Expand Down
Original file line number Diff line number Diff line change
@@ -1,20 +1,22 @@
module Algolia
module Http
class Response
attr_reader :status, :body, :error, :headers, :has_timed_out, :network_failure
attr_reader :status, :reason_phrase, :body, :error, :headers, :has_timed_out, :network_failure

# used for the echo requester
attr_reader :method, :path, :query_params, :host, :timeout, :connect_timeout

#
# @option status [String] Response status
# @option reason_phrase [String] Response reason phrase
# @option body [String] Response body
# @option error [String] Response error or caught error
# @option headers [String] Response headers
# @option has_timed_out [String] If the request has timed out
#
def initialize(opts = {})
@status = opts[:status]
@reason_phrase = opts[:reason_phrase]
@body = opts[:body]
@error = opts[:error] || ""
@headers = opts[:headers] || ""
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,11 @@ def request(call_type, method, path, body, opts = {})
network_failure: response.network_failure
)
if outcome == FAILURE
# handle HTML error
if response.headers["content-type"]&.include?("text/html")
raise Algolia::AlgoliaHttpError.new(response.status, response.reason_phrase)
end

decoded_error = JSON.parse(response.error, :symbolize_names => true)
raise Algolia::AlgoliaHttpError.new(response.status, decoded_error[:message])
end
Expand Down
5 changes: 5 additions & 0 deletions scripts/cts/testServer/timeout.ts
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,11 @@ function addRoutes(app: express.Express): void {

// no response, just hang
});

app.get('/1/html-error', (req, res) => {
res.setHeader('Content-Type', 'text/html');
res.status(429).send('<html><body>429 Too Many Requests</body></html>');
});
}

export function timeoutServer(): Promise<Server> {
Expand Down
2 changes: 1 addition & 1 deletion templates/ruby/tests/client/tests.mustache
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
{{#dynamicTemplate}}{{/dynamicTemplate}}
assert(false, 'An error should have been raised')
rescue => e
assert_equal({{#lambda.codeSnakeCase}}'{{{expectedError}}}'{{/lambda.codeSnakeCase}}.sub('%localhost%', ENV.fetch('CI', nil) == 'true' ? 'localhost' : 'host.docker.internal'), e.message)
assert_equal({{#lambda.codeSnakeCase}}%q({{{expectedError}}}){{/lambda.codeSnakeCase}}.sub('%localhost%', ENV.fetch('CI', nil) == 'true' ? 'localhost' : 'host.docker.internal'), e.message)
end
{{/isError}}
{{^isError}}
Expand Down
42 changes: 42 additions & 0 deletions tests/CTS/client/ingestion/api.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
[
{
"testName": "can handle HTML error",
"autoCreateClient": false,
"steps": [
{
"type": "createClient",
"parameters": {
"appId": "test-app-id",
"apiKey": "test-api-key",
"region": "us",
"customHosts": [
{
"port": 6676
}
]
}
},
{
"type": "method",
"method": "customGet",
"parameters": {
"path": "1/html-error"
},
"expected": {
"error": {
"csharp": "<html><body>429 too many requests</body></html>",
"go": "API error [429] Too Many Requests",
"java": "Status Code: 429 - Too Many Requests",
"javascript": "Too Many Requests",
"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": "429: Too Many Requests",
"python": "Too Many Requests",
"ruby": "429: Too Many Requests",
"scala": "<html><body>429 Too Many Requests</body></html>",
"swift": "HTTP error: Status code: 429 Message: No message"
}
}
}
]
}
]
2 changes: 1 addition & 1 deletion tests/CTS/client/search/indexExists.json
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@
"kotlin": "Client request(GET http://%localhost%:6681/1/indexes/indexExistsERROR/settings) invalid: 403 Forbidden. Text: \\\"{\\\"message\\\":\\\"Invalid API key\\\"}\\\"",
"php": "Invalid API key",
"python": "Invalid API key",
"ruby": "Invalid API key",
"ruby": "403: Invalid API key",
"swift": "HTTP error: Status code: 403 Message: Invalid API key"
}
}
Expand Down
3 changes: 1 addition & 2 deletions tests/CTS/client/search/saveObjects.json
Original file line number Diff line number Diff line change
Expand Up @@ -87,8 +87,7 @@
"kotlin": "Client request(POST http://%localhost%:6680/1/indexes/cts_e2e_saveObjects_kotlin/batch) invalid: 403 Forbidden. Text: \\\"{\\\"message\\\":\\\"Invalid Application-ID or API key\\\",\\\"status\\\":403}\\\"",
"php": "Invalid Application-ID or API key",
"python": "Invalid Application-ID or API key",
"ruby": "Invalid Application-ID or API key",
"scala": "Invalid Application-ID or API key",
"ruby": "403: Invalid Application-ID or API key",
"swift": "HTTP error: Status code: 403 Message: Invalid Application-ID or API key"
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,10 @@ package object algoliasearch {

def assertError(message: String)(call: => Unit)(implicit ec: ExecutionContextExecutor): Unit = {
val error = intercept[Exception](call)
assert(error.getMessage == message)
assert(
error.getMessage == message,
s"Error message does not match, expected: $message, got: ${error.getMessage}"
)
}

@targetName("assertErrorFuture")
Expand Down
Loading