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

[SYNPY-1199] Implement low-level functions for JSON Schema API #894

Merged
merged 35 commits into from Jun 26, 2023

Conversation

BrunoGrandePhD
Copy link
Contributor

@BrunoGrandePhD BrunoGrandePhD commented Dec 7, 2021

Example usage: https://gist.github.com/BrunoGrandePhD/481249a94196cbdd47e8524dab7ddbbc

Install using:

 pip install --force-reinstall git+https://github.com/Sage-Bionetworks/synapsePythonClient@bgrande/SYNPY-1199/json-schemas-low-level

@pep8speaks
Copy link

pep8speaks commented Dec 7, 2021

Hello @BrunoGrandePhD! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 684:20: E221 multiple spaces before operator

Line 3:1: F401 '.json_schema.JsonSchemaService' imported but unused

Line 9:1: F401 'typing.TYPE_CHECKING' imported but unused
Line 192:21: W503 line break before binary operator

Comment last updated at 2022-02-10 17:57:27 UTC

@thomasyu888 thomasyu888 changed the title Implement low-level functions for JSON Schema API [SYNPY-1199] Implement low-level functions for JSON Schema API Jan 3, 2022
@ychae
Copy link
Contributor

ychae commented Jan 31, 2022

@milen-sage are you still able to review this by EOD today?

@BrunoGrandePhD
Copy link
Contributor Author

BrunoGrandePhD commented Feb 9, 2022

It has come to my attention that the displayName field in the user profile has been deprecated. Hence, new Synapse users won't have it and thus the _loggedIn() function doesn't work as expected; it returns None instead of the display name, which evaluates to True. I've opened a Synapse Platform ticket to see how to perform the same check now that displayName has been deprecated.

Shout-out to @afwillia for finding the bug!

@milen-sage
Copy link

@ychae, @afwillia is going through the JSONSchema client features, since that aligns with his work for this sprint.

Copy link
Member

@thomasyu888 thomasyu888 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some linting issues and comments.

synapseclient/client.py Show resolved Hide resolved
synapseclient/services/json_schema.py Outdated Show resolved Hide resolved
synapseclient/services/json_schema.py Show resolved Hide resolved
@thomasyu888 thomasyu888 marked this pull request as ready for review April 10, 2023 05:46
@thomasyu888 thomasyu888 requested a review from a team as a code owner April 10, 2023 05:46
@thomasyu888 thomasyu888 marked this pull request as draft April 10, 2023 05:46
Copy link
Member

@vpchung vpchung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In Slack, @BrunoGrandePhD and I discussed adding attribute validation to the API so that users are aware of invalid values as soon as possible, e.g. organizationName should be at least 6 characters long. The current behavior does not catch this after a user tries to create:

>>> foo = js.JsonSchemaOrganization("foo")
>>> foo.create()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/vchung/Desktop/synapsePythonClient/synapseclient/services/json_schema.py", line 288, in create
    already_exists = self.get()
  File "/Users/vchung/Desktop/synapsePythonClient/synapseclient/services/json_schema.py", line 257, in get
    raise e
  File "/Users/vchung/Desktop/synapsePythonClient/synapseclient/services/json_schema.py", line 251, in get
    response = self.service.get_organization(self.name)
  File "/Users/vchung/Desktop/synapsePythonClient/synapseclient/services/json_schema.py", line 428, in wrapper
    result = func(self, *args, **kwargs)
  File "/Users/vchung/Desktop/synapsePythonClient/synapseclient/services/json_schema.py", line 446, in get_organization
    response = self.synapse.restGET(
  File "/Users/vchung/Desktop/synapsePythonClient/synapseclient/client.py", line 4091, in restGET
    response = self._rest_call('get', uri, None, endpoint, headers, retryPolicy, requests_session, **kwargs)
  File "/Users/vchung/Desktop/synapsePythonClient/synapseclient/client.py", line 4075, in _rest_call
    self._handle_synapse_http_error(response)
  File "/Users/vchung/Desktop/synapsePythonClient/synapseclient/client.py", line 4045, in _handle_synapse_http_error
    exceptions._raise_for_status(response, verbose=self.debug)
  File "/Users/vchung/Desktop/synapsePythonClient/synapseclient/core/exceptions.py", line 160, in _raise_for_status
    raise SynapseHTTPError(message, response=response)
synapseclient.core.exceptions.SynapseHTTPError: 400 Client Error: 
Organization name must be at least 6 characters

This is currently in my backlog TODOs.

@thomasyu888 thomasyu888 marked this pull request as ready for review June 3, 2023 15:37
@thomasyu888 thomasyu888 merged commit a8e4a0b into develop Jun 26, 2023
32 of 35 checks passed
@thomasyu888 thomasyu888 deleted the bgrande/SYNPY-1199/json-schemas-low-level branch June 26, 2023 21:26
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

Successfully merging this pull request may close these issues.

None yet

6 participants