-
-
Notifications
You must be signed in to change notification settings - Fork 29
Consistent RDF permalinks with content negotiation #158
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
Conversation
It's not really RESTful
Only what is currently stored in the SPARQL endpoint
# Conflicts: # src/main/java/org/commonwl/view/git/GitDetails.java
Previously regenerated using cwltool, which will take longer and is impossible for generic git additions
# Conflicts: # src/main/java/org/commonwl/view/cwl/CWLToolRunner.java # src/main/java/org/commonwl/view/workflow/WorkflowService.java
Workaround for common-workflow-language/cwltool#427
| manifest.setRetrievedBy(appAgent); | ||
| manifest.setRetrievedOn(manifest.getCreatedOn()); | ||
| manifest.setRetrievedFrom(new URI(gitInfo.getUrl())); | ||
| manifest.setRetrievedFrom(new URI(id + "?format=ro")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
id might include an #anchor, but ?query must go before anchors.
| Files.copy(png.toPath(), bundleRoot.resolve("visualisation.png")); | ||
| PathMetadata pngAggr = bundle.getManifest().getAggregation(bundleRoot.resolve("visualisation.png")); | ||
| pngAggr.setRetrievedFrom(new URI(appAgent.getUri() + workflow.getVisualisationPng())); | ||
| pngAggr.setRetrievedFrom(new URI("https://w3id.org/cwl/v/git" + workflow.getLastCommit() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This breaks for packed nested workflows as we would (if you look at two ROs) falsely say pav:retrievedFrom <thesame> for two different workflows, when actually none of those would be able to download (as it now throw 303).
Perhaps if we are anyway going for ?query parameters we could have &part=main to select the packed part? Trouble is that ordering of parameters in the URL matters semantically - so blah.cwl?part=main&format=png or blah.cwl?format=png&part=main?
(alternatively blah.cwl%23main?format=png as in HTML, or a new style like blah.cwl;?format=png ??)
| Files.copy(svg.toPath(), bundleRoot.resolve("visualisation.svg")); | ||
| PathMetadata svgAggr = bundle.getManifest().getAggregation(bundleRoot.resolve("visualisation.svg")); | ||
| svgAggr.setRetrievedFrom(new URI(appAgent.getUri() + workflow.getVisualisationSvg())); | ||
| svgAggr.setRetrievedFrom(new URI("https://w3id.org/cwl/v/git" + workflow.getLastCommit() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... you said "missing slash after /git" :)
| logger.error("Could not get RDF for workflow from raw URL", ex.getMessage()); | ||
| } | ||
| String rdfUrl = wfDetails.getUrl(workflow.getLastCommit()).replace("https://", ""); | ||
| if (rdfService.graphExists(rdfUrl)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rdfService should no longer use http://localhost:3030/sparql or similar as base URL, but should use our new permalinks so they match 1:1 with the RDF resource we are responding for here (needs to include the ?part or #part - but NOT ?format) - e.g. https://w3id.org/cwl/v/git/deadbeef1337/packedworkflow.cwl?part=main
| return getGraph("svg", gitDetails, response); | ||
| response.setHeader("Content-Disposition", "inline; filename=\"graph.svg\""); | ||
| FileSystemResource test = workflowService.getWorkflowGraph("svg", gitDetails); | ||
| return test; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unnecessary test variable, return directly
| public byte[] getRdfAsTurtle(@PathVariable("commitid") String commitId, | ||
| HttpServletRequest request) { | ||
| Workflow workflow = getWorkflow(commitId, request); | ||
| String rdfUrl = workflow.getRetrievedFrom().getUrl(commitId).replace("https://", ""); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to fetch workflow first as rdfUrl is (soon) directly creatable from commitId and request path.
| * @param request The HttpServletRequest from the controller to extract path | ||
| * @throws WorkflowNotFoundException If workflow could not be found (404) | ||
| */ | ||
| private Workflow getWorkflow(String commitId, HttpServletRequest request) throws WorkflowNotFoundException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need a similar getWorkflowURI() that reconstructs the https://w3id.org/cwl/v/... graph URI
| // Add the old commit for the purposes of permalinks | ||
| // TODO: Separate concept of commit from branch ref, see #164 | ||
| GitDetails byOldCommitId = workflow.getRetrievedFrom(); | ||
| byOldCommitId.setBranch(workflow.getLastCommit()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this still work OK if a Workflow already exists with that commit explicitly set?
| PathMetadata cwlAggregate = manifest.getAggregation( | ||
| bundleRoot.resolve("lobSTR-workflow.cwl")); | ||
| assertEquals("https://raw.githubusercontent.com/common-workflow-language/workflows/933bf2a1a1cce32d88f88f136275535da9df0954/lobstr-draft3/lobSTR-workflow.cwl", | ||
| assertEquals("https://w3id.org/cwl/v/git/null/lobstr-draft3/lobSTR-workflow.cwl?format=raw", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this accidentally be null for other reasons? Rather mock a fake value like deadbeef1337
|
Changes are now finished with the exception of the use of query parameters - currently |
This pull request tracks progress on RDF permalinking
Closes #146