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

What purpose does the path param "{type}" really serve? And why the "PLAIN_" types? #210

Open
uniqueg opened this issue Dec 7, 2021 · 1 comment

Comments

@uniqueg
Copy link
Collaborator

uniqueg commented Dec 7, 2021

Currently, multiple endpoints make use of the {type} path param to specify the descriptor type of a workflow resource (e.g., CWL, WDL). As we are currently implementing a WES that accepts a (versioned) TRS URI for workflow_url, we were wondering whether TRS URIs should also contain the workflow type. However, in this context this seems unnecessary, as a WES request MUST contain a workflow_type (as well as a workflow_type_version) to be specified anyway.

So here are a couple of thoughts:

  • Having {type} but not {type_version} as path params is inconsistent. If we assume that a workflow can exist in multiple languages (which it arguably can, and we even have (toy/test) examples for this), then surely it could exist in multiple versions of the same language (e.g., Nextflow DSL1 and DSL2). Having only {type} is thus not sufficient to represent all possible representations of a - functionally identical - workflow under a resource with the same {id} and {version_id}.
  • If the purpose of having {type} is just to account for the PLAIN_ types instead, this seems awfully complicated and we might be better off with getting rid of the PLAIN_ types altogether and using just content negotation (application/json vs text/plain) for this. I would anyway propose to do that, as usage of PLAIN_ types is only specified in the description in prose, and thus the behavior cannot be enforced/validated programmatically. Besides, the description is ambiguous, as the current specs potentially present two ways of getting either the one or the other (by using the PLAIN_ types or by asking for text/plain, and vice versa), and it is unclear what to do if these two contradict each other (say, I specify PLAIN_WDL but then ask for application/json in my header).
  • Endpoint GET /tools/{id}/versions/{version_id}/containerfile does also NOT include {type} even though there are multiple container types enumerated (currently Docker, Singularity and Conda). Again, this is not consistent with the behavior for workflows.

For the next major revision of the TRS specs, I would thus propose to

  • drop PLAIN_ types in favor of the more formally specified content types (which are already specified for some endpoints, though possibly not quite consistently)
  • drop the {type} path param and dictate that functionally identical workflows in multiple languages or language versions should be registered under different version identifiers (e.g., 1.0.1-nfl-dsl1, 1.0.1-nfl-dsl2, 1.0.1-wdl) OR add a {type_version path parm to account for the possibility of having functionally identical workflows of multiple versions of the same language
  • while we are at it, it might make sense to harmonize the behavior of tools and workflows, potentially even merging endpoints (.../descriptor and .../containerfile are really the same endpoint if it weren't for {type}) or getting rid of the distinction between workflows and tools altogether

┆Issue is synchronized with this Jira Story
┆Project Name: Zzz-ARCHIVE GA4GH tool-registry-service
┆Issue Number: TRS-53

@denis-yuen
Copy link
Member

If we assume that a workflow can exist in multiple languages (which it arguably can, and we even have (toy/test) examples for this), then surely it could exist in multiple versions of the same language

Hmmm, this might be something we didn't consider. I understand/understood a workflow changing between DSL1 and DSL2 with newer versions. But we probably didn't anticipate a workflow supporting both in one {version_id} in other words. If we were to add this, it would be nice for it to be optional.

using just content negotation

I think this is a good change to propose. If I recall correctly, I think some of this mess was just because we couldn't figure out how to represent some things in swagger 2.0 / associated code generator and never got removed. Now that we're on OpenAPI 3.0, it makes sense to me to clean this up especially if we are considering doing a next major revision.

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

No branches or pull requests

2 participants