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

Route to import explored datasources automatically #7176

Merged
merged 22 commits into from
Jul 31, 2023
Merged

Conversation

fm3
Copy link
Member

@fm3 fm3 commented Jun 26, 2023

New route to explore and immediately import remote datasets. POST /api/v5/datasets/exploreAndAddRemote with expected body {"remoteUri":"…", "datasetName": "…", "folderPath": "…"} (folderPath is optional, defaults to root)

TODO

  • Call add route from wk to datastore after successful explore
  • pass user token
  • assert valid new name
  • prettier error messages in failure case
  • design prettier API
  • specify target folder by path
    • create path if missing
    • get path object id, pass it to add route in datastore
    • there, forward it to reserveupload
  • forbid forward slashes in folder names in sql constraint

Steps to test:

Issues:


@fm3 fm3 self-assigned this Jun 26, 2023
@fm3
Copy link
Member Author

fm3 commented Jun 26, 2023

@philippotto @normanrz do you have opinions on the API?

Right now, the explore route takes a list of ExploreRemoteDatasetParameters (so that multiple layers can be explored into one datasource).

To prototype this, I just added shouldAutoAdd: Boolean and autoAddDatasetName: String to each element in this list, but use only the first.
What do you think would be a useful format for the app-to-app use case described in #7146?
I would suggest we wrap the list in an object, something like

{
  "layersToExplore": [
     {
       "remoteUri": "…",
       "credentialIdentifier": "…",
       "credentialSecret": "…",
     }
  ],
  "shouldAutoAdd": true
  "autoAddDatasetName": "myDataset",
}

@normanrz
Copy link
Member

@philippotto @normanrz do you have opinions on the API?

I would like something simple like a GET route /api/datasets/importRemote?uri=s3://bucket/dataset.zarr.

@philippotto
Copy link
Member

@philippotto @normanrz do you have opinions on the API?

I would like something simple like a GET route /api/datasets/importRemote?uri=s3://bucket/dataset.zarr.

Shouldn't GET be avoided when the route has side-effects?

@normanrz
Copy link
Member

@philippotto @normanrz do you have opinions on the API?

I would like something simple like a GET route /api/datasets/importRemote?uri=s3://bucket/dataset.zarr.

Shouldn't GET be avoided when the route has side-effects?

Fair enough. Then, maybe a POST with

{ "remoteUri": "..." }

@fm3
Copy link
Member Author

fm3 commented Jul 6, 2023

I made it to be POST /api/datasets/exploreAndAddRemote with expected body {"remoteUri":"…", "datasetName": "…"}

The passed datasetName must be available in the relevant webknossos organization.

Does this work for you? Or should we automatically generate dataset names to further simplify the api?

@fm3 fm3 marked this pull request as ready for review July 6, 2023 17:08
@fm3 fm3 changed the title WIP: Option to import explored datasources automatically Option to import explored datasources automatically Jul 6, 2023
@normanrz
Copy link
Member

Can you add an optional field for folder path? Ideally, the folders would be created if not existent.

@fm3
Copy link
Member Author

fm3 commented Jul 19, 2023

So far folders have only ever been referenced by id. If we now start using path literals with folder names, I’d vote to forbid forward slashes in folder names. There are existing folders with forward slashes in their names, so we would have to introduce a migration for that as well. Do you agree?

@fm3 fm3 changed the title Option to import explored datasources automatically Route to import explored datasources automatically Jul 19, 2023
@normanrz
Copy link
Member

So far folders have only ever been referenced by id. If we now start using path literals with folder names, I’d vote to forbid forward slashes in folder names. There are existing folders with forward slashes in their names, so we would have to introduce a migration for that as well. Do you agree?

Yes, folder names should not have slashes in them. I think we should be as restrictive as we are with dataset and layer names.

@fm3 fm3 requested a review from normanrz July 31, 2023 12:00
@fm3 fm3 enabled auto-merge (squash) July 31, 2023 12:13
@fm3 fm3 merged commit cf3a2a1 into master Jul 31, 2023
@fm3 fm3 deleted the explore-auto-add branch July 31, 2023 12:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Set uploader when adding a remote dataset Route for adding OME-Zarr datasets directly
3 participants