Add special mypy plugin/support for SQLAlchemy #6346

legoktm · 2022-03-16T21:35:23Z

Description

I did not verify this, but I suspect that #6342 could've been avoided if mypy was able to understand that nullable=True maps to Optional[...] in Python types and complained about not checking for is not None first.

In the past I've used https://github.com/dropbox/sqlalchemy-stubs for this purpose. SQLAlchemy also has its own mypy plugin based off the Dropbox one, https://docs.sqlalchemy.org/en/14/orm/extensions/mypy.html, but I haven't personally used it.

We should investigate one or both of these and see if it improves what mypy can detect in terms of type issues.

The text was updated successfully, but these errors were encountered:

legoktm · 2022-03-18T16:08:00Z

This was actually attempted in #5589, but missed the step of adding the plugin to mypy, so it didn't have much of an impact :(

When I enabled it locally (and upgraded mypy), the main thing it caught is that Source.filesystem_id is technically nullable, even though in practice it never is. Still have some more to clean up before I push a PR.

We already set this field all the time and in all places expect it to be a string, not checking if its None first. Discovered by the mypy work I'm doing in #6346. Refs #6226.

* Upgrade mypy to 0.941 and sqlalchemy-stubs to 0.4. Type stubs are no longer bundled with mypy, so we need to explicitly install various types-{name} packages. * The main issue spotted by the SQLAlchemy plugin, that sources.filesystem_id is nullable, is being fixed in a separate PR: #6350. * Use `--explicit-package-bases` when running mypy, so it treats the securedrop/admin directories as modules regardless of whether `__init__.py` files exist. This is needed so the two tests/ directories don't conflict with each other. * Add missing return types to all schema changes and the templates that generate them. * In the `create_source_uuid_column` migration, drop the use of `quoted_name()`. For some reason mypy doesn't like it, but more importantly we don't need it in SQLite and we don't use it in any other schema change. * Make SecureTemporaryFile.write()'s return type match its parent by returning the number of bytes that were written. Fixes #6346.

We already set this field all the time and in all places expect it to be a string, not checking if its None first. Discovered by the mypy work I'm doing in #6346. Refs #6226.

* Upgrade mypy to 0.941 and sqlalchemy-stubs to 0.4. Type stubs are no longer bundled with mypy, so we need to explicitly install various types-{name} packages. * The main issue spotted by the SQLAlchemy plugin, that sources.filesystem_id is nullable, is being fixed in a separate PR: #6350. * Use `--explicit-package-bases` when running mypy, so it treats the securedrop/admin directories as modules regardless of whether `__init__.py` files exist. This is needed so the two tests/ directories don't conflict with each other. * Add missing return types to all schema changes and the templates that generate them. * In the `create_source_uuid_column` migration, drop the use of `quoted_name()`. For some reason mypy doesn't like it, but more importantly we don't need it in SQLite and we don't use it in any other schema change. * Make SecureTemporaryFile.write()'s return type match its parent by returning the number of bytes that were written. Fixes #6346.

* Upgrade mypy to 0.941 and sqlalchemy-stubs to 0.4. Type stubs are no longer bundled with mypy, so we need to explicitly install various types-{name} packages. * The main issue spotted by the SQLAlchemy plugin, that sources.filesystem_id is nullable, is being fixed in a separate PR: #6350. * Drop the explicit type comments now that mypy can infer them. * Use `--explicit-package-bases` when running mypy, so it treats the securedrop/admin directories as modules regardless of whether `__init__.py` files exist. This is needed so the two tests/ directories don't conflict with each other. * Add missing return types to all schema changes and the templates that generate them. * In the `create_source_uuid_column` migration, drop the use of `quoted_name()`. For some reason mypy doesn't like it, but more importantly we don't need it in SQLite and we don't use it in any other schema change. * Make SecureTemporaryFile.write()'s return type match its parent by returning the number of bytes that were written. Fixes #6346.

* Upgrade mypy to 0.942 and sqlalchemy-stubs to 0.4. Type stubs are no longer bundled with mypy, so we need to explicitly install various types-{name} packages. * Since we need to install extra packages and need other dependencies like Flask, move mypy into test-requirements.txt and run it via the dev container. * The main issue spotted by the SQLAlchemy plugin, that sources.filesystem_id is nullable, is being fixed in a separate PR: #6350. * Drop the explicit type comments now that mypy can infer them. * Use `--explicit-package-bases` when running mypy, so it treats the securedrop/admin directories as modules regardless of whether `__init__.py` files exist. This is needed so the two tests/ directories don't conflict with each other. * Add missing return types to all schema changes and the templates that generate them. * In the `create_source_uuid_column` migration, drop the use of `quoted_name()`. For some reason mypy doesn't like it, but more importantly we don't need it in SQLite and we don't use it in any other schema change. * Make SecureTemporaryFile.write()'s return type match its parent by returning the number of bytes that were written. Fixes #6346. Fixes #6358.

We already set this field all the time and in all places expect it to be a string, not checking if its None first. The database migration will delete any sources that have a NULL filesystem_id, plus anything that references those sources. Discovered by the mypy work I'm doing in #6346. Refs #6226.

* Upgrade mypy to 0.942 and sqlalchemy-stubs to 0.4. Type stubs are no longer bundled with mypy, so we need to explicitly install various types-{name} packages. * Since we need to install extra packages and need other dependencies like Flask, move mypy into test-requirements.txt and run it via the dev container. * The main issue spotted by the SQLAlchemy plugin, that sources.filesystem_id is nullable, is being fixed in a separate PR: #6350. * Drop the explicit type comments now that mypy can infer them. * Use `--explicit-package-bases` when running mypy, so it treats the securedrop/admin directories as modules regardless of whether `__init__.py` files exist. This is needed so the two tests/ directories don't conflict with each other. * Add missing return types to all schema changes and the templates that generate them. * In the `create_source_uuid_column` migration, drop the use of `quoted_name()`. For some reason mypy doesn't like it, but more importantly we don't need it in SQLite and we don't use it in any other schema change. * Make SecureTemporaryFile.write()'s return type match its parent by returning the number of bytes that were written. Fixes #6346. Fixes #6358.

We already set this field all the time and in all places expect it to be a string, not checking if its None first. The database migration will delete any sources that have a NULL filesystem_id, plus anything that references those sources. Discovered by the mypy work I'm doing in #6346. Refs #6226.

* Upgrade mypy to 0.942 and sqlalchemy-stubs to 0.4. Type stubs are no longer bundled with mypy, so we need to explicitly install various types-{name} packages. * Since we need to install extra packages and need other dependencies like Flask, move mypy into test-requirements.txt and run it via the dev container. * The main issue spotted by the SQLAlchemy plugin, that sources.filesystem_id is nullable, is being fixed in a separate PR: #6350. * Drop the explicit type comments now that mypy can infer them. * Use `--explicit-package-bases` when running mypy, so it treats the securedrop/admin directories as modules regardless of whether `__init__.py` files exist. This is needed so the two tests/ directories don't conflict with each other. * Add missing return types to all schema changes and the templates that generate them. * In the `create_source_uuid_column` migration, drop the use of `quoted_name()`. For some reason mypy doesn't like it, but more importantly we don't need it in SQLite and we don't use it in any other schema change. * Make SecureTemporaryFile.write()'s return type match its parent by returning the number of bytes that were written. Fixes #6346. Fixes #6358.

legoktm mentioned this issue Mar 18, 2022

Various database columns should be NOT NULL #6226

Open

legoktm added a commit that referenced this issue Mar 18, 2022

Make sources.filesystem_id non-nullable

870ebcc

We already set this field all the time and in all places expect it to be a string, not checking if its None first. Discovered by the mypy work I'm doing in #6346. Refs #6226.

legoktm mentioned this issue Mar 18, 2022

Make sources.filesystem_id non-nullable #6350

Merged

5 tasks

legoktm mentioned this issue Mar 18, 2022

Upgrade mypy and enable SQLAlchemy plugin #6351

Merged

2 tasks

legoktm self-assigned this Mar 18, 2022

legoktm added a commit that referenced this issue Mar 21, 2022

Make sources.filesystem_id non-nullable

fc72191

We already set this field all the time and in all places expect it to be a string, not checking if its None first. Discovered by the mypy work I'm doing in #6346. Refs #6226.

legoktm added this to the 2.4.0 milestone Mar 30, 2022

zenmonkeykstop closed this as completed in #6351 Apr 20, 2022

legoktm mentioned this issue Mar 11, 2024

Add special mypy plugin/support for SQLAlchemy freedomofpress/securedrop-client#1901

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add special mypy plugin/support for SQLAlchemy #6346

Add special mypy plugin/support for SQLAlchemy #6346

legoktm commented Mar 16, 2022

legoktm commented Mar 18, 2022

Add special mypy plugin/support for SQLAlchemy #6346

Add special mypy plugin/support for SQLAlchemy #6346

Comments

legoktm commented Mar 16, 2022

Description

legoktm commented Mar 18, 2022