Upgrade SQLAlchemy to 2.0 #8452

marcospri · 2024-01-23T11:08:31Z

marcospri · 2024-01-23T14:31:51Z

h/db/mixins.py

@@ -9,12 +9,12 @@ class Timestamps:
    created = sa.Column(
        sa.DateTime,
        default=datetime.datetime.utcnow,
-        server_default=sa.func.now(),


Annoying pylint disables all around for these.

pylint-dev/pylint#8138

marcospri · 2024-01-23T14:32:17Z

h/services/bulk_api/group.py

@@ -33,7 +33,7 @@ def group_search(
        """

        query = (
-            sa.select([Group.authority_provided_id])
+            sa.select(Group.authority_provided_id)


Use the args, not list style of call

marcospri · 2024-01-23T14:32:37Z

h/services/job_queue.py

-                        "annotation_id", Annotation.id, "force", bool(force)
-                    ),
-                ]
+                literal_column(f"'{name}'"),


No list here either

marcospri · 2024-01-23T14:36:54Z

h/services/user_#.py

+            UserIdentity(
+                user=user,
+                provider=i_args["provider"],
+                provider_unique_id=str(i_args["provider_unique_id"]),


Added a bit more context on the commit message for this but we need this type cast for SQLA to produce the right SQL.

marcospri · 2024-01-23T14:37:20Z

tests/conftest.py

-            transaction.nested and not transaction._parent.nested
-        ):
-            session.begin_nested()
+    session = db_sessionfactory(


The 2.0 version is much simpler.

marcospri · 2024-01-23T14:38:16Z

tests/unit/h/services/search_index_test.py

@@ -293,6 +293,7 @@ def test_if_the_annotation_isnt_in_the_DB_it_deletes_the_job_from_the_queue(
        job = factories.SyncAnnotationJob(annotation=annotation)
        queue_service.get.return_value = [job]
        db_session.delete(annotation)
+        db_session.commit()


Need to commit for the object to be actually deleted and the test to see it gone.

marcospri · 2024-01-23T14:38:47Z

tests/unit/h/util/db_test.py

-def mock_transaction(db_session):
-    transaction = mock.Mock(spec=db_session.transaction)
+def mock_transaction():
+    transaction = mock.Mock(spec=SessionTransaction)


Use the class for the spec info. It's no longer accessible via the session.

seanh · 2024-01-30T14:15:50Z

Oh, we should delete this now:

h/tests/conftest.py

Lines 5 to 35 in 534cca8

    
           @pytest.fixture 
        
           def db_session(db_engine, db_sessionfactory): 
        
               """ 
        
               Return the SQLAlchemy database session. 
        
               h overrides the db_session fixture from h-testkit because h still uses 
        
               SQLAlchemy 1.4 so it has to use the older, 1.4 version of the SQLAlchemy 
        
               test suite technique: 
        
               https://docs.sqlalchemy.org/en/14/orm/session_transaction.html#joining-a-session-into-an-external-transaction-such-as-for-test-suites 
        
               When h is upgraded to SQLAlchemy 2 this fixture can be removed and it can 
        
               just use the one from h-testkit. 
        
               """ 
        
               connection = db_engine.connect() 
        
               transaction = connection.begin() 
        
               session = db_sessionfactory(bind=connection) 
        
               session.begin_nested() 
        
               @event.listens_for(session, "after_transaction_end") 
        
               def restart_savepoint(session, transaction): 
        
                   if (  # pylint:disable=protected-access 
        
                       transaction.nested and not transaction._parent.nested 
        
                   ): 
        
                       session.begin_nested() 
        
               try: 
        
                   yield session 
        
               finally: 
        
                   session.close() 
        
                   transaction.rollback()

I think it can just be straight deleted

…are passed positionally

Not sure if this is a bug in SQLA but while inserting more than one element in a relationship the generated statement seems to add typecasts based on the values, not the column definition. Casting provider_unique_id to a string here to fix the issue.

Without the explicit commit create_all fails because the UUID extension is not present.

sentry-io · 2024-02-03T10:32:44Z

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

‼️ OperationalError: (psycopg2.OperationalError) SSL connection has been closed unexpectedly status View Issue
‼️ OperationalError: (psycopg2.OperationalError) Connection refused status View Issue

_{Did you find this useful? React with a 👍 or 👎}

Base automatically changed from remove-reindex to main January 23, 2024 13:47

marcospri force-pushed the upgrade-sql branch from ec25651 to 8e67bea Compare January 23, 2024 14:30

marcospri changed the title ~~Upgrade sql~~ Upgrade SQLAlchemy to 2.0 Jan 23, 2024

marcospri commented Jan 23, 2024

View reviewed changes

marcospri requested a review from seanh January 23, 2024 15:10

marcospri marked this pull request as draft January 23, 2024 15:32

marcospri force-pushed the upgrade-sql branch from 52a5ccc to d6da25e Compare January 24, 2024 10:07

marcospri marked this pull request as ready for review January 24, 2024 10:10

marcospri force-pushed the upgrade-sql branch from d6da25e to 5e2821e Compare January 24, 2024 11:02

seanh approved these changes Jan 30, 2024

View reviewed changes

marcospri added 8 commits January 31, 2024 09:34

Upgrade SQLAlchemy to 2.0

047eaf5

Fix select() no longer accepts varied constructor arguments, columns …

5da88df

…are passed positionally

Adapt db_session fixture to SQLA2.0

f8d9a8c

Adapt transaction mocking to SQLA 2.0

a15aa6d

Explicitly commit deletion for test to see the expected DB state

fbf441b

Use text for raw SQL queries for compatibility with SQLA 2

c6197ca

Commit in pre-creation hooks to make changes visible in create_all

40430f5

Without the explicit commit create_all fails because the UUID extension is not present.

marcospri force-pushed the upgrade-sql branch from 5e2821e to 40430f5 Compare January 31, 2024 08:44

marcospri merged commit 9e507d7 into main Jan 31, 2024
9 checks passed

marcospri deleted the upgrade-sql branch January 31, 2024 08:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade SQLAlchemy to 2.0 #8452

Upgrade SQLAlchemy to 2.0 #8452

marcospri commented Jan 23, 2024 •

edited

Loading

marcospri Jan 23, 2024

marcospri Jan 24, 2024

marcospri Jan 23, 2024

marcospri Jan 23, 2024

marcospri Jan 23, 2024

marcospri Jan 23, 2024

marcospri Jan 23, 2024

marcospri Jan 23, 2024

seanh commented Jan 30, 2024

sentry-io bot commented Feb 3, 2024

Upgrade SQLAlchemy to 2.0 #8452

Upgrade SQLAlchemy to 2.0 #8452

Conversation

marcospri commented Jan 23, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanh commented Jan 30, 2024

sentry-io bot commented Feb 3, 2024

Suspect Issues

marcospri commented Jan 23, 2024 •

edited

Loading