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

Syncronize our SQLAlchemy type hints with db schema #2197

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

jonathangreen
Copy link
Member

Description

Make sure what what is defined in our SQLAlchemy schema is accuratly reflected in the type hints:

  • If a relationship is not a collection, and its foreign key column is nullable make sure the annotation is Optional
    • There were some cases where this caused a large number of mypy failures, since we were widely assuming that these relationships were not optional. In these cases I queried all our production instances, and if the the foreign key was never null, I added a nullable=False to the foreign key column.
  • Appropriately set the columns annotation to not Optional if:
    • nullable=False
    • primary_key=True
  • Add nullable=False and appropriately set annotation for columns that have default set

Motivation and Context

Our sqlalchemy type hints have been added over time, and some of them did not accurately reflect what was in the DB schema. I noticed this while working on #2194.

These changes meant that I had to add some guard code for errors that mypy rightly flagged. It also meant that I could remove some guard code that was no longer necessary.

How Has This Been Tested?

  • Running unit tests
  • Querying our production DBs to make sure our production data doesn't have any nulls set

Checklist

  • I have updated the documentation accordingly.
  • All new and existing tests passed.

@jonathangreen jonathangreen added the DB migration This PR contains a DB migration label Nov 27, 2024
Copy link

codecov bot commented Nov 27, 2024

Codecov Report

Attention: Patch coverage is 94.73684% with 14 lines in your changes missing coverage. Please review.

Project coverage is 91.09%. Comparing base (102e9f6) to head (0d48d10).

Files with missing lines Patch % Lines
src/palace/manager/api/odl/api.py 64.28% 3 Missing and 2 partials ⚠️
src/palace/manager/util/notifications.py 0.00% 2 Missing and 1 partial ⚠️
src/palace/manager/api/opds_for_distributors.py 0.00% 1 Missing and 1 partial ⚠️
src/palace/manager/feed/acquisition.py 75.00% 1 Missing and 1 partial ⚠️
src/palace/manager/marc/annotator.py 75.00% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2197      +/-   ##
==========================================
- Coverage   91.09%   91.09%   -0.01%     
==========================================
  Files         363      363              
  Lines       41248    41222      -26     
  Branches     8839     8831       -8     
==========================================
- Hits        37575    37550      -25     
  Misses       2406     2406              
+ Partials     1267     1266       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jonathangreen jonathangreen requested a review from a team November 27, 2024 20:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DB migration This PR contains a DB migration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant