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

Plain text /tools/{id}/versions/{version_id}/containerfile #162

Open
garyluu opened this issue Oct 28, 2020 · 3 comments
Open

Plain text /tools/{id}/versions/{version_id}/containerfile #162

garyluu opened this issue Oct 28, 2020 · 3 comments

Comments

@garyluu
Copy link
Contributor

garyluu commented Oct 28, 2020

The /tools/{id}/versions/{version_id}/containerfile endpoint returns an application/json only. It would be nice if there is a way to get the containerfile as text/plain. This is so that you can easily get the file via wget/curl instead of extracting the contents from the application/json

┆Issue is synchronized with this Jira Story
┆containerName: GA4GH tool-registry-service
┆Issue Number: TRS-47

@uniqueg
Copy link
Collaborator

uniqueg commented Nov 6, 2020

I agree that this would be nice, but there are some caveats I see:

  1. The endpoint returns an array in the current schema, as multiple container recipes can be associated with a given tool version. So which container recipe to return for the text/plain response?
  2. File contents may not always be available, as the specs require only one of file contents and an URL to the raw data. So what to return if only the URL is available?

Regarding problem 1: This problem could be avoided, if the container recipe type were included in the endpoint route as {type} as suggested in #155 (comment) and each tool version could only have a single container recipe associated for each container type. IMO this would also improve reproducibility. On the other hand, there may be reasonable use cases to have, say, multiple Dockerfiles associated with a single tool version. To allow multiple container recipes of the same type, yet still return only a single container recipe, something like {containerfile_id} could be introduced in the endpoint path. But as that would require a way to discover the different container identifiers (similar to how /tools/{id}/versions/{version_id}/{type}/descriptor/{files} allows to discover the relative paths of descriptor-associated files), it will hardly be easier for the client.

Regarding problem 2: It is probably not a good idea to return the URL for text/plain if file contents are unavailable, as the response would be ambiguous (a file with just a URL inside may well be a valid file content, if probably not a valid container recipe). Having the server attempt to download the file from the URL would be a possible solution, but this requires a call to an external resource, which may not be ideal. Moreover, the file contents would have to be downloaded every time, because downloading and then updating the resource with the contents would violate REST specs as GET requests should never modify a resource. Probably it's easiest/safest to return a 404 in case of missing file contents. But in that case the client would then need to request and parse the application/json response after all to get the URL. And that means the text/plain response only saves work sometimes, and other times it means more work.

@denis-yuen
Copy link
Member

re: problem 1

The array issue is indeed a pain when switching between representations. The test JSON endpoint https://github.com/ga4gh/tool-registry-service-schemas/blob/develop/openapi/ga4gh-tool-discovery.yaml#L359 has a similar problem in that it can also allow for multiple test parameter files which is a problem when switching types.

re: problem 2

We could return a redirection of some form like a https://en.wikipedia.org/wiki/HTTP_303 or a 307.

@uniqueg
Copy link
Collaborator

uniqueg commented Nov 16, 2020

Great suggestion on the redirection. Might actually be something to consider for all endpoints returning FileWrapper on application/json when requesting text/plain instead: return a 200 with the file content, if available, or else a 303 redirection.

Regarding GET .../tests: Endpoint could include a {file_name} path param as well, just like {relative_path} for (secondary) descriptor files.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants