looker / looker-sdk-ruby

Looker SDK for Ruby
Other
39 stars 43 forks source link

Model good error handling practices in examples #70

Open drstrangelooker opened 6 years ago

drstrangelooker commented 6 years ago

So I was working with a customer using the API who was struggling because the errors were not explanatory. I tried their call in the API web page and got back a detailed useful error message. I recommended that the user change their code to catch the error and display a useful error message.

This kind of pattern should be modeled in the examples.

begin
  req = {
    :name => group_name,
    :label => group_name,
    :type => 'string',
    :default_value => group_name,
    :user_can_view => false,
    :user_can_edit => false
  }
  attribute = sdk.create_user_attribute(req)
rescue LookerSDK::Error => e
  puts e.message
  puts e.errors if e.errors
  raise
end
jbandhauer commented 6 years ago

I'd also recommend adding puts e.class. Often without that context the message can be confusing.

jbandhauer commented 6 years ago

... and some generic info that an exception was raised calling whatever can be helpful if you use something like this as boilerplate around other calls. Otherwise it is just printing some stuff having to do with who-knows-what.

drstrangelooker commented 6 years ago

For the short examples like this, the raise makes sure that the call stack is printed so the developer can have a clue of what is happening where. For a more fully fledged application, absolutely.

jbandhauer commented 6 years ago

OK, sure. But, you didn't show something that is necessarily a full program; you showed a snippet, so no telling what code is gonna eat the exception if someone integrates that into their larger program. Just say'n ;)

jakemitchellxyz commented 6 years ago

+1 - I still have no clue how to catch errors with the SDK (aside from the generic catch above). It should be very clear in the readme or in the Docs or API reference (or all 3) how to check the status of an API call from the SDK. For my use case, I just need to know whether the call returned 200 or something else, but I don't know how to do that without looking at the SDK code. If I wanted to handle 409 in a special way, I (as a user) don't know how to check that.

jbandhauer commented 6 years ago

I fully agree that better docs would be great. Lacking that... 2xx responses simply return results, Other stuff throws exceptions as per the mapping in https://github.com/looker/looker-sdk-ruby/blob/master/lib/looker-sdk/error.rb#L14-L29

drstrangelooker commented 6 years ago

If you look at https://github.com/looker/looker-sdk-ruby/blob/master/lib/looker-sdk/error.rb you’ll see the various errors you can catch.

LookerSDK::Error catches any 4xx or 5xx return. LookerSDK:ClientError catches any 4xx return. LookerSDK:ServerError catches any 5xx return. LookerSDK::NotFound for example, catches a 404. You’ll need to look at that file to see all the other specific error classes.

So typically you might want to do something like...

begin
  # API code here
rescue LookerSDK::NotFound => e
  # handling for not found
  # This may not even be a real error case
rescue LookerSDK::ClientError => e
  # handling for any other client error
  # you can probably log a useful message
rescue LookerSDK::Error => e
  # hopefully should never get here, probably can’t recover
  # provide lots of logging and a stack trace
end
jbandhauer commented 6 years ago

I suspect that for most SDK use cases one wants to assume success and just report the errors (and fail) and not be highly concerned about specially processing specific error types - unless you are really writing something that knows exactly how to logically handle them and can work around any failures. That is why they are modeled as exceptions and not as a pattern that requires explicit checking of result code after each API call. Pardon the pun, but I think scripts that really know how to handle such errors without simply failing are the exceptional case.

jakemitchellxyz commented 6 years ago

That's great! Yeah, I saw that file, but people using the SDK shouldn't have to read the code to see how to handle errors. The use case @deangelo-llooker just showed is fantastic, I think that should go in the docs or somewhere more visible. I agree the exhaustive list of classes can stay hiding in the code though, like you said, I don't think many people will need to use that very often

drstrangelooker commented 6 years ago

Absolutely, but the kind of logging that might be useful may differ.

The one place that this model falls down a bit is the 404 response, which may or may not be an error. I usually catch the LookerSDK::NotFound then return a nil response or an empty collection.

On Tue, Jun 26, 2018, 2:38 PM John Bandhauer notifications@github.com wrote:

I suspect that for most SDK use cases one wants to assume success and just report the errors (and fail) and not be highly concerned about specially processing specific error types - unless you are really writing something that knows exactly how to logically handle them and can work around any failures. That is why they are modeled as exceptions and not as a pattern that requires explicit checking of result code after each API call. Pardon the pun, but I think scripts that really know how to handle such errors without simply failing are the exceptional case.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/looker/looker-sdk-ruby/issues/70#issuecomment-400419905, or mute the thread https://github.com/notifications/unsubscribe-auth/AKQyEWXbHVRdhf_BsbCei58qGkXbM88zks5uAn-xgaJpZM4UkzcS .

jakemitchellxyz commented 6 years ago

Also for context, in my use case, if I get a LookerSDK::Conflict, I can just pretend nothing happened, but any other error I need to log and fail