Persistent storage solution & refactor #24
Conversation
…ys, create db scripts
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| # Create async engine | ||
| engine = None | ||
| if os.getenv("ENV") != "TEST": | ||
| url = get_database_url().replace("postgres://", "postgresql+asyncpg://", 1) |
There was a problem hiding this comment.
is there a reason you're using "asyncpg"? the Heroku docs call for "psycopg2" https://devcenter.heroku.com/articles/connecting-heroku-postgres#connecting-in-python so this should be: url = get_database_url().replace("postgres://", "postgresql+psycopg2://", 1)
There was a problem hiding this comment.
Was using this as it's supposedly more efficient but will go with what Heroku docs stipulates
There was a problem hiding this comment.
Was using this as it's supposedly more efficient but will go with what Heroku docs stipulates
So I can use psycopg2 with heroku locally for async operations but when using in docker it doesn't work.
If we aren't concerned about it we can move on, otherwise we'll need to use something like asyncpg alongside it for cross compatability I think.
bot_1 | sqlalchemy.exc.InvalidRequestError: The asyncio extension requires an async driver to be used. The loaded 'psycopg2' is not async.
There was a problem hiding this comment.
In what context are we going to use Docker? if it's just for testing, we can potentially have the environment variable dictate what the database url prefix should be. Also, i'm curious if it's unspecified and we just used postgres:// will it work
There was a problem hiding this comment.
In what context are we going to use Docker? if it's just for testing, we can potentially have the environment variable dictate what the database url prefix should be. Also, i'm curious if it's unspecified and we just used
postgres://will it work
I was thinking in terms of 'backward' compatability so to say in the event we moved back to docker. After more thought this seems unecessary so I'll continue with psycopg2 and disregard the need for Docker to be used for the Notion integration.
There was a problem hiding this comment.
Lots of code changes so didn't scrutinize it super closely, but overall everything looks good. A few requested changes.
Made a new dev branch. Please update to target that branch, so we can add the other changes you mentioned in your Discord message and get them all merged in to main in one go (it will auto deploy).
As for migration, I think if the notifications aren't too crazy, we just run it the same way on production as we do on staging and let it "catch up". But if we see that it's an insane amount of notifications, then I'm ok with migrating "db.json -> postgres" on staging and then "postgres -> postgres" on production.
…nsitive information Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
| if os.getenv("ENV") != "TEST": | ||
| url = get_database_url().replace("postgres://", "postgresql+asyncpg://", 1) | ||
| sanitized_url = url.split('@')[0].split(':')[0] + ':***@' + url.split('@')[1] | ||
| print(f"Connecting to {sanitized_url}") |
Check failure
Code scanning / CodeQL
Clear-text logging of sensitive information High
This autofix suggestion was applied.
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix AI 11 days ago
To fix the problem, we should avoid logging any part of the database URL that contains sensitive information. Instead, we can log a generic message indicating that the connection to the database is being attempted without including the URL. This way, we maintain the functionality of logging the connection attempt without exposing any sensitive information.
- Remove the logging of
sanitized_urland replace it with a generic message. - Update the code in
src/infrastructure/config/database.pyto reflect this change.
| @@ -25,4 +25,3 @@ | ||
| url = get_database_url().replace("postgres://", "postgresql+asyncpg://", 1) | ||
| sanitized_url = url.split('@')[0].split(':')[0] + ':***@' + url.split('@')[1] | ||
| print(f"Connecting to {sanitized_url}") | ||
| print("Attempting to connect to the database...") | ||
|
|
Sarahtonein
committed this autofix suggestion 11 days ago.
Because of the seemingly missed updates / creates in the tinyDB implementation there would be quite a lot of notifications (50-100+ iirc) - can display this in the test/staging server. I think we should proceed with the postgres -> postgres option between staging and prod |
…nsitive information Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Whats new
How to test
docker-compose up -d dbpipenv shellpython -m src.scripts.migratepython -m src.scripts.queryOnce the migration has completed you can either run heroku local, or docker.
NOTE:
The existing db.json is a bit outdated, and formatted in a way that some things weren't titled appropriately this will result in extra discord notifications.
A suggestion is that we migrate the staging postgres DB into production, so that it continues from a point in time snapshot and reduces the number of discord notifications (if any)