refactor(installation): layer the module and push tenant filtering into SQL - #62
Merged
Merged
Conversation
…to SQL - repository.py owns every query, including the deleted_at predicate that five copy-pasted lookups each had to remember - github.py / anthropic.py are typed boundaries: InstallationToken and OrgMembership replace bare dicts, and one _translate_status_error maps GitHub statuses onto domain errors in a single place - service.py is use cases only; three near-identical lifecycle mutators collapse into one _apply_lifecycle_change - router.py is thin and builds responses through InstallationResponse.from_model instead of a hand-rolled dict Fixes found while layering: - tenant isolation was a Python list comprehension over every installation row in the database; the predicate now lives in the query - the dashboard refreshed byok_config once per installation (an N+1 sitting two lines above a comment boasting about avoiding N+1) — now a selectinload - label rules were implemented twice, in the schema and the service, with two different exception types for the same mistake; the schema is now the only copy - dead code removed: post_commit_status (no callers since the pivot) and get_default_suppression_labels (only referenced by its own test) Session queries moved to container/repository.py, which owns that table. 350 tests pass (was 317). test_service.py, test_byok_service.py and test_router.py are now free of unittest.mock; new test_github.py covers the boundary with httpx.MockTransport.
|
helPRs session created for this PR. Skill: |
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.
Second module through the layering pass, same shape as
identity: thin router → service (use cases) → repository (SQL) → typed boundaries (github.py,anthropic.py).Bugs this surfaced
get_installations_for_userselected every installation row in the database and filtered with a list comprehension. The predicate now lives in the query, so the database never returns rows the caller may not see.session.refresh(inst, ["byok_config"])once per installation. Replaced withselectinload.SuppressionLabelsRequestand again in the service — with two different exception types producing two different error envelopes for the same user mistake. The schema is now the single copy (it runs before any handler is entered).post_commit_status(no callers since the container pivot) andget_default_suppression_labels(referenced only by its own test, and never wired into installation creation, so "defaults" were never applied).Structural changes
select(...).where(id == ..., deleted_at.is_(None))blocks became repository functions; forgetting the soft-delete predicate is now impossible._build_installation_response— an HTTP response assembled as adictinside the router — becameInstallationResponse.from_model._translate_status_error.Verification
test_service.py,test_byok_service.pyandtest_router.pyno longer importunittest.mock; newtest_github.pycovers the boundary throughhttpx.MockTransportand asserts on real requests (URL, headers, body)ruffandmypyclean