Fix type hints in models, qa_loader

[rmol]

Status

Ready for review

Description of Changes

Corrects some type hinting problems in models.py and qa_loader.py.

Testing

CI is green. Run make lint if you feel like it.

Deployment

Dev only, 'cause type hints don't actually do anything.

Checklist

If you made changes to the server application code:

• Linting (make lint) and tests (make test) pass in the development container

If you made non-trivial code changes:

• I have written a test plan and validated it for this PR

Choose one of the following:

• I have opened a PR in the docs repo for these changes, or will do so later
• I would appreciate help with the documentation
• These changes do not require documentation

[emkll]

(rebased on latest develop)

[emkll]

Thanks @rmol lint target now passing after rebase and addresses lint failures in develop. One comment/clarification inline before merging

[emkll]

The database default value for hotp_counter is 0, why do we manually set to 0 here? Under which circumstances is hotp_counter is None? Shouldn't we be returning False?

[rmol]

Excellent question, which made me dig deeper into this. Here's my understanding of this whole house of cards:

Right now, Journalist.hotp_counter is defined as Column(Integer, default=0)

• without nullable=False, the database schema says hotp_counter can be null. (In fact, being SQLite, it can be 'what a wonderfully flexible database', too, but that's beside the point.)
• the default=0 is not the database default value; it's a value that SQLAlchemy will use in the case where no value was provided to the INSERT or UPDATE statement for that column.
• Mypy infers hotp_counter as builtins.int* -- I thought this might be because the sqlalchemy stubs that were just added were clever enough to notice the default clause, but I get the same result on develop before the stubs were added, and I've found some nullable columns without defaults (first_name, for instance) that are also not inferred as optional. I'm still looking, but in any case it does not match reality, because the column can be null.

So: adding # type: Column[Optional[int]] accurately describes what we can get from our database (as long as we disregard SQLite flexibility), but then if hotp_counter is not explicitly assigned, mypy knows it might be None, so catches that that should not be passed to range:

securedrop/models.py:641: error: Argument 1 to "range" has incompatible type "Optional[int]"; expected "int"
securedrop/models.py:642: error: Unsupported operand types for + ("None" and "int")
securedrop/models.py:642: note: Left operand is of type "Optional[int]"

So that's why it could be None, at least in mypy's understanding of the world, and why I made sure it was set to the default, but you're right, I was too far down this rabbit hole and not thinking about the actual method. We should treat a null hotp_counter as an error and return False from verify_token.

And as much as I'd like to grumble about the bolt-on type checker necessitating appeasement code, this is turning up valid problems with our ORM and database and the illusions they create. I think many of our columns should have been created with nullable=False. That would take a difficult data migration to fix. In the meantime, I'd like to propagate the Optional annotations to them. It doesn't have to be done in this PR, as I really just wanted to fix CI, but I think it's wrong to pretend we can't get null/None from the database. If we're going to check typing, let's be as accurate as possible.

Thoughts?

[emkll]

Thanks for digging into this. I agree with your assessment:

1. Immediately, to unblock this PR and CI: return false if hotp_counter is null
2. Medium term propagate the Optional annotation
3. Long term: Open an issue to add/migrate nullable=False database constrains (which as you say may be a complex migration for long-running instances.

[emkll]

lgtm