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

enhanced simularium url and firebase recipe path validation #230

Merged
merged 10 commits into from
Mar 21, 2024

Conversation

rugeli
Copy link
Collaborator

@rugeli rugeli commented Feb 13, 2024

Problem

What is the problem this work solves, including

  1. query parameters such as AWSAccessKeyId were not being stripped from the url when opening in a new tab
  2. closes validate firebase recipe packing paths with explicit error messaging #212

Solution

What I/we did to solve this problem

  • removed query params from the url not only in firebase but also when opening a new tab
  • a new field has been added to recipes/ in firebase, making it easier to copy the firebase recipe path for packing
Screenshot 2024-02-12 at 4 31 27 PM
  • enhanced validation for the input path to check each part of the path and provided with corresponding error messages

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)

Steps to Verify:

We have tested several cases:

  • fetching a recipe from an incorrect database (e.g. uploading recipe A to dev but fetching it from staging)
  • misnaming any part of the path including database name, collection name or recipe id

some of the expected behaviors:

  • run pack -r wrong_db_name:recipes/one_sphere_v_1.0.0, get error:
Screenshot 2024-02-12 at 4 20 12 PM
  • run pack -r firebase:wrong_collection_name/one_sphere_v_1.0.0, get error:
Screenshot 2024-02-12 at 4 21 42 PM
  • run pack -r firebase:recipes/wrong_id or choosing the wrong database, get error:
Screenshot 2024-02-12 at 4 22 31 PM

@rugeli rugeli requested review from mogres and meganrm February 13, 2024 00:32
Copy link

github-actions bot commented Feb 13, 2024

Packing analysis report

Analysis for packing results located at cellpack/tests/outputs/test_spheres/spheresSST

Ingredient name Encapsulating radius Average number packed
ext_A 25 236.0

Packing image

Packing image

Distance analysis

Expected minimum distance: 50.00
Actual minimum distance: 50.01

Ingredient key Pairwise distance distribution
ext_A Distance distribution ext_A

@rugeli rugeli changed the title Feature/input path validation enhanced simularium url and firebase recipe path validation Feb 13, 2024
Copy link
Collaborator

@mogres mogres left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

"""
Uploads a file to S3 and returns the presigned url
"""
file_name = self.upload_file(file_path)
if file_name:
return file_name, self.create_presigned_url(file_name)
base_url = self.create_presigned_url(file_name).split("?")[0]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might push this into the the create_presigned_url function so the function returns a url that the next function knows it can use without any adjustments. I also think this is a good bug for some tests. Ie make a list of urls that you know would be a problem and make sure we catch all of them.

@rugeli rugeli marked this pull request as draft March 8, 2024 00:43
@codecov-commenter
Copy link

codecov-commenter commented Mar 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.75%. Comparing base (72ecf15) to head (d4ce342).
Report is 11 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #230      +/-   ##
==========================================
+ Coverage   98.63%   98.75%   +0.12%     
==========================================
  Files          18       19       +1     
  Lines         511      563      +52     
==========================================
+ Hits          504      556      +52     
  Misses          7        7              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@rugeli
Copy link
Collaborator Author

rugeli commented Mar 8, 2024

@meganrm @mogres
Recent changes since the last review:

  • added function is_url_valid within AWSHandler class
  • added a new test file: cellpack/tests/test_aws_handler.py

@rugeli rugeli marked this pull request as ready for review March 8, 2024 21:13
@rugeli rugeli requested a review from mogres March 8, 2024 21:13
Copy link
Collaborator

@mogres mogres left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding tests!

@rugeli rugeli merged commit c78b89b into main Mar 21, 2024
7 checks passed
@rugeli rugeli deleted the feature/input-path-validation branch March 21, 2024 17:50
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.

validate firebase recipe packing paths with explicit error messaging
4 participants