Create proper enums for job statuses #412

michalkrzem · 2024-10-01T16:27:07Z

Your checklist for this pull request

I've read the contributing guideline.
I've tested my changes by building and running mquery, and testing changed functionality (if applicable)
I've added automated tests for my change (if applicable, optional)
I've updated documentation to reflect my change (if applicable)

What is the current behaviour?
Status column in Job model exists as a string

What is the new behaviour?
Status column in Job model is as a JobStatus enum.

msm-cert · 2024-10-02T14:21:32Z

src/migrations/versions/cbbba858deb0_init.py


 def upgrade() -> None:
-    op.create_table(


Why this change? We should not change older migrations in any case. (Please also don't reformat them, to keep unnecessary git changes to minimum)

Ok. I understand. I did this because the alembic upgrade head command in an existing environment causes errors that the table exists. So I check beforehand if the table exists.

msm-cert · 2024-10-02T14:21:45Z

src/migrations/versions/dbb81bd4d47f_add_jobagent.py


 def upgrade() -> None:
    # ### commands auto generated by Alembic - please adjust! ###
-    op.create_table(
-        "jobagent",


similarly, please don't change and reformat this file

msm-cert · 2024-10-02T14:24:11Z

INSTALL.md

 docker compose exec web python3 -m mquery.db
+docker compose exec web python3 -m alembic upgrade head


Not sure about this. We want to reduce the number of additional commands to zero (#402), not add more of them.

If you don't want to fix this in this PR and want to work separately in #402, it's OK to leave it as it is now (i.e. don't change the documentation in this PR. It works for fresh installations, just doesn't run migrations automatically).

If you want, I'll add some hints to #402 how to approach it

Not sure about this. We want to reduce the number of additional commands to zero (#402), not add more of them.

All because of alembic, which does not automatically recognize the modification from varchar to enum. I am able to create an enum type in postgres from the model level, but alembic does not replace it (it still remains VARCHAR) - that's why my manual modifications in migration files.

I found a library https://pypi.org/project/alembic-postgresql-enum/, quite new, I have to test it, but the question is whether to enter something like that at all. Or maybe look for solutions in sqlalchemy dialects for postgresql

don't change the documentation. After #421 alembic will be run with web automatically anyway.

msm-cert · 2024-10-02T14:27:56Z

src/migrations/versions/84796bdc2f77_add_jobstatus.py

+
+def upgrade() -> None:
+    op.execute(
+        "CREATE TYPE jobstatus AS ENUM ('done', 'new', 'cancelled', 'removed', 'processing');"


You don't need to know every possible type for this migration. Just change every value out of scope to cancelledand then convert to enum.

(But good point that we need to handle this correctly)

msm-cert · 2024-10-02T14:36:51Z

By the way, I've created an issue for removing the "removed" status. #414

It may be a good time to implement it (for you it's just a few lines of code now, but it'll require another migration if done separately). If that's OK, please also remove this status from code at this time.

msm-cert · 2024-10-15T01:21:02Z

INSTALL.md

 docker compose exec web python3 -m mquery.db
+docker compose exec web python3 -m alembic upgrade head


don't change the documentation. After #421 alembic will be run with web automatically anyway.

msm-cert · 2024-10-15T01:21:07Z

INSTALL.md

@@ -39,6 +40,7 @@ cd mquery
 vim .env
 docker compose -f docker-compose.dev.yml up  # this will take a while
 docker compose exec dev-web python3 -m mquery.db
+docker compose exec dev-web python3 -m alembic upgrade head


don't change the documentation. After #421 alembic will be run with web automatically anyway.

msm-cert · 2024-10-15T01:21:10Z

README.md

@@ -26,6 +26,7 @@ cd mquery
 vim .env  # optional - change samples and index directory locations
 docker compose up --scale daemon=3  # building the images will take a while
 docker compose exec web python3 -m mquery.db
+docker compose exec web python3 -m alembic upgrade head


don't change the documentation. After #421 alembic will be run with web automatically anyway.

msm-cert · 2024-10-15T01:21:41Z

src/migrations/env.py

@@ -1,3 +1,4 @@
+import alembic_postgresql_enum  # noqa: ignore=F401


I am basically sure there is no need for a new dependency here.

msm-cert · 2024-10-15T01:22:42Z

src/migrations/versions/0820dad794e6_delete_removed_status.py

+
+def downgrade() -> None:
+    # ### commands auto generated by Alembic - please adjust! ###
+    op.sync_enum_values(


shouldn't this drop the table instead?

msm-cert · 2024-10-15T01:23:49Z

src/migrations/versions/cbbba858deb0_init.py

+            "ursadb_url",
+            sqlmodel.sql.sqltypes.AutoString(),
+            nullable=False,


ideally this line should not change (this PR is completely unrelated to this file). Is there anything wong with the previous formatting?

I had to modify this because black raise errors.

msm-cert · 2024-10-15T01:24:22Z

src/migrations/versions/f552a27b4390_modify_job_status_type_to_enum.py

+def upgrade() -> None:
+    sa.Enum(
+        "done", "new", "cancelled", "removed", "processing", name="jobstatus"
+    ).create(op.get_bind())
+    op.sync_enum_values(
+        "public",
+        "jobstatus",
+        ["done", "new", "cancelled", "removed", "processing"],
+        [
+            TableReference(
+                table_schema="public", table_name="job", column_name="status"
+            )
+        ],
+        enum_values_to_rename=[],
+    )


why two migrations for a single change?

msm-cert · 2024-10-15T01:24:39Z

src/migrations/versions/f552a27b4390_modify_job_status_type_to_enum.py

+    op.sync_enum_values(
+        "public",
+        "jobstatus",
+        ["done", "new", "cancelled", "removed", "processing"],
+        [
+            TableReference(
+                table_schema="public", table_name="job", column_name="status"
+            )
+        ],
+        enum_values_to_rename=[],
+    )


this should probably drop the table (downgrade)

msm-cert · 2024-10-15T01:25:20Z

src/models/job.py

@@ -1,3 +1,6 @@
+import enum
+
+from sqlalchemy import Enum as PgEnum


why not just Enum? If you want to rename it, DbEnum would be more appropriate

msm-cert · 2024-10-15T01:25:32Z

src/mqueryfront/src/utils.js

@@ -16,6 +15,7 @@ export const openidLoginUrl = (config) => {
    if (config["openid_url"] === null || config["openid_client_id"] === null) {
        // Defensive programming - config keys can be null.
        return "#";
+        gi;


msm-cert · 2024-10-15T01:30:26Z

src/db.py

@@ -87,7 +91,7 @@ def get_valid_jobs(self, username_filter: Optional[str]) -> List[Job]:
        with self.session() as session:
            query = (
                select(Job)
-                .where(Job.status != "removed")
+                .where(Job.status != JobStatus.cancelled)


No need for this where - idea of #414 is to remove all jobs with this status from the database. So there are no more jobs that were removed and need to be filtered out.

Suggested change

.where(Job.status != JobStatus.cancelled)

msm-cert · 2024-10-15T01:30:51Z

src/db.py

+                update(Job)
+                .where(Job.id == job)
+                .values(status=JobStatus.cancelled)


Again, the idea of #414 is to drop the jobs from the database. So this should be delete instead of update.

msm-cert · 2024-10-15T01:32:55Z

src/migrations/versions/0820dad794e6_delete_removed_status.py

+
+
+def upgrade() -> None:
+    op.execute("UPDATE job SET status = 'cancelled' WHERE status = 'removed';")


Suggested change

op.execute("UPDATE job SET status = 'cancelled' WHERE status = 'removed';")

op.execute("DELETE FROM job WHERE status = 'removed';")

actually this works too and is even better

msm-cert · 2024-10-15T01:33:24Z

src/migrations/versions/0820dad794e6_delete_removed_status.py

+    # ### commands auto generated by Alembic - please adjust! ###
+    op.sync_enum_values(
+        "public",
+        "jobstatus",


actually I don't care about downgrates too much, and it's not even possible in this case. You can just raise NotImplementedError() here.

requirements.txt

… CASCADE

msm-cert · 2024-10-18T14:20:28Z

src/mqueryfront/src/utils.js

@@ -16,6 +15,7 @@ export const openidLoginUrl = (config) => {
    if (config["openid_url"] === null || config["openid_client_id"] === null) {
        // Defensive programming - config keys can be null.
        return "#";
+        gi;


msm-cert · 2024-10-18T14:33:20Z

src/migrations/versions/f654d7e4fcc7_on_delete_cascade_for_jobagent_and_match.py

+
+# revision identifiers, used by Alembic.
+revision = "f654d7e4fcc7"
+down_revision = "6b495d5a4855"


The order of migrations is wrong. This will run after the attempted jobs removal.

I've tested it:

mquery-dev-web-1 | WARNING: StatReload detected changes in 'tasks.py'. Reloading... mquery-dev-web-1 | INFO: Started server process [24] mquery-dev-web-1 | INFO: Waiting for application startup. mquery-dev-web-1 | INFO [alembic.runtime.migration] Context impl PostgresqlImpl. mquery-dev-web-1 | INFO [alembic.runtime.migration] Will assume transactional DDL. mquery-dev-web-1 | INFO [alembic.runtime.migration] Running upgrade dbb81bd4d47f -> 6b495d5a4855, add jobstatus mquery-dev-web-1 | Revision ID: 6b495d5a4855 mquery-dev-web-1 | Revises: dbb81bd4d47f mquery-dev-web-1 | Create Date: 2024-10-15 08:17:30.036531 mquery-postgres-1 | 2024-10-18 14:31:24.310 UTC [51] ERROR: update or delete on table "job" violates foreign key constraint "match_job_id_fkey" on table "match" mquery-postgres-1 | 2024-10-18 14:31:24.310 UTC [51] DETAIL: Key (internal_id)=(34) is still referenced from table "match". mquery-postgres-1 | 2024-10-18 14:31:24.310 UTC [51] STATEMENT: DELETE FROM job WHERE status = 'removed';

Both migrations can be probably merged into one to fix this.

Oh, yes, I forgot about this :/ So, one big migration file with create_foreign_key with ondelete="CASCADE", delete "removed" status, create jobstatus and alter table with new type?

Yeah, sounds good. A benefit of one big migration is that if something goes wrong then a rollback is performed (so the db is not stuck in an invalid state).

requirements.txt

src/migrations/versions/6b495d5a4855_add_jobstatus.py

msm-cert · 2024-10-18T14:42:36Z

(by the way, feel free to close resolved conversations)

msm-cert

LGTM!

michalkrzem added 3 commits October 1, 2024 14:03

Job status added

41313cd

Job status with alembic migrations

f4bae19

In INSTALL.md and README.md alembic command added.

d028b53

michalkrzem linked an issue Oct 1, 2024 that may be closed by this pull request

Create proper enums for Job statuses #370

Closed

michalkrzem added 3 commits October 1, 2024 18:41

ignore enum type error

1498a44

Creating jobstatus enum by job.py site

98621c7

Creating jobstatus enum in migration file

e0ce4f2

msm-cert changed the title ~~370 create proper enums for job statuses~~ Create proper enums for job statuses Oct 2, 2024

michalkrzem marked this pull request as ready for review October 2, 2024 14:11

msm-cert requested changes Oct 2, 2024

View reviewed changes

msm-cert mentioned this pull request Oct 2, 2024

Initialize the database automatically when starting mquery #402

Closed

michalkrzem added 10 commits October 9, 2024 00:13

After review

5525bdb

lint

db4e0e9

Merge branch 'master' into 370-create-proper-enums-for-job-statuses

3b4c309

Added recognized enum changes and removing removed status

02ae102

with nmp errors

690051d

delete removed

ef9e9d5

alembic_postgresql_enum for automatic enum type recognize

8556a75

lint

71f42ef

Create jobstatus enum type in migration file.

5d78c6b

black

9b83297

MichalJura mentioned this pull request Oct 14, 2024

Initialize the database automatically when starting mquery #421

Merged

4 tasks

msm-cert reviewed Oct 15, 2024

View reviewed changes

msm-cert requested changes Oct 15, 2024

View reviewed changes

michalkrzem added 5 commits October 17, 2024 10:08

After revie - job status as a enum, match and jobagent with ON DELETE…

e909efe

… CASCADE

Merge branch 'master' into 370-create-proper-enums-for-job-statuses

b3ddd37

'version' is obsolete

855380b

'version' is obsolete

34a5d2d

fix migrations

930b064

michalkrzem and others added 4 commits October 17, 2024 11:32

.

4bb1b89

.

beabd17

fix import in add_jobstatus migration

a63cdf1

black

7835dce

michalkrzem requested a review from msm-cert October 18, 2024 07:17

msm-cert requested changes Oct 18, 2024

View reviewed changes

michalkrzem added 2 commits October 21, 2024 17:11

After review - migration modified

3db4bfa

black

d9da4c5

michalkrzem requested a review from msm-cert October 21, 2024 15:25

msm-cert approved these changes Oct 22, 2024

View reviewed changes

msm-cert merged commit c218523 into master Oct 22, 2024
10 checks passed

msm-cert deleted the 370-create-proper-enums-for-job-statuses branch October 22, 2024 22:16

This was referenced Oct 22, 2024

Remove "removed" status. #414

Closed

Make fields nullable again #431

Merged

mickol34 pushed a commit that referenced this pull request Nov 6, 2024

Create proper enums for job statuses (#412)

807e01d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create proper enums for job statuses #412

Create proper enums for job statuses #412

michalkrzem commented Oct 1, 2024 •

edited

Loading

msm-cert Oct 2, 2024

michalkrzem Oct 3, 2024

msm-cert Oct 2, 2024

michalkrzem Oct 3, 2024

msm-cert Oct 2, 2024

msm-cert Oct 2, 2024 •

edited

Loading

michalkrzem Oct 3, 2024

msm-cert Oct 15, 2024

msm-cert Oct 2, 2024

msm-cert Oct 2, 2024

msm-cert commented Oct 2, 2024

msm-cert Oct 15, 2024

msm-cert Oct 15, 2024

msm-cert Oct 15, 2024

msm-cert Oct 15, 2024

msm-cert Oct 15, 2024

msm-cert Oct 15, 2024

michalkrzem Oct 15, 2024

msm-cert Oct 15, 2024

msm-cert Oct 15, 2024

msm-cert Oct 15, 2024

msm-cert Oct 15, 2024

msm-cert Oct 18, 2024

msm-cert Oct 15, 2024

msm-cert Oct 15, 2024

msm-cert Oct 15, 2024

msm-cert Oct 15, 2024

msm-cert Oct 18, 2024

msm-cert Oct 18, 2024

msm-cert Oct 18, 2024

michalkrzem Oct 18, 2024

msm-cert Oct 19, 2024

msm-cert commented Oct 18, 2024

msm-cert left a comment

		docker compose exec web python3 -m mquery.db
		docker compose exec web python3 -m alembic upgrade head

		@@ -1,3 +1,4 @@
		import alembic_postgresql_enum # noqa: ignore=F401

		@@ -1,3 +1,6 @@
		import enum

		from sqlalchemy import Enum as PgEnum



		def upgrade() -> None:
		op.execute("UPDATE job SET status = 'cancelled' WHERE status = 'removed';")

	op.execute("UPDATE job SET status = 'cancelled' WHERE status = 'removed';")
	op.execute("DELETE FROM job WHERE status = 'removed';")

Create proper enums for job statuses #412

Create proper enums for job statuses #412

Conversation

michalkrzem commented Oct 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msm-cert Oct 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msm-cert commented Oct 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msm-cert commented Oct 18, 2024

msm-cert left a comment

Choose a reason for hiding this comment

michalkrzem commented Oct 1, 2024 •

edited

Loading

msm-cert Oct 2, 2024 •

edited

Loading