Migrate the database layer from GORM to Ent by Aloento · Pull Request #43 · stackmon/StatusDashboard-Backend

Aloento · 2026-10-03T19:28:09Z

Replaces GORM with Ent in the storage layer. The facade keeps its signatures and domain structs; Ent entities stay inside internal/db behind mapper functions, and the migration SQL is unchanged.

Ent schemas for the five models, keeping nullable strings distinct from empty ones
Transactions and the notification outbox moved to Ent, with row-lock and visibility queries left as SQL
GORM dropped from go.mod; handler tests that asserted GORM SQL are gone (field-exposure cases now assert the payload, the rest are covered by tests/)

Verified against a restored production copy: read, write, transaction, filtered-read and outbox scenarios return the same rows and state as before.

Introduce ent/schema for Component, ComponentAttr, Incident, IncidentStatus and NotificationOutbox, matching the production DDL (serial ids, timestamp vs timestamptz, partial outbox indexes, varchar-backed type enum). Codegen is wired via go:generate with the sql/upsert and sql/versioned-migration features. GORM remains the active backend; this commit only adds the Ent layer so the DB facade can be swapped over incrementally.

Wrap the ORM transaction in a concrete Tx type so the *Tx facade methods and Publisher.PublishTx no longer expose *gorm.DB. GORM remains the backend and the generated SQL is unchanged.

…rites The facade keeps its signatures and domain structs; Ent entities stay inside internal/db behind mapper functions. Transactional, locked and outbox paths still run on GORM and move in a later step. Verified against a restored production copy: the read and write paths return the same rows and fields as before, apart from result ordering, which is now stable instead of following physical order.

The facade no longer holds a gorm handle. Transactions run on Ent through a driver that keeps edge writes on the caller's connection, and the outbox lease plus the public-visibility subqueries stay as hand-written SQL. Timestamps are set per call rather than through global hooks, so paths that previously bypassed the ORM hooks keep their behaviour. Verified against a restored production copy: the transaction, filtered-read and outbox scenarios return the same rows, ids and state as before.

The handler tests that mocked GORM statements are gone: the field-exposure cases now assert the serialized payload and the rest are covered by the integration suite. Integration helpers use database/sql, and the shared facade handle is closed per test, so the suite no longer exhausts the connection limit. Also fix ListNotificationsByStatus: Ent drops Limit(0) while SQL LIMIT 0 selects nothing, so a zero limit returns no rows and a negative one stays unbounded.

- saveIncidentFull refuses a nil start_date instead of persisting the zero time, and reconciles incident_component_relation against the loaded component set so the association is idempotent - split saveIncidentFull, modifyIncident and Enqueue so gocognit stays under the repository limit, and justify the pgx blank import for revive - statusesByIncident batches its incident_status lookup, and the unused GetOpenedIncidentsWithComponent is removed Verified with go build ./..., go vet ./..., go test ./internal/... -count 1 and golangci-lint run.

- saveIncidentFull now stamps modified_at on every save and defaults a nil created_at to now, matching the GORM BeforeSave/BeforeUpdate hooks, and mirrors both onto the returned struct - reject a nil or empty text, which the Ent schema declares NotEmpty, with ErrIncidentTextRequired in both the create and full-save paths Verified with go build ./..., go vet ./..., go test ./internal/... -count 1 and golangci-lint run.

- add a go-test-acc job that uses a runner-provided postgres service and loads tests/testdata/dump_test.sql before the suite, so tests/ actually runs without an external database - fix four stale PATCH negative assertions in tests/v2_test.go and tests/v2_events_test.go: the payloads changed the event type on an incident, so the type-immutability check always fired first Verified with make test-acc against the restored fixture (142 tests pass, internal/db coverage 80.1%).

…rror - remove the unreachable impact-to-zero branch in checkPatchDataForIncident and its sentinel: validateEffectiveTypeAndImpact already rejects an incident whose effective impact is 0, so the branch could never fire - let TestV2CreateComponentAndList own the component it creates (delete before, clean up after) and assert the fixture components by membership, and point TestV2GetComponentsAvailability at fixture component 1, so a second run on the same database passes instead of tripping over residue from the first Verified by running the suite twice in a row on the same database (both pass), plus go build, go vet, go test ./internal/... and golangci-lint.

- Enqueue maps a unique-constraint failure on dedup_key to ErrNotificationDuplicate, so a duplicate that loses the check-then-insert race is no longer reported to the caller as a fatal error - notification_outbox locked_by and last_error are nillable, keeping SQL NULL distinct from an empty string; the generated Ent code is regenerated - SaveIncidentTx validates before it mutates the caller's struct, incidentsByIDs chunks its id predicate, and reconcileIncidentComponents documents that it does not create components - CI regenerates the Ent code and fails when ent/ is out of sync with the schema Verified with go build, go vet, go test ./internal/..., golangci-lint, and the integration suite twice in a row against the same database.

ecosquad-autoreview

Summary

Replaces GORM with Ent behind the existing internal/db facade. CI is green (build, lint, go-test, go-test-acc with a real postgres:15, and an ent-codegen job that enforces generated code is in sync). The architecture is sound: Ent entities stay inside internal/db, mappers (internal/db/mappers.go) keep the domain structs as the public surface, and the hand-rolled SQL is limited to the genuinely awkward parts (row locks, the correlated incident_status visibility subquery in internal/db/raw.go and noPublicStatus() in internal/db/db.go).

The transaction design in internal/db/tx.go is a good catch: sharedTxDriver forces Ent's inner per-edge transactions onto the outer *sql.Tx connection via dialect.NopTx, which is correct — a second transaction from inside a *sql.Tx is impossible in database/sql, and raw queries via rawFor stay on the same connection, so writes and the outbox enqueue are genuinely atomic.

Findings

Warning

context.Background() throughout the read/write paths (internal/db/db.go lines 190, 254, 301, 385, 389–390, 481, 519, 537, 610, 640, 653, 670, 684, 723, 736, 854–865, 922–937, 963, 1036, 1056, 1072; also internal/db/event_types.go:15, internal/db/notification_ops.go:90).
- The facade methods take no context, so every DB call is unbounded and uncancelable. API handlers (internal/api/v2/v2.go:238, 299) and the RSS feed call these on request paths; a client disconnect or timeout does not propagate to the DB, and a slow query holds a pool connection (25 max) indefinitely.
- This is pre-existing facade design, not a regression this PR introduced, but the rewrite to Ent was the natural point to fix it: add a ctx context.Context parameter to the exported methods (GetEventsWithCount, GetIncident, SaveIncident, …) and thread c.Request.Context() from the handlers (which already do this in WithTx calls at v2.go:1073). If a signature change is too invasive for this PR, at minimum use the caller's context where it's already available (GetIncident is called from middleware.go:231 inside a request).

Suggestion

statusesByIncident runs up to N separate queries for large pages (internal/db/mappers.go:127-147): chunking by 1000 ids is fine for pagination, but GetEventsWithCount with LastCount and a very large result set issues one query per 1000 incidents plus the main query, with statuses ordered by ID globally. Correct today; just flag it as a scaling limit if unbounded LastCount is ever exposed.
saveIncidentFull always sets created_at (internal/db/write.go:74-82): createdAt falls back to now when inc.CreatedAt == nil, so updating an incident whose CreatedAt wasn't loaded (e.g. a caller that builds an Incident manually) will silently overwrite created_at to the current time. The comment says "created_at defaults to now", and GORM's auto-create behavior likely had the same hole, but it would be safer to only SetCreatedAt when inc.CreatedAt != nil (i.e. treat nil as "don't touch") and let the column default apply on insert only.
GetEventsWithCount does count + select as two queries (internal/db/db.go:196-207): the predicates are rebuilt once (good), but if the same preds slice is shared and mutated elsewhere it would be a bug; as written it's fine — just noting the count query pays the cost of the correlated subquery in noPublicStatus() again.
CI test-acc job (.github/workflows/ci.yaml:53-80): the fixture is loaded with psql from the host container into the service. This works, and ON_ERROR_STOP=1 is set. No issue beyond noting the DB is not reset between the fixture load and make test-acc — if tests assert on absolute counts, a rerun against the same service instance (GitHub reuses the container across jobs in one run, not across runs) is fine in practice.
reconcileIncidentComponents (internal/db/write.go:104-148): the read-then-write on the join table is fine inside the surrounding transaction (default READ COMMITTED, and the caller holds the tx), but there's no FOR UPDATE on the incident row itself; a concurrent SaveIncident of the same incident could interleave component reconciliation (both read the same currentIDs, both write the same final state — last writer wins, but "wins" is per-call, so interleaved component sets can be lost). Pre-existing concurrency semantics with GORM too; flagging only because the PR description claims verified equivalence — equivalence holds for single-writer scenarios.

Verdict

Approve. No correctness bugs found in the diff; the one warning (no context propagation) is pre-existing facade design worth a follow-up PR.

Aloento added 5 commits October 3, 2026 18:48

Introduce db.Tx to keep *gorm.DB out of the public API

8c7f9aa

Wrap the ORM transaction in a concrete Tx type so the *Tx facade methods and Publisher.PublishTx no longer expose *gorm.DB. GORM remains the backend and the generated SQL is unchanged.

This comment was marked as resolved.

Sign in to view