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.

Sign up for GitHub

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

Jump to bottom

fix uncaught user input processing in date util #1373

Open
melange396 opened this issue Jan 19, 2024 · 2 comments · May be fixed by #1402
Open

fix uncaught user input processing in date util #1373

melange396 opened this issue Jan 19, 2024 · 2 comments · May be fixed by #1402
Assignees
@dmytrotsko
Labels
api change affect the API and its responses bug python Pull requests that update Python code

Comments

@melange396
Copy link
Collaborator

An improperly formatted date causes an uninformative 500 error, and a useful message should instead be bubbled up to the user with a better error code.

This was brought to our attention by sentry, see: https://cmu-delphi.sentry.io/issues/4893721093/

One way to fix this: the offending date() constructor call that puked here (found in server/utils/dates.py:time_value_to_day()) can be wrapped to catch exceptions and then rethrow them as ValidationFailedexceptions. This is similar to what is done in other input processing methods in server/_params.py; those exceptions extend HTTPException which is handled and presented to the user gracefully by the web server.

We probbly also wanna do it with the similar Week() constructor call in time_value_to_week() as well, because i presume that will fail in an analogous way on unchecked/raw user input values.
@melange396
Copy link
Collaborator Author

Itll also be good practice to first add a test that fails for this, then do the fix, then make sure the test now passes

@melange396 melange396 added bug api change affect the API and its responses python Pull requests that update Python code labels Jan 19, 2024
@dmytrotsko dmytrotsko self-assigned this Feb 29, 2024
@dmytrotsko
Copy link
Contributor

dmytrotsko commented Mar 5, 2024 •
edited

Itll also be good practice to first add a test that fails for this, then do the fix, then make sure the test now passes

Actually, we already have tests for utils/dates.py here.
We also have integration test for the /csv endpoint here.
Here are list of params that were provided by user:

start_day=2023-01-01
end_day=2024-18-01 <----- the wrong date format, probably user missmatched date format, because we have YY-MM-DD.
geo_type=county
signal=chng:smoothed_outpatient_cli

@dmytrotsko dmytrotsko linked a pull request Mar 5, 2024 that will close this issue
OKRS24-94 #1402
Open
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@dmytrotsko dmytrotsko

Labels
api change affect the API and its responses bug python Pull requests that update Python code
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

OKRS24-94 cmu-delphi/delphi-epidata
2 participants
@melange396 @dmytrotsko