diff --git a/backend/cmd/server/config.go b/backend/cmd/server/config.go index 90fc0db..863ae24 100644 --- a/backend/cmd/server/config.go +++ b/backend/cmd/server/config.go @@ -15,6 +15,7 @@ import ( "github.com/OpenNSW/core/artifact/loaders/github" "github.com/OpenNSW/core/artifact/loaders/local" "github.com/OpenNSW/core/artifact/loaders/s3" + "github.com/OpenNSW/core/refid" "gopkg.in/yaml.v3" ) @@ -45,6 +46,11 @@ type Config struct { // officer may see, based on their own users.custom_data. Empty means no // rules are configured for this deployment, so scoping is a no-op. DataScopeRulesPath string + // RefIDGen declares the reference ID formats this deployment can issue + // (see github.com/OpenNSW/core/refid, and internal/taskconfig's refid + // block for how a task opts in). Empty means no format is configured, so + // no task can generate one. + RefIDGen refid.Config // Environment designates the deployment environment. It exists solely to // gate the insecure-TLS/sslmode escape hatches (see isDevEnvironment) — // unset or any value other than "development" is treated as production. @@ -73,6 +79,7 @@ type yamlConfig struct { NSW nswclient.Config `yaml:"nsw"` Authn authn.Config `yaml:"authn"` Web web.Config `yaml:"web"` + RefIDGen refid.Config `yaml:"refIDGen"` } // yamlArtifactLoaderConfig mirrors loaders.Config's shape (Type plus one @@ -236,6 +243,7 @@ func LoadConfig() (Config, error) { ConsignmentCustomDataSchemaPath: raw.ConsignmentCustomDataSchemaPath, DataScopeRulesPath: raw.DataScopeRulesPath, Environment: raw.Environment, + RefIDGen: raw.RefIDGen, DB: db, ArtifactLoader: artifactLoader.toLoadersConfig(), AllowedOrigins: allowedOrigins, diff --git a/backend/cmd/server/config_test.go b/backend/cmd/server/config_test.go index 9c74d80..b186ddd 100644 --- a/backend/cmd/server/config_test.go +++ b/backend/cmd/server/config_test.go @@ -606,3 +606,62 @@ func fieldNameSet(v any) map[string]bool { } return set } + +func TestLoadConfig_RefIDGen_Omitted(t *testing.T) { + writeConfig(t, buildConfig(t, "", "", "", "")) + + cfg, err := LoadConfig() + if err != nil { + t.Fatalf("LoadConfig: %v", err) + } + // No section means the feature is simply off — not an error. + if len(cfg.RefIDGen.Issuers) != 0 { + t.Errorf("RefIDGen.Issuers = %d, want 0 when no refIDGen section is present", len(cfg.RefIDGen.Issuers)) + } +} + +func TestLoadConfig_RefIDGen_Decoded(t *testing.T) { + writeConfig(t, buildConfig(t, "", "", "", `refIDGen: + issuers: + - issuer: NPQS + formats: + - idType: application_id + segments: + - {type: literal, value: "NPQS/"} + - {type: list, list: office_location, param: officeCode} + - {type: sequence, scopeKey: "{issuer}:{idType}:{officeCode}:{yyyyMMdd}", padding: 6} + lists: + office_location: [NPQS-KAT, SEA-CMB] +`)) + + cfg, err := LoadConfig() + if err != nil { + t.Fatalf("LoadConfig: %v", err) + } + + if len(cfg.RefIDGen.Issuers) != 1 { + t.Fatalf("RefIDGen.Issuers = %d, want 1", len(cfg.RefIDGen.Issuers)) + } + issuer := cfg.RefIDGen.Issuers[0] + if issuer.Issuer != "NPQS" { + t.Errorf("issuer = %q, want \"NPQS\"", issuer.Issuer) + } + if len(issuer.Formats) != 1 || issuer.Formats[0].IDType != "application_id" { + t.Fatalf("formats = %+v, want one application_id format", issuer.Formats) + } + segments := issuer.Formats[0].Segments + if len(segments) != 3 { + t.Fatalf("segments = %d, want 3", len(segments)) + } + // The camelCase yaml tags are the reason refid.Config can be inlined + // rather than needing a mirror struct — check the ones that would break. + if segments[2].ScopeKey != "{issuer}:{idType}:{officeCode}:{yyyyMMdd}" { + t.Errorf("scopeKey = %q, want the configured template", segments[2].ScopeKey) + } + if segments[2].Padding != 6 { + t.Errorf("padding = %d, want 6", segments[2].Padding) + } + if got := cfg.RefIDGen.Lists["office_location"]; len(got) != 2 || got[0] != "NPQS-KAT" { + t.Errorf("lists[office_location] = %v, want [NPQS-KAT SEA-CMB]", got) + } +} diff --git a/backend/cmd/server/main.go b/backend/cmd/server/main.go index 3818a80..7050022 100644 --- a/backend/cmd/server/main.go +++ b/backend/cmd/server/main.go @@ -22,6 +22,7 @@ import ( "github.com/OpenNSW/agency/backend/internal/logging" "github.com/OpenNSW/agency/backend/internal/nswclient" "github.com/OpenNSW/agency/backend/internal/rbac" + "github.com/OpenNSW/agency/backend/internal/refidstore" "github.com/OpenNSW/agency/backend/internal/scopes" "github.com/OpenNSW/agency/backend/internal/storage" "github.com/OpenNSW/agency/backend/internal/user" @@ -29,7 +30,9 @@ import ( "github.com/OpenNSW/core/artifact" "github.com/OpenNSW/core/artifact/loaders" "github.com/OpenNSW/core/authz" + "github.com/OpenNSW/core/refid" "github.com/OpenNSW/core/trace" + "gorm.io/gorm" ) func main() { @@ -159,8 +162,13 @@ func main() { consignmentService := consignment.NewService(consignmentStore, nswClient, dataScopeResolver) consignmentHandler := consignment.NewHandler(consignmentService) + refIDs, err := initRefIDs(cfg.RefIDGen, store.DB()) + if err != nil { + log.Fatalf("failed to initialize reference ID generation: %v", err) + } + // Initialize Agency service - service := application.NewService(store, artifactRegistry, nswClient, roleService, consignmentService, dataScopeResolver) + service := application.NewService(store, artifactRegistry, nswClient, roleService, consignmentService, dataScopeResolver, refIDs) defer func() { if err := service.Close(); err != nil { slog.Error("failed to close service", "error", err) @@ -325,3 +333,29 @@ func main() { slog.Info("NSW Agency service stopped") } + +// initRefIDs builds the reference ID registry for this deployment. +// +// The feature is optional: with no refIDGen section there is nothing to build, +// so the counter store and registry are skipped and a disabled registry stands +// in — a task declaring refid then fails its inject rather than silently +// generating nothing. NewRegistry validates every configured format up front, +// so a malformed section fails the boot rather than the first inject. +func initRefIDs(cfg refid.Config, db *gorm.DB) (refid.Registry, error) { + if len(cfg.Issuers) == 0 { + slog.Info("reference ID generation not configured; tasks declaring a refid block will fail at inject") + return refidstore.Disabled(), nil + } + + sequences, err := refidstore.New(db) + if err != nil { + return nil, fmt.Errorf("creating refid sequence store: %w", err) + } + registry, err := refid.NewRegistry(cfg, sequences) + if err != nil { + return nil, fmt.Errorf("invalid refIDGen config: %w", err) + } + + slog.Info("reference ID generation configured", "issuers", len(cfg.Issuers)) + return registry, nil +} diff --git a/backend/cmd/server/refid_test.go b/backend/cmd/server/refid_test.go new file mode 100644 index 0000000..e726c7b --- /dev/null +++ b/backend/cmd/server/refid_test.go @@ -0,0 +1,79 @@ +package main + +import ( + "context" + "errors" + "path/filepath" + "testing" + + "github.com/OpenNSW/core/refid" + "github.com/glebarez/sqlite" + "gorm.io/gorm" + "gorm.io/gorm/logger" +) + +// TestInitRefIDs_NotConfigured passes a nil *gorm.DB on purpose: with no +// refIDGen section the database must never be reached, so a nil handle is +// safe. Generate still fails, which is what makes a task declaring refid on +// such a deployment a loud misconfiguration rather than a silent no-op. +func TestInitRefIDs_NotConfigured(t *testing.T) { + registry, err := initRefIDs(refid.Config{}, nil) + if err != nil { + t.Fatalf("initRefIDs with no issuers: %v", err) + } + if registry == nil { + t.Fatal("initRefIDs returned a nil Registry; NewService panics on nil dependencies") + } + if _, err := registry.Generate(context.Background(), "NPQS", "application_id", nil); !errors.Is(err, refid.ErrUnknownIssuer) { + t.Errorf("Generate returned %v, want an error wrapping refid.ErrUnknownIssuer", err) + } +} + +func TestInitRefIDs_Configured(t *testing.T) { + db, err := gorm.Open(sqlite.Open(filepath.Join(t.TempDir(), "t.db")), &gorm.Config{ + Logger: logger.Default.LogMode(logger.Silent), + }) + if err != nil { + t.Fatalf("failed to open sqlite: %v", err) + } + + registry, err := initRefIDs(refid.Config{ + Issuers: []refid.IssuerConfig{{ + Issuer: "NPQS", + Formats: []refid.FormatConfig{{ + IDType: "application_id", + Segments: []refid.SegmentConfig{{Type: "literal", Value: "NPQS/"}}, + }}, + }}, + }, db) + if err != nil { + t.Fatalf("initRefIDs: %v", err) + } + if _, err := registry.Generate(context.Background(), "NPQS", "application_id", nil); err != nil { + t.Fatalf("Generate: %v", err) + } +} + +// TestInitRefIDs_MalformedConfig pins the fail-at-boot behaviour: a bad format +// must be rejected here rather than at the first inject that needs it. +func TestInitRefIDs_MalformedConfig(t *testing.T) { + db, err := gorm.Open(sqlite.Open(filepath.Join(t.TempDir(), "t.db")), &gorm.Config{ + Logger: logger.Default.LogMode(logger.Silent), + }) + if err != nil { + t.Fatalf("failed to open sqlite: %v", err) + } + + // A sequence segment with no scopeKey is what NewRegistry rejects. + if _, err := initRefIDs(refid.Config{ + Issuers: []refid.IssuerConfig{{ + Issuer: "NPQS", + Formats: []refid.FormatConfig{{ + IDType: "application_id", + Segments: []refid.SegmentConfig{{Type: "sequence", Padding: 6}}, + }}, + }}, + }, db); err == nil { + t.Fatal("initRefIDs accepted a sequence segment with no scopeKey, want an error") + } +} diff --git a/backend/config.example.yaml b/backend/config.example.yaml index 4df9650..a35a713 100644 --- a/backend/config.example.yaml +++ b/backend/config.example.yaml @@ -194,3 +194,52 @@ nsw: # DEV-ONLY: skip TLS verification for the NSW token endpoint. Keep false in # production; only honored when environment: development. tokenInsecureSkipVerify: false + +# Reference ID generation (github.com/OpenNSW/core/refid) — OPTIONAL. +# +# Declares the reference ID formats this deployment can issue. Omit the whole +# section and the feature is simply off: no task can generate a reference ID. +# A task opts in via the `refid` block in its own task config, naming an +# (issuer, idType) from here plus the path to store the result at — see +# docs/task-config-reference.md. A task naming a format that isn't configured +# here fails its inject rather than quietly skipping. +# +# Each format is an ordered list of segments, concatenated at generation time: +# literal fixed text +# list a caller-supplied value, validated against a named list below +# date the current UTC time, using a Go reference-date layout +# sequence a durable zero-padded counter +# +# A sequence's scopeKey decides both what the counter is scoped to and how +# often it resets. Placeholders: {issuer}, {idType}, {yyyy}, {yyyyMM}, +# {yyyyMMdd}, and any param name the task config supplies. Including +# {yyyyMMdd} resets daily; omitting every date placeholder never resets. +# +# Counters live in the refid_sequences table — run `migrate up` before +# starting a server with this configured. +#refIDGen: +# issuers: +# - issuer: "" +# formats: +# # Produces /COL/20260904/000001 — per office, reset daily. +# - idType: application_id +# segments: +# - type: literal +# value: "/" +# - type: list +# list: office_location +# param: officeCode # the task config maps this to a JSON Pointer +# - type: literal +# value: "/" +# - type: date +# layout: "20060102" # Go reference date for YYYYMMDD +# - type: literal +# value: "/" +# - type: sequence +# scopeKey: "{issuer}:{idType}:{officeCode}:{yyyyMMdd}" +# padding: 6 # 000001 … 999999, then generation fails +# lists: +# # A list segment rejects any value not in its set, which fails the +# # inject — keep these in step with whatever the injected data actually +# # sends. +# office_location: [COL, GAL, KAN] diff --git a/backend/docs/task-config-reference.md b/backend/docs/task-config-reference.md index c66693d..781d1db 100644 --- a/backend/docs/task-config-reference.md +++ b/backend/docs/task-config-reference.md @@ -11,12 +11,15 @@ this doc focuses on the struct itself, field by field, including ```go type TaskConfig struct { - TaskCode string `json:"taskCode"` - Meta TaskMeta `json:"meta"` - Forms TaskForms `json:"forms"` - Behavior TaskBehavior `json:"behavior"` - Permissions []Permission `json:"permissions,omitempty"` - Certificate *TaskCertificate `json:"certificate,omitempty"` + SchemaVersion int `json:"schemaVersion"` + TaskCode string `json:"taskCode"` + Meta TaskMeta `json:"meta"` + Forms TaskForms `json:"forms"` + Behavior TaskBehavior `json:"behavior"` + Permissions []Permission `json:"permissions,omitempty"` + Certificate *TaskCertificate `json:"certificate,omitempty"` + ConsignmentFields []ConsignmentField `json:"consignmentFields,omitempty"` + RefID *TaskRefID `json:"refid,omitempty"` } ``` @@ -397,6 +400,65 @@ will reject the request for that application. --- +## `refid` (`*TaskRefID`, optional — nil-able) + +Declares that this task's applications get an agency-issued reference ID, +generated by [`github.com/OpenNSW/core/refid`](https://github.com/OpenNSW/core/tree/main/refid) +when the application is first injected and written into the reviewer response. +Omit it entirely for tasks that need no reference number. + +```go +type TaskRefID struct { + Issuer string `json:"issuer"` + IDType string `json:"idType"` + Path string `json:"path"` + Params map[string]string `json:"params,omitempty"` +} +``` + +```json +"refid": { + "issuer": "NPQS", + "idType": "application_id", + "path": "/reference_number", + "params": { "officeCode": "/nppo_office_location" } +} +``` + +| Field | Meaning | +| --- | --- | +| `issuer`, `idType` | Which configured format to generate. Both required. | +| `path` | JSON Pointer into the reviewer response where the ID is written. | +| `params` | The format's inputs, each a JSON Pointer into this task's **injected data**. | + +**The format itself lives in the deployment's config**, not here — `refIDGen` +in `config.yaml` (see [`config.example.yaml`](../config.example.yaml)) declares +the issuers, their segments and any controlled lists. This split is deliberate: +which numbers an agency issues is a deployment decision, while which tasks get +one is a task decision. A task naming an `(issuer, idType)` the deployment +hasn't configured **fails the inject** rather than quietly skipping — an +application with no reference ID is not something to discover later. + +`params` is what lets one task config serve every office: `officeCode` is read +off the incoming application rather than fixed, so a per-office counter needs +no config per office. `refid` ignores params a format doesn't consume, so they +can be declared generously; a param the format *does* require but which cannot +be resolved from the injected data fails the inject as a `400`. + +**Generated exactly once, on first inject.** Re-injecting an existing +application keeps the number it already has, and a trader resubmitting after a +feedback request keeps it too. + +**The review form needs a control at `path`** or the officer never sees the +number — `path` targets the same document `forms.review` binds to, surfaced by +the API as `agencyActionData`. A review form whose schema sets +`"additionalProperties": false` without declaring the field would reject the +officer's submission outright. + +Marking that control read-only is the form author's call, and is a client-side +convention only: review submissions aren't validated server-side, so it doesn't +prevent the value being changed. + ## Migration checklist for existing task configs `permissions` (non-empty, must collectively grant `VIEW`/`REVIEW`, no actions diff --git a/backend/go.mod b/backend/go.mod index f7583f3..826a374 100644 --- a/backend/go.mod +++ b/backend/go.mod @@ -7,6 +7,7 @@ require ( github.com/OpenNSW/core/authn v0.3.0 github.com/OpenNSW/core/authz v0.1.0 github.com/OpenNSW/core/httputil v0.1.0 + github.com/OpenNSW/core/refid v0.1.0 github.com/OpenNSW/core/secret v0.2.0 github.com/OpenNSW/core/trace v0.2.0 github.com/glebarez/sqlite v1.11.0 @@ -48,12 +49,10 @@ require ( github.com/jackc/puddle/v2 v2.2.2 // indirect github.com/jinzhu/inflection v1.0.0 // indirect github.com/jinzhu/now v1.1.5 // indirect - github.com/kr/text v0.2.0 // indirect github.com/mattn/go-isatty v0.0.24 // indirect github.com/mattn/go-sqlite3 v1.14.44 // indirect github.com/ncruces/go-strftime v1.0.0 // indirect github.com/remyoudompheng/bigfft v0.0.0-20230129092748-24d4a6f8daec // indirect - github.com/rogpeppe/go-internal v1.16.0 // indirect golang.org/x/sync v0.22.0 // indirect golang.org/x/sys v0.47.0 // indirect golang.org/x/text v0.41.0 // indirect diff --git a/backend/go.sum b/backend/go.sum index 9682f9e..39125c4 100644 --- a/backend/go.sum +++ b/backend/go.sum @@ -6,6 +6,8 @@ github.com/OpenNSW/core/authz v0.1.0 h1:qWM+NObLNEaG6GZ2ZnKYmtkwrtlAmEzIljfpiQw2 github.com/OpenNSW/core/authz v0.1.0/go.mod h1:/LbQLPZtCGJ4DICQvLea8XFtSCe2M3xZCO+0SxTYsog= github.com/OpenNSW/core/httputil v0.1.0 h1:IEkr9sHOh065+pubjlPShqmW3X2GPB9+wlHoeNDsGiU= github.com/OpenNSW/core/httputil v0.1.0/go.mod h1:kZyjhx9ckd54vYxH/R65KW++X2Lrvt1N13ZvhMupk7M= +github.com/OpenNSW/core/refid v0.1.0 h1:r9sKtojHPRc17+VLIlBxlKPIyzTcMvpxNdaybLXClV8= +github.com/OpenNSW/core/refid v0.1.0/go.mod h1:XAsH8UTKJU4Zu6d9WcqBUP21xqmi8afqxAOH9T2pph4= github.com/OpenNSW/core/secret v0.2.0 h1:7kxJYbVNJhN9X9/YyTUci4bv9oYKcm0nT/wBVrxBChQ= github.com/OpenNSW/core/secret v0.2.0/go.mod h1:MEkX9vnlPRVTn+JQQsb192PoY6p3hbFSOIV+FxIMrc0= github.com/OpenNSW/core/shared v0.3.0 h1:HcWcXyMdwzMfsVvo1HG2OTBYaQTRipTpLI0AFx/5Wgs= @@ -48,7 +50,6 @@ github.com/aws/aws-sdk-go-v2/service/sts v1.47.1 h1:Sv2xPnRHlThSUtVujYuUBPI/Il8s github.com/aws/aws-sdk-go-v2/service/sts v1.47.1/go.mod h1:mKo/CzaCz8qytGW70NG4vIIGAx1HXTlb5lHNkC5k3lk= github.com/aws/smithy-go v1.28.1 h1:R/nXH00c8qcfCzQVELtRw+eLQWtzv+VAIEFJ1/xxXlQ= github.com/aws/smithy-go v1.28.1/go.mod h1:YE2RhdIuDbA5E5bTdciG9KrW3+TiEONeUWCqxX9i1Fc= -github.com/creack/pty v1.1.9/go.mod h1:oKZEueFk5CKHvIhNR5MUki03XCEU+Q6VDXinZuGJ33E= github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/dustin/go-humanize v1.0.1 h1:GzkhY7T5VNhEkwH0PVJgjz+fX1rhBrR7pRT3mDkpeCY= github.com/dustin/go-humanize v1.0.1/go.mod h1:Mu1zIs6XwVuF/gI1OepvI0qD18qycQx+mFykh5fBlto= diff --git a/backend/internal/application/refid.go b/backend/internal/application/refid.go new file mode 100644 index 0000000..0d229ae --- /dev/null +++ b/backend/internal/application/refid.go @@ -0,0 +1,44 @@ +package application + +import ( + "context" + "errors" + "fmt" + + "github.com/OpenNSW/agency/backend/internal/taskconfig" + "github.com/OpenNSW/agency/backend/pkg/jsonpointer" + "github.com/OpenNSW/core/refid" +) + +// generateRefID mints this task's reference ID. +// +// A params pointer that doesn't resolve to a string is skipped rather than +// rejected: refid ignores params the configured format doesn't consume, so a +// task may declare more than any one format needs. Whether an absent value +// matters is refid's call — it returns ErrInvalidParam for a param a segment +// requires, and for an unresolved scope-key placeholder. +// +// refid.ErrInvalidParam is wrapped in ErrInvalidInjectRequest, since it means +// the injected data couldn't supply what the format needs. Every other failure +// is returned as-is. +func generateRefID(ctx context.Context, reg refid.Registry, cfg *taskconfig.TaskRefID, data map[string]any) (string, error) { + params := make(map[string]string, len(cfg.Params)) + for param, pointer := range cfg.Params { + value, ok := jsonpointer.Get(data, pointer) + if !ok { + continue + } + if str, ok := value.(string); ok { + params[param] = str + } + } + + id, err := reg.Generate(ctx, cfg.Issuer, cfg.IDType, params) + if err != nil { + if errors.Is(err, refid.ErrInvalidParam) { + return "", fmt.Errorf("%w: %v", ErrInvalidInjectRequest, err) + } + return "", fmt.Errorf("failed to generate reference ID for issuer %q idType %q: %w", cfg.Issuer, cfg.IDType, err) + } + return id, nil +} diff --git a/backend/internal/application/service.go b/backend/internal/application/service.go index 8ab6bbd..2727f41 100644 --- a/backend/internal/application/service.go +++ b/backend/internal/application/service.go @@ -19,6 +19,7 @@ import ( "github.com/OpenNSW/agency/backend/pkg/jsonpointer" "github.com/OpenNSW/core/artifact" "github.com/OpenNSW/core/artifact/adapter/generictemplate" + "github.com/OpenNSW/core/refid" "gorm.io/gorm" ) @@ -101,7 +102,7 @@ type Application struct { TaskCode string `json:"taskCode"` ConsignmentID string `json:"consignmentId"` Data map[string]any `json:"data,omitempty"` // Data from NSW service to be rendered in the UI - AgencyActionData map[string]any `json:"agencyActionData,omitempty"` // Copy of the payload sent back to the NSW after review, for display in the UI + AgencyActionData map[string]any `json:"agencyActionData,omitempty"` // The reviewer response document: values pre-filled at inject (see refid.go), then the payload sent back to the NSW after review AllowedActions []string `json:"allowedActions,omitempty"` // Task metadata from config @@ -154,11 +155,17 @@ type service struct { roleService *rbac.RoleService consignmentService ConsignmentService dataScope *datascope.Resolver + refIDs refid.Registry } // NewService creates a new Agency service instance with database storage -func NewService(store *ApplicationStore, artifactRegistry *artifact.Registry, nsw NSWClient, roleService *rbac.RoleService, consignmentService ConsignmentService, dataScope *datascope.Resolver) Service { - if store == nil || artifactRegistry == nil || nsw == nil || roleService == nil || consignmentService == nil || dataScope == nil { +// refIDs must be non-nil even where no deployment format is configured: pass +// a registry built from an empty refid.Config, whose Generate returns +// ErrUnknownIssuer. A task declaring refid against an unconfigured deployment +// then fails its inject loudly, instead of a nil check quietly making the +// feature a no-op. +func NewService(store *ApplicationStore, artifactRegistry *artifact.Registry, nsw NSWClient, roleService *rbac.RoleService, consignmentService ConsignmentService, dataScope *datascope.Resolver, refIDs refid.Registry) Service { + if store == nil || artifactRegistry == nil || nsw == nil || roleService == nil || consignmentService == nil || dataScope == nil || refIDs == nil { panic("NewService: all dependencies must be non-nil") } return &service{ @@ -168,6 +175,7 @@ func NewService(store *ApplicationStore, artifactRegistry *artifact.Registry, ns roleService: roleService, consignmentService: consignmentService, dataScope: dataScope, + refIDs: refIDs, } } @@ -217,12 +225,27 @@ func (s *service) CreateApplication(ctx context.Context, req *InjectRequest) err if existing != nil { // CreateOrUpdate does a full-row Save, so any field left unset here // would be overwritten to NULL. Carry the claim forward so - // re-injecting an already-claimed application doesn't erase it. + // re-injecting an already-claimed application doesn't erase it, and + // the reviewer response so a re-inject keeps its reference ID. appRecord.ClaimedBy = existing.ClaimedBy appRecord.ClaimedAt = existing.ClaimedAt - } + appRecord.ReviewerResponse = existing.ReviewerResponse + } else { + // A reference ID is minted once, for a brand-new application only. + // Ahead of CreateConsignment so a failure here leaves nothing behind. + if config.RefID != nil { + id, err := generateRefID(ctx, s.refIDs, config.RefID, req.Data) + if err != nil { + return err + } + appRecord.ReviewerResponse = JSONB{} + if !jsonpointer.Set(appRecord.ReviewerResponse, config.RefID.Path, id) { + // Unreachable — Validate already checked Path. Still an error: + // dropping an issued ID would be silent loss. + return fmt.Errorf("failed to write reference ID to %q", config.RefID.Path) + } + } - if existing == nil { if err := s.consignmentService.CreateConsignment(ctx, req.ConsignmentID); err != nil { // TODO: revert application creation when inject and consignment writes share a transaction. slog.WarnContext(ctx, "failed to create consignment after application inject", @@ -230,10 +253,7 @@ func (s *service) CreateApplication(ctx context.Context, req *InjectRequest) err } } - if err := s.store.CreateOrUpdate(appRecord, pushedFields); err != nil { - return err - } - return nil + return s.store.CreateOrUpdate(appRecord, pushedFields) } // GetApplications returns a paginated list of applications. List items are diff --git a/backend/internal/application/service_test.go b/backend/internal/application/service_test.go index ea1977c..ddffd16 100644 --- a/backend/internal/application/service_test.go +++ b/backend/internal/application/service_test.go @@ -19,12 +19,15 @@ import ( "github.com/OpenNSW/agency/backend/internal/datascope" "github.com/OpenNSW/agency/backend/internal/nswclient" "github.com/OpenNSW/agency/backend/internal/rbac" + "github.com/OpenNSW/agency/backend/internal/refidstore" "github.com/OpenNSW/agency/backend/internal/taskconfig/taskconfigart" "github.com/OpenNSW/agency/backend/internal/user" "github.com/OpenNSW/agency/backend/pkg/httpclient" "github.com/OpenNSW/core/artifact" "github.com/OpenNSW/core/artifact/adapter/generictemplate" "github.com/OpenNSW/core/artifact/loaders/local" + "github.com/OpenNSW/core/refid" + "gorm.io/gorm" ) // writeTaskConfigFile writes content to /task-configs/. @@ -174,6 +177,13 @@ func newTestRegistry(t *testing.T, root string) *artifact.Registry { // /task-configs/ and /forms/ as needed. func newServiceHarness(t *testing.T, writeFn func(root string)) *serviceHarness { t.Helper() + return newServiceHarnessWithRefIDs(t, unconfiguredRefIDs(), writeFn) +} + +// newServiceHarnessWithRefIDs is newServiceHarness with an explicit reference +// ID registry, for tests exercising generation at inject. +func newServiceHarnessWithRefIDs(t *testing.T, refIDs refid.Registry, writeFn func(root string)) *serviceHarness { + t.Helper() root := t.TempDir() for _, sub := range []string{"task-configs", "forms"} { @@ -194,8 +204,7 @@ func newServiceHarness(t *testing.T, writeFn func(root string)) *serviceHarness // stub server is what routes callbacks there. hc := httpclient.NewClientBuilder().WithBaseURL(srv.URL).Build() - roleService := rbac.NewRoleService(store.db) - svc := newWiredService(t, store, reg, nswclient.NewWithClient(hc), roleService) + svc := newWiredServiceWithRefIDs(t, store, reg, nswclient.NewWithClient(hc), refIDs) return &serviceHarness{ t: t, @@ -215,7 +224,7 @@ func newWiredService(t *testing.T, store *ApplicationStore, reg *artifact.Regist if roleService == nil { roleService = rbac.NewRoleService(store.db) } - svc := NewService(store, reg, nsw, roleService, consignment.NewService(consignment.NewConsignmentStore(store.db), cNSW, unrestrictedResolver()), unrestrictedResolver()) + svc := NewService(store, reg, nsw, roleService, consignment.NewService(consignment.NewConsignmentStore(store.db), cNSW, unrestrictedResolver()), unrestrictedResolver(), unconfiguredRefIDs()) t.Cleanup(func() { _ = svc.Close() }) return svc } @@ -227,6 +236,52 @@ func unrestrictedResolver() *datascope.Resolver { return datascope.NewResolver(nil, nil) } +// stubRefIDRegistry is a refid.Registry returning a canned ID, so inject tests +// assert on what gets persisted rather than re-testing refid's own generation +// (covered against the real driver in internal/refidstore). +type stubRefIDRegistry struct { + id string + err error + calls []stubRefIDCall +} + +type stubRefIDCall struct { + issuer string + idType string + params map[string]string +} + +func (s *stubRefIDRegistry) Generate(_ context.Context, issuer, idType string, params map[string]string) (string, error) { + s.calls = append(s.calls, stubRefIDCall{issuer: issuer, idType: idType, params: params}) + if s.err != nil { + return "", s.err + } + return s.id, nil +} + +// unconfiguredRefIDs mirrors a deployment with no refIDGen section. It uses +// the same disabled registry main() wires up in that case, rather than a stub, +// so these tests exercise the real thing. Most helpers pass it, since no task +// config in these tests declares a refid block. +func unconfiguredRefIDs() refid.Registry { + return refidstore.Disabled() +} + +// newWiredServiceWithRefIDs is newWiredService with an explicit reference ID +// registry, for tests exercising generation at inject. +func newWiredServiceWithRefIDs(t *testing.T, store *ApplicationStore, reg *artifact.Registry, nsw NSWClient, refIDs refid.Registry) Service { + t.Helper() + cNSW, ok := nsw.(consignment.NSWClient) + if !ok { + t.Fatal("nsw client must implement consignment.NSWClient") + } + svc := NewService(store, reg, nsw, rbac.NewRoleService(store.db), + consignment.NewService(consignment.NewConsignmentStore(store.db), cNSW, unrestrictedResolver()), + unrestrictedResolver(), refIDs) + t.Cleanup(func() { _ = svc.Close() }) + return svc +} + // newWiredServiceWithScope is newWiredService with an explicit data-scope // resolver, for tests exercising scoped behavior. func newWiredServiceWithScope(t *testing.T, store *ApplicationStore, reg *artifact.Registry, nsw NSWClient, roleService *rbac.RoleService, resolver *datascope.Resolver) Service { @@ -238,7 +293,7 @@ func newWiredServiceWithScope(t *testing.T, store *ApplicationStore, reg *artifa if roleService == nil { roleService = rbac.NewRoleService(store.db) } - svc := NewService(store, reg, nsw, roleService, consignment.NewService(consignment.NewConsignmentStore(store.db), cNSW, resolver), resolver) + svc := NewService(store, reg, nsw, roleService, consignment.NewService(consignment.NewConsignmentStore(store.db), cNSW, resolver), resolver, unconfiguredRefIDs()) t.Cleanup(func() { _ = svc.Close() }) return svc } @@ -1898,3 +1953,371 @@ func TestCreateApplication_ConsignmentFieldsPushedOnResubmission(t *testing.T) { t.Errorf("custom_data[district] = %v, want Gampaha (resubmission must re-push updated fields)", got["district"]) } } + +// refIDTaskConfig is a task config declaring a refid block, used by the +// generation tests below. No view form, so injected data isn't schema-checked +// and each test can pass just the fields its params need. +const refIDTaskConfig = `{ + "schemaVersion": 1, + "meta": {"title": "RefID Task"}, + "permissions": [{"role": "officer", "actions": ["VIEW", "REVIEW"]}], + "forms": {"review": "refid_review"}, + "behavior": {"type": "statusMap", "statusMap": {"approve": "APPROVED"}}, + "refid": { + "issuer": "NPQS", + "idType": "application_id", + "path": "/reference_number", + "params": {"officeCode": "/nppo_office_location"} + } +}` + +func TestCreateApplication_RefID_GeneratedAndPersisted(t *testing.T) { + stub := &stubRefIDRegistry{id: "NPQS/NPQS-KAT/000042"} + h := newServiceHarnessWithRefIDs(t, stub, func(root string) { + writeTaskConfigFile(t, root, "refid_task.json", refIDTaskConfig) + }) + + if err := h.service.CreateApplication(context.Background(), &InjectRequest{ + TaskID: "t-refid-1", + TaskCode: "refid_task", + ConsignmentID: "c-refid-1", + Data: map[string]any{"nppo_office_location": "NPQS-KAT"}, + }); err != nil { + t.Fatalf("CreateApplication: %v", err) + } + + rec, err := h.store.GetByTaskID("t-refid-1") + if err != nil { + t.Fatalf("GetByTaskID: %v", err) + } + if got := rec.ReviewerResponse["reference_number"]; got != "NPQS/NPQS-KAT/000042" { + t.Fatalf("reviewer_response reference_number = %v, want the generated ID", got) + } + + // The format's params must come from the injected data, not be dropped. + if len(stub.calls) != 1 { + t.Fatalf("Generate called %d times, want 1", len(stub.calls)) + } + call := stub.calls[0] + if call.issuer != "NPQS" || call.idType != "application_id" { + t.Errorf("Generate called with (%q, %q), want (\"NPQS\", \"application_id\")", call.issuer, call.idType) + } + if call.params["officeCode"] != "NPQS-KAT" { + t.Errorf("officeCode param = %q, want \"NPQS-KAT\"", call.params["officeCode"]) + } +} + +func TestCreateApplication_RefID_ReinjectKeepsOriginalID(t *testing.T) { + stub := &stubRefIDRegistry{id: "NPQS/NPQS-KAT/000001"} + h := newServiceHarnessWithRefIDs(t, stub, func(root string) { + writeTaskConfigFile(t, root, "refid_task.json", refIDTaskConfig) + }) + + req := &InjectRequest{ + TaskID: "t-refid-2", + TaskCode: "refid_task", + ConsignmentID: "c-refid-2", + Data: map[string]any{"nppo_office_location": "NPQS-KAT"}, + } + if err := h.service.CreateApplication(context.Background(), req); err != nil { + t.Fatalf("first inject: %v", err) + } + + // A second inject must neither mint a new number nor NULL out the column + // via CreateOrUpdate's full-row Save. + stub.id = "NPQS/NPQS-KAT/999999" + if err := h.service.CreateApplication(context.Background(), req); err != nil { + t.Fatalf("re-inject: %v", err) + } + + rec, err := h.store.GetByTaskID("t-refid-2") + if err != nil { + t.Fatalf("GetByTaskID: %v", err) + } + if got := rec.ReviewerResponse["reference_number"]; got != "NPQS/NPQS-KAT/000001" { + t.Fatalf("reference_number = %v after re-inject, want the original ID", got) + } + if len(stub.calls) != 1 { + t.Fatalf("Generate called %d times across two injects, want 1", len(stub.calls)) + } +} + +// TestCreateApplication_RefID_UnconfiguredDeployment_FailsInject covers the +// fail-closed guarantee: an inject that can't mint its reference ID leaves +// nothing behind — no application, and no consignment either, which is what +// keeping generation ahead of CreateConsignment buys. +// +// It needs a mock NSW client whose consignment fetch succeeds: +// CreateConsignment fetches NSW extras before inserting, so with the default +// stub server (whose fetch fails) no consignment is created regardless of +// ordering, making that assertion vacuous. +func TestCreateApplication_RefID_UnconfiguredDeployment_FailsInject(t *testing.T) { + store := newTestStore(t) + root := t.TempDir() + mustMkdirTaskConfigsAndForms(t, root) + writeTaskConfigFile(t, root, "refid_task.json", refIDTaskConfig) + + nswMock := &mockNSWClient{consignment: &nswclient.ConsignmentAgency{ + ConsignmentID: "c-refid-3", + TraderCompanyName: "CEYLON EXPORTS", + }} + // unconfiguredRefIDs is the registry main() wires up with no refIDGen + // section, so every Generate fails. + svc := newWiredServiceWithRefIDs(t, store, newTestRegistry(t, root), nswMock, unconfiguredRefIDs()) + + err := svc.CreateApplication(context.Background(), &InjectRequest{ + TaskID: "t-refid-3", + TaskCode: "refid_task", + ConsignmentID: "c-refid-3", + Data: map[string]any{"nppo_office_location": "NPQS-KAT"}, + }) + if err == nil { + t.Fatal("expected inject to fail when the deployment configures no matching format") + } + // A deployment fault, not a bad request — must not be a 400. + if errors.Is(err, ErrInvalidInjectRequest) { + t.Errorf("error wraps ErrInvalidInjectRequest (400), want an unwrapped 500: %v", err) + } + if _, err := store.GetByTaskID("t-refid-3"); !errors.Is(err, gorm.ErrRecordNotFound) { + t.Errorf("application row exists after a failed generation, want none (got %v)", err) + } + + var consignments int64 + if err := store.db.Model(&consignment.ConsignmentRecord{}). + Where("id = ?", "c-refid-3").Count(&consignments).Error; err != nil { + t.Fatalf("counting consignments: %v", err) + } + if consignments != 0 { + t.Error("a failed generation left an orphan consignment; generation must run before CreateConsignment") + } + if nswMock.fetchCount != 0 { + t.Errorf("NSW consignment fetch ran %d times despite generation failing, want 0", nswMock.fetchCount) + } +} + +func TestCreateApplication_NoRefIDBlock_LeavesReviewerResponseEmpty(t *testing.T) { + h := newServiceHarnessWithRefIDs(t, unconfiguredRefIDs(), func(root string) { + writeTaskConfigFile(t, root, "plain.json", `{ + "schemaVersion": 1, + "meta": {"title": "Plain"}, + "permissions": [{"role": "officer", "actions": ["VIEW", "REVIEW"]}], + "forms": {"review": "plain_review"}, + "behavior": {"type": "statusMap", "statusMap": {"approve": "APPROVED"}} + }`) + }) + + if err := h.service.CreateApplication(context.Background(), &InjectRequest{ + TaskID: "t-plain", + TaskCode: "plain", + ConsignmentID: "c-plain", + Data: map[string]any{"anything": "goes"}, + }); err != nil { + t.Fatalf("CreateApplication: %v", err) + } + + rec, err := h.store.GetByTaskID("t-plain") + if err != nil { + t.Fatalf("GetByTaskID: %v", err) + } + if len(rec.ReviewerResponse) != 0 { + t.Fatalf("reviewer_response = %v, want empty for a task with no refid block", rec.ReviewerResponse) + } +} + +// TestCreateApplication_RefID_RealRegistry_EndToEnd wires the real refid +// registry, counter table and store together, so the whole path is exercised +// at once: task config -> params resolved from injected data -> a list segment +// validating the office code -> the durable counter -> the ID written at the +// configured pointer. The other RefID tests stub the registry to isolate +// persistence; this one is the integration seam between them. +func TestCreateApplication_RefID_RealRegistry_EndToEnd(t *testing.T) { + store := newTestStore(t) + mustCreateRefIDSequences(t, store) + + seq, err := refidstore.New(store.db) + if err != nil { + t.Fatalf("refidstore.New: %v", err) + } + reg, err := refid.NewRegistry(refid.Config{ + Issuers: []refid.IssuerConfig{{ + Issuer: "NPQS", + Formats: []refid.FormatConfig{{ + IDType: "application_id", + Segments: []refid.SegmentConfig{ + {Type: "literal", Value: "NPQS/"}, + {Type: "list", List: "office_location", Param: "officeCode"}, + {Type: "literal", Value: "/"}, + {Type: "sequence", ScopeKey: "{issuer}:{idType}:{officeCode}:{yyyy}", Padding: 6}, + }, + }}, + }}, + Lists: map[string][]string{"office_location": {"NPQS-KAT", "SEA-CMB"}}, + }, seq) + if err != nil { + t.Fatalf("refid.NewRegistry: %v", err) + } + + root := t.TempDir() + mustMkdirTaskConfigsAndForms(t, root) + writeTaskConfigFile(t, root, "refid_task.json", refIDTaskConfig) + srv, _ := newCallbackServer(t) + hc := httpclient.NewClientBuilder().WithBaseURL(srv.URL).Build() + svc := newWiredServiceWithRefIDs(t, store, newTestRegistry(t, root), nswclient.NewWithClient(hc), reg) + + inject := func(taskID, office string) error { + return svc.CreateApplication(context.Background(), &InjectRequest{ + TaskID: taskID, + TaskCode: "refid_task", + ConsignmentID: "c-" + taskID, + Data: map[string]any{"nppo_office_location": office}, + }) + } + refIDOf := func(taskID string) any { + t.Helper() + rec, err := store.GetByTaskID(taskID) + if err != nil { + t.Fatalf("GetByTaskID(%s): %v", taskID, err) + } + return rec.ReviewerResponse["reference_number"] + } + + // Two applications at the same office share a counter and advance it. + for i, want := range []string{"NPQS/NPQS-KAT/000001", "NPQS/NPQS-KAT/000002"} { + taskID := fmt.Sprintf("t-e2e-kat-%d", i) + if err := inject(taskID, "NPQS-KAT"); err != nil { + t.Fatalf("inject %s: %v", taskID, err) + } + if got := refIDOf(taskID); got != want { + t.Fatalf("reference_number = %v, want %q", got, want) + } + } + + // A different office is a different scope key, so it starts at 1. + if err := inject("t-e2e-cmb", "SEA-CMB"); err != nil { + t.Fatalf("inject t-e2e-cmb: %v", err) + } + if got, want := refIDOf("t-e2e-cmb"), "NPQS/SEA-CMB/000001"; got != want { + t.Fatalf("reference_number = %v, want %q", got, want) + } + + // An office code outside the configured list fails the inject as a bad + // request, and must not create the application. + err = inject("t-e2e-bad", "NOT-AN-OFFICE") + if !errors.Is(err, ErrInvalidInjectRequest) { + t.Fatalf("inject with an unlisted office returned %v, want ErrInvalidInjectRequest", err) + } + if _, err := store.GetByTaskID("t-e2e-bad"); !errors.Is(err, gorm.ErrRecordNotFound) { + t.Errorf("application row exists after a rejected office code, want none (got %v)", err) + } + + // Likewise when the param is absent entirely. generateRefID passes no + // officeCode rather than an empty one, and refid — not us — is what + // rejects it, which is the contract the skip-unresolved behaviour relies on. + err = svc.CreateApplication(context.Background(), &InjectRequest{ + TaskID: "t-e2e-missing", + TaskCode: "refid_task", + ConsignmentID: "c-t-e2e-missing", + Data: map[string]any{"something_else": "x"}, + }) + if !errors.Is(err, ErrInvalidInjectRequest) { + t.Fatalf("inject with no officeCode returned %v, want ErrInvalidInjectRequest", err) + } + if _, err := store.GetByTaskID("t-e2e-missing"); !errors.Is(err, gorm.ErrRecordNotFound) { + t.Errorf("application row exists after a missing required param, want none (got %v)", err) + } +} + +// mustCreateRefIDSequences creates the counter table refid's store expects, +// picking DDL for the store's dialect — newTestStore runs against PostgreSQL +// when AGENCY_DB_DRIVER=postgres, which has no datetime('now'). Mirrors the +// counter-table migration; keep the two in step. +func mustCreateRefIDSequences(t *testing.T, store *ApplicationStore) { + t.Helper() + + var ddl string + switch name := store.db.Name(); name { + case "postgres": + ddl = ` + CREATE TABLE IF NOT EXISTS refid_sequences ( + scope_key TEXT NOT NULL PRIMARY KEY, + counter BIGINT NOT NULL DEFAULT 0, + updated_at TIMESTAMPTZ NOT NULL DEFAULT now() + )` + case "sqlite": + ddl = ` + CREATE TABLE IF NOT EXISTS refid_sequences ( + scope_key TEXT NOT NULL PRIMARY KEY, + counter INTEGER NOT NULL DEFAULT 0, + updated_at TEXT NOT NULL DEFAULT (datetime('now')) + )` + default: + t.Fatalf("no refid_sequences DDL for driver %q", name) + } + + if err := store.db.Exec(ddl).Error; err != nil { + t.Fatalf("failed to create refid_sequences: %v", err) + } + // Persistent backends keep the table between tests, so counters would + // carry over and break the per-office assertions below. + if store.db.Name() != "sqlite" { + if err := store.db.Exec("TRUNCATE TABLE refid_sequences").Error; err != nil { + t.Fatalf("failed to truncate refid_sequences: %v", err) + } + } +} + +// refIDTaskConfigExtraParam declares a param no configured format consumes, +// which refid ignores — so an absent pointer for it must not fail the inject. +const refIDTaskConfigExtraParam = `{ + "schemaVersion": 1, + "meta": {"title": "RefID Task"}, + "permissions": [{"role": "officer", "actions": ["VIEW", "REVIEW"]}], + "forms": {"review": "refid_review"}, + "behavior": {"type": "statusMap", "statusMap": {"approve": "APPROVED"}}, + "refid": { + "issuer": "NPQS", + "idType": "application_id", + "path": "/reference_number", + "params": { + "officeCode": "/nppo_office_location", + "unusedByFormat": "/not_in_this_payload" + } + } +}` + +func TestCreateApplication_RefID_UnusedParamNotResolved_StillGenerates(t *testing.T) { + stub := &stubRefIDRegistry{id: "NPQS/NPQS-KAT/000007"} + h := newServiceHarnessWithRefIDs(t, stub, func(root string) { + writeTaskConfigFile(t, root, "refid_task.json", refIDTaskConfigExtraParam) + }) + + if err := h.service.CreateApplication(context.Background(), &InjectRequest{ + TaskID: "t-refid-extra", + TaskCode: "refid_task", + ConsignmentID: "c-refid-extra", + Data: map[string]any{"nppo_office_location": "NPQS-KAT"}, + }); err != nil { + t.Fatalf("a declared-but-unused param with no value must not fail the inject: %v", err) + } + + rec, err := h.store.GetByTaskID("t-refid-extra") + if err != nil { + t.Fatalf("GetByTaskID: %v", err) + } + if got := rec.ReviewerResponse["reference_number"]; got != "NPQS/NPQS-KAT/000007" { + t.Fatalf("reference_number = %v, want the generated ID", got) + } + + // The unresolved param is omitted rather than passed as an empty string, + // which refid would treat as a present-but-invalid value. + if len(stub.calls) != 1 { + t.Fatalf("Generate called %d times, want 1", len(stub.calls)) + } + if _, present := stub.calls[0].params["unusedByFormat"]; present { + t.Errorf("unresolved param was passed to Generate as %q, want it omitted", + stub.calls[0].params["unusedByFormat"]) + } + if stub.calls[0].params["officeCode"] != "NPQS-KAT" { + t.Errorf("officeCode = %q, want \"NPQS-KAT\"", stub.calls[0].params["officeCode"]) + } +} diff --git a/backend/internal/refidstore/refidstore.go b/backend/internal/refidstore/refidstore.go new file mode 100644 index 0000000..c5de646 --- /dev/null +++ b/backend/internal/refidstore/refidstore.go @@ -0,0 +1,55 @@ +// Package refidstore selects the refid.SequenceStore backend matching a GORM +// connection's dialect, so callers wire reference ID generation without +// branching on the driver themselves. +package refidstore + +import ( + "context" + "fmt" + + "github.com/OpenNSW/core/refid" + refidpg "github.com/OpenNSW/core/refid/store/postgres" + refidsqlite "github.com/OpenNSW/core/refid/store/sqlite" + "gorm.io/gorm" +) + +// New returns a refid.SequenceStore that shares db's existing connection pool. +// Reusing the pool matters for SQLite: a second sql.Open on ":memory:" is a +// different database entirely, and on a file it is a second writer competing +// for the same lock. +// +// The refid_sequences table it reads and writes is created by this repo's own +// migrations, not by refid's Migrate helpers. +func New(db *gorm.DB) (refid.SequenceStore, error) { + sqlDB, err := db.DB() + if err != nil { + return nil, fmt.Errorf("refidstore: failed to get sql.DB from gorm: %w", err) + } + + // db.Name() is the dialector name, "postgres" or "sqlite" — the same + // values pkg/jsonquery switches on. + switch name := db.Name(); name { + case "postgres": + return refidpg.New(sqlDB) + case "sqlite": + return refidsqlite.New(sqlDB) + default: + return nil, fmt.Errorf("refidstore: unsupported driver %q", name) + } +} + +// Disabled returns a Registry for a deployment with no refIDGen section, where +// there is no format to generate from. Every Generate fails, so a task +// declaring a refid block is a loud misconfiguration rather than a silent +// no-op — and a value rather than a nil Registry keeps callers' +// non-nil-dependency invariants intact (see application.NewService). +func Disabled() refid.Registry { return disabledRegistry{} } + +type disabledRegistry struct{} + +// Generate wraps ErrUnknownIssuer so callers classifying refid errors need no +// special case; the message names the cause, which the sentinel alone doesn't. +func (disabledRegistry) Generate(_ context.Context, issuer, idType string, _ map[string]string) (string, error) { + return "", fmt.Errorf("%w: no refIDGen section is configured for this deployment, so (%q, %q) cannot be generated", + refid.ErrUnknownIssuer, issuer, idType) +} diff --git a/backend/internal/refidstore/refidstore_test.go b/backend/internal/refidstore/refidstore_test.go new file mode 100644 index 0000000..538031c --- /dev/null +++ b/backend/internal/refidstore/refidstore_test.go @@ -0,0 +1,115 @@ +package refidstore_test + +import ( + "context" + "errors" + "path/filepath" + "strings" + "testing" + + "github.com/OpenNSW/agency/backend/internal/refidstore" + "github.com/OpenNSW/core/refid" + "github.com/glebarez/sqlite" + "gorm.io/gorm" + "gorm.io/gorm/logger" +) + +// refidSequencesDDL mirrors the sqlite branch of the counter-table migration. +// Unit tests don't replay the migrator, so the table is created here — keep +// the two in sync. +const refidSequencesDDL = ` +CREATE TABLE IF NOT EXISTS refid_sequences ( + scope_key TEXT NOT NULL PRIMARY KEY, + counter INTEGER NOT NULL DEFAULT 0, + updated_at TEXT NOT NULL DEFAULT (datetime('now')) +)` + +// newTestStore builds a SequenceStore over this module's actual SQLite driver +// (github.com/glebarez/sqlite), which is the whole point of these tests: +// refid's queries use RETURNING and ?N ordinal placeholders, and upstream only +// exercises them against modernc.org/sqlite. An on-disk file rather than +// ":memory:" so every pooled connection sees the same database. +func newTestStore(t *testing.T) refid.SequenceStore { + t.Helper() + db, err := gorm.Open(sqlite.Open(filepath.Join(t.TempDir(), "refid.db")), &gorm.Config{ + Logger: logger.Default.LogMode(logger.Silent), + }) + if err != nil { + t.Fatalf("failed to open sqlite: %v", err) + } + t.Cleanup(func() { + if sqlDB, err := db.DB(); err == nil { + _ = sqlDB.Close() + } + }) + if err := db.Exec(refidSequencesDDL).Error; err != nil { + t.Fatalf("failed to create refid_sequences: %v", err) + } + store, err := refidstore.New(db) + if err != nil { + t.Fatalf("refidstore.New: %v", err) + } + return store +} + +func TestNext_StartsAtOneAndIncrements(t *testing.T) { + store := newTestStore(t) + ctx := context.Background() + + for want := int64(1); want <= 3; want++ { + got, err := store.Next(ctx, "NPQS:application_id:NPQS-KAT:20260904", 999999) + if err != nil { + t.Fatalf("Next: %v", err) + } + if got != want { + t.Fatalf("Next returned %d, want %d", got, want) + } + } +} + +func TestNext_IsolatesScopeKeys(t *testing.T) { + store := newTestStore(t) + ctx := context.Background() + + // Two offices on the same day must not share a counter. + for range 3 { + if _, err := store.Next(ctx, "NPQS:application_id:NPQS-KAT:20260904", 999999); err != nil { + t.Fatalf("Next: %v", err) + } + } + got, err := store.Next(ctx, "NPQS:application_id:SEA-CMB:20260904", 999999) + if err != nil { + t.Fatalf("Next: %v", err) + } + if got != 1 { + t.Fatalf("second scope key started at %d, want 1", got) + } +} + +func TestNext_CounterOverflow(t *testing.T) { + store := newTestStore(t) + ctx := context.Background() + + if _, err := store.Next(ctx, "scope", 1); err != nil { + t.Fatalf("first Next: %v", err) + } + _, err := store.Next(ctx, "scope", 1) + if !errors.Is(err, refid.ErrCounterOverflow) { + t.Fatalf("Next past max returned %v, want refid.ErrCounterOverflow", err) + } +} +func TestDisabled_GenerateAlwaysFails(t *testing.T) { + _, err := refidstore.Disabled().Generate(context.Background(), "NPQS", "application_id", nil) + if err == nil { + t.Fatal("Disabled().Generate returned no error, want one") + } + // Classified like any other unknown format, so callers mapping refid + // errors to HTTP statuses need no special case. + if !errors.Is(err, refid.ErrUnknownIssuer) { + t.Errorf("error does not wrap refid.ErrUnknownIssuer: %v", err) + } + // ...but the message must name the real cause, not just "unknown issuer". + if !strings.Contains(err.Error(), "no refIDGen section is configured") { + t.Errorf("error message doesn't explain the cause: %v", err) + } +} diff --git a/backend/internal/taskconfig/task_config.go b/backend/internal/taskconfig/task_config.go index c9e4c46..30fb2ab 100644 --- a/backend/internal/taskconfig/task_config.go +++ b/backend/internal/taskconfig/task_config.go @@ -41,6 +41,10 @@ type TaskConfig struct { // on each entry, and docs/consignment-custom-data.md for the full // design (in particular: why arrays are unsupported by design here). ConsignmentFields []ConsignmentField `json:"consignmentFields,omitempty"` + // RefID declares that this task's applications get an agency-issued + // reference ID, generated once on first inject and written into the + // reviewer response at Path. Optional — nil means this task mints none. + RefID *TaskRefID `json:"refid,omitempty"` } // Action names a permission action a role can be granted. These are the @@ -134,6 +138,25 @@ func (c TaskConfig) Validate() error { return fmt.Errorf("taskconfig %q: consignmentFields[%d].target must be a JSON Pointer (e.g. \"/district\"), got %q", c.TaskCode, i, f.Target) } } + if r := c.RefID; r != nil { + if strings.TrimSpace(r.Issuer) == "" { + return fmt.Errorf("taskconfig %q: refid.issuer is required", c.TaskCode) + } + if strings.TrimSpace(r.IDType) == "" { + return fmt.Errorf("taskconfig %q: refid.idType is required", c.TaskCode) + } + if !jsonpointer.Valid(r.Path) { + return fmt.Errorf("taskconfig %q: refid.path must be a JSON Pointer (e.g. \"/reference_number\"), got %q", c.TaskCode, r.Path) + } + for param, pointer := range r.Params { + if strings.TrimSpace(param) == "" { + return fmt.Errorf("taskconfig %q: refid.params has an entry with an empty param name", c.TaskCode) + } + if !jsonpointer.Valid(pointer) { + return fmt.Errorf("taskconfig %q: refid.params[%q] must be a JSON Pointer (e.g. \"/nppo_office_location\"), got %q", c.TaskCode, param, pointer) + } + } + } return nil } @@ -192,6 +215,25 @@ type ConsignmentField struct { Target string `json:"target"` } +// TaskRefID configures reference ID generation for a task (see +// github.com/OpenNSW/core/refid). Issuer and IDType select one of the formats +// declared in the deployment's own refIDGen config; a task naming a format the +// deployment hasn't configured fails the inject rather than quietly skipping, +// since an application with no reference ID is not something to discover later. +type TaskRefID struct { + Issuer string `json:"issuer"` + IDType string `json:"idType"` + // Path is a JSON Pointer into the reviewer response naming where the + // generated ID is written, e.g. "/reference_number". The task's review + // form needs a control at this path or the officer never sees it. + Path string `json:"path"` + // Params supplies the format's own inputs, keyed by refid param name; + // each value is a JSON Pointer resolved against the injected data, e.g. + // {"officeCode": "/nppo_office_location"}. refid ignores params a format + // doesn't consume, so these may be declared generously. + Params map[string]string `json:"params,omitempty"` +} + // DefaultOutcomeField is the field name read from the review submission // body when TaskBehavior.OutcomeField is not set. const DefaultOutcomeField = "review_outcome" diff --git a/backend/internal/taskconfig/task_config_test.go b/backend/internal/taskconfig/task_config_test.go index 6ce98a7..b888c71 100644 --- a/backend/internal/taskconfig/task_config_test.go +++ b/backend/internal/taskconfig/task_config_test.go @@ -340,3 +340,92 @@ func TestValidate_ConsignmentFields_MissingSlash(t *testing.T) { }) } } + +// baseConfigWithRefID returns a minimal valid config carrying refID, so each +// case below varies only the refid block. +func baseConfigWithRefID(refID *TaskRefID) TaskConfig { + return TaskConfig{ + SchemaVersion: CurrentSchemaVersion, + TaskCode: "alpha", + Forms: TaskForms{Review: "review-form"}, + Behavior: validBehavior(), + Permissions: validPermissions(), + RefID: refID, + } +} + +func TestValidate_RefID_Omitted(t *testing.T) { + if err := baseConfigWithRefID(nil).Validate(); err != nil { + t.Errorf("expected no error when refid is omitted, got %v", err) + } +} + +func TestValidate_RefID_Valid(t *testing.T) { + cases := []struct { + name string + refID TaskRefID + }{ + {"without params", TaskRefID{ + Issuer: "NPQS", IDType: "application_id", Path: "/reference_number", + }}, + {"with params", TaskRefID{ + Issuer: "NPQS", IDType: "application_id", Path: "/reference_number", + Params: map[string]string{"officeCode": "/nppo_office_location"}, + }}, + {"nested path", TaskRefID{ + Issuer: "NPQS", IDType: "application_id", Path: "/registration/reference_number", + }}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if err := baseConfigWithRefID(&tc.refID).Validate(); err != nil { + t.Errorf("expected no error for a valid refid, got %v", err) + } + }) + } +} + +func TestValidate_RefID_Rejected(t *testing.T) { + cases := []struct { + name string + refID TaskRefID + }{ + {"empty issuer", TaskRefID{ + Issuer: "", IDType: "application_id", Path: "/reference_number", + }}, + {"blank issuer", TaskRefID{ + Issuer: " ", IDType: "application_id", Path: "/reference_number", + }}, + {"empty idType", TaskRefID{ + Issuer: "NPQS", IDType: "", Path: "/reference_number", + }}, + {"empty path", TaskRefID{ + Issuer: "NPQS", IDType: "application_id", Path: "", + }}, + {"path missing leading slash", TaskRefID{ + Issuer: "NPQS", IDType: "application_id", Path: "reference_number", + }}, + {"path with a bad escape", TaskRefID{ + Issuer: "NPQS", IDType: "application_id", Path: "/ref~2num", + }}, + {"param pointer missing leading slash", TaskRefID{ + Issuer: "NPQS", IDType: "application_id", Path: "/reference_number", + Params: map[string]string{"officeCode": "nppo_office_location"}, + }}, + {"param pointer empty", TaskRefID{ + Issuer: "NPQS", IDType: "application_id", Path: "/reference_number", + Params: map[string]string{"officeCode": ""}, + }}, + {"empty param name", TaskRefID{ + Issuer: "NPQS", IDType: "application_id", Path: "/reference_number", + Params: map[string]string{"": "/nppo_office_location"}, + }}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if err := baseConfigWithRefID(&tc.refID).Validate(); err == nil { + t.Errorf("expected an error for %s, got nil", tc.name) + } + }) + } +} diff --git a/backend/migrations/000010_create_refid_sequences.sql b/backend/migrations/000010_create_refid_sequences.sql new file mode 100644 index 0000000..24c2283 --- /dev/null +++ b/backend/migrations/000010_create_refid_sequences.sql @@ -0,0 +1,29 @@ +-- Created at: 2026-09-04T00:00:00Z +-- +-- Durable sequence counters for github.com/OpenNSW/core/refid (one row per +-- resolved scope key). Created here rather than via refid's own +-- postgres.Migrate/sqlite.Migrate helpers so the .sql file stays the single +-- source of truth for the schema (docs/migrations.md) and the table gets +-- down/status like every other one. Keep the shape identical to those +-- helpers' DDL — refid's queries run against this table unchanged. +-- +-- Dialect-split because the timestamp column genuinely differs: SQLite has no +-- TIMESTAMPTZ and no now(), and rejects the DEFAULT expression outright. + +-- @UP +-- @postgres +CREATE TABLE IF NOT EXISTS refid_sequences ( + scope_key TEXT NOT NULL PRIMARY KEY, + counter BIGINT NOT NULL DEFAULT 0, + updated_at TIMESTAMPTZ NOT NULL DEFAULT now() +); + +-- @sqlite +CREATE TABLE IF NOT EXISTS refid_sequences ( + scope_key TEXT NOT NULL PRIMARY KEY, + counter INTEGER NOT NULL DEFAULT 0, + updated_at TEXT NOT NULL DEFAULT (datetime('now')) +); + +-- @DOWN +DROP TABLE IF EXISTS refid_sequences; diff --git a/deployments/helm/values-example.yaml b/deployments/helm/values-example.yaml index 3325521..c476565 100644 --- a/deployments/helm/values-example.yaml +++ b/deployments/helm/values-example.yaml @@ -133,6 +133,37 @@ config: - "nsw:storage:read" - "nsw:storage:write" + # Reference ID formats this deployment can issue — OPTIONAL, and left off + # here. Omit the section entirely and no task can generate a reference ID; + # nothing else changes. A task opts in via the `refid` block in its own task + # config, naming an (issuer, idType) from here plus the JSON Pointer to + # store the result at — see backend/docs/task-config-reference.md, and + # backend/config.example.yaml for the full segment schema. + # + # Counters live in the refid_sequences table, so the migration Job below + # must have run before a server with this configured starts. + # + # Substitute your own issuer, segments and lists — the values below are + # illustrative, not any particular agency's. + #refIDGen: + # issuers: + # - issuer: "" + # formats: + # # Produces /COL/20260904/000001 — per office, reset daily. + # - idType: application_id + # segments: + # - { type: literal, value: "/" } + # - { type: list, list: office_location, param: officeCode } + # - { type: literal, value: "/" } + # - { type: date, layout: "20060102" } + # - { type: literal, value: "/" } + # - { type: sequence, padding: 6, + # scopeKey: "{issuer}:{idType}:{officeCode}:{yyyyMMdd}" } + # lists: + # # A list segment rejects any value not in this set, failing the inject — + # # keep in step with what the injected data actually sends. + # office_location: [COL, GAL, KAN] + # Static files config.yaml points at above (consignmentCustomDataSchemaPath / # dataScopeRulesPath) — NPQS's actual content from backend/config/npqs/. # Mounted read-only alongside config.yaml (same ConfigMap, same volume).