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.

Sign up for GitHub

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

Add POST endpoint for additional access methods to existing object #47

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from
Open

Add POST endpoint for additional access methods to existing object #47

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cafae27
Add POST endpoint for additional access methods to existing object
suhaspete Nov 14, 2023
4a19ccd
Update additional.data_repository_service.swagger.yaml
suhaspete Nov 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 59 additions & 1 deletion drs_filer/api/additional.data_repository_service.swagger.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,47 @@ paths:
tags:
- DRS-Filer
x-swagger-router-controller: ga4gh.drs.server
'/objects/{object_id}/access':
Copy link
Member

Choose a reason for hiding this comment

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

As briefly discussed, I'm afraid we will need to create a different endpoint here, maybe /objects/{object_id}/access:register, because in the latest versions of the DRS specification, there is (unfortunately) already a POST method defined for /objects/{object_id}/access that has a different non-REST-compliant meaning (see here).

Actually, supporting the new DRS versions will require a lot of code changes on DRS-Filer, including some breaking changes, but for now let's just focus on this issue and try to avoid conflicts in the future, when we will do the migration to the latest version.

post:
summary: Add additional access method to an existing object.
Copy link
Member

Choose a reason for hiding this comment

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

Add an additional access method to an existing object

operationId: PostObjectAccess
Copy link
Member

Choose a reason for hiding this comment

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

For the same reason as above, maybe it's better to write PostObjectAccessRegister here.

responses:
'201':
description: Successfully added access method.
Copy link
Member

Choose a reason for hiding this comment

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

To keep in the style of the other descriptions, maybe better to write:

The access method was successfully added.

'400':
description: The request is malformed.
schema:
$ref: '#/definitions/Error'
'401':
description: The request is unauthorized.
schema:
$ref: '#/definitions/Error'
'403':
description: The requester is not authorized to perform this action.
schema:
$ref: '#/definitions/Error'
'409':
description: Conflict - Object with the same ID already exists.
schema:
$ref: '#/definitions/Error'
Comment on lines +132 to +135
Copy link
Member

Choose a reason for hiding this comment

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

Given that in a POST request the resource ID will generally be created by the implementation, it is the implementation's responsibility to ensure that this type of error does not happen. So you can remove it from the specs.

'500':
description: An unexpected error occurred.
schema:
$ref: '#/definitions/Error'
parameters:
- name: object_id
in: path
required: true
type: string
- in: body
name: AdditionalAccessMethod
description: Additional access method details.
schema:
$ref: '#/definitions/AdditionalAccessMethod'
Copy link
Member

Choose a reason for hiding this comment

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

Please use AccessMethodRegister, see below.

tags:
- DRS-Filer
x-swagger-router-controller: ga4gh.drs.server

delete:
summary: Delete object.
description: >-
Expand Down Expand Up @@ -148,7 +189,7 @@ paths:
tags:
- DRS-Filer
x-swagger-router-controller: ga4gh.drs.server
'/objects/{object_id}/access/{access_id}':
'/objects/{object_id}/access/{access_id}':
delete:
summary: Delete object's access method.
description: >-
Expand Down Expand Up @@ -501,3 +542,20 @@ definitions:
- name
tags:
- name: DataRepositoryService
AdditionalAccessMethod:
Copy link
Member

Choose a reason for hiding this comment

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

I think you can just reuse the AccessMethodRegister model/component that is already defined in this file. No need to create a new one.

type: object
properties:
method_id:
type: string
description: ID of the access method.
Comment on lines +548 to +550
Copy link
Member

Choose a reason for hiding this comment

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

This should be created by the implementation/service, so no need to put it here.

method_type:
type: string
description: Type of the access method (e.g., S3, HTTPS).
method_url:
type: string
description: URL for the access method.
required:
- method_id
- method_type
- method_url