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

feat(robot-server): add a new endpoint to upload csv data files #15481

Merged

Conversation

sanni-t
Copy link
Member

@sanni-t sanni-t commented Jun 20, 2024

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

  • On a robot, upload a data file using the POST /dataFiles endpoint and providing a file in the request. Verify that-
    • the file is uploaded to the persistent storage at location- ../opentrons-robot-server/data_files/<file_id>/
    • the file's info is added to the database
    • the file's ID is returned in the response
  • On a robot, send a file to POST /dataFiles by providing a file_path to a data file on the robot (the file should be outside the persistent directory) and verify all the above
  • Send an already existing file to the server and verify that no new file is saved and no new entry is added to database. Verify that the response contains the previously assigned file ID

Changelog

  • added new router, models and data files store for the new endpoint
  • added unit & integration tests

Review requests

  • check that the implementation meets client's expectations
  • any suggestions on placement of file_reader_writer and file_hasher classes/functions? They have been so far associated with only protocols and hence were part of the protocols/ api packages but I think it's time to move it to a common utility
  • any other code suggestions

Risk assessment

None to the existing system. New, isolated feature.

Copy link
Contributor

A PR has been opened to address analyses snapshot changes. Please review the changes here: https://github.com/Opentrons/opentrons/pull/

@sanni-t sanni-t marked this pull request as ready for review June 21, 2024 19:02
@sanni-t sanni-t requested review from a team as code owners June 21, 2024 19:02
Copy link
Contributor

A PR has been opened to address analyses snapshot changes. Please review the changes here: #15495

Copy link
Contributor

@jbleon95 jbleon95 left a 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"):
Copy link
Contributor

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

Copy link
Member Author

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.

@sanni-t sanni-t merged commit 3d99b54 into edge Jun 26, 2024
36 checks passed
@sanni-t sanni-t deleted the AUTH-426-rs-create-a-new-endpoint-to-upload-csv-data-files branch June 27, 2024 18:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants