Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update all remaining legacy SQL queries to use the new syntax #1648

Closed
14 of 16 tasks
kalbfled opened this issue Feb 14, 2024 · 4 comments
Closed
14 of 16 tasks

Update all remaining legacy SQL queries to use the new syntax #1648

kalbfled opened this issue Feb 14, 2024 · 4 comments

Comments

@kalbfled
Copy link
Member

kalbfled commented Feb 14, 2024

User Story - Business Need

SQLAlchemy v1.4 supporters two syntaxes for making database queries, but SQLAlchemy v2 only supports the newer syntax. Before we can update to v2, we need to convert all instances of legacy syntax to the newer syntax.

This work began with #1538. This ticket should finish the job.

User Story(ies)

As a VA Notify developer
I want to update legacy database query syntax to the new version
So that we can upgrade to SQLAlchemy v2.

  • discuss w/ QA

Additional Info and Resources

app queries
image.png

Engineering Checklist

  • Review relevant upgrade/migration docs before getting started

IMPORTANT NOTE: Legacy queries generally start with <model class>.query. Depending on context, updated syntax might use a new style query (i.e. stmt = select(Notification).where(...) followed by an execution statement) or an updated ORM call (i.e. replace Notification.query.count() == 1 with notify_db_session.session.get(Notification, notification_id) is not None, or write a statement to select the count).

  • Refactor all query statements the match these regular expressions, which overlap significantly:
    • grep -rn -E "\.query(\.|\()"
    • grep -rn filter_by (not all matches are part of queries)
  • Update the following ./app/ queries:
    • app/dao/complaint_dao.py
    • app/dao/services_dao.py
    • The remaining ./app/ queries need a TODO: comment and to be deleted, ensuring the methods return appropriate values.
      • TODO comments have tickets listed. These should be cleanup tickets. We do not use fido2, organisations, nor letters.
  • Collect unit tests that need updating and/or investigation into a new ticket. No changes to tests are required for this task!
    • We sometimes see unit tests that query SMS and email but only add notifications for one or the other. Or there are multiple services queried but the test only does one. Add a TODO comment and add these to a ticket, so we can investigate them.

Acceptance Criteria

  • No legacy queries remain
  • All unit tests pass

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 any dependency. Separate tickets exist for those upgrades.
  • Do not update unit tests as part of this
@kalbfled kalbfled changed the title Copy of Upgrade SQLAlchemy to v2 Update all remaining legacy SQL queries to use the new syntax Feb 14, 2024
@mjones-oddball
Copy link

@kalbfled
Copy link
Member Author

I upgrade all remaining legacy queries in app/ as part of the first task (the grep statements). Ergo, the TODO task became a non-issue.

@cris-oddball
Copy link

Needing @kalbfled to check the AC boxes off in this ticket. Will look at performance on Monday.

@cris-oddball
Copy link

Checked DD for performance the past month, no significant changes positive or negative.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants