Add SQLAlchemy Helpers integration #23
Conversation
| is_sqlite, | ||
| ) | ||
| from sqlalchemy_helpers import Base, get_or_create, update_or_create, is_sqlite, exists_in_db | ||
| from sqlalchemy_helpers.flask_ext import DatabaseExtension, get_or_404, first_or_404 |
There was a problem hiding this comment.
Those lines are causing ruff/flake8 error F401, which is normal, but please add the comment to ignore it.
| @@ -42,7 +44,7 @@ def run_migrations_offline() -> None: | |||
| script output. | |||
|
|
|||
| """ | |||
| url = str(get_config().database.sqlalchemy.url) | |||
| url = str(get_url_from_app(create_app)) | |||
There was a problem hiding this comment.
There's another line down this file where get_config() should be replaced, otherwise things crash.
The docs recommend getting the URL once and setting it in alembic's config object:
url = get_url_from_app(create_app)
config.set_main_option("sqlalchemy.url", url)
And then you can use the regular alembic template for the env.py file downwards.
Or you could get the URL in a variable in the top-level section, and replace the engine_from_config call by a simple create_engine call that would use this variable.
|
FYI you can test database creation with: |
gridhead
force-pushed
the
sqla
branch
2 times, most recently
from
f6079c9
to
3b3eaff
Compare
|
@abompard It does not work here either and I could use some help with defining TZ-aware objects I am currently doing it like this https://github.com/fedora-infra/webhook-to-fedora-messaging/pull/23/files#diff-d89f48631b32644376fe0ac1a41a9197091ddbdbeb10f08a437278bab2fbfec4R23-R25 but I suspect that is a mistaken approach. Following is a truncated traceback. |
|
Ok I got over the previous part but now I have a question - What pattern is the first parameter to the get_config() is very different as it simply returns a dictionary - which won't suffice here.
|
|
Ok ignore everything I mentioned alright? It works and the solution was as easy as replacing the No dialects added yet so |
webhook_to_fedora_messaging/main.py
Outdated
| @@ -35,13 +31,11 @@ def create_app(): | |||
|
|
|||
| # Then load the variables up from the custom configuration file | |||
| try: | |||
| confdata = get_config() | |||
| confdata = get_config() | |||
| main.config.from_mapping(confdata) | |||
There was a problem hiding this comment.
Do you expect the next three lines to raise ConfigError? If not, let's get them out of the block.
webhook_to_fedora_messaging/main.py
Outdated
| confdata = get_config() | ||
| confdata = get_config() | ||
| main.config.from_mapping(confdata) | ||
| db.init_app(main) |
There was a problem hiding this comment.
I don't think this should be run inside the try/except block, or could this call raise ConfigError?
| confdata = get_config() | ||
| main.config.from_mapping(confdata) | ||
| db.init_app(main) | ||
| dictConfig(confdata["logsconf"]) | ||
| except ConfigError as expt: | ||
| logging.error(f"Exiting - Reason - {str(expt)}") |
There was a problem hiding this comment.
It says "exiting" but it's neither exiting nor raising the exception. I think you should add a raise here so that the exception is propagated upwards the call stack.
| @compiles(utcnow, "postgresql") | ||
| def _postgresql_utcnow(element, compiler, **kwargs): | ||
| return "(NOW() AT TIME ZONE 'utc')" | ||
| return str(datetime.now(UTC)) |
There was a problem hiding this comment.
I don't think the postgresql driver will be very happy if we give it a string instead of a datetime. The SQLite driver will probably not mind though, because it's storing datetimes as strings anyway.
As a result, the utcnow function will probably boil down to functools.partial(datetime.now, tz=UTC). But you can keep it as a function either, I don't have reasons to think one is better than the other :-)
There was a problem hiding this comment.
I did not completely understand the request here. Should I revert the _postgresql_utcnow function for use with the Postgres compiler and keep the string converted datetime.now(UTC) object for Sqlite cases? I removed the utcnow() function for now for a oneliner string converted functools.partial(datetime.now, tz=UTC) but I am 100% sure that would not satiate the Postgres compiler.
gridhead
force-pushed
the
sqla
branch
2 times, most recently
from
9b17470
to
9f72c12
Compare
| @@ -0,0 +1,60 @@ | |||
| [flaskapp] | |||
There was a problem hiding this comment.
How is this file used? Are those the defaults? Is it an example file? Please add a comment at the top to explain.
There was a problem hiding this comment.
Ahh. This got added to the commit by mistake. I will remove this. The example config is already there in the project root.
| @@ -37,4 +24,4 @@ class CreatableMixin: | |||
| An SQLAlchemy mixin to store the time when an entity was created | |||
| """ | |||
|
|
|||
| creation_date = Column("creation_date", SQLDateTime, nullable=False, server_default=utcnow()) | |||
| creation_date = Column("creation_date", SQLDateTime, nullable=False, server_default=str(partial(datetime.now, tz=UTC))) | |||
There was a problem hiding this comment.
This is not going to work, here is the table schema that is generated:
sqlite> .schema services
CREATE TABLE services (
id INTEGER NOT NULL,
user_id INTEGER NOT NULL,
name TEXT NOT NULL,
type TEXT NOT NULL,
"desc" TEXT NOT NULL,
disabled BOOLEAN NOT NULL,
uuid TEXT NOT NULL,
creation_date DATETIME DEFAULT 'functools.partial(<built-in method now of type object at 0x7fd402005820>, tz=datetime.timezone.utc)' NOT NULL,
CONSTRAINT pk_services PRIMARY KEY (id),
CONSTRAINT fk_services_user_id_users FOREIGN KEY(user_id) REFERENCES users (id) ON DELETE CASCADE,
CONSTRAINT uq_services_uuid UNIQUE (uuid)
);
We could however use default=partial(datetime.now, tz=UTC) instead of server_default, since it expects a python callable.
| TEMPLATES_AUTO_RELOAD = "" | ||
| MAX_COOKIE_SIZE = "" | ||
|
|
||
| [flaskapp.database] |
There was a problem hiding this comment.
This will not be used anymore, sqlalchemy-helpers uses a config key named SQLALCHEMY_DATABASE_URI (like flask-sqlalchemy)
This is also true for the config.toml.example file at the root of the project.
There was a problem hiding this comment.
Gotcha. Nuking the entire database section of the example config.
| @@ -23,13 +23,6 @@ PREFERRED_URL_SCHEME = "http" | |||
| TEMPLATES_AUTO_RELOAD = "" | |||
| MAX_COOKIE_SIZE = "" | |||
|
|
|||
There was a problem hiding this comment.
Please also add the SQLALCHEMY_DATABASE_URI variable with an example value of, say, sqlite:////tmp/w2fm.db, so that people have an example of how to configure the database.
I would put it close to the top because it's pretty important.
Signed-off-by: Akashdeep Dhar <[email protected]>
Add SQLAlchemy Helpers integration