So long flask, and thanks for all the fish!#526
So long flask, and thanks for all the fish!#526lukasjuhrich wants to merge 31 commits intodevelopfrom
Conversation
this held back the fastapi version we can install.
use with `fastapi dev -e sipa.asgi:app`
lukasjuhrich
force-pushed
the
fastapi
branch
3 times, most recently
from
33efa50 to
ea1a42a
Compare
Or what I believe the current assumptions are
This prepares the use of the user system in fastapi. The plan will be that view functions will be able to pull `api: PycroftApi` and `user: User` directly. If necessary, we will make `user` available (after loading from session) in `request.state.user`. This would allow for a `current_user` context processor which “just works” like in flask. The sample backend will be replaced by a stub of the `PycroftAPI` itself.
See the description in the previous commit.
This change is incomplete, but mainly because I do not want to drag the dependencies through the whole chain. We will no start working “top-down” by first building a correct login, stubbing out everything which is missing dependencies, and adding them one-by-one.
This probably needs to be reverted.
lukasjuhrich
force-pushed
the
fastapi
branch
from
ea1a42a to
7ebb4ee
Compare
lukasjuhrich
force-pushed
the
fastapi
branch
from
7ebb4ee to
1b8875f
Compare
There was a problem hiding this comment.
Pull request overview
This PR migrates from Flask + Jinja to FastAPI + Jinjax, marking a significant architectural shift. The changes focus on establishing FastAPI endpoints alongside existing Flask routes, introducing type-safe configuration via Pydantic, and refactoring user models to be more testable.
Changes:
- Introduces FastAPI routers for generic, news, pages, usersuite, and features blueprints
- Migrates configuration to Pydantic-validated settings (
Settingsclass) - Refactors the
Usermodel to accept dependencies via constructor injection rather than relying on Flask globals
Reviewed changes
Copilot reviewed 38 out of 41 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/model/test_user.py | Removed legacy base user tests (entire file deleted) |
| sipa/warnings.py | Added warning infrastructure for incomplete FastAPI migration |
| sipa/utils/graph_utils.py | Added TODO for passing NonceInfo parameter |
| sipa/utils/init.py | Added type hints, replaced global config access with dependency injection for calendar/support functions |
| sipa/templates/usersuite/index.html | Updated template to use current_user.realname directly instead of .value property |
| sipa/templates/usersuite/_index_status.html | Refactored to use macro-based rendering with simplified row structure |
| sipa/templates/support-fragment.html | New fragment template for support calendar rendering |
| sipa/templates/meetingcal-fragment.html | New fragment template for meeting calendar rendering |
| sipa/templates/login.html | Simplified to use fragment-based login form |
| sipa/templates/login-fragment.html | New fragment for login form with htmx support |
| sipa/templates/hotline-fragment.html | New fragment template for hotline availability |
| sipa/templates/base.html | Updated url_for calls to use path parameter instead of filename |
| sipa/model/user.py | Removed BaseUser and related abstractions, keeping only TableRow |
| sipa/model/sample/user.py | Removed sample user implementation (entire file deleted) |
| sipa/model/sample/init.py | Removed sample datasource (entire file deleted) |
| sipa/model/pycroft/userdb.py | Refactored to dataclass with explicit dependencies instead of inheriting from BaseUserDB |
| sipa/model/pycroft/user.py | Major refactor: removed BaseUser inheritance, made User final, added dependency injection via constructor |
| sipa/model/pycroft/schema.py | Added comment documenting property usage |
| sipa/model/pycroft/api.py | Added TODOs for API improvements |
| sipa/model/pycroft/init.py | Added TODO for config migration, marked user_class as deprecated |
| sipa/model/misc.py | Converted PaymentDetails from namedtuple to dataclass with with_purpose method |
| sipa/model/init.py | Removed sample datasource from available datasources |
| sipa/mail.py | Updated to use concrete User type instead of BaseUser |
| sipa/initialization.py | Extracted init_jinja_env function for reuse in FastAPI |
| sipa/forms.py | Fixed trailing whitespace |
| sipa/fastapi.py | New file implementing FastAPI application creation and template initialization |
| sipa/deps.py | New dependency injection module for FastAPI routes |
| sipa/config/typed_config.py | New Pydantic settings implementation with validated configuration |
| sipa/blueprints/usersuite.py | Added FastAPI router alongside Flask blueprint, introduced rows_from_user helper |
| sipa/blueprints/pages.py | Added FastAPI router with stub implementation |
| sipa/blueprints/news.py | Added FastAPI router with stub implementation |
| sipa/blueprints/generic.py | Added FastAPI router, introduced HtmxRedirectResponse helper |
| sipa/blueprints/features.py | Migrated to FastAPI with dependency injection for settings |
| sipa/base.py | Marked load_user as deprecated/not implemented |
| sipa/backends/extension.py | Added deprecation warning, updated user fetching to use new API |
| sipa/backends/datasource.py | Added TODO for removing abstraction |
| sipa/asgi.py | New ASGI entry point for FastAPI |
| pyproject.toml | Updated dependencies: email-validator, recurring_ical_events, added fastapi and uvicorn-hmr |
| AGENTS.md | New file with PR review rules (pirate voice requirement) |
Comments suppressed due to low confidence (4)
sipa/utils/init.py:1
- The
TimeTableclass is defined after its first usage inget_bustimes(line 37). While Python's runtime type checking allows this, placing the definition before its usage improves code readability and follows common conventions.
sipa/utils/init.py:1 - The
MeetingInfoTypedDict is defined but never used in the visible code. Consider removing it if unused, or ensure it's actually utilized by the meeting calendar functionality.
sipa/templates/login-fragment.html:1 - The
<input>element is self-closing and should not have a closing tag. Use<input class="form-control" type="text" name="login">instead.
sipa/templates/login-fragment.html:1 - The
<input>element is self-closing and should not have a closing tag. Use<input class="form-control" type="password" name="password">instead.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # so in _some sense_ it is internally controlled, but in another it is not. | ||
| # Perhaps a conscious cast at this one point should be fine. | ||
| # TODO pass these dependencies somehow (perhaps `UserLoader`? or deferring injection until needed) | ||
| return User(user_data, api, __TODO_payment_details, __TODO_ip_mask, __TODO_engine) |
There was a problem hiding this comment.
The variable names __TODO_payment_details, __TODO_ip_mask, and __TODO_engine use dunder prefix convention which typically indicates private/magic methods. These should be renamed to _TODO_payment_details, _TODO_ip_mask, and _TODO_engine (single underscore) or proper descriptive names once implemented.
| return AnonymousUserMixin() | ||
|
|
||
| # TODO pass these dependencies somehow (perhaps `UserLoader`? or deferring injection until needed) | ||
| return User(user_data, api, __TODO_payment_details, __TODO_ip_mask, __TODO_engine) |
There was a problem hiding this comment.
The variable names __TODO_payment_details, __TODO_ip_mask, and __TODO_engine use dunder prefix convention which typically indicates private/magic methods. These should be renamed to _TODO_payment_details, _TODO_ip_mask, and _TODO_engine (single underscore) or proper descriptive names once implemented.
| raise NotAuthenticated | ||
|
|
||
| # TODO fetch user from pycroft API | ||
| # TODO allow passing UserData model directly |
There was a problem hiding this comment.
This hardcoded user data appears to be test/stub data for development. Consider adding a comment indicating this is temporary stub data for the migration, or better yet, use a dedicated fixture or mock data generator.
| # TODO allow passing UserData model directly | |
| # TODO allow passing UserData model directly | |
| # NOTE: The following UserData is hardcoded stub/test data used during the migration. | |
| # It must be replaced by real user data fetched from the Pycroft API (see TODOs above). |
| ) from e | ||
|
|
||
|
|
||
| type Mask = t.Annotated[str | None, AfterValidator(_validate_ip_mask), Field(default="10.0.7.%")] |
There was a problem hiding this comment.
The type alias uses type statement which requires Python 3.12+. Consider using TypeAlias from typing for better compatibility: Mask: t.TypeAlias = t.Annotated[str | None, AfterValidator(_validate_ip_mask), Field(default="10.0.7.%")].
| type Mask = t.Annotated[str | None, AfterValidator(_validate_ip_mask), Field(default="10.0.7.%")] | |
| Mask: t.TypeAlias = t.Annotated[str | None, AfterValidator(_validate_ip_mask), Field(default="10.0.7.%")] |
| delete_url: URL | None = None | ||
|
|
||
|
|
||
| def rows_from_user(user: User, r: Request) -> t.Iterable[Row]: |
There was a problem hiding this comment.
The rows_from_user function lacks a docstring explaining its purpose, parameters, and return value. Given its importance in rendering user data, documentation would improve maintainability.
| def rows_from_user(user: User, r: Request) -> t.Iterable[Row]: | |
| def rows_from_user(user: User, r: Request) -> t.Iterable[Row]: | |
| """Generate display rows for the usersuite overview of a given user. | |
| The returned rows are used to render key account information such as | |
| identifiers, membership status, addressing, and network-related data | |
| in the usersuite index page. | |
| :param user: The user whose account and connection data should be | |
| represented as display rows. | |
| :param r: The current HTTP request, providing request context if | |
| needed when constructing the rows. | |
| :return: An iterable of :class:`Row` instances, each describing a | |
| single labeled piece of user information for display. | |
| """ |
| csrf_token = "" | ||
|
|
||
| def months(*a, **kw): | ||
| return Markup("""<input type="number" class="form-control" value=6></input>""") |
There was a problem hiding this comment.
The FakePaymentForm is a temporary stub class with a hardcoded HTML string. The <input> element should be self-closing: use <input type="number" class="form-control" value=6> without the closing tag.
| return Markup("""<input type="number" class="form-control" value=6></input>""") | |
| return Markup("""<input type="number" class="form-control" value=6>""") |
| background: BackgroundTask | None = None, | ||
| ) -> None: | ||
| super().__init__(content=b"", status_code=status_code, headers=headers, background=background) | ||
| self.headers["HX-Location"] = quote(str(url), safe=":/%#?=@[]!$&'()*+,;") |
There was a problem hiding this comment.
The HtmxRedirectResponse class lacks a docstring explaining its purpose and the significance of the HX-Location header for htmx compatibility.
| :param app: The flask app object to register against | ||
| """ | ||
| warnings.warn( | ||
| "Use constructor with pre-onfigured datasource instead", |
There was a problem hiding this comment.
Corrected spelling of 'pre-onfigured' to 'pre-configured'.
| "Use constructor with pre-onfigured datasource instead", | |
| "Use constructor with pre-configured datasource instead", |
lukasjuhrich
force-pushed
the
fastapi
branch
from
a6b36e2 to
8e312da
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 38 out of 41 changed files in this pull request and generated 23 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return AnonymousUserMixin() | ||
|
|
||
| # TODO pass these dependencies somehow (perhaps `UserLoader`? or deferring injection until needed) | ||
| return User(user_data, api, __TODO_payment_details, __TODO_ip_mask, __TODO_engine) |
There was a problem hiding this comment.
Avast! Same issue as above, matey - the undefined placeholders __TODO_payment_details, __TODO_ip_mask, and __TODO_engine will cause a NameError at runtime. This critical bug needs fixin' before ye can fetch users by IP address.
| def render_payment_details(details: UserPaymentDetails, months, contribution: int | None = None): | ||
| contrib = contribution or current_app.config["MEMBERSHIP_CONTRIBUTION"] |
There was a problem hiding this comment.
Arr! The render_payment_details function signature changed to include a contribution parameter with a default of None, but then on line 339 ye try to use it with or current_app.config["MEMBERSHIP_CONTRIBUTION"]. This be mixin' Flask's current_app in what should be a pure function. When called from FastAPI endpoints, current_app won't be available and will raise a RuntimeError. Ye should make contribution a required parameter or pass it explicitly from the FastAPI endpoint.
| def render_payment_details(details: UserPaymentDetails, months, contribution: int | None = None): | |
| contrib = contribution or current_app.config["MEMBERSHIP_CONTRIBUTION"] | |
| def render_payment_details(details: UserPaymentDetails, months, contribution: int): | |
| contrib = contribution |
| <a href="{{ url_for('news.show').include_query_params(start=0) }}" class="dropdown-item"> | ||
| <span class="bi-skip-start-fill"></span> | ||
| {{ _("Neueste") }} | ||
| </a> | ||
| <a href="{{ url_for('news.show', end=-1) }}" class="dropdown-item"> | ||
| <a href="{{ url_for('news.show').include_query_params(end=-1) }}" class="dropdown-item"> | ||
| <span class="bi-skip-end-fill"></span> | ||
| {{ _("Älteste") }} | ||
| </a> | ||
| <a href="{{ url_for('news.show', start=0, end=-1) }}" class="dropdown-item"> | ||
| <a href="{{ url_for('news.show').include_query_params(start=0, end=-1) }}" class="dropdown-item"> |
There was a problem hiding this comment.
Shiver me timbers! Ye be usin' .include_query_params() method on the result of url_for() (lines 118, 122, 126), but Flask's url_for() returns a plain string, not a URL object with an include_query_params() method. This be a FastAPI/Starlette pattern that won't work with Flask's url_for. This'll cause an AttributeError when the template renders. Ye need to use Flask's url_for with query string parameters directly, or provide a FastAPI-compatible url_for in the Jinja globals.
| add_url: URL | None = None | ||
| edit_url: URL | None = None | ||
| delete_url: URL | None = None |
There was a problem hiding this comment.
Blimey! The Row dataclass be usin' URL type from Starlette for the add_url, edit_url, and delete_url fields (lines 147-149), but when ye look at line 175-176 where they're used, r.url_for() returns a URL object. However, in the template at sipa/templates/usersuite/_index_status.html lines 47, 53, 59, these URLs be used directly in href attributes. Starlette's URL objects should work fine when rendered, but make sure the template can handle them properly or convert to strings explicitly.
| bic=settings.payment_bic, | ||
| ), | ||
| ip_mask=settings.db_helios_ip_mask, | ||
| engine=create_engine(str(settings.db_helios_uri)), |
There was a problem hiding this comment.
Arr! Ye be creatin' a new database engine on every request here, which be a terrible waste of resources and a performance problem! Database engines should be created once at application startup and reused. This'll cause connection pool exhaustion and slow response times. Consider movin' the engine creation to application initialization and storing it in app.state or using proper dependency injection with a singleton scope.
| :param app: The flask app object to register against | ||
| """ | ||
| warnings.warn( | ||
| "Use constructor with pre-onfigured datasource instead", |
There was a problem hiding this comment.
Arr! Ye made an oopsie in the deprecation warning message - it says "pre-onfigured" instead of "pre-configured" (line 73). A simple typo that needs fixin'!
| "Use constructor with pre-onfigured datasource instead", | |
| "Use constructor with pre-configured datasource instead", |
| return self.datasource.user_class.from_ip(ip) | ||
| # TODO move closer to where it's actually used | ||
| from sipa.model.pycroft.user import fetch_by_ip | ||
| return fetch_by_ip(__TODO_API, ip) |
There was a problem hiding this comment.
Ahoy! Ye made an oopsie here, matey! The __TODO_API placeholder be undefined and will cause a NameError at runtime. This here be a critical issue that'll sink yer ship when someone tries to fetch a user by IP address. Ye need to pass the actual PycroftApi instance or inject it properly through dependency injection before this code can sail.
| return fetch_by_ip(__TODO_API, ip) | |
| pycroft_api = current_app.extensions.get("pycroft_api") | |
| if pycroft_api is None: | |
| logger.warning( | |
| "PycroftApi extension not configured; returning anonymous user for IP %s", | |
| ip, | |
| ) | |
| return AnonymousUserMixin() | |
| return fetch_by_ip(pycroft_api, ip) |
| # TODO fetch user from pycroft API | ||
| # TODO allow passing UserData model directly | ||
| user = PycroftUser( | ||
| user_data=UserData( | ||
| id=10564, | ||
| user_id="10564", | ||
| login=username, | ||
| name="Hans Franz", | ||
| status=UserStatus( | ||
| member=True, | ||
| traffic_exceeded=False, | ||
| network_access=True, | ||
| account_balanced=True, | ||
| violation=False, | ||
| ), | ||
| room="Wu3 3-43", | ||
| mail="hans.franz@agdsn.de", | ||
| mail_forwarded=False, | ||
| mail_confirmed=True, | ||
| properties=[ | ||
| "mail", | ||
| "member", | ||
| "network_access", | ||
| "sipa_login", | ||
| "userdb", | ||
| ], | ||
| traffic_history=[], | ||
| interfaces=[ | ||
| Interface(id=1, mac="00:de:ad:be:ef:00", ips=["141.30.228.39"]), | ||
| ], | ||
| finance_balance=Decimal(200), | ||
| finance_history=[], | ||
| last_finance_update=date(2020, 1, 1), | ||
| # TODO introduce properties once they can be excluded | ||
| birthdate=date(2000, 1, 1), | ||
| membership_end_date=None, | ||
| membership_begin_date=None, | ||
| wifi_password="YouShallNotPassword", | ||
| mpsk_clients=[], | ||
| ).model_dump(), | ||
| api=PycroftApi( | ||
| endpoint=str(settings.pycroft_endpoint), api_key=str(settings.pycroft_api_key) | ||
| ), |
There was a problem hiding this comment.
Ahoy! The get_user function be returning hardcoded test data instead of actually fetchin' the user from the Pycroft API. This be a placeholder implementation that won't work in production. Ye need to implement the actual API call to fetch real user data before this can be considered production-ready.
| # TODO fetch user from pycroft API | |
| # TODO allow passing UserData model directly | |
| user = PycroftUser( | |
| user_data=UserData( | |
| id=10564, | |
| user_id="10564", | |
| login=username, | |
| name="Hans Franz", | |
| status=UserStatus( | |
| member=True, | |
| traffic_exceeded=False, | |
| network_access=True, | |
| account_balanced=True, | |
| violation=False, | |
| ), | |
| room="Wu3 3-43", | |
| mail="hans.franz@agdsn.de", | |
| mail_forwarded=False, | |
| mail_confirmed=True, | |
| properties=[ | |
| "mail", | |
| "member", | |
| "network_access", | |
| "sipa_login", | |
| "userdb", | |
| ], | |
| traffic_history=[], | |
| interfaces=[ | |
| Interface(id=1, mac="00:de:ad:be:ef:00", ips=["141.30.228.39"]), | |
| ], | |
| finance_balance=Decimal(200), | |
| finance_history=[], | |
| last_finance_update=date(2020, 1, 1), | |
| # TODO introduce properties once they can be excluded | |
| birthdate=date(2000, 1, 1), | |
| membership_end_date=None, | |
| membership_begin_date=None, | |
| wifi_password="YouShallNotPassword", | |
| mpsk_clients=[], | |
| ).model_dump(), | |
| api=PycroftApi( | |
| endpoint=str(settings.pycroft_endpoint), api_key=str(settings.pycroft_api_key) | |
| ), | |
| # fetch user from pycroft API | |
| api = PycroftApi( | |
| endpoint=str(settings.pycroft_endpoint), | |
| api_key=str(settings.pycroft_api_key), | |
| ) | |
| # Retrieve user data for the authenticated username from the Pycroft API. | |
| # The API is expected to return either a dict compatible with UserData or a UserData instance. | |
| user_data_raw = api.get_user(username) | |
| if isinstance(user_data_raw, UserData): | |
| user_data = user_data_raw.model_dump() | |
| else: | |
| user_data = user_data_raw | |
| user = PycroftUser( | |
| user_data=user_data, | |
| api=api, |
| @@ -51,12 +113,12 @@ <h2 id="hot-buttons">{{ _("Häufige Aktionen") }}</h2> | |||
|
|
|||
| <div> | |||
| {% if current_user.wifi_password.raw_value %} | |||
There was a problem hiding this comment.
Avast! On line 115, ye be accessin' current_user.wifi_password.raw_value, but if ye look at the new User class in sipa/model/pycroft/user.py, the wifi_password property returns an ActiveProperty, not a plain string. However, in the new FastAPI code, current_user might not always have the .raw_value attribute depending on how it's implemented. This could cause an AttributeError. Make sure the User class implementation be consistent with template expectations.
| "jinja2 ~= 3.1.2", | ||
| "MarkupSafe ~= 2.1.1", | ||
| "recurring_ical_events ~= 1.0.2b0", | ||
| "recurring_ical_events ~= 3.8.0", |
There was a problem hiding this comment.
Shiver me timbers! Ye upgraded recurring_ical_events from ~1.0.2b0 to ~3.8.0, which be a massive version jump (from beta 1.x to stable 3.x). This likely includes API breakin' changes. Make sure to verify that the calendar fetching code in sipa/utils/__init__.py (functions like meetingcal and support_cal) still works with the new version, or ye'll have broken calendar displays!
| "recurring_ical_events ~= 3.8.0", | |
| "recurring_ical_events ~= 1.0.2b0", |
1 participant
This is going to be a big PR where I will try out switching from flask + jinja to fastapi + jinjax.
This PR however should be merged as soon as viable, meaning as soon as
Later, we can discuss things like
IcalClientand using it as a dependencyasyncroutes where sensible.Global things to migrate:
current_userCategorizedFlatPagesflashfunctionalityBlueprints to migrate: