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: fix explorer API for handling subpaths in S3 #792

Merged
merged 1 commit into from
Feb 26, 2024

Conversation

atarashansky
Copy link
Contributor

@atarashansky atarashansky commented Feb 26, 2024

Fixed some bugs that were only visible when testing on a deployed environment. This has been tested with (way too many) pirated deployments to dev and all is working.

.happy/config.json Outdated Show resolved Hide resolved
client/src/globals.ts Outdated Show resolved Hide resolved
@@ -204,9 +204,11 @@ def __init__(self, app_config: AppConfig):
methods=["GET"],
)
self.app.add_url_rule(
f"/{url_dataroot}/<path:dataset>.cxg/",
f"/{url_dataroot}/{CELLGUIDE_CXG_KEY_NAME}/<path:dataset>.cxg/",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This explicitly ensures that we only capture URLs that for this rule that are prefixed by "cellguide-cxgs/". path:dataset was interacting poorly with string:dataset because one subsumes the other.

@atarashansky atarashansky requested a review from ebezzi February 26, 2024 19:00
Copy link

codecov bot commented Feb 26, 2024

Codecov Report

Attention: Patch coverage is 85.71429% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 77.74%. Comparing base (5027bf2) to head (1e85c9b).

Files Patch % Lines
server/common/rest.py 66.66% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #792   +/-   ##
=======================================
  Coverage   77.73%   77.74%           
=======================================
  Files          88       88           
  Lines        6792     6802   +10     
=======================================
+ Hits         5280     5288    +8     
- Misses       1512     1514    +2     
Flag Coverage Δ
frontend 77.74% <85.71%> (+<0.01%) ⬆️
javascript 77.74% <85.71%> (+<0.01%) ⬆️
smokeTest ∅ <ø> (∅)
unitTest 77.74% <85.71%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Linted and added task_launch_type: ec2 which is now required for latest happy versions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This regex was updated to assume a one-character dataroot, which is the point at which it starts scanning for subpaths leading up to /api. This is required because otherwise it's impossible to exclude the /cellxgene/ api prefex from a regex that can dynamically get an arbitrary number of subpaths.

Copy link
Contributor

@prathapsridharan prathapsridharan left a comment

Choose a reason for hiding this comment

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

lgtm

@atarashansky atarashansky merged commit 4ba67ac into main Feb 26, 2024
7 of 8 checks passed
@atarashansky atarashansky deleted the atar-test-rdev branch February 26, 2024 19:21
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