CloudinaryException is too vague

[benjaminoakes]

We've seen messages like Resource not found and Error in loading, which we'd like to handle as generic HTTP client errors (400-499). Unfortunately, we have to rescue the CloudinaryException, check its error message, and then determine what should be done.

I would propose having more than one exception class to make it possible for us to rescue something like Cloudinary::HttpClientError to ignore errors like HTTP 403 (Forbidden), 404 (Not Found), 408 (Request Timeout), etc.

One way to do this would be to add class Cloudinary::HttpClientError < CloudinaryException; end and then use that error for the cases that represent HTTP 400-499.

[nilbus]

👍 I like the idea of segmenting these errors into ones that should be retriable and not. Sometimes a CloudinaryException can represent a transient network error or downtime event that should be retried, like these:

cloudinary_gem/lib/cloudinary/uploader.rb

Lines 331 to 342 in 25df5fd

	raise CloudinaryException, "Server returned unexpected status code - #{response.code} - #{response.body}" unless [200, 400, 401, 403, 404, 500].include?(response.code)
	begin
	result = Cloudinary::Utils.json_decode(response.body)
	rescue => e
	# Error is parsing json
	raise CloudinaryException, "Error parsing server response (#{response.code}) - #{response.body}. Got - #{e}"
	end
	if result["error"]
	if return_error
	result["error"]["http_code"] = response.code
	else
	raise CloudinaryException, result["error"]["message"]

[nilbus]

For backward compatibility, the specific errors raised can both be subclasses of CloudinaryException.

[roeeba]

Hi @benjaminoakes, @nilbus. Thank you for your feedback! I'm going to pass it forward to our engineers to review. We'll update here with any updates.