refactor(api): extract internal/webhooks, and stop there - #255
Merged
Merged
Conversation
internal/project is 172 Go files and 37.7k lines because it accumulated everything the session API serves, not because those things belong together. The webhook routes are the one group that can leave without dragging anything behind: nothing outside webhook_handler.go referenced a single thing it declared, the webhooks and webhook_logs models are its own, its four tests are pure unit tests over webhookJSON and the url checks that build no router, and the three Settings fields it reads (WebhookAllowedIPs, WebhookAllowedHosts, WebhookDisallowedDomains) were read by no other file in the package. Moving it is a rename plus a package clause, which is what makes it worth doing first: it proves the seam is real on the cheapest possible subject rather than on a group that would need a query engine duplicated to follow it. The small kit the handlers need — authenticated, requireWorkspaceRole and its role lookup, invalidDetail, internalError, isUniqueViolation, newUUID — is copied byte-for-byte into the new package rather than exported from internal/project. That is this repo's established pattern and not laziness: authenticated, internalError, newUUID and decodeJSON each already exist five times over across internal/space, internal/workspace, internal/externalapi, internal/project and internal/user, and the file_assets model seven times. Ninety duplicated lines buys a package that imports nothing from a 37k-line one; exporting instead would have coupled the two permanently for the same ninety lines. Only roleAdmin is copied of the three role constants, since every route here is admin only and the other two would be dead. Route behaviour is unchanged and the counts say so: internal/project's inventory test goes from 304 registrations to 297 and the new TestWebhookRouteInventory asserts the missing 7, so the composed router still serves exactly what it did. TestEveryCutOverPathIsFullyServed in internal/server is the independent check — the community Caddyfile cuts over all of /api/*, so it requires every /api/ row of the 640-row Django fixture to be served, and a webhook path lost in the move would fail it by name. apps/api/README.md records why the package exists, next to the module note it belongs to, so the next reader does not have to re-derive it from the file listing.
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.
Description
Moves the 7 webhook routes out of
internal/projectintointernal/webhooks. A proof of concept that the seam is real, not the start of a general split.internal/projectis 172 files and ~37.7k lines with 672 methods on one*Handlerserving 304 routes, and it is tempting to carve up. The analysis behind this PR says only two groups can move with zero leaked internals — webhooks (7 routes) and notifications (10) — and that every other candidate either drags 15-26 cross-boundary helpers, collides with #250, or contradicts a decisionapps/api/README.mdalready records. This is the first of the two; the second waits for #250.Zero leaked internals is achieved the way this repo already does it: copying the small helper kit rather than exporting it. That is the established pattern, not a shortcut —
authenticated,internalError,newUUIDanddecodeJSONeach exist five times across packages,liteUserJSONthree times, and thefile_assetsGORM model seven. Verified by readinginternal/userandinternal/externalapibefore copying.Type of Change
Screenshots and Media (if applicable)
N/A — backend only.
Test Scenarios
go build ./...,go vet ./...,gofmt -l .clean;go test -count=1 ./...no failures.internal/project/handler_test.go'sTestProjectRouteInventoryasserts an exact bijection withrouter.Routes(), so a lost or renamed route fails the build. The new package gets its own equivalent inventory test.git diff -M --find-renames=90%records the moves as renames rather than delete-plus-add, which is what makes this diff readable as a move.References
Corrections to the plan, verified rather than taken:
roleGuest/roleMember/roleAdmin. OnlyroleAdminis used — every webhook route isrequireWorkspaceRole(c, user, roleAdmin)— so copying the other two would plant two dead constants in a new file, in a repo with an open PR removing dead code. One constant, with a comment saying why the others are absent.webhook_handler_test.go. Doing that would drop that file below git's 90% rename-similarity threshold and destroy the rename proof the plan's own verification depends on. It lives in a newwebhooks/handler_test.go, which also matches whereworkspaceandprojectkeep theirs.Webhook*keys from theprojectapi.Settingsliteral changes which key is longest, so gofmt re-aligns the whole literal — a 25-line cosmetic diff a reviewer should expect.Not done, deliberately: extracting
internal/notificationsmust wait for #250, which rewritesproject/notification_handler.go— moving the file first turns every one of its hunks into a manual resolution in a file that no longer exists on main. The README paragraph recording where the split stops belongs after that second move; the subsection added here explains only whyinternal/webhooksexists.