refactor: split the 50-method workflow Store interface #447

New issue

Open

opened 2026-07-25 23:31:18 +02:00 by dries · 0 comments

dries commented

2026-07-25 23:31:18 +02:00

Owner

Code-quality audit (P2). internal/workflows/service.go:389-440 — the Store interface declares 50 methods and has exactly one implementation (*state.DB).

type Store interface {
	InsertWorkflowVersion(state.WorkflowVersion) (state.WorkflowVersion, error)
	GetWorkflowVersion(string) (*state.WorkflowVersion, error)
	...
	ExhaustWorkflowRepeat(string, string, string, int64) error   // 50 methods total
}

Why it matters

This is not an abstraction boundary — it is a verbatim copy of the concrete type's surface, so it buys no decoupling. It does impose a cost: every new query forces the test fake to grow a matching no-op method, which makes unrelated PRs touch the fake and makes the fake a poor model of real store behaviour.

Suggested fix

Split by concern and have Service depend on the narrow interfaces it actually uses:

versionStore — insert/get/list/activate/deactivate.
runStore — run + node lifecycle (start/settle/skip/repeat/exhaust).
triggerStore
resourceStore — leases, budgets.
artifactStore

*state.DB satisfies all of them; tests fake only the slice they need.

Acceptance criteria

No single workflow store interface exceeds ~15 methods.
Service fields are typed to the narrow interfaces.
Existing tests pass with fakes that implement only the interfaces under test.

Effort: M.

Code-quality audit (P2). `internal/workflows/service.go:389-440` — the `Store` interface declares 50 methods and has exactly one implementation (`*state.DB`). ```go type Store interface { InsertWorkflowVersion(state.WorkflowVersion) (state.WorkflowVersion, error) GetWorkflowVersion(string) (*state.WorkflowVersion, error) ... ExhaustWorkflowRepeat(string, string, string, int64) error // 50 methods total } ``` ## Why it matters This is not an abstraction boundary — it is a verbatim copy of the concrete type's surface, so it buys no decoupling. It does impose a cost: every new query forces the test fake to grow a matching no-op method, which makes unrelated PRs touch the fake and makes the fake a poor model of real store behaviour. ## Suggested fix Split by concern and have `Service` depend on the narrow interfaces it actually uses: - `versionStore` — insert/get/list/activate/deactivate. - `runStore` — run + node lifecycle (start/settle/skip/repeat/exhaust). - `triggerStore` - `resourceStore` — leases, budgets. - `artifactStore` `*state.DB` satisfies all of them; tests fake only the slice they need. ## Acceptance criteria - [ ] No single workflow store interface exceeds ~15 methods. - [ ] `Service` fields are typed to the narrow interfaces. - [ ] Existing tests pass with fakes that implement only the interfaces under test. Effort: M.