Upgrade SQLAlchemy to v2

[kalbfled]

User Story - Business Need

Upgrade SQLAlchemy from v1.4.x to the latest 2.x version.

User Story(ies)

As a VA Notify developer
I want to upgrade SQLAlchemy
So that we minimize vulnerabilities and future upgrade pains.

• discuss w/ QA

Additional Info and Resources

• SQLAlchemy v2.x changelog
• Migration Guide

Engineering Checklist

• Review relevant upgrade/migration docs before getting started
• Upgrade the effected packages in requirements-app.txt
• Build a new Docker image
• Run the app locally. Observe failures. Fix them.
• Deploying the app may produce different results than local runs, so deploy to dev and observe/fix failures.
  • Run Regression
• Update requirements.txt as described in README.md

Acceptance Criteria

• The image builds
• All unit tests pass with the latest versions of the effected packages

QA Considerations

• QA regression passes
• Pay extra care to JSON responses
• Performance Testing to ensure there are no significant slowdowns to investigate

Potential Dependencies

Leave blank if n/a

Out of Scope

Do not upgrade Celery, marshmallow, or marshmallow-sqlalchemy. Separate tickets exist for those upgrades.

[mjones-oddball]

Hey team! Please add your planning poker estimate with Zenhub @cris-oddball @justaskdavidb2 @k-macmillan @kalbfled @ldraney @nikolai-efimov

[mjones-oddball]

Please add your planning poker estimate with Zenhub @edDocMe360

[mjones-oddball]

Please add your planning poker estimate with Zenhub @EvanParish

[ldraney]

Dave brought up that there may be implications here when moving to AWS.

[mjones-oddball]

Was an 8. Going to break out query updates and repoint when ready.

[kalbfled]

@k-macmillan While revising this and creating #1648, I removed your legacy syntax "straggler" because there are actually more than one still on app/. #1648 should remove all of them and provides a grep statement to find them.

[EvanParish]

After meeting with Kyle yesterday and discussing this, there's a little more complexity than we had hoped. I'm still working on getting the app running locally. It looks like I might have to manually define the history tables.

[EvanParish]

I created a PR for the progress so far and will have a discussion with @mchlwellman when he's ready to pick up this ticket.

[mchlwellman]

Just getting going on this, I have spoken with Evan. I'll have more of an update tomorrow once I look more in depth.

[mchlwellman]

I am still resolving errors, the current one that is taking some time is this -

Traceback (most recent call last):
  File "/Users/michaelwellman/.pyenv/versions/va-notify/bin/flask", line 8, in <module>
    sys.exit(main())
  File "/Users/michaelwellman/.pyenv/versions/3.10.13/envs/va-notify/lib/python3.10/site-packages/flask/cli.py", line 1107, in main
    cli.main()
  File "/Users/michaelwellman/.pyenv/versions/3.10.13/envs/va-notify/lib/python3.10/site-packages/click/core.py", line 1078, in main
    rv = self.invoke(ctx)
  File "/Users/michaelwellman/.pyenv/versions/3.10.13/envs/va-notify/lib/python3.10/site-packages/click/core.py", line 1688, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/Users/michaelwellman/.pyenv/versions/3.10.13/envs/va-notify/lib/python3.10/site-packages/click/core.py", line 1434, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/Users/michaelwellman/.pyenv/versions/3.10.13/envs/va-notify/lib/python3.10/site-packages/click/core.py", line 783, in invoke
    return __callback(*args, **kwargs)
  File "/Users/michaelwellman/.pyenv/versions/3.10.13/envs/va-notify/lib/python3.10/site-packages/click/decorators.py", line 33, in new_func
    return f(get_current_context(), *args, **kwargs)
  File "/Users/michaelwellman/.pyenv/versions/3.10.13/envs/va-notify/lib/python3.10/site-packages/flask/cli.py", line 385, in decorator
    app = ctx.ensure_object(ScriptInfo).load_app()
  File "/Users/michaelwellman/.pyenv/versions/3.10.13/envs/va-notify/lib/python3.10/site-packages/flask/cli.py", line 337, in load_app
    app = locate_app(import_name, name)
  File "/Users/michaelwellman/.pyenv/versions/3.10.13/envs/va-notify/lib/python3.10/site-packages/flask/cli.py", line 247, in locate_app
    __import__(module_name)
  File "/Users/michaelwellman/dev/va/notification-api/application.py", line 31, in <module>
    create_app(application)
  File "/Users/michaelwellman/dev/va/notification-api/app/__init__.py", line 221, in create_app
    register_blueprint(application)
  File "/Users/michaelwellman/dev/va/notification-api/app/__init__.py", line 238, in register_blueprint
    from app.service.rest import service_blueprint
  File "/Users/michaelwellman/dev/va/notification-api/app/service/rest.py", line 73, in <module>
    from app.schemas import (
  File "/Users/michaelwellman/dev/va/notification-api/app/schemas.py", line 127, in <module>
    class UserSchema(BaseSchema):
  File "/Users/michaelwellman/.pyenv/versions/3.10.13/envs/va-notify/lib/python3.10/site-packages/marshmallow/schema.py", line 114, in __new__
    klass._declared_fields = mcs.get_declared_fields(
  File "/Users/michaelwellman/.pyenv/versions/3.10.13/envs/va-notify/lib/python3.10/site-packages/marshmallow_sqlalchemy/schema/sqlalchemy_schema.py", line 91, in get_declared_fields
    fields.update(mcs.get_declared_sqla_fields(fields, converter, opts, dict_cls))
  File "/Users/michaelwellman/.pyenv/versions/3.10.13/envs/va-notify/lib/python3.10/site-packages/marshmallow_sqlalchemy/schema/sqlalchemy_schema.py", line 130, in get_declared_sqla_fields
    converter.fields_for_model(
  File "/Users/michaelwellman/.pyenv/versions/3.10.13/envs/va-notify/lib/python3.10/site-packages/marshmallow_sqlalchemy/convert.py", line 111, in fields_for_model
    field = base_fields.get(key) or self.property2field(prop)
  File "/Users/michaelwellman/.pyenv/versions/3.10.13/envs/va-notify/lib/python3.10/site-packages/marshmallow_sqlalchemy/convert.py", line 146, in property2field
    field_class = field_class or self._get_field_class_for_property(prop)
  File "/Users/michaelwellman/.pyenv/versions/3.10.13/envs/va-notify/lib/python3.10/site-packages/marshmallow_sqlalchemy/convert.py", line 210, in _get_field_class_for_property
    column = prop.columns[0]
  File "/Users/michaelwellman/.pyenv/versions/3.10.13/envs/va-notify/lib/python3.10/site-packages/sqlalchemy/util/langhelpers.py", line 1327, in __getattr__
    return self._fallback_getattr(key)
  File "/Users/michaelwellman/.pyenv/versions/3.10.13/envs/va-notify/lib/python3.10/site-packages/sqlalchemy/util/langhelpers.py", line 1296, in _fallback_getattr
    raise AttributeError(key)
AttributeError: columns

[kalbfled]

I'm picking up this ticket because Michael is out today.

• ORM Mapped Class Overview
• Composing Mapped Hierarchies with Mixins
• flask-sqlalchemy Models and Tables (initializing a base class)
• Facing a "ValueError: Invalid fields for" after upgrading from 2.15.0 to 3.17.0 marshmallow-code/marshmallow#2020

[mchlwellman]

I am using these versions of libs -

app-1  | Flask                                        3.0.2
app-1  | Flask-Bcrypt                                 1.0.1
app-1  | Flask-Cors                                   4.0.0
app-1  | Flask-JWT-Extended                           4.6.0
app-1  | flask-marshmallow                            1.2.0
app-1  | Flask-Migrate                                4.0.7
app-1  | Flask-SQLAlchemy                             3.1.1
app-1  | flask-marshmallow                            1.2.0
app-1  | marshmallow                                  3.21.1
app-1  | marshmallow-sqlalchemy                       1.0.0
app-1  | Flask-SQLAlchemy                             3.1.1
app-1  | marshmallow-sqlalchemy                       1.0.0
app-1  | SQLAlchemy                                   2.0.28

This error is still occurring -
sqlalchemy.exc.InvalidRequestError: When initializing mapper Mapper[User(users)], expression 'user_to_organisation' failed to locate a name ("name 'user_to_organisation' is not defined"). If this is a class name, consider adding this relationship() to the <class 'app.model.user.User'> class after both dependent classes have been defined.

[kalbfled]

I upgraded SQLAlchemy to v2 and upgrade related packages to their newest versions. I have been converting model declarations to the SQLAlchemy v2 "Declarative" syntax and removing inputs to schemas' "exclude" attribute in app/schema.py. I should know some time Monday if this is going to get the app in a runnable condition.

[kalbfled]

I managed to run the app on my branch (draft PR), but there are many errors:

1523 failed, 1270 passed, 16 skipped, 79 xfailed, 3 xpassed, 1528 errors

This branch does not include the recently merged Poetry changes. I don't want to mess with that until all the tests pass.

I suspect that I need to continue updating model definitions in models.py to use the newer syntax.

[kalbfled]

I updated models.py to the newer declarative format, but that didn't change the test results. Error messages suggest that I need to update how models relationships (foreign keys, etc.) are defined. I'm going to try to troubleshoot by running a specific test

[kalbfled]

After discussion with @k-macmillan and @mchlwellman, we have decided to pause work on this ticket and #1224, which is part of the same task. I do not have any new tests passing compared to what I posted yesterday. I began updating relationships in app/models.py and app/models/user.py because I was seeing many test errors related to foreign keys, but that work is tedious because doing it right requires knowing the intent behind the code. Is the relationship 1-to-many? Many-to-many? Etc.? Once the intent is understood by exploring how the relationship attributes are actually used in the code, we can update the syntax using the appropriate declarative relationship definition.

We discussed that completing the tickets that remove unused tables before returning to this ticket might be the best course of action.

A draft PR exists with the work Michael and I have completed thus far. I have not incorporated the Poetry implementation yet.

Below are the big picture steps I took to get the app and unit tests to run. See the PR for details. All steps other than altering model/table definitions are necessary to get the application to run locally or, once running, to run the tests. Michael pushed changes to upgrade migration code, which also needs to change to use SQLAlchemy v2.

1. Upgrade SQLAchemy, flask-sqlachemy, marshmallow, flask-marshmallow, and marshmallow-sqlalchemy to the newest versions. Without updating at least some of the supporting packages in addition to SQLAlchemy, the app won't start. It's easiest to do all these at once. Rebuild the application and test images.
2. Make all changes as described in the flask-sqlalchemy v3.1 quick start guide. In particular, note the "Initialize the Extension" section. All read the Models and Tables page.
3. Using the PR, view the changes to app/history_meta.py. I commented out code to clear an error that prevents the app from running. This is not a solution, and I expect any test involving model histories to fail as a consequence.
4. In app/schemas.py, I removed many elements from schemas' "exclude" meta-attribute because I was getting errors about the model not containing those fields when I tried running the app. At a glance, the offending fields either aren't defined on the model or are relationship fields. Once relationships are properly defined, restoring those fields to the "exclude" list might be appropriate. If this is the case, I expect tests of GET responses to fail.
5. In tests/conftest.py, see the changes I made for "reflection" and correctly to access table metadata. These are breaking syntax changes when upgrading to SQLAlchemy v2.

[npmartin-oddball]

Leaving for now.