Arvados

Reviewing 8295ba7

• If the API server goes away, I get this message:
  

  # Connection refused - connect(2) for "localhost" port 3030
  # Please fix the error and try again.
  

  
  Most users probably won't be able to make any fix in this situation. I realize I'm breaking from the story a little bit here, but I wonder if this retry logic should only be used when there's an error in the user input, or the API server returns a well-formed error? In other cases, the old behavior of saying "I can't do that right now; I saved your edits here" seems more appropriate.
• If we keep retrying for any non-200 API response, perhaps the rest of the status line ("Forbidden", "Unprocessable Entity", whatever) could be a better default error message when none were provided in the JSON body? (Currently it's an empty string.)
• I think it could be extra helpful if the error message briefly described the category of error: "There was an error parsing your input," "The API server rejected your changes," etc. Some of the more specific error messages can be cryptic, so providing category information could help a user understand more quickly whether they broke the rules of JSON or Arvados.
• arv edit doesn't write any message if it successfully updates the object. Should it? That seems like it better follows the general UX patterns for these tools.
• Not a blocker, just FYI, any? optionally takes a block, so you can write .any? { condition } rather than .select { condition }.any?

Thanks.

Brett Smith wrote:

Reviewing 8295ba7

• If the API server goes away, I get this message:
  [...]
  Most users probably won't be able to make any fix in this situation. I realize I'm breaking from the story a little bit here, but I wonder if this retry logic should only be used when there's an error in the user input, or the API server returns a well-formed error? In other cases, the old behavior of saying "I can't do that right now; I saved your edits here" seems more appropriate.

Done.

• If we keep retrying for any non-200 API response, perhaps the rest of the status line ("Forbidden", "Unprocessable Entity", whatever) could be a better default error message when none were provided in the JSON body? (Currently it's an empty string.)

I had this thought too, but the HTTP library documentation doesn't say how to get the status line (let me know if you find it: https://github.com/lostisland/faraday)

• I think it could be extra helpful if the error message briefly described the category of error: "There was an error parsing your input," "The API server rejected your changes," etc. Some of the more specific error messages can be cryptic, so providing category information could help a user understand more quickly whether they broke the rules of JSON or Arvados.

Done.

• arv edit doesn't write any message if it successfully updates the object. Should it? That seems like it better follows the general UX patterns for these tools.

Done.

• Not a blocker, just FYI, any? optionally takes a block, so you can write .any? { condition } rather than @.select { condition }.

Done.

Thanks.

Peter Amstutz wrote:

Brett Smith wrote:

• If we keep retrying for any non-200 API response, perhaps the rest of the status line ("Forbidden", "Unprocessable Entity", whatever) could be a better default error message when none were provided in the JSON body? (Currently it's an empty string.)

I had this thought too, but the HTTP library documentation doesn't say how to get the status line (let me know if you find it: https://github.com/lostisland/faraday)

Pfft. We can require net/http and ask the standard library for the normal message, at least:

2.1.1 :037 > status = 422
 => 422
2.1.1 :038 > Net::HTTPResponse::CODE_TO_OBJ[status.to_s].to_s.sub(/^Net::HTTP/, "").titleize
 => "Unprocessable Entity" 

I don't exactly like this code, but one grody line seems worthwhile to give the user something to work with.

The names HttpResponse and its status attribute make it seem like I'm dealing with an arbitrary HTTP response, rather than an exception and its message. Please consider a different name for the class (ArvadosAPIError?). And then I don't think you have to do anything else to get the messaging behavior you want:

2.1.1 :040 > class BCSError < RuntimeError
2.1.1 :041?>   end
 => nil
2.1.1 :042 > e = BCSError.new("foobar")
 => #<BCSError: foobar>
2.1.1 :043 > "#{e.class}: #{e}" 
 => "BCSError: foobar" 

Thanks.

Brett Smith wrote:

Pfft. We can require net/http and ask the standard library for the normal message, at least:

[...]

I don't exactly like this code, but one grody line seems worthwhile to give the user something to work with.

Done.

The names HttpResponse and its status attribute make it seem like I'm dealing with an arbitrary HTTP response, rather than an exception and its message. Please consider a different name for the class (ArvadosAPIError?). And then I don't think you have to do anything else to get the messaging behavior you want:

Done.

Peter Amstutz wrote:

Brett Smith wrote:

Pfft. We can require net/http and ask the standard library for the normal message, at least:

[...]

I don't exactly like this code, but one grody line seems worthwhile to give the user something to work with.

Done.

• The titleize is on the wrong side of the parentheses. When the API server returns a full error, that gets titleized into something like "Properties Must Be A Hash, Not A Nil Class." Which Provides It With An Amusing Coat Of Inappropriate Formality.
• Please move the require statement up near the top of the files with the others, for consistency.

Other bugs found while I was kicking these tires:

• JSON parse errors aren't being retried, because Oj.parse raises an Oj::ParseError, rather than a JSON::ParseError. (Huh, it's almost like arbitrarily switching between two different JSON modules could confuse programmers…)
• The successful update message for arv edit assumes that the UUID of the object hasn't been changed, but if I'm an admin I can totally do that. It should render the UUID from the object returned in the response, just like arv create.

Thanks.

Brett Smith wrote:

• The titleize is on the wrong side of the parentheses. When the API server returns a full error, that gets titleized into something like "Properties Must Be A Hash, Not A Nil Class." Which Provides It With An Amusing Coat Of Inappropriate Formality.

Fixed.

• Please move the require statement up near the top of the files with the others, for consistency.

Fixed.

Other bugs found while I was kicking these tires:

• JSON parse errors aren't being retried, because Oj.parse raises an Oj::ParseError, rather than a JSON::ParseError. (Huh, it's almost like arbitrarily switching between two different JSON modules could confuse programmers…)

Fixed.

• The successful update message for arv edit assumes that the UUID of the object hasn't been changed, but if I'm an admin I can totally do that. It should render the UUID from the object returned in the response, just like arv create.

Fixed.

Peter Amstutz wrote:

Brett Smith wrote:

• JSON parse errors aren't being retried, because Oj.parse raises an Oj::ParseError, rather than a JSON::ParseError. (Huh, it's almost like arbitrarily switching between two different JSON modules could confuse programmers…)

Fixed.

Please collapse the two JSON rescue blocks in check_result to reduce duplicate code (rescue JSON::ParserError, Oj::ParseError => e) and then merge. Thanks.