-
Notifications
You must be signed in to change notification settings - Fork 4
Add API support for crash victim images #1916
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
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for atd-vze-staging canceled.
|
| """ | ||
| import datetime | ||
| import json | ||
| import os |
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.
mostly just shuffling around imports to be closer to PEP8
| AWS_S3_BUCKET_ENV = getenv("AWS_S3_BUCKET_ENV", "") | ||
| AWS_S3_CR3_LOCATION = f"{AWS_S3_BUCKET_ENV}/cr3s/pdfs" | ||
| AWS_S3_PERSON_IMAGE_LOCATION = f"{AWS_S3_BUCKET_ENV}/images/person" | ||
| AWS_S3_BUCKET = getenv("AWS_S3_BUCKET", "") |
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 reworked a few of these env vars and updated our 1pass entry accordingly. we're going to need to bring the ECS environments in sync with these changes
| { | ||
| "code": "invalid_header", | ||
| "description": f"{e}: Unable to parse authentication token.", | ||
| "description": f"Unable to parse authentication token.", |
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 ran the auth route through an LLM and it wisely suggested to not dump exception messages into the API response 🔒
| if not safe_person_id: | ||
| return jsonify(error="Missing or invalid person_id"), 400 | ||
|
|
||
| if request.method == "GET": |
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.
For the GET endpoint, it seemed fine to continue the CR3 download pattern of returning a pre-signed URL from S3 (as opposed to streaming the image from this API).
In the VZE, we will need to create an image component that will:
- Ask this API for an image of person ID
12345
The API returns a presigned URL to the VZE (if one exists) - The VZE passes the presigned URL into an
<image>element
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.
fwiw when I recently chatted with Claude about CloudFront@Edge serving profile pics in Moped, it recommended pre-signed urls (or streaming from the API as a next alternative) instead of CF@Edge. so, this sounds great to me, and it is nice to both images and CR3s working the same way! 🚀
api/server.py
Outdated
| ) | ||
| return jsonify(url=url) | ||
|
|
||
| elif request.method == "POST": |
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.
The POST endpoint allows the client to upsert an image for a given person ID. So long as the image passes our validation, we store it in S3 and save the object key and original uploaded filename in our DB. The filename of the file saved in S3 will follow the format <person_id>.<png | jpg>.
|
Folks, I just reworked the docker compose setup a bit so that we have a separate compose file for testing. Charlie encountered that the entire local stack setup was previously broken until you created an |
Charlie-Henry
left a comment
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 got all of the tests to work! A couple stumbling blocks for me to get things tested
- you might need to create a blank
api/.env.testfile to run./vision-zero local-stack-upthe first time - In step 6 I had to use
--network atd-vz-data_defaultbecause my local repo is still in the old name
Love the pytest cases, easy to understand!
|
|
||
|
|
||
| def _handle_image_upload(person_id, file, s3, old_image_obj_key=None): | ||
| """Uploads an image to S3 after validating the image and removing EXIF data |
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.
Nice I would've completely forgot about EXIF data
frankhereford
left a comment
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.
You got a lot work done in this PR, John! It worked great, and the local testing experience was 💯. I love the pytest pattern you're establishing, and all your code is clear and concise. There's a lot of great stuff in here.
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.
So much 🧹 🧼 🧹 going on in this file -- thank you!
mddilley
left a comment
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! This was a really fun one to review since I haven't looked at an API in a while. All the tests passed, and I ran through the Python commands as well and saw the expected changes in S3 and my local DB. 🚢 🙌
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.
awesome! i've been keeping the pytest coverage for the Moped API that we shelved in the back of my mind, and this will be a nice reference for a future version.
are you thinking about including this in the release testing if the API code changes? Just thinking about how dusty the Moped tests became - we've come a long way on maintaining this kind of thing and mostly wondering what you have in mind. What a level up! 🙌
api/tests/test_person_images.py
Outdated
| from PIL import Image | ||
| import io | ||
|
|
||
| TEST_PERSON_ID = 102580 # must be a person record ID available in your local DB |
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.
maybe this could be another env var? I've lost context on how likely it would be for this person to be removed from the data though...
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 went back and forth on this too. This person ID should be in the DB for a long time to come, but I like your suggestion. I'll set this as a default person ID that also supports passing an optional override in the env 🙏
| @pytest.fixture | ||
| def test_image_jpg(): | ||
| """Create a 500x500 JPEG test image.""" | ||
| img = Image.new("RGB", (500, 500), color="blue") |
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.
PIL is a TIL 😄 Sure was cool seeing that blue square come up with the test presigned url!
| return img | ||
|
|
||
|
|
||
| def strip_exif(img): |
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.
nice detail! this makes me think we should do the same in Moped. i'll test it out but I don't think that we are doing this for image uploads.
|
|
||
|
|
||
| @APP.route("/cr3/download/<crash_id>") | ||
| @app.route("/cr3/download/<int:crash_id>") |
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.
this int converter is a TIL 🙏
| if not safe_person_id: | ||
| return jsonify(error="Missing or invalid person_id"), 400 | ||
|
|
||
| if request.method == "GET": |
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.
fwiw when I recently chatted with Claude about CloudFront@Edge serving profile pics in Moped, it recommended pre-signed urls (or streaming from the API as a next alternative) instead of CF@Edge. so, this sounds great to me, and it is nice to both images and CR3s working the same way! 🚀
chiaberry
left a comment
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.
nice to get a refresher on the api and test via pytest. I am trying to wrap my head around what needs to be done when this is merged and I am really fuzzy on how this gets deployed.
@chiaberry despite being intentional about updating docs on this topic it is not well documented in the repo or gitbook. we can expect the API to redeploy as the last step in the image build action. I will update this README to make that more clear 🙏 EDIT: And before we merge we'll need to figure out the most graceful way to handle the env var changes. This one might need some manual finessing of the ECS config. |
mddilley
left a comment
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 reviewed the latest commits and ran the test suite which passed without issue - 14 passed in 7.83s. 😎 The API code in the last commit makes sense to me too. 🚢 🚀
mateoclarke
left a comment
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.
14 passed in 14.27s 🚢
chiaberry
left a comment
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.
tests pass, I can still download cr3s too. I didnt test creating a new user, but dont see why that would change with the latest updates
Associated issues
This PR adds a new route to our API to support victim photo uploads. Since we don't have a UI to test this with I went ahead and created
pytesttests.The setup and testing should be pretty straightforward, with the one gotcha that your auth token expires every 5 minutes. This was pretty annoying to dev around! I went just a little bit down the rabbit hole of configuring auth0 so that we could request JWTs programmatically, but Auth0 has some big scary warnings about how risky this is. So I opted to stick with copy-pasta JWTs. One maybe reasonable thing we could do would be to extend the TTL of our JWTs in staging—but considering that our staging data tends to be quite real and sensitive this doesn't seem like a great idea to me.
Testing
URL to test: Local
In the
./apidirectory, create your.envfile based on the template file. All secrets are available in theDEVELOPMENTsection of the 1pass item called Vision Zero (VZ) User & CR3 API Secrets. If you already had an.envfile, note that some values have been added and removed (see diff).From the root of the repo, start your local hasura setup including the CR3 api:
In your
./editordirectory, check your.env.localfile to make sure thatNEXT_PUBLIC_CR3_API_DOMAINis set tohttp://localhost:8085. Then start your VZE:npm run devFrom the
./databasedirectory, apply migrations and reload metadataTo run the API tests, follow the instructions in the
./api/tests/README.Optionally, if you'd like to explore how the API works interactively you can spin up a container to manually execute some python commands.
# starts a container with everything you need to interact with the API docker run -it --rm --network vision-zero_default cr3-user-api /bin/bashTest that the user management API works normally. Login to your local VZE as an admin and create a user with the email address
<yourfirstname>.<yourlastname>[email protected]. Refresh the page and delete this user. Confirm the user was deleted successfully.Test that the VZE CR3 download works normally—visit crash ID
13625639and download the CR3.Ship list
mainbranch