Make manual reserveManualUpload route return datasetId and directoryName
#8476
Conversation
📝 WalkthroughWalkthroughThis pull request introduces a new parameter, Changes
Possibly related PRs
Suggested labels
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
…ow-passing-ds_id-on-manual-upload
…ectory name f new dataset
MichaelBuessemeyer
changed the title
reserveUpload route return datasetId and directoryName
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
CHANGELOG.unreleased.md (1)
20-20: Great changelog entry!The entry clearly communicates that you've added a parameter to the reserve manual upload route and enhanced the response with additional information.
Consider changing "returned in the response" to "returned to the response" which is a more common collocation in English.
🧰 Tools
🪛 LanguageTool
[grammar] ~20-~20: The usual collocation for “returned” is “to”, not “in”.
Context: ...new dataset's id and directory name are returned in the response. [#8476](https://github.co...(RETURN_IN_THE)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
CHANGELOG.unreleased.md(1 hunks)app/controllers/WKRemoteDataStoreController.scala(1 hunks)app/models/dataset/DatasetService.scala(1 hunks)conf/messages(1 hunks)webknossos-datastore/app/com/scalableminds/webknossos/datastore/controllers/DataSourceController.scala(3 hunks)webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/uploading/ComposeService.scala(1 hunks)webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/uploading/UploadService.scala(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
app/controllers/WKRemoteDataStoreController.scala (1)
app/models/dataset/DatasetService.scala (1)
createPreliminaryDataset(68-82)
🪛 LanguageTool
CHANGELOG.unreleased.md
[grammar] ~20-~20: The usual collocation for “returned” is “to”, not “in”.
Context: ...new dataset's id and directory name are returned in the response. [#8476](https://github.co...
(RETURN_IN_THE)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
🔇 Additional comments (10)
conf/messages (1)
116-116: Good addition of error messageThis new error message provides appropriate feedback when dataset creation fails due to name conflicts.
webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/uploading/ComposeService.scala (1)
66-67: Parameter addition is consistent with other changesSetting
requireUniqueName = falseas the default for the compose dataset operation maintains backward compatibility while supporting the new functionality.app/models/dataset/DatasetService.scala (2)
68-71: Method signature update correctly implements new parameterThe addition of the
requireUniqueNameparameter to the method signature properly supports the new functionality requested in the PR.
74-77: Good implementation of name uniqueness logicThe implementation correctly:
- Checks if a dataset with the same name already exists
- Fails with an appropriate error message when
requireUniqueNameis true and the name is taken- Generates a unique directory name when needed by appending the dataset ID
app/controllers/WKRemoteDataStoreController.scala (1)
86-90: ModifiedcreatePreliminaryDatasetcall to enforce unique dataset namesThe method now passes the
uploadInfo.requireUniqueNameparameter to the dataset service, allowing the API to fail predictably when attempting to create a dataset with a name that already exists in the organization. The error message has also been updated to be more general.This change aligns with the PR objective to make the route fail when an organization already has a dataset with the same name.
webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/uploading/UploadService.scala (2)
42-42: AddedrequireUniqueNameparameter toReserveUploadInformationThis parameter allows clients to specify whether dataset creation should fail if a dataset with the same name already exists in the organization.
47-52: AddedrequireUniqueNameparameter toReserveManualUploadInformationThe
requireUniqueNameparameter now has a default value offalse, which means manual uploads will, by default, allow duplicate dataset names but can be configured to require unique names when needed.webknossos-datastore/app/com/scalableminds/webknossos/datastore/controllers/DataSourceController.scala (3)
116-129: EnhancedreserveManualUploadmethod to return dataset ID and directory nameThe method now captures the
reservedDatasetInfoinstead of discarding it, and passesrequest.body.requireUniqueNameto enforce unique dataset names when requested. This change aligns with the PR objective to return both the dataset ID and directory name upon successful execution.
131-133: Updated response to include dataset ID and directory nameThe response now includes both the new dataset ID and directory name in a JSON object, providing clients with the necessary information to reference the created dataset.
426-426: Set default value forrequireUniqueNameinaddmethodThe
addmethod now explicitly setsrequireUniqueNametofalse, ensuring consistent behavior with the default value inReserveManualUploadInformation.
100'% correct imo. This feature to make the publication task fail in case of a not unique name was requested by @philippotto. Could you please elaborate your reasoning for this @philippotto 🤔 🙏 ? |
MichaelBuessemeyer
changed the title
reserveUpload route return datasetId and directoryNamereserveManualUpload route return datasetId and directoryName
…ow-passing-ds_id-on-manual-upload
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
CHANGELOG.unreleased.md (1)
20-20: Revise Changelog Wording for Clarity.
The changelog entry currently states "the new dataset's id and directory name are returned in the response." According to common English collocations (as noted by the static analysis hint), consider rephrasing this to "returned to the client" or "returned by the API" for improved clarity and consistency.🧰 Tools
🪛 LanguageTool
[grammar] ~20-~20: The usual collocation for “returned” is “to”, not “in”.
Context: ...new dataset's id and directory name are returned in the response. [#8476](https://github.co...(RETURN_IN_THE)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
CHANGELOG.unreleased.md(1 hunks)app/controllers/WKRemoteDataStoreController.scala(1 hunks)webknossos-datastore/app/com/scalableminds/webknossos/datastore/controllers/DataSourceController.scala(3 hunks)webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/uploading/ComposeService.scala(1 hunks)webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/uploading/UploadService.scala(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/uploading/UploadService.scala
- webknossos-datastore/app/com/scalableminds/webknossos/datastore/controllers/DataSourceController.scala
- webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/uploading/ComposeService.scala
- app/controllers/WKRemoteDataStoreController.scala
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.unreleased.md
[grammar] ~20-~20: The usual collocation for “returned” is “to”, not “in”.
Context: ...new dataset's id and directory name are returned in the response. [#8476](https://github.co...
(RETURN_IN_THE)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
When we use voxelytics internally for projects, it is good practice to give a new dataset that is about to be published to WK a unique name (so that it can easily be distinguished from other experiments). This is why I voted for being able to have this asserted, even though WK supports non-unique names. |
google drive supports non-unique file names in the same folder, too. maybe I'm in the minority with my opinion 🤷 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
CHANGELOG.unreleased.md (1)
20-22: Clarify changelog language and add parameter specificity.
The changelog entry currently reads:
- Added a parameter to the reserve manual upload route allowing to make the request fail if the name is already taken. Moreover, the new dataset's id and directory name are returned in the response. #8476
To improve clarity and precision:
- Parameter Naming: Consider explicitly stating the name of the new parameter (e.g.,
allowDuplicateDatasetNameor the actual parameter name used) so that users can immediately identify it in their configurations.- Grammar Refinement: According to a static analysis hint, the usual collocation for “returned” is “to”, not “in”. You might rephrase the sentence to something like: “... and the new dataset’s id and directory name are returned to the client” or “... are included in the response.”
🧰 Tools
🪛 LanguageTool
[grammar] ~21-~21: The usual collocation for “returned” is “to”, not “in”.
Context: ...new dataset's id and directory name are returned in the response. [#8476](https://github.co...(RETURN_IN_THE)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CHANGELOG.unreleased.md(1 hunks)webknossos-datastore/app/com/scalableminds/webknossos/datastore/controllers/DataSourceController.scala(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- webknossos-datastore/app/com/scalableminds/webknossos/datastore/controllers/DataSourceController.scala
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.unreleased.md
[grammar] ~21-~21: The usual collocation for “returned” is “to”, not “in”.
Context: ...new dataset's id and directory name are returned in the response. [#8476](https://github.co...
(RETURN_IN_THE)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
Yeah, I think that's the idea :D |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
CHANGELOG.unreleased.md (1)
22-22: Changelog wording clarity improvement: Adjust phrase "returned in the response".The changelog entry accurately documents the update; however, consider rephrasing the clause for clarity. Instead of "returned in the response," which may sound non-standard grammatically, "included in the response" provides a clearer description.
- ... and the new dataset's id and directory name are returned in the response. + ... and the new dataset's id and directory name are included in the response.🧰 Tools
🪛 LanguageTool
[grammar] ~22-~22: The usual collocation for “returned” is “to”, not “in”.
Context: ...new dataset's id and directory name are returned in the response. [#8476](https://github.co...(RETURN_IN_THE)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
CHANGELOG.unreleased.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.unreleased.md
[grammar] ~22-~22: The usual collocation for “returned” is “to”, not “in”.
Context: ...new dataset's id and directory name are returned in the response. [#8476](https://github.co...
(RETURN_IN_THE)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
CHANGELOG.unreleased.md (1)
23-23: Enhance Clarity in the Changelog EntryThe changelog entry clearly documents the new parameter and updated response information. To improve readability and adhere to conventional phrasing, consider rephrasing "returned in the response" to "returned as part of the response".
- Added a parameter to the reserve manual upload route allowing to make the request fail if the name is already taken. Moreover, the new dataset's id and directory name are returned in the response. [#8476](https://github.com/scalableminds/webknossos/pull/8476) + Added a parameter to the reserve manual upload route that causes the request to fail if the name is already taken. Additionally, the new dataset's id and directory name are returned as part of the response. [#8476](https://github.com/scalableminds/webknossos/pull/8476)🧰 Tools
🪛 LanguageTool
[grammar] ~23-~23: The usual collocation for “returned” is “to”, not “in”.
Context: ...new dataset's id and directory name are returned in the response. [#8476](https://github.co...(RETURN_IN_THE)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
CHANGELOG.unreleased.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.unreleased.md
[grammar] ~23-~23: The usual collocation for “returned” is “to”, not “in”.
Context: ...new dataset's id and directory name are returned in the response. [#8476](https://github.co...
(RETURN_IN_THE)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
CHANGELOG.unreleased.md (1)
23-23: Nitpick: Rephrase for improved clarity and natural collocation.
The changelog entry currently states, "Moreover, the new dataset's id and directory name are returned in the response." Consider rewording this line to something like:
"Moreover, the response now includes the new dataset's ID and directory name."
This change not only improves clarity but also aligns better with common usage as noted by LanguageTool.🧰 Tools
🪛 LanguageTool
[grammar] ~23-~23: The usual collocation for “returned” is “to”, not “in”.
Context: ...new dataset's id and directory name are returned in the response. [#8476](https://github.co...(RETURN_IN_THE)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
CHANGELOG.unreleased.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.unreleased.md
[grammar] ~23-~23: The usual collocation for “returned” is “to”, not “in”.
Context: ...new dataset's id and directory name are returned in the response. [#8476](https://github.co...
(RETURN_IN_THE)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
This is a suggestion to help out with vx issue: https://github.com/scalableminds/voxelytics/issues/3894
URL of deployed dev instance (used for testing):
Steps to test:
TODOs:
/reserveManualUploadallowDuplicateDatasetNameto make the request fail in case the organization already has a dataset with that name (and the resultingdirectoryNamewould be something likedatasetName-datasetId)Issues:
(Please delete unneeded items, merge only when none are left open)