-
Notifications
You must be signed in to change notification settings - Fork 179
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
feat(robot-server): add a new endpoint to upload csv data files #15481
feat(robot-server): add a new endpoint to upload csv data files #15481
Conversation
A PR has been opened to address analyses snapshot changes. Please review the changes here: https://github.com/Opentrons/opentrons/pull/ |
A PR has been opened to address analyses snapshot changes. Please review the changes here: #15495 |
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.
Looks great! Only one concern about how we'll be limiting the POST /dataFiles endpoint but this is good to go, those can always be addressed in a future conversation and more discussion with the team and other developers.
except FileNotFoundError as e: | ||
raise FileNotFound(detail=str(e)).as_error(status.HTTP_404_NOT_FOUND) from e | ||
# TODO (spp, 2024-06-18): probably also validate CSV file *contents* | ||
if not buffered_file.name.endswith(".csv"): |
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 agree that we should be doing some validation on files before storing them on the robot and adding them to the dataFiles database, but only allowing CSVs to this endpoint seems limiting for now. We can of course change this to make it more permissive later, but maybe instead of limiting the types of files, we can either do something like:
- blacklist certain file types
- limit the size of files
- whitelist file paths on the robot to prevent any security risks
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'll prefer to have a whitelist of select allowed file types rather than having a blacklist of disallowed ones so that we can be absolutely sure of the file types that get added, but ya I like these suggestions. Let's talk to the team and finalize allowed types, sizes and file paths.
Closes AUTH-426
Overview
Adds a new router for uploading and managing data files. The new endpoint can accept a single data file, either as a file upload or a path to the file on the robot.
The endpoint saves the uploaded file to the persistent directory on the robot, adds the file's info to the data_files database, and returns the file ID and other deets in the response.
Duplicate file uploads use the previously saved file & database entry instead of creating a new one.
Test Plan
POST /dataFiles
endpoint and providing a file in the request. Verify that-../opentrons-robot-server/data_files/<file_id>/
POST /dataFiles
by providing afile_path
to a data file on the robot (the file should be outside the persistent directory) and verify all the aboveChangelog
Review requests
Risk assessment
None to the existing system. New, isolated feature.