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

feat: Create users in metadata backend via API #289

Merged
merged 6 commits into from
Apr 20, 2021
Merged

feat: Create users in metadata backend via API #289

merged 6 commits into from
Apr 20, 2021

Conversation

sewardgw
Copy link
Contributor

@sewardgw sewardgw commented Apr 7, 2021

Summary of Changes

  • Exposed a PUT method for the user details API resource
  • Added corresponding swagger docs
  • New function to create or update user was added to the base proxy
    • Function was implemented for Neo4j, added as placeholder for other proxies

This implementation heavily relies on the validation and pre-processing being done as part of UserSchema().load({...}) to validate the inputs of the data provided in the API. As new fields / validation rules are added to the User schema there should hopefully not be any changes to this API. When invoking the API any number of fields can be provided as long as the minimum required fields are provided.

Sample invocations

To invoke the API directly, locally:

import requests as r
data = {'email': 'an_email@aol.com', 'first_name': 'first', 'last_name': 'last'}
url = 'http://0.0.0.0:5002/user'
resp = r.put(url, json=data)
resp.json()
# {
#   "profile_url": null,
#   "is_active": true,
#   "slack_id": null,
#   "user_id": null,
#   "manager_fullname": "",
#   "last_name": "last",
#   "full_name": null,
#   "team_name": null,
#   "employee_type": null,
#   "manager_email": null,
#   "email": "some_email@aol.com",
#   "other_key_values": {},
#   "github_username": null,
#   "first_name": "first",
#   "display_name": null,
#   "role_name": null,
#   "manager_id": null
# }

Similarly, an invalid payload will return the reason why:

import requests as r
data = {}
url = 'http://0.0.0.0:5002/user'
resp = r.put(url, json=data)
resp.json()
# {'message': 'User inputs provided are not valid: {\'_schema\': [\'"display_name", "full_name", or "email" must be provided\']}'}

To invoke from the amundsen frontend:

from amundsen_application.api.utils.request_utils import request_metadata
from amundsen_application.models.user import load_user

def get_create_user() -> User:    
    url = '{0}{1}'.format(app.config['METADATASERVICE_BASE'], '/user')
    _email_ = 'abc_test_email_123'
    user_info = {
        'email': _email_,
        'user_id': _email_,
        'first_name': 'Firstname',
        'last_name': 'Lastname'
    }
    user_resp = request_metadata(url=url, method='PUT', json=user_info)
    return load_user(json.loads(user_resp.json()))

Tests

  • Tests added for PUT method as well as Neo4j proxy

Documentation

  • Added corresponding swagger docs under swagger_doc/user/detail_put.yml

CheckList

Make sure you have checked all steps below to ensure a timely review.

  • PR title addresses the issue accurately and concisely. Example: "Updates the version of Flask to v1.0.2"
  • PR includes a summary of changes.
  • PR adds unit tests, updates existing unit tests, OR documents why no test additions or modifications are needed.
  • In case of new functionality, my PR adds documentation that describes how to use it.
    • All the public functions and the classes in the PR contain docstrings that explain what it does
  • PR passes make test

sewardgw added 3 commits April 7, 2021 14:20
…a API.

Signed-off-by: Grant Seward <grant@stemma.ai>
Signed-off-by: Grant Seward <grant@stemma.ai>
Signed-off-by: Grant Seward <grant@stemma.ai>
@dorianj
Copy link
Contributor

dorianj commented Apr 10, 2021

LGTM pending a test!

Signed-off-by: Grant Seward <grant@stemma.ai>
Signed-off-by: Grant Seward <grant@stemma.ai>
@codecov-io
Copy link

codecov-io commented Apr 14, 2021

Codecov Report

Merging #289 (a1be96b) into master (2752492) will increase coverage by 1.95%.
The diff coverage is 69.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #289      +/-   ##
==========================================
+ Coverage   74.10%   76.06%   +1.95%     
==========================================
  Files          25       27       +2     
  Lines        1255     1483     +228     
  Branches      136      180      +44     
==========================================
+ Hits          930     1128     +198     
+ Misses        297      296       -1     
- Partials       28       59      +31     
Impacted Files Coverage Δ
metadata_service/api/popular_tables.py 100.00% <ø> (ø)
metadata_service/api/system.py 66.66% <ø> (ø)
metadata_service/api/user.py 100.00% <ø> (ø)
metadata_service/entity/dashboard_query.py 100.00% <ø> (ø)
metadata_service/entity/dashboard_summary.py 100.00% <ø> (ø)
metadata_service/entity/description.py 100.00% <ø> (ø)
metadata_service/entity/tag_detail.py 100.00% <ø> (ø)
metadata_service/proxy/statsd_utilities.py 81.25% <ø> (ø)
metadata_service/util.py 100.00% <ø> (ø)
metadata_service/proxy/shared.py 28.57% <28.57%> (ø)
... and 23 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2336439...a1be96b. Read the comment docs.

@sewardgw sewardgw changed the title WIP: Create users in metadata backend via API feat: Create users in metadata backend via API Apr 14, 2021
@sewardgw sewardgw marked this pull request as ready for review April 14, 2021 16:25

user_result = result.single()
if not user_result:
raise RuntimeError('Failed to create user with data %s' % user_data)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this should be a RuntimeError since no result existing would likely be an issue with neo4j, not this program.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that's a fair point. This specific error was used to keep in line with the existing paradigm for how Neo4j integration errors are raised, e.g.:

There are 6-7 places where this is used the the thought was that reusing this exception would make it easier to catch and handle these errors if we only need to watch for one type of exception.

Given this, if you'd like to continue with a more descriptive exception hat are your thoughts on something akin to a Neo4jErrorException class and raise that here in place of the RuntimeError the code above (with migration of other runtime errors to this new exception occurring through a refactor in the future)?

Signed-off-by: Grant Seward <grant@stemma.ai>
Copy link
Contributor

@dorianj dorianj left a comment

Choose a reason for hiding this comment

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

Thanks @sewardgw for taking this over the line, and thank you @alran for doing this originally! This should be compatible with the API shape that Square is using, so should be able to remove your customization

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.

5 participants