Target.Webhook and Webhook.Targets form a reference cycle that will recurse once anything marshals a preloaded model #177

New Issue

clawbot · 2026-08-18T01:10:13+02:00

clawbot commented

2026-08-18 01:10:13 +02:00

Raised by the independent review of #174. Deliberately NOT milestoned 1.0.0 — it cannot fire today.

internal/database/model_target.go:37 gives Target a Webhook back-reference, and Webhook has Targets. That is a cycle. encoding/json does not detect cycles; it recurses until the stack dies.

It is harmless right now for two reasons that both happen to hold rather than being designed to: omitzero hides the back-reference while it stays zero, and nothing in the service marshals these models at all — /api/v1 is registered and empty. The first handler that marshals a preloaded Webhook or Target will recurse.

The reviewer described it as deliberate-looking, and it may well be — GORM needs the association for preloading. This issue is not asking to remove it, only to make the safety explicit rather than incidental.

Definition of done

Marshalling a preloaded Webhook (with Targets populated) and a preloaded Target (with Webhook populated) is either safe or impossible, and a test pins whichever it is.
If the answer is json:"-" on the back-reference, confirm GORM's preloading is unaffected — GORM does not use json tags for association mapping, but verify rather than assume, and check nothing round-trips these models through JSON in tests or fixtures.
Check the other associations in internal/database/ for the same shape while you are there, and list what you found.

Related context: #160 added json:"-" to Target.Config, APIKey.Key and Setting.Value for the adjacent reason — that TargetView is the masking barrier for the HTML path only, and the first handler to marshal a model directly would leak. This is the same "the models are not safe to marshal yet, and nothing marshals them" territory, so it may be worth doing both audits at once when someone picks up /api/v1.

Implementation requirements

Branch from next, PR based on next, single commit, title ending (closes #N).
Do not modify TODO.md (see #112).
Run make bootstrap in a fresh clone before gating — browser assets are fetched at build time, and make lint now needs Docker.
Gate on make check plus the Docker lint path with the cache defeated. All linting runs in Docker, never on the host.

Raised by the independent review of https://git.eeqj.de/sneak/webhooker/pulls/174. Deliberately NOT milestoned 1.0.0 — it cannot fire today. `internal/database/model_target.go:37` gives `Target` a `Webhook` back-reference, and `Webhook` has `Targets`. That is a cycle. `encoding/json` does not detect cycles; it recurses until the stack dies. It is harmless right now for two reasons that both happen to hold rather than being designed to: `omitzero` hides the back-reference while it stays zero, and nothing in the service marshals these models at all — `/api/v1` is registered and empty. The first handler that marshals a **preloaded** `Webhook` or `Target` will recurse. The reviewer described it as deliberate-looking, and it may well be — GORM needs the association for preloading. This issue is not asking to remove it, only to make the safety explicit rather than incidental. ## Definition of done - Marshalling a preloaded `Webhook` (with `Targets` populated) and a preloaded `Target` (with `Webhook` populated) is either safe or impossible, and a test pins whichever it is. - If the answer is `json:"-"` on the back-reference, confirm GORM's preloading is unaffected — GORM does not use json tags for association mapping, but verify rather than assume, and check nothing round-trips these models through JSON in tests or fixtures. - Check the other associations in `internal/database/` for the same shape while you are there, and list what you found. Related context: https://git.eeqj.de/sneak/webhooker/issues/160 added `json:"-"` to `Target.Config`, `APIKey.Key` and `Setting.Value` for the adjacent reason — that `TargetView` is the masking barrier for the HTML path only, and the first handler to marshal a model directly would leak. This is the same "the models are not safe to marshal yet, and nothing marshals them" territory, so it may be worth doing both audits at once when someone picks up `/api/v1`. ## Implementation requirements - Branch from `next`, PR based on `next`, single commit, title ending ` (closes #N)`. - Do not modify `TODO.md` (see https://git.eeqj.de/sneak/webhooker/issues/112). - Run `make bootstrap` in a fresh clone before gating — browser assets are fetched at build time, and `make lint` now needs Docker. - Gate on `make check` plus the Docker lint path with the cache defeated. All linting runs in Docker, never on the host.

clawbot self-assigned this 2026-08-18 01:10:13 +02:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/webhooker#177