Return Synapse client error instead of ValueError where applicable

Description

Background: Consider the following instruction where `team_name` is the name of a team that already exists on Synapse.

```
syn.store(synapseclient.Team(name=team_name, description=desc, canPublicJoin=can_public_join))
```

Issue: The above instruction raises an exception of type `ValueError`.
1. See https://github.com/Sage-Bionetworks/synapsePythonClient/blob/4047cd179df5d965049ab1df938c51295096aa95/synapseclient/client.py#L920
2. See https://github.com/Sage-Bionetworks/synapsePythonClient/blob/3b3155aa3c1aaa81ee561724e991d3caca8cf58e/synapseclient/dict_object.py#L17

Suggested solution: It would be great to return a `SynapseHTTPError` object to the user of the python client so that he/she can handle different scenarios based on the value of a status code documented by Synapse (e.g. status code of the rest api response).

Environment

None

Activity

Show:
Kimyen Truong
March 5, 2019, 2:59 AM

Hi , can you include the current error?

Also, why would you prefer to use SynapseHTTPError instead of ValueError? Was there a best practice where HTTP error is prefer over ValueError in implementing a RESTful client? (independent to the current implementation of synapseclient)

Thomas Schaffter
March 5, 2019, 6:34 AM
Edited

can you include the current error?

I am using the synapse python client programmatically. The error is not relevant here. An exception ValueError is raised when the line of code previously mentioned is executed, which is expected (see Background section of the ticket).

Also, why would you prefer to use SynapseHTTPError instead of ValueError?

It is a good design practice to encapsulate the exceptions of a package into custom exception like the ones that already exist such as SynapseHTTPError, SynapseFileCacheError and the generic SynapseError. You already have that in place and this is great! In my program, when I execute the following line of code:

, which is a method provided by the synapse python client, an ValueError is raised. The reason I suggest to raise an exception that implement SynapseError is because the exception raised is the result of the logic of the python client to not accept the creation of team from a name that is already used. Another logic could have been to return the handle to the team if the user who execute the program is the owner of the team (note that this logic is actually implemented when creating a Project, i.e. a handle to the Project is return to the user if the Project exists and if the user is is the owner/has edit access to the project).

Therefore, it is fine if the python client raise ValueError if this is an unexpected result. However, if we provide the user with a method to create a Team, the case where the user requests the creation of a Team with a name that is already being used is expected and thus the exception raised should implement/extend SynapseError.

Kimyen Truong
March 5, 2019, 7:26 PM

, I totally agree with with raising an SynapseError. The description mentioned SynapseHTTPError, which is an error that comes from Synapse backend. In the case that the backend throws an error, we should returns a SynapseHTTPError. In the case that “the logic of the python client to not accept the creation of team from a name that is already used,” we should returns a client error.

Thank you for suggesting the change! I am going to change the title of the ticket to remind myself what this ticket is about. Please feel free to comment if it no longer describes what you asked for.

Assignee

Unassigned

Reporter

Thomas Schaffter

Labels

Validator

Thomas Schaffter

Development Area

None

Release Version History

None

Components

Affects versions

Priority

Minor
Configure