Skip to content

Commit

Permalink
Make updates for Python 3.12 & update various packages to their lates…
Browse files Browse the repository at this point in the history
…t versions (#206)

## Ticket

#205

## Changes

Make package updates to make Python 3.12 work without warnings

## Context for reviewers

Package updates were done via `poetry add <package>@latest [--group
dev]` (the group being included for dev dependencies). I did skim the
changelogs for any of the libraries we updated, at least the major
versions to see if there was anything signficantly breaking, but most of
the packages are pretty minimal in changes (bug fixes, small feature
updates).

The package updates fall into a few categories:
1. Broken due to new version of Python (cffi was the main one)
2. Warnings through linting or pytest deprecation warnings - newer
package versions had corrected the issue (SQLAlchemy, APIFlask, pytz,
pytest, mypy)
3. Package only had a few minor updates, figured we might as well update
(alembic, isort, flake8, Faker, factory-boy)

The only package of note that I didn't update was Pydantic, which I'll
do in a follow-up as I know it has several breaking changes.

This also resolves a few Dependabot alerts for the gitpython,
cryptography and urllib3 packages (package.lock has been updated to have
the latest version for each of those past the required ones).

## Testing

Running `make format lint test` both inside and outside of the Docker
container to verify that there are no issues reported / pytest warnings
given. Will verify that this builds as well in CI successfully when the
PR is created / later merged.

---------

Co-authored-by: nava-platform-bot <[email protected]>
  • Loading branch information
chouinar and nava-platform-bot authored Oct 10, 2023
1 parent 58610f4 commit 4252983
Show file tree
Hide file tree
Showing 14 changed files with 1,065 additions and 862 deletions.
2 changes: 1 addition & 1 deletion app/.python-version
Original file line number Diff line number Diff line change
@@ -1 +1 @@
3.11
3.12
3 changes: 3 additions & 0 deletions app/openapi.generated.yml
Original file line number Diff line number Diff line change
Expand Up @@ -239,6 +239,9 @@ components:
properties:
type:
description: The name of the role
enum:
- USER
- ADMIN
User:
type: object
properties:
Expand Down
1,846 changes: 1,022 additions & 824 deletions app/poetry.lock

Large diffs are not rendered by default.

43 changes: 22 additions & 21 deletions app/pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -7,37 +7,38 @@ authors = ["Nava Engineering <[email protected]>"]

[tool.poetry.dependencies]
python = "^3.10"
SQLAlchemy = {extras = ["mypy"], version = "2.0"}
alembic = "^1.8.1"
SQLAlchemy = {version = "^2.0.21", extras = ["mypy"]}
alembic = "^1.12.0"
python-dotenv = "^0.20.0"
pydantic = "^1.10.0"
botocore = "^1.27.67"
boto3 = "~1.24.67"
botocore = "^1.31.62"
boto3 = "^1.28.62"
smart-open = "^6.1.0"
pytz = "^2022.2.1"
APIFlask = "^1.1.3"
pytz = "^2023.3.post1"
APIFlask = "^2.0.2"
marshmallow-dataclass = {extras = ["enum", "union"], version = "^8.5.8"}
marshmallow = "^3.18.0"
marshmallow = "^3.20.1"
gunicorn = "^21.2.0"
psycopg = {extras = ["binary"], version = "^3.1.10"}

[tool.poetry.group.dev.dependencies]
black = "^22.6.0"
flake8 = "^5.0.4"
flake8-bugbear = "^22.8.23"
black = "^23.9.1"
flake8 = "^6.1.0"
flake8-bugbear = "^23.9.16"
flake8-alfred = "^1.1.1"
isort = "^5.10.1"
mypy = "^0.971"
isort = "^5.12.0"
mypy = "^1.5.1"
moto = {extras = ["s3"], version = "^4.0.2"}
types-pytz = "^2022.2.1"
coverage = "^6.4.4"
Faker = "^14.2.0"
factory-boy = "^3.2.1"
bandit = "^1.7.4"
pytest = "^6.0.0"
types-pytz = "^2023.3.1.1"
coverage = "^7.3.2"
Faker = "^19.8.0"
factory-boy = "^3.3.0"
bandit = "^1.7.5"
pytest = "^7.4.2"
pytest-watch = "^4.2.0"
pytest-lazy-fixture = "^0.6.3"
types-pyyaml = "^6.0.12.11"
setuptools = "^68.2.2"

[build-system]
requires = ["poetry-core>=1.0.0"]
Expand Down Expand Up @@ -81,6 +82,8 @@ warn_redundant_casts = true
warn_unreachable = true
warn_unused_ignores = true

plugins = ["pydantic.mypy"]

[tool.bandit]
# Ignore audit logging test file since test audit logging requires a lot of operations that trigger bandit warnings
exclude_dirs = ["./tests/src/logging/test_audit.py"]
Expand All @@ -98,9 +101,7 @@ disallow_untyped_defs = false
# When a library has addressed its deprecation issues and we've updated the
# library, we can remove the ignore filter for that library.
filterwarnings = [
"ignore::DeprecationWarning:botocore.*",
"ignore::DeprecationWarning:apispec.*",
"ignore::DeprecationWarning:certifi.*"] # pytest-watch errors if the closing bracket is on it's own line
"ignore::DeprecationWarning:botocore.*"] # pytest-watch errors if the closing bracket is on it's own line

markers = [
"audit: mark a test as a security audit log test, to be run isolated from other tests"]
Expand Down
9 changes: 6 additions & 3 deletions app/src/adapters/db/flask_db.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ def health():
# db_client.get_connection() or db_client.get_session()
"""
from functools import wraps
from typing import Any, Callable, Concatenate, ParamSpec, TypeVar
from typing import Callable, Concatenate, ParamSpec, TypeVar

from flask import Flask, current_app

Expand Down Expand Up @@ -115,10 +115,13 @@ def fiz(db_session: db.Session, x, y, z):

def decorator(f: Callable[Concatenate[db.Session, P], T]) -> Callable[P, T]:
@wraps(f)
def wrapper(*args: Any, **kwargs: Any) -> T:
def wrapper(*args: P.args, **kwargs: P.kwargs) -> T:
with get_db(current_app, client_name=client_name).get_session() as session:
return f(session, *args, **kwargs)

return wrapper

return decorator
# mypy says the type should be something slightly different
# Callable[[Arg(Callable[[Session, **P], T], 'f')], Callable[P, T]]
# but also mentions that "Arg" is deprecated, so not sure how to make it happy
return decorator # type: ignore
4 changes: 2 additions & 2 deletions app/src/api/users/user_routes.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@


@user_blueprint.post("/v1/users")
@user_blueprint.input(user_schemas.UserSchema)
@user_blueprint.input(user_schemas.UserSchema, arg_name="user_params")
@user_blueprint.output(user_schemas.UserSchema, status_code=201)
@user_blueprint.auth_required(api_key_auth)
@flask_db.with_db_session()
Expand All @@ -32,7 +32,7 @@ def user_post(db_session: db.Session, user_params: users.CreateUserParams) -> di
# Allow partial updates. partial=true means requests that are missing
# required fields will not be rejected.
# https://marshmallow.readthedocs.io/en/stable/quickstart.html#partial-loading
@user_blueprint.input(user_schemas.UserSchema(partial=True))
@user_blueprint.input(user_schemas.UserSchema(partial=True), arg_name="patch_user_params")
@user_blueprint.output(user_schemas.UserSchema)
@user_blueprint.auth_required(api_key_auth)
@flask_db.with_db_session()
Expand Down
1 change: 0 additions & 1 deletion app/src/db/migrations/env.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@

# Initialize logging
with src.logging.init("migrations"):

# add your model's MetaData object here
# for 'autogenerate' support
# from myapp import mymodel
Expand Down
2 changes: 1 addition & 1 deletion app/src/db/models/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ class Base(DeclarativeBase):
}

def _dict(self) -> dict:
return {c.key: getattr(self, c.key) for c in inspect(self).mapper.column_attrs} # type: ignore
return {c.key: getattr(self, c.key) for c in inspect(self).mapper.column_attrs}

def for_json(self) -> dict:
json_valid_dict = {}
Expand Down
4 changes: 3 additions & 1 deletion app/src/logging/audit.py
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,9 @@ def handle_audit_event(event_name: str, args: tuple[Any, ...]) -> None:
def log_audit_event(event_name: str, args: Sequence[Any], arg_names: Sequence[str]) -> None:
"""Log a message but only log recently repeated messages at intervals."""
extra = {
f"audit.args.{arg_name}": arg for arg_name, arg in zip(arg_names, args) if arg_name != "_"
f"audit.args.{arg_name}": arg
for arg_name, arg in zip(arg_names, args, strict=True)
if arg_name != "_"
}

key = (event_name, repr(args))
Expand Down
2 changes: 1 addition & 1 deletion app/src/logging/formatters.py
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ def __init__(self, message_width: int = HUMAN_READABLE_FORMATTER_DEFAULT_MESSAGE
def format(self, record: logging.LogRecord) -> str:
message = super().format(record)
return decodelog.format_line(
datetime.utcfromtimestamp(record.created),
datetime.fromtimestamp(record.created),
record.name,
record.funcName,
record.levelname,
Expand Down
2 changes: 0 additions & 2 deletions app/src/services/users/patch_user.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ def patch_user(
user_id: str,
patch_user_params: PatchUserParams,
) -> User:

with db_session.begin():
# TODO: move this to service and/or persistence layer
user = db_session.get(User, user_id, options=[orm.selectinload(User.roles)])
Expand All @@ -50,7 +49,6 @@ def patch_user(


def _handle_role_patch(db_session: Session, user: User, request_roles: list[RoleParams]) -> None:

current_role_types = set([role.type for role in user.roles])
request_role_types = set([role["type"] for role in request_roles])

Expand Down
6 changes: 3 additions & 3 deletions app/tests/src/logging/test_audit.py
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ def test_audit_hook(
pass

assert len(caplog.records) == len(expected_records)
for record, expected_record in zip(caplog.records, expected_records):
for record, expected_record in zip(caplog.records, expected_records, strict=True):
assert record.levelname == "AUDIT"
assert_record_match(record, expected_record)

Expand All @@ -161,7 +161,7 @@ def test_os_kill(init_audit_hook, caplog: pytest.LogCaptureFixture):
]

assert len(caplog.records) == len(expected_records)
for record, expected_record in zip(caplog.records, expected_records):
for record, expected_record in zip(caplog.records, expected_records, strict=True):
assert record.levelname == "AUDIT"
assert_record_match(record, expected_record)

Expand Down Expand Up @@ -232,7 +232,7 @@ def test_repeated_audit_logs(
]

assert len(caplog.records) == len(expected_records)
for record, expected_record in zip(caplog.records, expected_records):
for record, expected_record in zip(caplog.records, expected_records, strict=True):
assert record.levelname == "AUDIT"
assert_record_match(record, expected_record)

Expand Down
2 changes: 1 addition & 1 deletion app/tests/src/logging/test_flask_logger.py
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ def test_request_lifecycle_logs(
# then the log message in the after_request handler.

assert len(caplog.records) == len(expected_extras)
for record, expected_extra in zip(caplog.records, expected_extras):
for record, expected_extra in zip(caplog.records, expected_extras, strict=True):
assert_dict_contains(record.__dict__, expected_extra)


Expand Down
1 change: 0 additions & 1 deletion app/tests/src/logging/test_logging.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ def test_init(caplog: pytest.LogCaptureFixture, monkeypatch, log_format, expecte
monkeypatch.setenv("LOG_FORMAT", log_format)

with src.logging.init("test_logging"):

records = caplog.records
assert len(records) == 2
assert re.match(
Expand Down

0 comments on commit 4252983

Please sign in to comment.