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

Better message needed prompting admin to setup Remote Site Setting for ApexUML #6

Closed
ImJohnMDaniel opened this issue Apr 3, 2014 · 6 comments
Assignees
Milestone

Comments

@ImJohnMDaniel
Copy link
Collaborator

When ApexUML is installed, the usual experience is that the admin forgets to add a Remote Site Setting record to allow ApexUML to utilize the connection to the ToolingAPI. (I know I keep forgetting). Right now the exception comes back as very convoluted and is not a "user friendly" message explaining what needs to be done. We can do better!!! ;-)

@afawcett
Copy link
Owner

afawcett commented Apr 4, 2014

Ha, we can indeed and will! 👍

@ImJohnMDaniel
Copy link
Collaborator Author

@afawcett - What is your opinion about more exception classes in the ToolingAPI class?

I am working on this ticket now. Basically, it will be solved by wrapping the http.send() call in the submitRestCall(String, String, Object) method with a try catch block which will catch a System.CalloutException.

Currently, the ToolingAPI class has a single exception (ToolingAPIException). The ToolingAPIException has a distinct "purpose." It supports relaying error codes from the tooling api but it does not really support the concept of errors within the ToolingAPI Apex class and, as with this case, communication errors prior to reaching the SFDC Tooling API endpoint.

I am reluctant to use this ToolingAPIException class as the exception to be thrown here for this issue. It does not convey the errors from the SFDC Tooling API methods; it is instead a CalloutException. My reluctance comes from trying to get the ToolingAPI consumer Apex class to interpret what the exception is regarding. In this situation, the ToolingAPIException would not have ErrorResponse records.

In general, I see a potential need to segment out the exception types for the ToolingAPI class in one of three manners:

  1. multiple exception classes: There could be multiple exception classes setup to handle the various types. One type would be a "general" exception. Another type would be a "Salesforce Tooling api method error." There are probably others. A distinct class (or "error contract") would allow the consuming code of the ToolingAPI service to understand when it needs to look for a list and when the issue is something else. There are obvious impacts to the ToolingAPI project and potential for changes to the exception level service contracts in that project, but it is the option that I would recommend.

  2. single exception with ENUM types: Another way to approach this is to add a TYPE enum to the ToolingAPIException class and adjust the constructor. This would allow us to program a general "error type" to the exception and, through documentation, the programmer would know to look for ErrorResponses in one case and not in another.

  3. make no change at all: In this case, the ToolingAPI does what it is doing now. It does not catch the System.CalloutException at all. I am not a big fan of this approach because, IMO, it violates the idea of separation of concerns. The ToolingAPI can be considered a "fine grain service". Any issues that occur in the "fine grain service" should be contained there and then the "fine grain service" should convey the appropriate message to the calling code. I do not like the idea that the calling code should also need to trap other "types of exceptions" that the "fine grain service" is not trapping. (While I am not a fan of this approach, there is an argument to be made that with enough documentation of the possibility that other, un-caught exceptions could occur, this approach could be used. I am open to debate on the topic if you are interested in pursuing this course.) Without a doubt though, this is the simplest course of action.

What are your thoughts about these options?

@dancinllama, do you have any thoughts about this discussion?

@ImJohnMDaniel ImJohnMDaniel modified the milestone: 1.6 Apr 6, 2014
ImJohnMDaniel pushed a commit that referenced this issue Apr 6, 2014
Temporarily am trapping the CalloutException in the ToolingAPI and
throwing a ToolingAPIException.

The UmlService is now trapping the ToolingAPI.ToolingAPIException in
certain places and creates a new UmlServiceException to throw to the
client tier (i.e. UmlController).

This is a temporary solution until the debate about expanding the
ToolingAPI.ToolingAPIException (see comments on this issue) is
concluded.  

These changes should remain on this branch until debate is comcluded.
@afawcett
Copy link
Owner

afawcett commented Apr 7, 2014

I like option 1, it's OO and the way exceptions should work, catching an exception then inspecting an enum or expecting the call to do some work is not nice. Though since all exceptions extend Exception if the caller is simply not interested they can catch this as well, and all we need to do is ensure all our exception types honour the base class needs to provide a reasonable message. So i am in favour of having another exception class basically, option 1. 👍

@afawcett
Copy link
Owner

afawcett commented Apr 7, 2014

John, after a brief conflab with co-contributor James, it was a no brainer, your now a committer, it makes sense as the Tooling API and Apex UML are somewhat developing together. And I've been keeping both in sync as a commit into Apex UML and then into Tooling API to avoid divergence (must go see that film!).

@ImJohnMDaniel
Copy link
Collaborator Author

Ah. Very cool! Thank you both for that. Cheers!

cc: @dancinllama

@ImJohnMDaniel ImJohnMDaniel modified the milestones: 1.7, 1.6 Apr 7, 2014
ImJohnMDaniel pushed a commit that referenced this issue Apr 8, 2014
ToolingAPI
* added ToolingAPIAuthorizationException
* submitRestCall(string, string, Object) method now throws
ToolingAPIAuthorizationException if a CalloutException occurs where the
endpoint is not authorized.

UmlService
* reorganized methods so that private methods appear first then public
and all methods are sorted alphabetically.
* added new makeException(ToolingApi.ToolingAPIAuthorizationException)
method to handle endpoint authorization issues.
* other methods updated to make use of new makeException method
* spaced some of the methods out some for readability.
@afawcett
Copy link
Owner

This has been included in v1.7.

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

No branches or pull requests

2 participants