-
Notifications
You must be signed in to change notification settings - Fork 531
Feature to limit file uploads to datasets #11520
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
Feature to limit file uploads to datasets #11520
Conversation
src/main/java/edu/harvard/iq/dataverse/api/dto/DataverseDTO.java
Outdated
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Just a quick status update: |
This comment has been minimized.
This comment has been minimized.
|
Very happy to see this implemented. |
| @Column(insertable = false, updatable = false) private String dtype; | ||
|
|
||
| @Column( nullable = true ) | ||
| private Integer datasetFileCountLimit; |
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.
It is super important to have this config setting implemented for both collections AND datasets. In real life, I'm guessing it's going to be a somewhat common case where a specific dataset will need to be given a higher limit, because of some respectable reason. It appears to be working consistently when defined on either level.
However - and this may be penny-pinching, admittedly - I'm wondering if we want this column to be in the DvObject table; seeing how most DvObjects are files. Please at least consider making it a DvObjectContainer-only element. (see dvObjectContainer.storageDriver for an example; it ends up being an extra column in the Dataverse and Dataset tables each).
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.
I'm confused about the penny-pinching comment. Columns in the database that are null take up no space and therefore add no pennies to all the DvObject Datafiles.
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.
It does have to get loaded into the objects though. Further - it's just odd that a file has a datasetFileCountLimit, which wouldn't be the case if it's on DvObjectContainer.
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.
Ok. I'll move it
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.
Yeah, phrasing it in terms of objects makes more sense. I just wanted to emphasize that it was possible, even though there is no dedicated table in the db for DvObjectContainer.
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.
I moved it
|
Taking a look at this. Thanks for finding this |
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
|
📦 Pushed preview images as 🚢 See on GHCR. Use by referencing with full name as printed above, mind the registry name. |
|
great fix, merging! |
|
Does this make sense: dataverse/src/main/java/edu/harvard/iq/dataverse/api/Datasets.java Lines 1995 to 1996 in 1c31486
|
What this PR does / why we need it: Need a way to prevent thousands of file from being uploaded due to the issues that causes.
Which issue(s) this PR closes: #11275
Special notes for your reviewer:
Suggestions on how to test this: See IT tests
Does this PR introduce a user interface change? If mockups are available, please link/include them here:
Is there a release notes update needed for this change?: Included
Additional documentation: native-api and config