Move API endpoints from the config file to the code #118

tommydeboer · 2019-04-18T13:30:19Z

Depends on: #117 Error when the config contains (old) unknown properties

This PR is a preparation for the compatibility framework

After this PR you can no longer override the API endpoints in the config file, so running integration tests against a MOLGENIS 8 will cause some tests (involving groups) to fail.

Checklist

marikaris

Do we really want to move this from the config? I thought we found it elegant that when the name of an API in molgenis is changed, it can still be used, unless functionality of the API was changed of course.

dennishendriksen

I suspect most encoding issues could be prevented by using the MOLGENIS Python API client.

dennishendriksen · 2019-05-07T04:41:43Z

mcmd/client/api.py

+
+@endpoint
+def member(group_name):
+    return 'api/plugin/security/group/{}/member/'.format(group_name)


group_name should be encoded (something like https://stackoverflow.com/a/6431284)

@tommydeboer group name should still be encoded?

dennishendriksen · 2019-05-07T04:46:14Z

mcmd/commands/delete.py

@@ -98,15 +98,15 @@ def _delete_package_contents(args):

 def _delete_entity_types_in_package(package_id):
    response = client.get(
-        config.api('rest2') + ResourceType.ENTITY_TYPE.get_entity_id() + '?attrs=id&q=package==' + package_id)
+        api.rest2() + ResourceType.ENTITY_TYPE.get_entity_id() + '?attrs=id&q=package==' + package_id)


package_id should be RSQL encoded (as described in https://github.com/jirutka/rsql-parser)

package_id should be encoded (something like https://stackoverflow.com/a/51649797)

dennishendriksen · 2019-05-07T04:46:29Z

mcmd/commands/delete.py

    entity_ids = [entity_type['id'] for entity_type in response.json()['items']]
    if len(entity_ids) > 0:
        _delete_rows(ResourceType.ENTITY_TYPE.get_entity_id(), entity_ids)


 def _delete_packages_in_package(package_id):
    response = client.get(
-        config.api('rest2') + ResourceType.PACKAGE.get_entity_id() + '?attrs=id&q=parent==' + package_id)
+        api.rest2() + ResourceType.PACKAGE.get_entity_id() + '?attrs=id&q=parent==' + package_id)


see previous comment on encoding

dennishendriksen · 2019-05-07T04:47:03Z

mcmd/commands/delete.py



 def _delete_rows(entity_type, rows):
-    url = '{}{}'.format(config.api('rest2'), entity_type)
+    url = '{}{}'.format(api.rest2(), entity_type)


entity_type needs to be encoded

dennishendriksen · 2019-05-07T04:48:07Z

mcmd/commands/set.py

@@ -66,7 +66,7 @@ def set_(args):
    io.start(
        'Updating {} of {} settings to {}'.format(highlight(args.setting), highlight(args.type), highlight(args.value)))
    row = _get_row_id(entity)
-    url = '{}{}/{}/{}'.format(config.api('rest1'), entity, row, args.setting)
+    url = '{}{}/{}/{}'.format(api.rest1(), entity, row, args.setting)


URL components should be encoded

dennishendriksen · 2019-05-07T04:50:52Z

mcmd/utils/resources.py

@@ -53,14 +53,14 @@ def detect_resource_type(resource_id, types: List[ResourceType]):
 def resource_exists(resource_id, resource_type):
    log.debug('Checking if %s %s exists' % (resource_type.get_label(), resource_id))
    query = '?q={}=={}'.format(resource_type.get_identifying_attribute(), resource_id)


encoding issues (see previous comments)

dennishendriksen · 2019-05-07T04:51:09Z

mcmd/utils/resources.py

@@ -53,14 +53,14 @@ def detect_resource_type(resource_id, types: List[ResourceType]):
 def resource_exists(resource_id, resource_type):
    log.debug('Checking if %s %s exists' % (resource_type.get_label(), resource_id))
    query = '?q={}=={}'.format(resource_type.get_identifying_attribute(), resource_id)
-    response = get(config.api('rest2') + resource_type.get_entity_id() + query)
+    response = get(api.rest2() + resource_type.get_entity_id() + query)


resource_type.get_entity_id() should be encoded

dennishendriksen · 2019-05-07T04:51:35Z

mcmd/utils/resources.py

@@ -53,14 +53,14 @@ def detect_resource_type(resource_id, types: List[ResourceType]):
 def resource_exists(resource_id, resource_type):
    log.debug('Checking if %s %s exists' % (resource_type.get_label(), resource_id))
    query = '?q={}=={}'.format(resource_type.get_identifying_attribute(), resource_id)
-    response = get(config.api('rest2') + resource_type.get_entity_id() + query)
+    response = get(api.rest2() + resource_type.get_entity_id() + query)
    return int(response.json()['total']) > 0


 def one_resource_exists(resources, resource_type):
    log.debug('Checking if one of [{}] exists in [{}]'.format(','.join(resources), resource_type.get_label()))
    query = '?q={}=in=({})'.format(resource_type.get_identifying_attribute(), ','.join(resources))


encoded issues (see previous comments)

dennishendriksen · 2019-05-07T04:51:50Z

mcmd/utils/resources.py

    return int(response.json()['total']) > 0


 def one_resource_exists(resources, resource_type):
    log.debug('Checking if one of [{}] exists in [{}]'.format(','.join(resources), resource_type.get_label()))
    query = '?q={}=in=({})'.format(resource_type.get_identifying_attribute(), ','.join(resources))
-    response = get(config.api('rest2') + resource_type.get_entity_id() + query)
+    response = get(api.rest2() + resource_type.get_entity_id() + query)


encoding issues (see previous comments)

tests/unit/version/molgenis_version_test.py

dennishendriksen · 2019-05-07T05:08:18Z

@tommydeboer could you address the comment of @marikaris?

tommydeboer · 2019-05-07T09:33:20Z

@dennishendriksen @marikaris

Do we really want to move this from the config? I thought we found it elegant that when the name of an API in molgenis is changed, it can still be used, unless functionality of the API was changed of course.

Moving the location of these strings to the code makes it easier to switch between them based on versions (see #119). If we would want to do this in the config we would have to invent new keys for each endpoint (group7 and group8 for example) and add extra code for figuring out which key to use when. This PR is based on the idea that APIs won't constantly change names or move to another location. Even if that happens, it's a matter of adding one method in the code and doing a patch release. I have thought about also making it possible to override the endpoints in the config, but that seems like another feature of which I don't think it's worth the cost to build it.

tommydeboer · 2019-05-07T09:41:21Z

@dennishendriksen

I suspect most encoding issues could be prevented by using the MOLGENIS Python API client.

We're not using the Python client (yet), because:

There are a lot of other endpoints being used that are not the REST API
The Python client uses REST API v1 which will be deprecated
Not using it gives us more flexibility of when and how to authenticate
Not using it gives us a generalised way to handle all requests to MOLGENIS (+ error handling)

I think we can fix the encoding issues in the molgenis_client module.

marikaris · 2019-05-08T13:31:16Z

@dennishendriksen

I suspect most encoding issues could be prevented by using the MOLGENIS Python API client.

We're not using the Python client (yet), because:

There are a lot of other endpoints being used that are not the REST API

The Python client uses REST API v1 which will be deprecated

Not using it gives us more flexibility of when and how to authenticate

Not using it gives us a generalised way to handle all requests to MOLGENIS (+ error handling)

I think we can fix the encoding issues in the molgenis_client module.

@tommydeboer
In the molgenis py client (which soon will be doing get requests over API v2 rather than v1) we use quote_plus from urllib.parse to fix those encoding issues. Maybe we can use that?

tommydeboer · 2019-05-09T14:09:40Z

@dennishendriksen @marikaris
Concerning the escaping of RSQL values: I couldn't find a (reliable looking) Python library that does this. I've added an issue for this problem (#125) and propose to fix this another time since it is not introduced by this PR.

tommydeboer · 2019-05-09T15:45:12Z

@dennishendriksen
I've added an issue for using the Python REST Client (#126)

marikaris

I left a few comments, no sure if you should do something with it.

marikaris · 2019-05-10T08:59:37Z

mcmd/client/api.py

+
+@endpoint
+def member(group_name):
+    return 'api/plugin/security/group/{}/member/'.format(group_name)


@tommydeboer group name should still be encoded?

marikaris · 2019-05-10T09:09:39Z

mcmd/commands/add.py



 @command
 def add_token(args):
    io.start('Adding token %s for user %s' % (highlight(args.token), highlight(args.user)))

-    user = get(config.api('rest2') + 'sys_sec_User?attrs=id&q=username==%s' % args.user)
+    user = get(api.rest2('sys_sec_User'),


Do you want this tablename hardcoded here?

marikaris · 2019-05-10T09:10:07Z

mcmd/commands/add.py

@@ -173,7 +178,7 @@ def add_token(args):
    data = {'User': user_id,
            'token': args.token}

-    post(config.api('rest1') + 'sys_sec_Token', data)
+    post(api.rest1('sys_sec_Token'), data=data)


Do you want this tablename hardcoded here?

marikaris · 2019-05-23T08:45:52Z

mcmd/commands/make.py



 def _find_group(role):
    io.debug('Fetching groups')
-    groups = get(config.api('rest2') + 'sys_sec_Group?attrs=name')
+    groups = get(api.rest2('sys_sec_Group'),


Do we want this table name hardcoded here?

marikaris · 2019-05-23T08:46:13Z

mcmd/commands/set.py

-    query = 'q=extends==sys_set_settings&attrs=~id'
-    molgenis_settings = _quick_get(entity, query)
+    molgenis_settings = get(api.rest2('sys_md_EntityType'),
+                            params={


Do we want a hardcoded table name?

marikaris · 2019-05-23T08:47:08Z

mcmd/utils/principals.py

    return int(response.json()['total']) > 0


 def role_exists(rolename):
    log.debug('Checking if role %s exists' % rolename)
-    response = get(config.api('rest2') + 'sys_sec_Role?q=name==' + rolename.upper())
+    response = get(api.rest2('sys_sec_Role'),


Hardcoded tablename

marikaris · 2019-05-23T08:47:15Z

mcmd/utils/principals.py

@@ -30,13 +30,20 @@ def principal_exists(principal_name, principal_type):

 def user_exists(username):
    log.debug('Checking if user %s exists' % username)
-    response = get(config.api('rest2') + 'sys_sec_User?q=username==' + username)
+    response = get(api.rest2('sys_sec_User'),
+                   params={


hardcoded tablename

Outdated

tommydeboer changed the title ~~Move API endpoints from the config file to the code base~~ Move API endpoints from the config file to the code Apr 18, 2019

tommydeboer added the 🚧 work in progress label Apr 18, 2019

tommydeboer force-pushed the feature/api branch from dc931f8 to c9f3c8f Compare April 18, 2019 14:11

tommydeboer removed the 🚧 work in progress label Apr 18, 2019

tommydeboer mentioned this pull request Apr 18, 2019

Add compatibility framework #119

Merged

7 tasks

marikaris reviewed Apr 24, 2019

View reviewed changes

tommydeboer added 6 commits April 25, 2019 09:34

Add MolgenisOfflineError

49bb9ad

Remove endpoint information in client and simplify @request

ecd5745

Add api module and remove api section from config defaults

fbf3ac3

Add molgenis_version module

c174e5b

Refactor all places where api endpoints are used

f82a586

Fix @endpoint decorator not passing arguments

0d8f4c5

tommydeboer force-pushed the feature/api branch from aa210ef to 0d8f4c5 Compare April 25, 2019 07:35

dennishendriksen previously requested changes May 7, 2019

View reviewed changes

dennishendriksen mentioned this pull request May 7, 2019

Add delete --attribute option #120

Merged

7 tasks

tommydeboer mentioned this pull request May 9, 2019

Enable/disable languages #123

Merged

7 tasks

URL encoding

3db7fd3

tommydeboer force-pushed the feature/api branch from d66ec99 to 3db7fd3 Compare May 10, 2019 20:26

marikaris reviewed May 23, 2019

View reviewed changes

marikaris self-requested a review May 23, 2019 10:36

marikaris approved these changes May 23, 2019

View reviewed changes

marikaris merged commit c98087c into molgenis:master May 23, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move API endpoints from the config file to the code #118

Move API endpoints from the config file to the code #118

tommydeboer commented Apr 18, 2019 •

edited by marikaris

Loading

marikaris left a comment

dennishendriksen left a comment

dennishendriksen May 7, 2019

marikaris May 10, 2019

dennishendriksen May 7, 2019

dennishendriksen May 7, 2019

dennishendriksen May 7, 2019

dennishendriksen May 7, 2019

dennishendriksen May 7, 2019

dennishendriksen May 7, 2019

dennishendriksen May 7, 2019

dennishendriksen May 7, 2019

dennishendriksen commented May 7, 2019

tommydeboer commented May 7, 2019

tommydeboer commented May 7, 2019

marikaris commented May 8, 2019

tommydeboer commented May 9, 2019

tommydeboer commented May 9, 2019

marikaris left a comment

marikaris May 10, 2019

marikaris May 10, 2019

marikaris May 10, 2019

marikaris May 23, 2019

marikaris May 23, 2019

marikaris May 23, 2019

marikaris May 23, 2019

Move API endpoints from the config file to the code #118

Move API endpoints from the config file to the code #118

Conversation

tommydeboer commented Apr 18, 2019 • edited by marikaris Loading

Checklist

marikaris left a comment

Choose a reason for hiding this comment

dennishendriksen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dennishendriksen commented May 7, 2019

tommydeboer commented May 7, 2019

tommydeboer commented May 7, 2019

marikaris commented May 8, 2019

tommydeboer commented May 9, 2019

tommydeboer commented May 9, 2019

marikaris left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tommydeboer commented Apr 18, 2019 •

edited by marikaris

Loading