Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CLAUDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -424,6 +424,7 @@ GoBricks breaks its own API surface when justified. Greenfield work uses the new
- **Dead app lifecycle surface removed (ADR-067):** `MessagingInitializer` and `ConnectionPreWarmer` (constructors and methods included), `Options.Database` and `Options.MessagingClient` are gone; the eight debug response types are unexported with their JSON unchanged.
- **One delivery pipeline (ADR-068):** `messaging.StartConsumeSpan` is removed — a service driving its own consume loop starts its own span — and the AMQP `messaging.client.consumed.messages` counter is recorded at completion with `error.type` instead of at receive without it.
- **Scheduler timeouts normalize (ADR-075):** `scheduler.timeout.shutdown`/`slowjob` default in `config.Validate` (30s/25s — hand-built configs move 30s → 25s), negatives fail validation, and `scheduler.Module.Init` requires a NORMALIZED `deps.Config` — non-nil, both timeouts positive — so a config assembled outside app construction must go through `config.Validate` first.
- **Section-qualified config errors (ADR-076):** a non-root database section's `ConfigError.Field` names that section (`databases.reporting.host`, `multitenant.tenants.acme.database.host`); match with a database-scoped predicate, not equality and not a bare suffix — `cache.redis.host` ends in `.host` too. Root and the connect door keep the root spelling.

## File Organization

Expand Down
48 changes: 44 additions & 4 deletions config/database_section.go
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
package config

import (
"errors"
"fmt"
"maps"
"slices"
"strings"
)

// dbPlacement is where a database section sits in the configuration tree. It
Expand Down Expand Up @@ -145,10 +147,7 @@ func normalizeDatabaseSection(db *DatabaseConfig, section dbSection) error {
}

if err := normalizeDatabaseValues(db, dbStrictnessStartup); err != nil {
if section.placement == dbPlacementRoot {
return err
}
return fmt.Errorf("%s: %w", section.path, err)
return section.qualify(err)
}

if section.placement != dbPlacementRoot && db.Manager.isSet() {
Expand All @@ -162,6 +161,47 @@ func normalizeDatabaseSection(db *DatabaseConfig, section dbSection) error {
return nil
}

// qualify re-addresses an error raised against the root spelling to this section, so a
// consumer matching on ConfigError.Field learns WHICH section failed. The root section
// returns the error untouched; every other placement gets a rewritten copy — the original
// is left alone because normalization shares its error constructors with the connect door,
// which has no section path to speak of.
//
// The path is carried by Field alone, never also by a wrapping message: printing it in both
// places is how the same section path ends up in one error twice.
//
// Action is deliberately left as the root spelling names it. Rewriting operator hints is a
// different problem from addressing an error, and it is tracked separately — see ADR-076.
func (s dbSection) qualify(err error) error {
if s.placement == dbPlacementRoot {
return err
}
var cfgErr *ConfigError
if !errors.As(err, &cfgErr) {
return fmt.Errorf("%s: %w", s.path, err)
}
qualified := *cfgErr
qualified.Field = s.qualifyField(cfgErr.Field)
qualified.Details = slices.Clone(cfgErr.Details)
return &qualified
}

// qualifyField rewrites one root-spelled field to this section. A key under the root
// section swaps its "database" head for the section path, so "database.host" reads
// "databases.reporting.host" and the tenant spelling keeps its own trailing ".database".
// A field that is not key-shaped — the Oracle connection-identifier check names one — is
// prefixed instead, which keeps the offending name rather than dropping it.
func (s dbSection) qualifyField(field string) string {
switch {
case field == "" || field == fieldDatabase:
return s.path
case strings.HasPrefix(field, fieldDatabase+"."):
return s.path + strings.TrimPrefix(field, fieldDatabase)
default:
return s.path + "." + field
}
}

// forEachDatabaseSection visits every database section the deployment
// consumes: the root, each databases.* entry, and — only when multitenancy is
// enabled, since a leftover tenants block is inert otherwise — each static
Expand Down
154 changes: 152 additions & 2 deletions config/database_section_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package config

import (
"errors"
"strings"
"testing"

"github.com/stretchr/testify/assert"
Expand Down Expand Up @@ -112,8 +113,6 @@ func TestNormalizeDatabaseSectionPlacementRules(t *testing.T) {
wantCategory: errCategoryInvalid,
wantField: "multitenant.tenants.t.database.manager",
},
{name: "named_normalization_error_wrapped_with_path", section: namedDatabaseSection("r"), cfg: DatabaseConfig{Type: "mysql", Host: "h"}, wantErr: "databases.r: "},
{name: "tenant_normalization_error_wrapped_with_path", section: tenantDatabaseSection("t"), cfg: DatabaseConfig{Type: "mysql", Host: "h"}, wantErr: "multitenant.tenants.t.database: "},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
Expand All @@ -134,6 +133,157 @@ func TestNormalizeDatabaseSectionPlacementRules(t *testing.T) {
}
}

// TestNormalizeDatabaseSectionQualifiesFieldWithSectionPath pins the addressing rule
// across every placement: a consumer matching on ConfigError.Field must be told WHICH
// section failed, not the root spelling of a key that belongs to another section.
func TestNormalizeDatabaseSectionQualifiesFieldWithSectionPath(t *testing.T) {
missingIdentity := DatabaseConfig{Type: PostgreSQL, Host: "h", Username: "u"}
typeConflict := DatabaseConfig{Type: Oracle, ConnectionString: "postgres://x/y"}
tlsMaterial := DatabaseConfig{
Type: PostgreSQL, Host: "h", Port: 5432, Database: "d", Username: "u",
TLS: TLSConfig{CertFile: "c.pem"},
}
// The one field the database path names that is not key-shaped.
oracleOutlier := DatabaseConfig{Type: Oracle, Host: "h", Port: 1521, Username: "u"}

tests := []struct {
name string
section dbSection
cfg DatabaseConfig
wantField string
}{
{name: "root_missing_identity", section: rootDatabaseSection(), cfg: missingIdentity, wantField: "database.port"},
{name: "root_type_conflict", section: rootDatabaseSection(), cfg: typeConflict, wantField: "database.type"},
{name: "root_tls_material", section: rootDatabaseSection(), cfg: tlsMaterial, wantField: "database.tls"},
{name: "root_oracle_outlier", section: rootDatabaseSection(), cfg: oracleOutlier, wantField: "oracle connection identifier"},

{name: "named_missing_identity", section: namedDatabaseSection("reporting"), cfg: missingIdentity, wantField: "databases.reporting.port"},
{name: "named_type_conflict", section: namedDatabaseSection("reporting"), cfg: typeConflict, wantField: "databases.reporting.type"},
{name: "named_tls_material", section: namedDatabaseSection("reporting"), cfg: tlsMaterial, wantField: "databases.reporting.tls"},
{name: "named_oracle_outlier", section: namedDatabaseSection("reporting"), cfg: oracleOutlier, wantField: "databases.reporting.oracle connection identifier"},

{name: "tenant_missing_identity", section: tenantDatabaseSection("acme"), cfg: missingIdentity, wantField: "multitenant.tenants.acme.database.port"},
{name: "tenant_type_conflict", section: tenantDatabaseSection("acme"), cfg: typeConflict, wantField: "multitenant.tenants.acme.database.type"},
{name: "tenant_tls_material", section: tenantDatabaseSection("acme"), cfg: tlsMaterial, wantField: "multitenant.tenants.acme.database.tls"},
{name: "tenant_oracle_outlier", section: tenantDatabaseSection("acme"), cfg: oracleOutlier, wantField: "multitenant.tenants.acme.database.oracle connection identifier"},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
cfg := tt.cfg

err := normalizeDatabaseSection(&cfg, tt.section)

var cfgErr *ConfigError
require.ErrorAs(t, err, &cfgErr)
assert.Equal(t, tt.wantField, cfgErr.Field)
if tt.section.placement != dbPlacementRoot {
assert.False(t, strings.HasPrefix(err.Error(), tt.section.path+": "),
"the path is carried by Field; a wrapper prefix would print it twice")
assert.Equal(t, 1, strings.Count(err.Error(), tt.section.path),
"and it appears exactly once in the rendered error")
}
})
}
}

// TestQualifiedFieldMatchesDeliveredEmptySpelling cross-checks the two producers instead of
// trusting that two hand-typed tables agree: ADR-076's whole point is that a key has ONE
// spelling per section, and the delivered-empty check (ADR-051) is the other producer of it.
func TestQualifiedFieldMatchesDeliveredEmptySpelling(t *testing.T) {
sections := []dbSection{
rootDatabaseSection(),
namedDatabaseSection("reporting"),
tenantDatabaseSection("acme"),
}

for _, section := range sections {
t.Run(section.path, func(t *testing.T) {
for _, key := range databaseIdentityKeys {
// What validateNoDeliveredEmptyDatabase reports for this key and section.
deliveredEmpty := section.path + "." + key

// What a normalization error against the same key is re-addressed to.
qualified := section.qualifyField(fieldDatabase + "." + key)

assert.Equal(t, deliveredEmpty, qualified, "key %q must have one spelling", key)
}
})
}
}

// TestNormalizeDatabaseValuesConnectKeepsRootSpelling pins the other half of the seam:
// the connect door shares these error constructors but has no section to speak of, so it
// must keep the root spelling rather than inherit a path from the startup door.
// TestDbSectionQualifyContract exercises the rewriter directly. Its two defensive
// branches cannot be reached through normalizeDatabaseSection today — every producer on
// that path returns a *ConfigError with a "database."-shaped field — but they are the
// module's contract with a future validator, so they are pinned here rather than left as
// untested code that only looks safe.
func TestDbSectionQualifyContract(t *testing.T) {
named := namedDatabaseSection("reporting")

t.Run("root_returns_the_error_untouched", func(t *testing.T) {
original := NewInvalidFieldError("database.host", "bad", nil)

got := rootDatabaseSection().qualify(original)

assert.Same(t, original, got, "the root section has nothing to add")
})

t.Run("non_config_error_keeps_the_path_in_the_message", func(t *testing.T) {
got := named.qualify(errors.New("boom"))

var cfgErr *ConfigError
assert.False(t, errors.As(got, &cfgErr))
assert.ErrorContains(t, got, "databases.reporting: boom")
})

t.Run("original_error_is_left_alone", func(t *testing.T) {
original := NewInvalidFieldError("database.host", "bad", nil)

got := named.qualify(original)

var cfgErr *ConfigError
require.ErrorAs(t, got, &cfgErr)
assert.Equal(t, "databases.reporting.host", cfgErr.Field)
assert.Equal(t, "database.host", original.Field,
"the connect door shares these constructors and must not inherit a section path")
})

t.Run("field_shapes", func(t *testing.T) {
tests := []struct {
name string
field string
want string
}{
{name: "key_under_database", field: "database.host", want: "databases.reporting.host"},
{name: "nested_key", field: "database.pool.max.connections", want: "databases.reporting.pool.max.connections"},
{name: "bare_database", field: fieldDatabase, want: "databases.reporting"},
{name: "empty", field: "", want: "databases.reporting"},
{name: "not_key_shaped", field: "oracle connection identifier", want: "databases.reporting.oracle connection identifier"},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
assert.Equal(t, tt.want, named.qualifyField(tt.field))
})
}
})
}

func TestNormalizeDatabaseValuesConnectKeepsRootSpelling(t *testing.T) {
cfg := DatabaseConfig{
Type: PostgreSQL, Host: "h", Port: 5432, Database: "d", Username: "u",
TLS: TLSConfig{CertFile: "c.pem"},
}

err := normalizeDatabaseValues(&cfg, dbStrictnessConnect)

var cfgErr *ConfigError
require.ErrorAs(t, err, &cfgErr)
assert.Equal(t, "database.tls", cfgErr.Field)
}

func TestNormalizeDatabaseSectionRootAbsentLeavesConfigUntouched(t *testing.T) {
cfg := DatabaseConfig{}
require.NoError(t, normalizeDatabaseSection(&cfg, rootDatabaseSection()))
Expand Down
103 changes: 103 additions & 0 deletions wiki/adr_076_section_qualified_config_error_field.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
# ADR-076: A database section's errors are addressed to that section

- **Status**: Accepted
- **Date**: 2026-08-20
- **Related**: [ADR-047](adr_047_database_absence_vs_misconfiguration.md) (absence vs misconfiguration, which is what the root section's placement rule encodes) · [ADR-051](adr_051_delivered_empty_database_identity.md) (the delivered-empty check, whose key spelling this now matches)

## Context

A deployment can carry several database sections: the root `database`, any number
of `databases.<name>`, and — under multitenancy — `multitenant.tenants.<id>.database`.
They share one normalization module, and therefore one set of error constructors,
all of which name their fields in the root spelling: `database.host`,
`database.type`, `database.tls`.

At the startup door the section path was attached by wrapping —
`fmt.Errorf("%s: %w", section.path, err)` — so the rendered message read
`databases.reporting: config_missing: database.database required`. The path was in
the text, but the `*ConfigError` a consumer reaches through `errors.As` still said
`Field = "database.database"`. A consumer that switches on `Field` — to point an
operator at the offending key, to decide whether a tenant is safe to skip — could
not tell which section failed, and would act on the root section's name for a
failure in someone else's.

That also put the module at odds with its neighbour: ADR-051's delivered-empty check
already emits fully path-qualified keys for exactly these sections
(`databases.reporting.host`), and `NewNamedDatabaseError` already addresses a named
section by name. Two spellings for the same key, from the same package.

## Decision

The section path is carried by `Field`, and only by `Field`.

`normalizeDatabaseSection` — the startup door, the one place that knows both the
section path and its placement — rewrites the error it gets back before returning
it. A key under the root section swaps its `database` head for the section path, so
`database.host` becomes `databases.reporting.host` and, for a tenant, the section's
own trailing `.database` is preserved: `multitenant.tenants.acme.database.host`.
That is byte-identical to what the delivered-empty check emits for the same key.

A field that is not key-shaped is prefixed rather than rewritten. The Oracle
connection-identifier check names one (`oracle connection identifier`), and prefixing
keeps the offending name — `databases.reporting.oracle connection identifier` — where
a strict rewrite would have to drop it.

The rewrite works on a copy. The constructors are shared with the connect door
(`dbStrictnessConnect`), which resolves a config with no section path to speak of, so
mutating the original would leak a path into errors that have no section. The root
section is returned untouched, and the wrapping message is gone: the path in `Field`
is the one copy.

## Consequences

- A consumer matching `ConfigError.Field` against a literal now sees the qualified
spelling for non-root sections, so `field == "database.host"` stops matching. The
replacement has to be scoped to the database field families rather than a bare suffix
match: `Field` is not a database-only namespace, and `cache.redis.host` ends in `.host`
as well. Match `database.<key>` exactly, and require the `databases.` or
`multitenant.tenants.` prefix before accepting a suffix — C60.16 carries the predicate.
- Root-section errors are unchanged, in both `Field` and rendered text.
- The rendered message for a non-root section loses its `databases.reporting: `
prefix and gains the path inside the field instead. Log greps that pinned the old
prefix need repointing; the path is still there, spelled once.
- `Action` still carries the root env-var hint (`set DATABASE_DATABASE env var …`)
even for a named section, whose real variable is `DATABASES_REPORTING_DATABASE`. Left
alone deliberately — rewriting hint text is a different problem from addressing an
error — but the deferral has a cost worth stating: the error is now internally
inconsistent, `Field` naming one section while `Action` names the root's variable.
Before this change the wrapping prefix framed the whole error as the section's, so the
root-spelled hint read as a template. Tracked as #1114.
- The RUNTIME door stays root-spelled, so the codebase is qualified at startup only. A
dynamic `DBConfigProvider` tenant resolved through `database.DbManager` reports
`Field = "database.tls"` with the tenant key in the wrapping message, where the same
tenant declared statically now reports `multitenant.tenants.acme.database.tls`. A
consumer routing on `strings.HasPrefix(field, "multitenant.tenants.")` therefore fires
for static tenants and never for dynamic ones. That is the connect door's asymmetry
(ADR-050), which this ADR does not touch; it is tracked as #1113, together with the
three other spellings the tenant tree still uses for sibling failures.

Migration: [C60.16](migrations.md).

## Alternatives considered

**Thread the section path into every error constructor.** It would put the path at
the point each error is raised, which reads well — and it changes every call site in
the package for a property only two of the three doors have, then has to answer what
the connect door passes. The door that knows the section is the door that should say
so.

**Give `normalizeDatabaseValues` the section, so its errors are born addressed.** The
middle option between the two above: one parameter at one seam, no `errors.As`, no
prefix surgery, and `Action` would be built from the right head for free. It is the
better shape and it is deliberately not taken here, because it changes the signature
the connect door shares — the door that has no section — and this change is meant to be
readable as an addressing fix, not a re-plumbing of the module. If the runtime door is
ever qualified too, this is where to start.

**Keep the wrapper and leave `Field` alone.** The status quo: the path is in the
message, so a human reading logs is fine. It fails the consumer reading the typed
error, which is the audience `ConfigError` exists for.

**Rewrite the Oracle outlier into a key path at the same time.** Tempting, and out of
scope here: renaming a field is a separate behaviour change for anyone matching on it,
and it would ride in unannounced under an addressing change.
Loading