Would moving route groups out of main.py help long-term maintenance?

[splitsec2]

@calibrain not a PR, but more a question about direction.

After the large number of PRs merging for 1.5.0 I was concerned about bugs slipping in as things were woven together, so I had Claude run a pass over it. Nothing significant came up (a few small PRs are pending), but the maintainability side kept coming back to main.py. It went from 3587 to 3890 lines in this release, and five of the 1.5.0 PRs added routes and helpers there (#1506, #1419, #1489, #1422/#1488, #1501). Three of those are mine, so I am part of the reason for the growth, which is why I'm asking.

The repo already has two ways of adding routes. Most of them are defined directly in main.py (41 @app.route handlers now), but seven modules register their own through register_*_routes(app, ...) (activity, requests, admin, self user, OIDC, release inspect, admin settings), and main.py hands them what they need as parameters. The question is whether we want new route groups to keep going the second way, and whether it is worth moving the five recent ones over and standardizing on it.

What I think it buys:

• Fewer overlapping changes. Two PRs that each add a route to main.py land next to each other in the same import block and the same stretch of file, and that is where rebases conflict. With one file per route group, two PRs touch two files and the only shared line is the register call.
• Review size. A reviewer sees the whole route group in one file instead of scrolling a 3900-line module for the pieces.
• The helpers that belong to one group (the release-listing gates, the zip response builder, the viewer-id helpers) stop being module globals that anything can reach.
• For people carrying a fork (which I do, so I benefit from this so it's a little self-serving), rebases on top of your releases get cheaper, because our changes live in the route modules rather than in main.py where you are also changing things.

What it costs:

• Churn with no behaviour change, and a few PRs of it. Each extraction is 100 to 300 lines moved and the tests should not change.
• Import cycles. Some of the helpers lean on orchestrator and policy objects that are module globals in main.py today. Those become parameters, which is the same shape activity_routes already uses, but it has to be done carefully.
• git blame gets a layer of "moved" on those lines.
• It is your house style. If main.py as the place for small routes is a deliberate choice, this is just more files to keep in your head.

If it sounds useful, I am willing to do the PRs to make it work, and I'd start with /api/stats since it is 35 lines and core/stats.py already imports nothing from main, so it is a clean sample of the shape. If it doesn't, no problem, I'll keep new routes wherever you prefer and we can leave main.py as it is.

[calibrain]

I completely agree, we are having lots of code fragmentation right now

Same thing with the providers, we are having more and more providers, so having some sort of plugin system would help too there

By the way, I added you as a contributor, so you should have privilege to merge PRs too

You did tremendous work on this project and I trust you !

[splitsec2]

Thanks, and thanks for the access. I'll start with /api/stats as a sample extraction so we can see the shape before moving anything bigger. On plugins: release sources and metadata providers already register through a decorator and a registry (there's a plugin guide in docs/dev), so are you picturing loading providers from outside the repo, like a plugins folder, or tidying the in-repo pattern? Happy to write up the options in a separate discussion if that helps.

[calibrain]

A bit of both, basically a folder per "source", where someone could add their own source if they want, or use our sources
Ideally in the long future, we would fork out all the "sources" from shelfmark (as to avoid any legal issues ?) and just link to a json / git for each source users wants

[calibrain]

But for that, the initial minimum is to have each source be independant and be able to just get hooked automatically by the core

[splitsec2]

On sources and plugins, I took your answer to mean a folder per source that the core picks up automatically, each one independent of the others, with the longer-term idea of taking the sources out of the repo and linking to them. I would like to treat that as a second phase after the route work (see the new comment below), so the two are not changing the same files at once. Two related things have merged since: #1566 brought the plugin guides in docs/dev up to date with the current registry pattern, and #1576 put the supported-format lookup in one place that the sources share.

When we get there, the first step would be a short write-up in a separate discussion of what ties a source to the core today (for example the list of built-in source modules that has to import each one, and the settings modules) and what the minimum for dropping a folder in and having it register would be. It would cover #1035 too.

[splitsec2]

@calibrain an update on the route work, and what I would like to do with it.

The /api/stats sample went in as #1563 (merged Oct 23). It lives in core/stats_routes.py and registers through register_stats_routes(app, ...) like the other route modules, with no route logic or test changes. main.py is about the same size since, so nothing has been shortened yet.

Moving only the groups the 1.5.0 PRs added would leave the file half done, so I would like to do all of it. main.py is 3897 lines today: 42 route handlers (about 1880 lines), 67 other top-level functions and 3 Socket.IO handlers. This is the plan for the routes, one PR per row, smallest and least coupled first:

#	New module	Routes	Handler lines	Helpers that move with it	Notes
0	stats_routes	/api/stats	35	none	Done, the sample (#1563, merged Oct 23).
1	onboarding_routes	/api/onboarding (GET, POST), /api/onboarding/skip	59	none	Smallest, a good second sample.
2	settings_routes	/api/settings, /api/settings/<tab> (GET, PUT), .../action/<key>	131	none	Calls straight into settings_registry, so little needs passing in.
3	system_routes	/api/config, /api/health, /api/debug, /api/restart, plus the static and SPA routes	194	admin_required, _serve_index_html, _base_href (about 36)	/api/config is 102 lines of its own.
4	metadata_routes	/api/metadata/ providers, config, search, field-options, book, and the three targets routes	403	_resolve_metadata_provider, _handle_target_errors, _session_may_use_provider_account (about 56)	Shares _attach_library_ownership with releases.
5	auth_routes	/api/auth/login, /logout, /check	275	the lockout helpers: record_failed_login, _get_lockout_until, is_account_locked, clear_failed_logins and three more (about 107)	Security sensitive, so I would review it twice.
6	queue_routes	cancel, retry, priority, reorder, order, active downloads, /api/status, /api/localdownload, /api/covers	429	the ownership and visibility helpers (_task_owned_by_actor, _queue_task_visible_to_actor, _resolve_viewer_download_id and others) and _zip_download_response (about 134)	These are the checks that decide who can see whose downloads.
7	release_routes	/api/releases (277 lines), /api/releases/download, /api/release-sources and its records route	386	the policy and listing helpers: _resolve_release_content_type, _resolve_policy_mode_for_current_user, _policy_block_response and the rest (about 257)	Last, since it is the biggest and most tangled.

login_required, get_auth_mode, _attach_library_ownership and _resolve_status_scope are used by more than one group. They stay in one place and are handed to the route modules as parameters, the way register_release_inspect_routes already takes login_required.

Added later: a docs PR that writes the convention down comes before any of the moves (#1622), so the rest of them match it. After row 7 come the four groups that are not routes, one PR each, with the new home still a proposal:

#	New home	What moves
8	middleware	the proxy-auth and API-key middleware
9	download_recording	_record_download_queued, _record_download_terminal_snapshot and the code around them that notifies the admin when a download finishes
10	socket_handlers	the three Socket.IO handlers
11	app_setup	the secret key, security headers and log filter

What I would hold every PR to, since this is a lot of moved code:

• No behaviour change and no edits to the existing tests.
• The full suite passing, plus ruff and basedpyright.
• The Flask URL map (every rule, its methods and its endpoint name) identical before and after, checked with a short script and shown in the PR.
• Helpers that are globals in main.py today become parameters, the way activity_routes does it, so a route module never imports main. Where tests patch something on main after import, I would pass a callable like refactor: move /api/stats into core/stats_routes.py #1563 did so the patch still works.
• If a group has behaviour that no existing test covers (I would check as I go, and auth and the queue ownership checks are the likeliest), I would add a test for it in a separate commit before moving it, so the move is covered.

I would rather this take longer and be right than land quickly and pick up a subtle change on the way. If you would rather I stop after a few groups to see how it goes, say so and I will. Otherwise I will start with the first row.

[splitsec2]

Before any of the moves go up, I put the convention itself in a PR, #1622. It is one paragraph on a new docs/dev/routes.md page saying a new route group gets its own register_<group>_routes module and main.py calls it once, so the route PRs after it have something written to match, and the next contributor has somewhere to look.

I also added the four groups I had left out of the plan as rows 8 to 11 in the plan comment above (the middleware, the download recording, the Socket.IO handlers and the app setup). Rows 3 to 7 are unchanged and start next.

[splitsec2]

Row 4 is up as #1623: the eight /api/metadata/... routes now live in core/metadata_routes.py, and main.py is about 490 lines shorter. The tests went in first as their own commit (39 of them, passing on current main), and the URL map is identical before and after.

Rows 3 and 5 are being prepared at the same time, so they will touch main.py near this one and I will rebase each on whatever lands first.

[splitsec2]

Rows 3 and 5 are up too: the system routes are #1624 (config, health, debug, restart and the frontend serving routes) and the auth routes are #1625 (login, logout, check and the lockout). Each has its tests as a first commit that passes on current main, and the URL map is identical in both. The auth one is the one I would read twice.

#1623, #1624 and #1625 all edit main.py, so whichever you take first I will rebase the other two on it.

[splitsec2]

Three more are up: the queue routes (status, cancel, retry, priority, reorder, order, active downloads, local download and covers) as #1637, the Socket.IO handlers as #1638 and the app setup (secret key, session cookie, security headers and log filter) as #1639. Each has its tests as a first commit that passes on current main, and the URL map is identical. #1625 (auth) was rebased onto main after #1623 and #1624 merged.

Left after these: the release routes, the proxy-auth and API-key middleware, and the download recording.

[splitsec2]

Last three from the plan are up: #1640 (release routes), #1641 (proxy-auth and API-key middleware) and #1642 (download recording). Each has its tests first and the same checks as before, the URL map is identical. With these main.py goes from about 1800 lines to about 665 when all three are in. I'll rebase whichever lands second and third, they only touch the import block of main.py.