Feat/index record alias api #248
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* WIP design for adding /index/{GUID}/aliases endpoint to indexd
- Deprecate old '/alias' endpoint
- Add 'AliasInfo' JSON object description
- Remove DELETE /index/{GUID}/aliases/{ALIAS} endpoint
- Change behavior of DELETE /index/{GUID}/aliases/ endpoint -- delete
only some aliases, not all aliases
- Update descriptions of all endpoints and parameters for /{GUID}/alias/
- Add 404 (GUID not found) error to GET /index/{GUID}/aliases
- Add 404 (GUID not found), 403 (Non-unique aliases) to PUT, POST /index/{GUID}/aliases
- Add 404 (GUID not found or alias not found) to DELETE /index/{GUID}/aliases
- Specify behavior if subset of aliases to be deleted are
- Add error and response schema to /index/{GUID}/aliases
* Edit error response description to clarify that no records are deleted if a 400-Invalid Request error occurs.
* Removes constraint requiring aliases to not be duplicated in the request body. * Reasoning: aliases must be globally unique, but aliases in the request body can be duplicates of each other and the server can still fullfil the request, so there is no need for the constraint that aliases must not be duplicated in the request.
* Allow random alias functions to be used by tests * Separate 'guid_aliases' fixture into separate 'guid' and 'aliases' fixtures
* Refactor test fixtures
* Change DELETE endpoint behavior for consistency with other gen3 APIs
- New API behavior:
- DELETE `/index/{GUID}/aliases` (delete all)
- DELETE `/index/{GUID}/aliases/{ALIAS}` (delete one)
* Correctly compare alias payloads * Fix test fixtures not included in some tests * Fix some typos
- Basic GET and POST: no validation or error handling, only direct db manipulation - Reason: basic GET and POST required to bootstrap tests in `tests/test_aliases_endpoints.py`
* Validate results of good GET request, not just status code
- Add test for aliases with same name as GUID
- Add test for aliases with same name as app
endpoints on root of API
- Reason: Because of the `/{GUID|ALIAS}` endpoint, if an alias
shared the name of an endpoint on the root of the API such as `/latest`,
if an alias was named "latest" it would cause difficulty resolving the alias.
- Rename `get_random_guid` -> `create_random_record` to clarify it adds
a record to the indexd db
- Fix some errors in tests
- [x] invalid_GUID - [~] nonunique_aliases - [ ] previously_used_valid_alias - [ ] GUID_already_has_alias - [ ] duplicate_aliases_in_request - [ ] valid_GUID_empty_aliases - [ ] alias_has_name_of_endpoint_on_root - [ ] alias_has_name_of_existing_GUID BRAINDUMP 5:04pm - Was trying to figure out why test_PUT_aliases_nonunique_aliases wasn't working -- it seems like the alias IS being put into the table with the other_guid's value, but then the second write with the 'guid' value goes thru no problem. I think I might have misunderstood how the unique constraint on foreign keys works -- it looks like the insert might be updating the field instead ????? I figured it would fail....
- [x] invalid_GUID - [x] nonunique_aliases - [x] previously_used_valid_alias - [x] GUID_already_has_alias - [ ] duplicate_aliases_in_request - [x] valid_GUID_empty_aliases - [ ] alias_has_name_of_endpoint_on_root - [ ] alias_has_name_of_existing_GUID
- invalid_GUID
- nonunique_aliases
- previously_used_valid_alias
- GUID_already_has_alias
- duplicate_aliases_in_request
- valid_GUID_empty_aliases
- alias_has_name_of_endpoint_on_root
- alias_has_name_of_existing_GUID
NOTE - 'aliases do not have name of existing GUID' requirement
may conflict with feature implemented by [PR#123](#123)
- Add tests to POST and PUT for requests with emtpy body
- Reason: avoid error in blueprints due to behavior of flask.request.get_json() (returns None if request has content-type other than application/json)
- Write tests for DELETE all aliases endpoint - Write tests for DELETE one alias endpoint
- URL encode aliases in request - Fix bug caused by list.remove() mutating list
- To disambiguate the tests for the deprecated /alias/ endpoint
from the tests for the /index/{GUID}/aliases endpoint
- Update docs to indicate no body in response to PUT and POST aliases
Avantol13
reviewed
Oct 28, 2019
Avantol13
reviewed
Nov 5, 2019
paulineribeyre
requested changes
Nov 5, 2019
There was a problem hiding this comment.
looks gooood
- please check whether the README's mentions of aliases shoulb be updated
- sheepdog creates aliases via the indexclient with endpoint "PUT /alias/" - we need to discuss how to handle that. should we update the alias creation in sheepdog or should we just remove it? and we should update indexclient either way
| '/index/{GUID}/aliases': | ||
| get: | ||
| tags: | ||
| - index |
There was a problem hiding this comment.
I'd argue that alias endpoints should still be tagged "alias", I like docs sections
but i could be convinced otherwise :p
|
The style in this PR agrees with This formatting comment was generated automatically by a script in uc-cdis/wool. |
* Move some error handling from blueprint to driver for consistency
* Fix bug in test cases in which behavior of POST/PUT returning records was not tested * Add spaces to aliases in test cases * Move repeated 'get a record or None' logic into own function * Use flask.request.get_json(force=True) * Add statements to asserts in test code * Small changes and fixes suggested by reviewers * Move tests for deprecated alias endpoints to separate file
README looks good to me, it describes aliases as linking to GUIDs (which is the case with the changes from this PR) |
- Update POST/PUT to return aliases on success, as per spec - Update swagger documentation to reflect the spec
indexd/index/blueprint.py
Outdated
|
|
||
| return flask.jsonify(ret), 200 | ||
| aliases = blueprint.index_driver.get_aliases_for_did(record) |
There was a problem hiding this comment.
instead of querying the db, replace_aliases_for_did can return the list
paulineribeyre
approved these changes
Nov 7, 2019
This was referenced Dec 18, 2023
Open
Open
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PXP-4353
Implement API to manipulate index_record_alias
Add ability to create, update and delete aliases for GUIDs. Aliases can be used to query the root endpoint for a record instead of that record's GUID. Aliases are associated with one GUID only.
API uses
createandupdateArborist permissionsChange IndexRecordAlias table -- add unique constraint to alias name column
New Features
Bug Fixes
/ga4gh/dos/v1/dataobjects/<path:record>) preventing aliases from being returned when querying for a record's baseID.