Repository navigation
Refactor: dead code, single CRUD path, polymorphism, single-responsibility, no global state - #44
Merged
Merged
Conversation
added 30 commits
September 18, 2026 09:35
auth_service, auth_strategy, cache_handler, cache_service, crud_sync, crud_async, queryset, http_dispatcher, middleware_pipeline and route_builder are leftovers of an earlier attempt to split lightapi.py into classes. The split was never wired: register() still builds its handlers through handler_factory, and RestEndpoint still owns the CRUD. No file in the package, tests, examples or docs imports any of them, none is exported from lightapi/__init__.py, and no open PR touches them. queryset.py was only used by the two crud_* modules. They had already drifted from the live code, so reading them gave the wrong picture of how a request is handled. pyproject.toml loses only the mypy override entries that named the deleted files. swagger.py is also unimported but stays: .cursorrules lists it as frozen. Behaviour is unchanged: 327 passed, 1 xfailed, 2 xpassed, same as before the change. ruff, ruff format and mypy are clean.
swagger.py (SwaggerGenerator plus two route functions, 323 lines) is not imported by the package, tests, examples or docs, and no route ever serves it. docs/api-reference/swagger.md already tells users that v2 ships no built-in Swagger UI or OpenAPI endpoint and points them to Starlette tooling, so the module contradicted the documented behaviour. It was kept out of the previous commit because .cursorrules lists it as frozen. That list dates from 2026-03-05 and is dropped in the next commit. Tests are unchanged: 327 passed, 1 xfailed, 2 xpassed.
The list was written on 2026-03-05 for the 1-v2-full-refactor branch and never edited again. The files it froze kept changing anyway: 14 commits to lightapi.py since then, 4 to auth.py, 3 to cache.py. An agent that reads the file is told not to touch code the project itself edits every month, so the rule only blocks work and protects nothing. Removes the section and the three inline "FROZEN" notes that repeated it. The rest of the file is untouched.
When the request handlers moved to handler_factory.py the originals stayed behind: LightApi._make_collection_handler, _make_detail_handler and nine module functions (_read_body, _check_auth, the two middleware runners, the cache helpers, _wrap_dict_response). register() has used handler_factory.make_* since then, so the only callers of the old functions were each other. The copies had drifted. The dead _check_auth treated AllowAny differently from the live auth_checker.check_auth, so anyone reading the main file read the wrong rule. _validate_async_dependencies stays: run() uses it and a test imports it. Nine imports that only served the dead code go with it. The handlers doc now names the functions that really build the routes. Tests are unchanged: 327 passed, 1 xfailed, 2 xpassed.
Each of these is referenced only by itself or by another item in this list. Nothing in the package, tests, examples or docs uses them, and none is exported from lightapi/__init__.py: - cache.py: CacheBackend, CacheManager, _cache_manager, _get_redis. The live cache path is RedisCacheBackend behind get_cached, set_cached and invalidate_cache_prefix. RedisCache and BaseCache stay: they are public. - schema.py: SchemaHelper (a class that only forwarded to the four module functions), register_column_type with its empty registry, and _strip_lightapi_kwargs. - session_manager.py: EngineProtocol, AsyncEngineProtocol, EngineType, LoginValidator, clear_test_registries, and the SessionManager members is_async, get_registry_and_metadata, session, async_session, dispose. CRUD code opens its own sessions, so these were never reached. The docstrings now say what the class really does. - pagination.py: PaginatorProtocol and PaginatorFactory. The protocol described a wrap() signature that neither paginator has. - exceptions.py: MissingHandlerImplementationError. Tests are unchanged: 327 passed, 1 xfailed, 2 xpassed.
- constants.py: the HttpMethod enum (only the deleted http_dispatcher used it; the live HttpMethod is the mixin namespace in methods.py) and ten constants nothing reads: RESPONSE_KEY_ERROR, RESPONSE_KEY_ALLOWED_METHODS, RESERVED_JWT_CLAIMS, the three DEFAULT_RATE_LIMIT_*, CLEANUP_INTERVAL, DEFAULT_MODE, VALID_MODES and DEFAULT_AUTH_PATH. Checked against lightapi/, tests/, examples/ and docs/. - config.py: Config.update, Authentication.permission_value, and an Authentication.__post_init__ whose body was `pass`. No caller, and not shown in any doc or example. - rest.py: _test_isolation_table_name was written on every endpoint class and never read. The try/except around `from sqlalchemy import Uuid` could never fail: the dependency floor is SQLAlchemy 2.0.30 and Uuid is present there (checked on 2.0.30 and 2.0.40), so the PG_UUID fallback was unreachable. Tests are unchanged: 327 passed, 1 xfailed, 2 xpassed.
pyproject.toml still carried a ruff per-file-ignore and a mypy ignore_errors entry for base_endpoint, database, handlers and models. Those four files were deleted in e2faf25 ("Remove legacy features and fix documentation"), so the entries matched nothing. Only the [tool.ruff] and [tool.mypy] tables change. ruff and mypy give the same result as before.
RestEndpoint carried every CRUD method twice: list, retrieve, create, update and destroy, and again as _list_async ... _destroy_async, copied line by line with `await` added. update alone was 107 lines, twice. Any fix had to be made in both copies. The logic now lives once in _list, _retrieve, _create, _update and _destroy, which take an open sync Session. The public methods are thin: - list/retrieve/create/update/destroy open a sync session (get_sync_session) and call the core. - the _*_async helpers call the same core through AsyncSession.run_sync, which is SQLAlchemy's supported way to run sync ORM code on an async driver. Names and signatures do not change. The _*_async helpers and paginate_async stay because the README, docs/advanced/async.md and three examples tell users to call them. paginate_async now delegates to paginate the same way; it had no test, so it gets one. Checked on aiosqlite: the five test_async_* files and test_background_tasks pass through the new path (insert with refresh, update with rowcount, delete with RETURNING, both paginators, async queryset, reflection). Not checked on asyncpg: no Postgres here. Tests: 329 passed (327 + 2 new), 1 xfailed, 2 xpassed.
With a sync Engine the route handlers are `async def` but called endpoint.list(), create(), update() and the rest directly, so every database round trip ran on the event loop thread. One slow query stalled all other requests on that worker. The Redis cache calls had the same problem. The sync branches now go through session.run_blocking, which uses Starlette's run_in_threadpool. Cache invalidation goes the same way and is skipped for GET, which never invalidates. Engines on StaticPool or SingletonThreadPool keep running in place. Those are the in-memory SQLite pools: SingletonThreadPool gives every thread its own empty database, and StaticPool shares one connection that is not safe across threads. The docs install snippet and most examples use them, and they would have broken under a plain thread pool. tests/test_sync_offload.py pins both cases by comparing the thread that runs the queryset with the event loop thread. It fails without this change. Tests: 331 passed, 1 xfailed, 2 xpassed.
maybe_cached and the cache invalidation lived only in the sync branches of the route handlers. An app on an AsyncEngine with `cache = Cache(...)` never read, wrote or invalidated anything, with no warning. cache_helper now splits the work into _cached_response and _store_response. maybe_cached (sync) and the new maybe_cached_async share them, so the rules exist once. The Redis client is blocking, so the async path sends those calls to a worker thread and skips the hop when the endpoint has no cache configured. invalidate_cache_after_write replaces maybe_invalidate_cache and is called for both engine kinds, which removes one `if not is_async` from each handler. tests/test_cache_async.py covers hit, miss with the stored key, body and ttl, invalidation on POST, and no invalidation on GET. Three of the four fail without this change. Tests: 335 passed, 1 xfailed, 2 xpassed.
make_collection_handler and make_detail_handler were two 80-line closures with the same prologue and epilogue and an if/elif on request.method in the middle. Each branch repeated the same three-way choice: async override, async engine, sync engine. EndpointHandler keeps that choice in one place (_serve) and describes each HTTP method as data: which user method overrides it, which CRUD method serves it on each engine kind, whether it reads a body, whether GET caching applies. Collection and detail routes are the same class with a different verb table. body_reader.py and response_wrapper.py each held one small function used only here, so they move in as private helpers. The per-request lookups and the order of steps are unchanged: pre middlewares, auth, verb, dict-to-JSON, cache invalidation, background tasks, post middlewares. Route names stay `<Class>_collection` and `<Class>_detail`. Tests: 335 passed, 1 xfailed, 2 xpassed.
The handler only used a user's get/post/put/patch/delete when it was a coroutine function. A plain `def get(self, request)` was skipped without any warning and the built-in list() ran instead, although the tutorial, the endpoints guide and the REST reference all teach the sync form. Sync overrides now run through run_blocking on a sync engine. On an async engine they run inside AsyncSession.run_sync, so an override that calls self.list() or self.create() reaches the async driver. Only real functions count as overrides: an endpoint with a column named `post` must keep working, and a test pins that. tests/test_sync_verb_override.py: three of the four tests fail without this change. Tests: 339 passed, 1 xfailed, 2 xpassed.
The framework built backends by comparing class names: `if backend.__name__ == "JWTAuthentication"` in register() and again in check_auth(). A subclass with another name fell into the `else` branch and was built with no settings. The login route was worse: it validated through a bare JWTAuthentication() with validate_credentials patched at runtime, so the subclass override the README tells users to write was never called and login answered 401. Backends now build themselves: BaseAuthentication.from_config, overridden by JWTAuthentication and BasicAuthentication. A subclass that defines its own __init__ is still built without arguments, as before. Authentication answers the questions that were asked about it from outside: permission_for(method), requires_login(method), build_backend(). EndpointGuard (was check_auth) uses them and receives the app's login_validator through its constructor. That removes the object.__setattr__ on the frozen Authentication dataclass that smuggled it in. LoginEndpoint (was login_handler plus a closure) gets the endpoint's real backend. What a successful login returns is the backend's call: login_response() gives token and user for JWT, user for Basic, which removes the has_jwt flag and four jwt_* parameters. validate_credentials may be sync or async. The login rate limits (1000/10000/100000) were written twice in lightapi.py; they are one constant now. tests/test_login_backend_subclass.py: three of six fail without the fix. The 1002-line test_login_auth.py passes unchanged. Tests: 345 passed, 1 xfailed, 2 xpassed.
_list picked the paginator with `if pagination_cfg.style == "cursor"` and then had to know that the two paginators return different tuples and take different wrap() arguments. No pagination was a third code path in the same method. Each paginator now has render(request, qs, session, serialize) -> dict, which is its own paginate() followed by its own wrap(). NoPagination is the third implementation. Pagination.build_paginator() picks the class from a table, and _list is five lines with no branch on style. paginate(), paginate_async() and wrap() keep their signatures. PageNumberPaginator() and CursorPaginator() still work with no arguments; page_size is an optional constructor argument now, which is also what docs/api-reference/pagination.md already shows. Tests: 345 passed, 1 xfailed, 2 xpassed.
The metaclass left five loose attributes on every endpoint class (_reflect_deferred, _schema_deferred, _all_columns, _reflect_partial_columns and, until recently, _test_isolation_table_name) and register() read them back with getattr(cls, "_flag", False) to decide how to map the table. The mapping itself went through TableMapper, ImperativeTableBuilder, ReflectedTableBuilder, TableNameResolver and FieldInfoStripper plus a module singleton, for two operations. The test-isolation naming rule was written twice, in register() and in TableNameResolver. The metaclass now stores one object, cls._table_source: DeclaredTable (columns from the annotated fields) or ReflectedTable (Meta.reflect). register() calls cls._table_source.map(cls, session_manager) and knows nothing else. ReflectedTable also builds the schemas, since they depend on the reflected columns. SessionManager answers what was being read from its private attributes: is_async, table_name_for(base_name) and create_tables_now(). The isolation naming rule lives only there. table_mapping.py: 239 lines to 133. register() loses 70 lines. Tests: 345 passed, 1 xfailed, 2 xpassed, including the 460-line test_reflection.py and test_async_reflection.py unchanged.
build_from_reflected_table mapped column types to Python types with a 70-line chain: isinstance checks, then __mro__ checks, then a second pass over lowercase type names, plus two try/except ImportError blocks for Uuid types that the dependency floor already guarantees. SQLAlchemy keeps that mapping itself as column.type.python_type. The function now asks for it and accepts the answer when it is one of the nine scalar types the chain knew (int, str, float, bool, Decimal, UUID, datetime, date, time). Anything else, or a type with no python_type, stays Any with the same warning, so JSON and binary columns validate as before. Checked that python_type agrees with the old chain for Integer, BigInteger, SmallInteger, String, Text, Numeric, Float, Boolean, DateTime, Date, Time, Uuid and the PostgreSQL UUID. schema.py: 302 lines to 241. The reflection suites pass unchanged. Tests: 345 passed, 1 xfailed, 2 xpassed.
RestEndpointMeta._process did seven numbered steps in one function: collect annotations, build columns, inject the auto columns, build schemas, parse Meta, scan the MRO for HTTP method mixins, pick the table source. Reading how one field becomes one column meant scrolling past the other six. Each step is a function now: _field_annotations, _declared_columns, _column_for, _auto_columns, _parsed_meta, _allowed_methods and _reject_redeclared_auto_fields. _process is the 20 lines that put them in order, and a reflected endpoint returns early instead of threading `if reflect` through every step. Three things that did nothing are gone: - cls._fields_info was stored on every endpoint and never read. - The block that removed "fields inherited from RestEndpoint itself" could not match anything: RestEndpoint only has underscore annotations, which the line above already skips. - ReflectedTable's partial_columns was always an empty list, because columns are only built when Meta.reflect is falsy. Partial reflection maps columns that already exist in the database; it never added any. Tests: 345 passed, 1 xfailed, 2 xpassed.
register() was 270 lines when this work started. After the table sources and the auth change it still validated the mapping, detected async mode, checked the engine, mapped tables, built routes and rebuilt the login routes, all inline. Now it reads as the four steps it performs. The checks are functions (_require_rest_endpoint, _has_async_handlers, _require_async_engine) and one endpoint's routes are built in _register_endpoint. The constructor delegates to _engine_from_url, _checked_mode and _as_rate_limiter. run() built its own Starlette app, a copy of build_app() with debug added. It calls build_app(debug=debug) now; debug is a new keyword-only argument with the old default. The login routes were torn out of self._routes and re-inserted on every register() call. They are composed in _asgi_routes() when the app is built: login routes, endpoint routes, /healthz, in the same order as before. The "queryset is async but engine is sync" warning is removed. It could not fire: an async queryset switches the app to async mode in the first pass, before the second pass that checked for it. lightapi.py: 910 lines on dev, 395 now. Tests: 345 passed, 1 xfailed, 2 xpassed.
_update was 100 lines that validated input, built a pydantic model, inspected the mapper, ran three SQL statements and chose between four HTTP responses. _retrieve, _create and _destroy mixed SQL and HTTP the same way. Repository(model, session) now owns storage: get, add, update, delete, the created_at/updated_at timestamps and the optimistic lock on `version`. It knows nothing about HTTP and reports problems as RowNotFound and VersionConflict. RestEndpoint validates, calls it, and turns those two errors into 404 and 409 through one table, so the status and wording of each error exist once. The PATCH schema (every field optional) was rebuilt with pydantic.create_model on every PATCH request. patch_schema() builds it once per create schema and caches it. Public CRUD methods and the _*_async helpers are untouched; they still call _retrieve, _create, _update and _destroy. tests/test_repository.py covers the lock directly: a stale version raises and leaves the row alone, a current one bumps the version. It fails if the version check or the cache is removed. rest.py: 819 lines on dev, 505 now. Tests: 350 passed, 1 xfailed, 2 xpassed.
yaml_loader.py was 705 lines doing three jobs: resolving class names, describing the YAML document as pydantic models, and turning a validated document into endpoint classes. - yaml_names.py: the built-in name registry and dotted-path resolution. - yaml_schema.py: the pydantic models and the field type map. - yaml_loader.py: the translation and load_config (358 lines). The rule "endpoint settings win, defaults fill the gaps" was written out field by field twice, once in _make_authentication and again inside the per-method branch of _build_meta_class. AuthConfig.merged_over(defaults) states it once, on the object that owns the fields. _build_meta_class was 123 lines; its per-method permission logic is now _per_method_authentication and _permission_name. The branch that handled a default permission that was neither str nor dict is gone: the pydantic model only allows those two. The one test change is an import: _resolve_name is resolve_name in yaml_names. The 491-line test_yaml_config.py is otherwise untouched. Tests: 350 passed, 1 xfailed, 2 xpassed.
SchemaFactory.build and build_from_reflected_table ended with the same twenty lines: add the four auto-managed read fields, create the two pydantic models, rebuild them. build also imported Optional, datetime and ConfigDict a second time inside the function, and set read_fields identically in both arms of an if/else. _schema_pair does that tail once. Field order is unchanged (the endpoint's fields first, then any auto field not already present), and that order is the key order of every JSON response. build: 73 lines to 40. Tests unchanged: 350 passed, 1 xfailed, 2 xpassed.
cache.py created its process-wide RedisCacheBackend as a module global. The constructor reads LIGHTAPI_REDIS_URL, so the URL was fixed to whatever the environment held before `import lightapi`. Setting it later in code, or in a test, had no effect. _default_backend() builds the client on the first cache call and keeps it. get_cached, set_cached, invalidate_cache_prefix and _ping_redis keep their names and behaviour; invalidate_cache_prefix is documented for users and the tests patch the others. tests/test_cache_backend_lazy.py mocks redis.from_url and checks the late URL is used and that three cache calls share one client. Both tests fail with the eager global. Tests: 352 passed, 1 xfailed, 2 xpassed.
from_dict is a second way to build endpoint classes from data, next to the YAML loader. It has its own 137-line builder, its own rule for class names (/api/v1/items becomes ApiV1ItemsEndpoint here and ItemsEndpoint in YAML) and, until now, no test at all. One page of the docs mentions it; no example uses it. It keeps working and now emits a DeprecationWarning that names the two replacements. The docs page and the changelog say the same. tests/test_from_dict_deprecation.py checks the warning and that the app it builds still serves a POST. Tests: 354 passed, 1 xfailed, 2 xpassed.
Three steps of the refactor plan had to be dropped half way because a name that looked private was in fact taught by the README or the docs (_list_async and friends, paginate_async, _meta as a dict). Nothing in the suite would have caught a break there. tools/differential/snapshot_public_api.py collects every `from lightapi... import ...` found in README.md, docs/ and examples/, keeps the ones that really import in the running version, and records the signatures of those classes and functions, plus the underscore helpers the docs tell users to call. tests/data/public_api.json is that snapshot taken from lightapi 0.1.29 installed from PyPI: 52 imports and 71 signatures. tests/test_public_api.py checks the working tree against it. A name may gain an optional parameter. It may not disappear, lose a parameter, reorder one, or start requiring one. The four additions made by the refactor (build_app(debug=), JWTAuthentication(extra_claims=), and page_size= on both paginators) pass because they are optional. Checked that it fails when it should: making build_app's debug required and dropping prev_cursor from CursorPaginator.wrap turns two tests red. Tests: 477 passed (354 + 123), 1 xfailed, 2 xpassed.
A refactor is supposed to change nothing a client can see, and the unit tests only check what someone thought of checking. tools/differential runs one fixed script of requests against whichever lightapi is installed and saves, for every step, the status, a few headers and the raw response text. compare.py then diffs two recordings. Raw text, not parsed JSON, because the key order of a response is part of what clients see. The script covers CRUD and optimistic locking on an in-memory and on a pooled engine, validation errors, 404/405/409, HEAD and OPTIONS, the four Serializer forms, field/search/ordering filters, page and cursor pagination, JWT and Basic auth with every permission shape, the login routes (JSON, Basic header, malformed, rate limited, custom auth_path), sync and async middleware, both CORS paths, full and partial reflection, a YAML app plus twelve invalid YAML documents, the same CRUD on an async engine with async querysets and verb overrides, background tasks, the response cache through a fake Redis, and the CREATE TABLE text of every table for SQLite and PostgreSQL. Only timestamps, JWTs, the rate-limit reset time and the order of the Allow header are normalised; the last two change with the process, not with the code. Recording the same version twice gives 412 identical steps, which is what makes a difference between two versions mean something. compare.py lists the differences the CHANGELOG declares and exits non-zero on any other. record.py refuses to run against the working tree, so it always tests an installed wheel or release, the thing users get.
The previous commit replaced the 70-line type chain with
column.type.python_type and said the two agreed. They did not. I had
compared python_type with what I expected the chain to return, not with
what it returned.
In SQLAlchemy Float, Double and REAL are subclasses of Numeric, and the
old chain tested Numeric first, so every reflected float column has
always been annotated as Decimal. python_type says float. Clients see
the difference: a Decimal is serialized as a JSON string ("0.5"), a float
as a JSON number (0.5). The old chain also returned UUID for a Uuid
column with as_uuid=False, where python_type says str.
Running the old function from dev next to the new one over 43 column
types (generic, PostgreSQL, MySQL, SQLite) showed 8 differences, all in
those two families. _reflected_annotation now answers Decimal for any
Numeric and UUID for any Uuid before asking python_type, and the same
comparison shows 0 differences.
Whether a reflected float should be a JSON number is a fair question,
but it is a change to the wire format and belongs in the changelog with
a decision behind it, not in a refactor.
tests/test_reflected_annotations.py pins 25 column types. 8 of them fail
without this fix. Found by tools/differential, which compares every
response with the published 0.1.29.
Tests: 370 passed, 1 xfailed, 2 xpassed.
While checking a fix, two comparisons in a row reported the old behaviour although the rebuilt wheel on disk had the new code. `uv run --with <local wheel>` had reused the environment it built for the previous wheel: a rebuild keeps the file name and the version, and --refresh-package did not change that. record.py now tells people to install each version into a venv of its own (with --no-cache --reinstall for a local wheel), and stores a fingerprint of the installed source in the recording. compare.py prints both fingerprints, so a B that did not change after a rebuild is easy to spot. Verified run: 0.1.29 against itself, 412 identical steps; 0.1.29 against the wheel of this branch, 406 identical, 6 different and all declared in the CHANGELOG, 0 undeclared.
…ed 500
A duplicate on a unique=True column raised IntegrityError out of
Repository.add()/update() and the request died with a 500 and a
traceback, leaving the session in the failed state. The Repository rolls
back and raises ConstraintViolation; the endpoint maps it to
409 {"detail": "constraint violation"} on POST, PUT and PATCH, sync and
async.
Planned by e2e scenario 17 (Field column options), which proves it end
to end against a real unique index on SQLite and Postgres.
The wrapper stripped decimal_places before pydantic saw it, and the column it fed, Numeric(scale=N) with no precision, renders as plain NUMERIC on SQLite and Postgres. The option did nothing: "1.999" was stored as sent on a decimal_places=2 column, while five doc tables said it set the scale. The wrapper now forwards decimal_places to pydantic, so more places than declared is a 422 at the edge. The column type is unchanged and the docs say so. Found by e2e scenario 17 (Field column options) on both databases.
…value is 409 Scenario 17 reads the schema back (sqlite3 or psql) to prove unique, index, foreign_key and exclude landed, then drives the constraint path: a duplicate sku on POST and PUT is 409, the row is untouched and writes keep working. decimal_places is proven at the edge: three places is 422, two are kept. Ids come from the responses because Postgres consumes a sequence value on the refused INSERT. 16 steps on SQLite and Postgres.
/ is a 404 on every LightAPI app, so docker reported the image as unhealthy no matter what. /healthz is the route the app registers for exactly this. e2e scenario 16 waits for `docker inspect` to say healthy.
…uest With the path fixed, e2e scenario 16 still read "unhealthy": the probe exceeded its timeout. Measured inside the image on Docker Desktop (3 runs each): import urllib.request alone takes 1.8 to 2.9 s, the request 0.18 to 0.38 s; a socket-only probe takes 0.42 to 0.53 s to import plus 0.24 to 0.26 s to answer. The 3 s timeout now keeps 3.8x headroom on this host and far more on a native Linux one.
…althy Scenario 16 builds the wheel from this checkout (uv build), builds the repository image from it through tools/e2e/Dockerfile.local (same probe and timeout, shorter interval), mounts a YAML document, drives CRUD and validation through the published port, and waits for docker inspect to report healthy. It also checks the process runs as uid 1001. Tagged docker: run_all skips it unless E2E_DOCKER=1. 7 steps.
…stgres drivers The 3.10 run failed every [pg] scenario with ModuleNotFoundError: the fresh .venv310 built from the lock with --extra dev had neither asyncpg nor psycopg2, while the 3.13 .venv had both only because they had been installed by hand outside the lock. The scripts now run with --extra async (asyncpg from the lock) and run_all refuses to start the [pg] runs until both drivers import, naming the install command for psycopg2-binary, which is in no manifest. That first uv run also builds a fresh environment before any server has to answer within its start window, which is what made 01_crud_sync time out on the first 3.10 run.
…the JSON 405 The first fix (6ad5247) put every CRUD verb on every route so the handler could answer the documented JSON 405. The whole-branch review showed what that cost: two classes registered on one path (a GET-only reader and a POST-only writer) stopped working because the first route now matched every method, and a disallowed verb reached the authentication guard (401 instead of 405 on a protected endpoint), the middlewares and the login rate limiter (a GET /auth/login used a slot). The routes list only their own methods again, as at the base, and the Starlette app registers a 405 exception handler that renders the JSON body and the sorted Allow header (HEAD left out: Starlette serves it on every GET route). /auth/login is POST-only again and answers the same body as the CRUD routes; LoginEndpoint's own 405 branch and ApiError.LOGIN_METHOD_NOT_ALLOWED are gone with it.
…refusal is logged The whole-branch review found two gaps in the constraint fix: delete() was not wrapped, so a DELETE of a row another table references died with a 500 although README and the REST reference already listed "Constraint violation (unique, foreign key) 409"; and the IntegrityError was turned into the 409 with nothing logged, which hides a NOT NULL or CHECK failure (usually a schema/table mismatch) from the operator. Repository._refused() now does the rollback, logs the table and the driver's text at INFO and builds the ConstraintViolation for add(), update() and delete(); _destroy maps it to the 409.
patch_schema() rebuilt every field as (Optional[annotation], None) and
dropped its FieldInfo, so min_length, ge, decimal_places and every other
constraint applied to POST and PUT but never to PATCH: {"name": ""}
against min_length=3 was a 200. Older than this branch; the whole-branch
review caught it because it made the new decimal_places promise untrue
on PATCH. The optional copy now keeps the FieldInfo and only replaces
the default, so a PATCH answers 422 like a PUT.
…s definition
B6-006 forwarded decimal_places to pydantic so a Decimal with more
places is a 422. The whole-branch review showed the other side: on a
float or int field, where the option used to be silently ignored,
pydantic raises TypeError ("Unable to apply constraint") at validation,
which is not a ValidationError, so every write became a 500. Same
through YAML, because decimal_places is a FIELD_CONSTRAINT_KEY.
DeclaredTable, which already knows whether the annotation is Decimal
when it picks the column type, now refuses the option on any other type
with a ConfigurationError that names the field.
Two messages this branch added printed the raw URL: the async-driver ConfigurationError (B6-001) formatted the URL as given, and the YAML load error (B2-012) printed pydantic's own text, whose input_value= holds the rejected block, so a typo like databse: dumped the database URL, password included, into whatever log collects the startup output. Found by the whole-branch review. The URL is rendered with the password masked, and the YAML error lists each problem as location and message only, which still names the key.
FieldSpec keeps extra="allow" so constraint keys can pass through, but field_info() forwards only FIELD_CONSTRAINT_KEYS: min_lenght: 3 loaded fine and the constraint silently vanished, the exact failure B2-012 fixed for every other block, in the most common place to make a typo. Found by the whole-branch review. A model validator on FieldSpec refuses any extra key outside the constraint keys and `default`, naming it, as the configuration guide promises.
… repository dist/ Scenario 03 registers a GET-only and a POST-only class on one path and proves Starlette splits them by method. Scenario 17 enables SQLite's foreign keys on every connection (Postgres always enforces them) and drives the two review gaps: a PATCH that breaks decimal_places or min_length is 422, and the DELETE of a referenced category is 409 with the row still there. Scenario 16 builds the wheel into its own directory instead of wiping the repository dist/, which may hold a release build.
README says the mode is auto-detected when omitted. It was not: an AsyncEngine, or a database URL with an async driver, left the app in sync mode, so the tables were never created and every request was a 500 (with a "Table creation skipped" warning). The docs' own async testing fixture hit it verbatim, and two tests pinned that as a known bug. An AsyncEngine, or a URL whose dialect is async (sqlite+aiosqlite, postgresql+asyncpg, also through YAML), now selects async mode. An explicit mode="sync" with either is a ConfigurationError. The two doc snippet tests now assert the examples pass.
…sed at register() The request guard calls BasicAuthentication.validate_credentials without await. An `async def` there returns a coroutine, which is truthy, so every Basic credential passed and request.state.user held a coroutine. The login route awaits the same method, which is why the README's async example (on a JWT subclass) works there. register() now refuses a BasicAuthentication backend whose validate_credentials is a coroutine function, with a ConfigurationError naming the backend. JWT backends keep theirs.
?pages=99999999999999999999 passed int() and reached the driver, which raised instead of comparing (OverflowError on SQLite): a 500 for a value no column can hold, the same class as B1-008. The 64-bit bounds the repository already used for primary keys move to lightapi.constants as INT64_MIN/INT64_MAX and the filter coercion applies them, answering the same 422 as any other value the column cannot parse.
Meta.reflect on an AsyncEngine reflects with asyncio.run() at register(), before the serving loop exists, and the connection went back to the pool bound to that loop. asyncpg refuses such a connection on the first request that draws it, the B7-003 mechanism, this time at reflection. The engine is disposed after the reflection unless its pool is a StaticPool (the one in-memory SQLite connection, which holds the database). Pinned with a file-based aiosqlite engine: the pool is empty after register() and the app serves.
…the missing field Review minors 12 and 13: Review Focus 1 was exercised through PATCH only, so scenario 01 now sends a PUT with "version": "4" too; scenario 11's "reflected patch without version" step checks that the first error names `version` instead of only being an array.
… rows for the minor fixes The README and REST reference error tables document the two 401 bodies separately (JWT answers `detail`, Basic answers `error`, each with its challenge header); three tables stop calling decimal_places a "precision"; the class diagram lists ApiError's current members. The audit records the four minor review findings fixed after the fix pass and the final run numbers.
The branch was rebased once, commit by commit, to take two credential-shaped literals out of every patch (the loopback Postgres container password in tools/e2e/run_all.sh and the dummy URL of the secrets test), so every hash the audit rows cite changed. Same subjects, same order, same trees apart from those two files.
refactor: group loose constants/functions into classes and an enum
refactor: second OO pass (enums, no loose functions, one backend per route)
test: end-to-end curl suite and the 20 library fixes it forced
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Consolidates the five stacked refactor PRs plus the validation work into a single review. No commit history was rewritten: all 40 commits from the original branches are still here, in order, for blame and partial revert.
Closes #38, #39, #40, #41, #42, #43 — folded into this one.
What changed
AsyncSession.run_sync)EndpointHandler, authfrom_config,LoginEndpoint, paginatorrender(),TableSource, reflected types from SQLAlchemy'spython_type)register/run/build_app,Repository, the YAML loader,_schema_pair)LightApi.from_dictahead of 0.2.0User-visible behavior changes
def get/post/put/patch/deleteoverride is now served. Before, onlyasync defoverrides were honored; a sync one was silently ignored.Authenticationbackend that's a subclass now uses that subclass's ownvalidate_credentials(previously fell through to the base class and returned 401).jwt_expiration) now reach it.Meta.cachenow works on async engines. On 0.1.29 it was silently ignored there.float/NumericandUuid(as_uuid=False)columns validating differently than 0.1.29) was found and fixed on the refactor: replace type checks with polymorphism (handler, auth, pagination, table mapping) #40 branch itself, before it reached here (commit67b8d78). Confirmed zero difference after the fix.Validated
59 test cases across 9 levels: static gates, the existing suite across Python 3.10-3.13 and dependency floor/ceiling, a byte-for-byte differential harness against the published 0.1.29, real Postgres/Redis integration, a real server and CLI, Docker/Helm, docs and examples, each declared behavior change, and performance. Full case-by-case table and live results: https://claude.ai/artifact/C1chg9jpoWwaJtUequp77N
Result: 50 passed, 3 failed, 5 not run, 1 skipped.
Confirmed correct, with real evidence
CREATE TABLEfor the same 45 tables is identical in SQLite and Postgres: a database created by 0.1.29 opens with this version with no migration (also proven live: booted this version against a volume seeded by 0.1.29, full CRUD round-trip, no missing-column/table errors)./healthzblocks ~2s behind apg_sleep(2)request; on this branch it answers in under 100ms. This is the core claim of refactor: one CRUD path for sync and async engines #39, now measured, not just inferred from which thread runs.kindcluster, memory stable over 10k requests.Needs a decision before merging to master
devcheckout fails the same seeds with the same counts, so it's an old ordering dependency, not a regression here.HEALTHCHECKchecks/instead of/healthz; identical on the published image. The Helm chart's own probes are correct and unaffected.from_dictremoval.Full bug list (including the two pre-existing ones above, B19/B20) and the original refactor rationale: https://claude.ai/artifact/8bvar8iWNwtNNrQodtSsXp