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

External tool not found #116

Open
buckett opened this issue Nov 14, 2019 · 2 comments
Open

External tool not found #116

buckett opened this issue Nov 14, 2019 · 2 comments

Comments

@buckett
Copy link
Contributor

buckett commented Nov 14, 2019

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
Copy link
Contributor Author

buckett commented Nov 14, 2019

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."}]}

@ToeBee
Copy link
Member

ToeBee commented Nov 15, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants