canvas-api icon indicating copy to clipboard operation
canvas-api copied to clipboard

External tool not found

Open buckett opened this issue 5 years ago • 2 comments

If I attempt to get an External Tool from Canvas using:

    ExternalToolReader reader = apiFactory.getReader(ExternalToolReader.class, token);
    Optional<ExternalTool> tool = reader.getExternalToolInAccount(accountId, toolId)

then if the tool doesn't exist in Canvas I don't get back an empty Optional but instead a edu.ksu.canvas.exception.ObjectNotFoundException is thrown.

It seems like either when the toolId doesn't exist we should either:

  • get an empty Optional
  • throw an exception, but not have the method return an Optional or is there another reason why we should have both the Optional and the exception?

buckett avatar Nov 14 '19 15:11 buckett

A side note on this is that when the tool doesn't exist you also get logging warnings:

2019-11-14 15:50:55.440 ERROR 44831 --- [           main] edu.ksu.canvas.net.SimpleRestClient      : Object not found in Canvas. Requested URL: https://instance.instructure.com/api/v1/accounts/1/external_tools/789
2019-11-14 15:50:55.443 ERROR 44831 --- [           main] edu.ksu.canvas.net.SimpleRestClient      : Body of error response from Canvas: {"errors":[{"message":"The specified resource does not exist."}]}

buckett avatar Nov 14 '19 15:11 buckett

Yeah, the use of Optional for single object returns was questionable. Our team was pretty new to Java 8 and I'm not sure we did the right thing here. If you request an object that doesn't exist in Canvas, that is an error state that the calling application probably needs to know about. An exception allows you to pass back more details about the error so that the calling application might be able to handle it in some way. Just returning an empty Optional kind of leaves you hanging as to what actually happened. Doesn't exist? Not authorized to view as current user? Network problem?

I'm kind of tempted to remove it but of course that would break everything.

ToeBee avatar Nov 15 '19 21:11 ToeBee