Code Review Comments (Salif) #2
Description
Right now, POSTGRES_URI is directly fetched in database.py without any validation. It would be a good idea to add a check to make sure this URI is valid before establishing a connection, and maybe log a meaningful error if the connection fails. It’s better than the API just breaking if there’s a misconfiguration.
In auth.py, the API key is pulled from .env without any validation. A missing or malformed key could break the authentication flow entirely. A quick check to confirm the key’s existence—and a fallback option in case it’s missing—would make this setup more robust.
Storing API keys in plain text might not be the safest choice. In auth.py, consider hashing the keys or even using a token-based system. This could add an extra layer of security, especially if there are plans to handle more sensitive data down the line.
Custom error messages are great, but in auth.py, the message “YOU FORGOT TO AUTHORIZE WITH THE API KEY!!!” feels a bit too informal and specific. It’s usually better to keep these messages uniform and simple. Something like “Authorization required” might work universally and won’t reveal much about internal logic.
Some queries in bill.py (like those in create_bill and get_bill) are pretty verbose. Using more joins or subqueries could reduce the number of times we hit the database. This might improve performance and make the functions a bit cleaner.
The format of responses across the API is not fully standardized. For instance, in get_bill, some fields feel redundant, and the casing style (camelCase vs. snake_case) varies. Standardizing the format will help keep things predictable for anyone using the API.
Certain functions (like assign_chore in chore_assignment.py) are missing detailed docstrings. Making sure each function has a short, clear docstring would be a nice touch, especially for new developers joining the project.
Logging is currently spread out and inconsistent. Setting up a centralized logging configuration would make it easier to troubleshoot issues and track down errors. Plus, we could log useful metadata (like timestamps and user actions) to help with debugging.
In server.py, CORS is configured to allow all origins, which makes sense for development but not for production. Once deployed, restricting origins to only trusted sources would be wise, adding an extra layer of security.
In bill.py, the StatusEnum doesn’t fully validate input to match only the predefined enum types. Ensuring the values are strictly validated (perhaps using Pydantic validators) will prevent unwanted data from being saved.
In schema.sql, data is prepopulated with ON CONFLICT DO NOTHING, which is fine for testing but could hide errors or data conflicts in production. Switching to an upsert or more nuanced conflict resolution approach would provide better control.
Right now, authentication relies solely on API keys, which doesn’t allow for much flexibility. Implementing role-based access (e.g., admin vs. user roles) could offer more control, especially if certain actions need stricter access permissions.
It looks like the database configuration (like pool size, pre-ping, etc.) is set to defaults. Fine-tuning these settings based on expected usage (e.g., setting a higher pool size for higher concurrency) could help optimize performance and handle scale better.
Some error handling is minimal. For example, instead of simply returning an error code, we could catch specific exceptions and return detailed messages to the client. This would make error messages more informative and tailored to what went wrong.
It’s worth checking if numbers, dates, and other critical inputs are validated consistently. Pydantic provides cool validators that can enforce formats and value ranges, ensuring that data entering the API is valid before it even hits the database.