Add CKAN file source with import and export support#23116
Conversation
|
nice, I will test it locally, can you fix the linting ? |
|
Can someone allow other tests to run? thanks! |
| # lists all datasets that are available to the user | ||
| # users private datasets are listed first, then the public ones | ||
| def _list_all_datasets(self) -> list[str]: | ||
| return self._list_private_datasets() + self._list_public_datasets() |
There was a problem hiding this comment.
Showing the all public + private datasets for pulling files from a CKAN server is fine but pushing files to public datasets on CKAN should not happen, but only to private / user created datasets.
for example, I can pull all these datasets from Harvard's Dataverse repo:
but for pushing, it allows only these datasets that I have access to:
|
@davelopez, @PlushZ would you also like to have a look at this PR? thanks a lot! |
davelopez
left a comment
There was a problem hiding this comment.
Thanks a lot for the contribution @AdrianJaeger!
I'm not familiar with CKAN, but I have a quick look at the PR, and in particular 2 of the limitations I see from the PR description and Anup's comment:
Export only uploads resources into an existing CKAN dataset. Creating a new dataset from Galaxy is not supported.- Displaying only private folders with access when exporting (you need to use the
write_intentflag for that)
These and the fact that the fsspec implementation is living directly in the Galaxy codebase as opposed to an external library that can be reused, make me think that the right abstraction for this file source should be the RDMFileSource instead of fsspec for this particular case.
I've also run a quick review through the GLM 5.2 agent and come up with the same conclusion plus some other improvements.
Show review details
Verdict: Request changes. There is a major architectural mismatch and several structural issues that should be addressed before merge.
The single most important finding: CKAN is a research-data-management repository, and Galaxy already has a canonical abstraction for exactly this category — RDMFilesSource / RDMRepositoryInteractor (used by Dataverse, Invenio, Zenodo). This PR instead bolts CKAN onto the FsspecFilesSource base with a hand-rolled AbstractFileSystem. That is the wrong layer, and it causes real, observable problems — not just stylistic ones.
1. Structural: CKAN is an RDM repository and should use the RDMFilesSource pattern
CKAN has the exact shape that RDMFilesSource was created to model: a one-level list of containers (datasets) that hold files (resources), with create-container / upload-to-container / download-from-container semantics. The existing RDM family — dataverse.py, invenio.py, zenodo.py — all share this shape and all subclass RDMFilesSource + RDMRepositoryInteractor.
This PR instead:
- Implements a custom
CKANFileSystem(AbstractFileSystem)withls/info/_open, - Subclasses
FsspecFilesSource, - Reimplements listing, path splitting, entry building, and upload from scratch.
The cost of choosing the wrong base class is not theoretical. It shows up concretely:
1a. The write_intent problem (the open review comment is a symptom of this)
The existing review thread from anuprulez flags that users can push to public datasets they don't own. That bug exists because the PR is on the wrong base class.
RDMFilesSource.get_file_containers(..., write_intent=...) is the canonical hook for "when the user is browsing in order to export, only show containers they can write to." Dataverse implements this correctly:
# dataverse.py
if write_intent:
params["fq"] = "publicationStatus:Draft"FsspecFilesSource._list receives write_intent but silently drops it — it never reaches the filesystem. So by choosing FsspecFilesSource, CKAN has no way to filter the dataset list for the export flow, and the user is presented with every public dataset on the instance as an upload target. That is the root cause of the review comment.
This is exactly the "feature logic leaking into the wrong layer / missing canonical helper" pattern the skill calls out. The fix is not to patch write_intent into FsspecFilesSource; the fix is to use the base class that already models this.
1b. Reimplemented RDM concepts under new names
| RDM canonical name | CKAN reimplementation |
|---|---|
ContainerAndFileIdentifier |
_split_path returning (dataset_id, resource_name) |
get_file_containers |
_list_all_datasets + _dataset_entry |
get_files_in_container |
_list_resources + _resource_entry |
upload_file_to_draft_container |
_post_resource |
download_file_from_container |
_open |
parse_path / get_container_id_from_path |
_split_path |
Every one of these is a near-duplicate of an existing RDM concept. The skill is explicit: "Bespoke helpers where the codebase already has a canonical utility for the job" and "Logic added in the wrong layer/package when it should live somewhere more central" are presumptive blockers.
1c. Lost RDM capabilities
RDMFilesSource also provides _create_entry (create a new dataset/container from Galaxy). The PR description lists "Creating a new dataset from Galaxy is not supported" as a known limitation, and the template description even says "Your registered email address will be used to create datasets in CKAN" — which is misleading because the PR cannot create datasets. Adopting the RDM base would make adding that capability a natural extension instead of a future rewrite.
Recommendation: Reframe ckan.py as CKANRDMFilesSource(RDMFilesSource) + CKANRepositoryInteractor(RDMRepositoryInteractor), mirroring dataverse.py's structure. This would:
- Delete the entire
CKANFileSystemclass (~180 lines), - Delete the hand-rolled
ls/info/_open/entry-building code, - Inherit
write_intentfiltering for free (fixing the open review comment), - Inherit
_create_entryplumbing for future dataset creation, - Reuse
parse_path/ContainerAndFileIdentifierinstead of_split_path.
This is the "code judo" move: the change becomes dramatically smaller and the public-dataset upload bug disappears as a consequence of the structure rather than a special-case filter.
2. Duplicated error-handling logic (structural, not cosmetic)
_get_response and _post_resource contain byte-for-byte identical error handling:
self._raise_for_ckan_error(response)
payload = response.json()
if not payload.get("success", False):
error = payload.get("error", {})
raise Exception(f"CKAN API call failed: {error.get('message') or error}")This is copy-pasted logic instead of an extracted helper. If this stays (i.e., if the RDM refactor is declined), at minimum extract a _check_ckan_response(response) -> Any that both GET and POST paths call. Better still, the RDM base already has a _get_response pattern to build on.
Also: raise Exception(...) is too generic. Galaxy has MessageException, ObjectNotFound, AuthenticationRequired etc. (see how omero.py and _fsspec.py use them). Bare Exception bypasses Galaxy's error handling.
3. _write_from bypasses the canonical path helpers
def _write_from(self, target_path, native_path, context) -> None:
fs = self._open_fs(context, {})
dataset_id, resource_name = fs._split_path(target_path)
...
fs._post_resource(dataset_id, resource_name, native_path)Two issues:
- It does not call
self._to_filesystem_path(target_path, context.config), which the base_write_fromdoes. If any path normalization ever lives in_to_filesystem_path, this silently skips it. - It reaches into
fs._split_path(a private method) from the file-source layer. The filesystem's private API is leaking across the layer boundary. In the RDM formulation this becomesself.parse_path(target_path), which is the public, intended API.
4. _open returns response.raw with no lifetime management
response = requests.get(url, stream=True, headers=headers, timeout=DEFAULT_SOCKET_TIMEOUT)
self._raise_for_ckan_error(response)
response.raw.decode_content = True
return response.rawThe requests.Response is not kept alive; only response.raw is returned. If the underlying connection is garbage-collected or the response object is finalized, the stream can close prematurely. Other Galaxy code that streams generally keeps the response object referenced. This is the kind of "works in testing, brittle in production" pattern the skill flags. At minimum, return a small wrapper that holds a reference to response, or use response.iter_content into a buffered stream.
Additionally, _raise_for_ckan_error is named for CKAN but is being called on a response from an arbitrary resource URL (CKAN resources can be hosted externally). A 404 from an external S3 bucket will produce a "Not found (HTTP 404)" message that's fine, but the method name implies CKAN-specific semantics it doesn't have for this call path.
5. _is_same_host token-leak guard is fragile
def _is_same_host(self, url: str) -> bool:
return urlparse(url).hostname == urlparse(self.base_url).hostname- Hostname-only comparison ignores scheme and port. A resource at
http://attacker.example.comon a different port still matcheshttps://ckan.example.com:5000. urlparse(self.base_url)is recomputed on every call; cache it in__init__.- If
urlis malformed,urlparse(url).hostnameisNone, which won't match — safe by accident, but worth being explicit.
This is a security-adjacent check (token leakage); it should be precise. Compare normalized host (and consider scheme) and precompute the base host once.
6. Misleading template description
production_ckan.yml:
Note: Your registered email address will be used to create datasets in CKAN.
The PR explicitly does not support creating datasets. This text is inherited from the RDM templates (Dataverse/Invenio use public_name/email for dataset creation) but is false for CKAN here. It will mislead users. Remove or correct it.
Summary
The PR is functional, but it implements an RDM repository as an fsspec filesystem, and that choice has structural consequences — not just style:
- Wrong base class — CKAN should use
RDMFilesSource/RDMRepositoryInteractor, like Dataverse/Invenio/Zenodo. This is the code-judo move: it deletes ~180 lines, reuses canonical helpers, and fixes the open review comment about uploading to public datasets as a structural consequence. - Duplicated error handling between
_get_responseand_post_resource. _write_frombypasses_to_filesystem_pathand reaches into private filesystem methods._openstream lifetime is not managed;response.rawcan be prematurely closed._is_same_hostis a loose, recomputed security check guarding token leakage.- Misleading template description claims dataset creation support that doesn't exist.
| if resource_name is None: | ||
| raise FileNotFoundError(path) | ||
| resource = self._find_resource(dataset_id, resource_name) | ||
| url = resource.get("url") or resource.get("download_url") or None |
There was a problem hiding this comment.
Since this url comes from CKAN resource metadata, should it be validated with Galaxy’s validate_non_local() before Galaxy makes the request? this validation is used for example in the cBioPortal file source.
b8a8c4b to
7aab920
Compare
| token: str | None = None | ||
|
|
||
|
|
||
| class CKANFilesSource(FsspecFilesSource[CKANFileSourceTemplateConfiguration, CKANFileSourceConfiguration]): |
There was a problem hiding this comment.
Really appreciated that you are working on this 🙏
I still think that FsspecFilesSource is not the best abstraction for CKAN. Sorry if I may have suggested that at some point, but I would have expected something else.
The idea of using fsspec pays for itself when you have an external library that can be reused somewhere else (and doesn't need to be maintained by Galaxy). If the implementation lives here, then the result is pure overhead: a CKANFileSystem class that exists only to satisfy the fsspec interface, an unnecessary dict-to-RemoteEntry translation layer, meaningless cache-options fields in the config, and a put_file path that's never used because _write_from is overridden entirely 😅
Since the API is just using requests, I suggest looking at other file source implementations that do exactly that. If trying to fit this into an RDMFilesSource is also too much, you can just inherit from BaseFilesSource and then you can simplify a lot of the code and make it even more efficient by using proper server-side pagination, etc.
Thanks for keeping up with the good work!
This PR adds a new CKAN file source, based on Galaxy's existing
fsspecfile source plugin pattern, connecting Galaxy to a CKAN instance.Key capabilities:
Known limitations:
How to test the changes?
(Select all options that apply)
http://rather thanhttps://) or an existing server — and generate an API token for a user with at least one private dataset.lib/galaxy/files/templates/examples/production_ckan.ymlto your file source templates, and create a CKAN file source instance with the endpoint and token.License