From 4dedd2c5612eaacec2081b10c115eafe2a696ab9 Mon Sep 17 00:00:00 2001 From: Thanikan Date: Fri, 4 Sep 2026 20:55:30 +0530 Subject: [PATCH 1/6] feat(refid): generate agency reference IDs on application inject MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An agency had no way to issue its own reference number for an application — the only identifier was the opaque NSW task ID. Where a number was needed it was typed by hand into the review form, with nothing guaranteeing it unique, sequential, correctly formatted, or scoped to the issuing office. Adopts github.com/OpenNSW/core/refid, split across two config layers so that what an agency can issue is a deployment decision while which tasks get one is a task decision: - refIDGen in config.yaml declares the formats (issuers, segments, lists). Optional — omit it and no task can generate a reference ID. - A new optional refid block in a task config names an (issuer, idType) from there, the JSON Pointer to store the result at, and params mapped to JSON Pointers into the injected data. Sourcing params from the data is what lets one task config serve every office rather than needing one config per office. Generated once, on first inject only; a re-inject keeps the number it already has. This required carrying ReviewerResponse forward in CreateApplication, since CreateOrUpdate does a full-row Save that would otherwise NULL the column and destroy an issued ID — the same reason ClaimedBy/ClaimedAt are already carried over. Generation failure fails the inject, so an application never exists without its reference ID. An unresolvable param maps to 400; an unconfigured issuer/idType, counter overflow or a database error to 500. Counters live in a new refid_sequences table (migration 000010) rather than refid's own Migrate helpers, keeping the .sql file the single source of truth for schema and getting down/status with it. The store reuses the existing GORM pool: a second sql.Open would be a different database for sqlite :memory: and a second competing writer for a file. internal/refidstore is tested against this module's real SQLite driver (glebarez), not modernc — refid's queries use RETURNING and ?N ordinal placeholders, which upstream only exercises against modernc. Requires the driver-registration fix in OpenNSW/core refid/store/*; the go.mod replace directive is temporary and must be dropped, and the require repointed at the merged ref, before this merges. Closes #306 --- backend/cmd/server/config.go | 8 + backend/cmd/server/config_test.go | 59 ++++ backend/cmd/server/main.go | 20 +- backend/config.example.yaml | 49 +++ backend/docs/task-config-reference.md | 78 ++++- backend/go.mod | 7 +- backend/go.sum | 1 - backend/internal/application/refid.go | 57 ++++ backend/internal/application/service.go | 30 +- backend/internal/application/service_test.go | 319 +++++++++++++++++- backend/internal/refidstore/refidstore.go | 38 +++ .../internal/refidstore/refidstore_test.go | 148 ++++++++ backend/internal/taskconfig/task_config.go | 42 +++ .../internal/taskconfig/task_config_test.go | 89 +++++ .../000010_create_refid_sequences.sql | 29 ++ deployments/helm/values-example.yaml | 31 ++ 16 files changed, 987 insertions(+), 18 deletions(-) create mode 100644 backend/internal/application/refid.go create mode 100644 backend/internal/refidstore/refidstore.go create mode 100644 backend/internal/refidstore/refidstore_test.go create mode 100644 backend/migrations/000010_create_refid_sequences.sql 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..ddf25c9 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,6 +30,7 @@ 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" ) @@ -159,8 +161,24 @@ func main() { consignmentService := consignment.NewService(consignmentStore, nswClient, dataScopeResolver) consignmentHandler := consignment.NewHandler(consignmentService) + // Reference ID generation (optional per deployment). NewRegistry validates + // every configured format up front, so a malformed refIDGen section fails + // the boot rather than the first inject that needs it. With no section + // configured the registry holds zero formats and Generate returns + // ErrUnknownIssuer, which is what makes a task declaring refid against an + // unconfigured deployment fail loudly. + refIDSequences, err := refidstore.New(store.DB()) + if err != nil { + log.Fatalf("failed to create refid sequence store: %v", err) + } + refIDs, err := refid.NewRegistry(cfg.RefIDGen, refIDSequences) + if err != nil { + log.Fatalf("invalid refIDGen config: %v", err) + } + slog.Info("reference ID generation configured", "issuers", len(cfg.RefIDGen.Issuers)) + // 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) 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..af271d7 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,69 @@ 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. Counter state lives in the `refid_sequences` +table (migration `000010`). + +**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`. Every other field of that document stays absent +until the officer fills it in, which is expected: status and `reviewedAt` are +what mark an application reviewed, not a non-empty reviewer response. Note that +a review form whose schema sets `"additionalProperties": false` without +declaring the field would reject the officer's submission. + +> Making that control read-only is up to whoever authors the form, and is a +> client-side convention only: review submissions are not validated +> server-side, so an officer can still overwrite the number. Enforcing that +> properly needs backend validation of the review payload. + ## 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..cb6e6d6 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.0.0-00010101000000-000000000000 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 @@ -61,3 +60,7 @@ require ( modernc.org/mathutil v1.7.1 // indirect modernc.org/memory v1.12.1 // indirect ) + +// TODO: drop before merging — points at the local core checkout while the +// driver-import removal in refid/store/* is still unreleased. +replace github.com/OpenNSW/core/refid => ../../core/refid diff --git a/backend/go.sum b/backend/go.sum index 9682f9e..0370c48 100644 --- a/backend/go.sum +++ b/backend/go.sum @@ -48,7 +48,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..5f346a2 --- /dev/null +++ b/backend/internal/application/refid.go @@ -0,0 +1,57 @@ +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 and returns the reviewer +// response document to store it in, with the ID written at cfg.Path. +// +// Unlike resolvePushedFields, an unresolved pointer here is an error rather +// than a silent skip: a params entry the configured format requires is the +// difference between a correct ID and none at all, and CreateApplication +// fails the whole inject rather than persisting an application without one. +// +// Param resolution errors and refid.ErrInvalidParam wrap +// ErrInvalidInjectRequest (a 400 — the injected data couldn't supply a value +// the format needs). Everything else — an issuer/idType this deployment +// hasn't configured, counter overflow, a database failure — stays unwrapped +// and surfaces as a 500, since those are deployment or infrastructure faults +// rather than anything wrong with the request. +func generateRefID(ctx context.Context, reg refid.Registry, cfg *taskconfig.TaskRefID, data map[string]any) (JSONB, error) { + params := make(map[string]string, len(cfg.Params)) + for param, pointer := range cfg.Params { + value, ok := jsonpointer.Get(data, pointer) + if !ok { + return nil, fmt.Errorf("%w: refid param %q: injected data has no value at %q", ErrInvalidInjectRequest, param, pointer) + } + str, ok := value.(string) + if !ok { + return nil, fmt.Errorf("%w: refid param %q: value at %q must be a string, got %T", ErrInvalidInjectRequest, param, pointer, value) + } + params[param] = str + } + + id, err := reg.Generate(ctx, cfg.Issuer, cfg.IDType, params) + if err != nil { + if errors.Is(err, refid.ErrInvalidParam) { + return nil, fmt.Errorf("%w: %v", ErrInvalidInjectRequest, err) + } + return nil, fmt.Errorf("failed to generate reference ID for issuer %q idType %q: %w", cfg.Issuer, cfg.IDType, err) + } + + reviewerResponse := JSONB{} + if !jsonpointer.Set(reviewerResponse, cfg.Path, id) { + // Unreachable: the document is empty and Validate already checked + // Path is a well-formed pointer. Still an error rather than a + // discard — losing an already-issued ID would be silent corruption. + return nil, fmt.Errorf("failed to write reference ID to %q", cfg.Path) + } + return reviewerResponse, nil +} diff --git a/backend/internal/application/service.go b/backend/internal/application/service.go index 8ab6bbd..fb21f9f 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,9 +225,12 @@ 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 doesn't destroy an + // already-issued reference ID (see generateRefID). appRecord.ClaimedBy = existing.ClaimedBy appRecord.ClaimedAt = existing.ClaimedAt + appRecord.ReviewerResponse = existing.ReviewerResponse } if existing == nil { @@ -230,6 +241,17 @@ func (s *service) CreateApplication(ctx context.Context, req *InjectRequest) err } } + // Last thing before the write: Generate claims a counter value that a + // failed CreateOrUpdate can't give back, so keep the window small. Only + // for a brand-new application — a re-inject keeps the ID it already has. + if existing == nil && config.RefID != nil { + reviewerResponse, err := generateRefID(ctx, s.refIDs, config.RefID, req.Data) + if err != nil { + return err + } + appRecord.ReviewerResponse = reviewerResponse + } + if err := s.store.CreateOrUpdate(appRecord, pushedFields); err != nil { return err } diff --git a/backend/internal/application/service_test.go b/backend/internal/application/service_test.go index ea1977c..ced09eb 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,51 @@ 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, where +// every Generate fails with ErrUnknownIssuer. This is what the existing +// helpers pass, since no task config in these tests declares a refid block. +func unconfiguredRefIDs() refid.Registry { + return &stubRefIDRegistry{err: refid.ErrUnknownIssuer} +} + +// 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 +292,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 +1952,260 @@ 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)) + } +} + +func TestCreateApplication_RefID_UnconfiguredDeployment_FailsInject(t *testing.T) { + // unconfiguredRefIDs mirrors a deployment with no refIDGen section. + h := newServiceHarnessWithRefIDs(t, unconfiguredRefIDs(), func(root string) { + writeTaskConfigFile(t, root, "refid_task.json", refIDTaskConfig) + }) + + err := h.service.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) + } + // Fail-closed: no application may exist without its reference ID. + if _, err := h.store.GetByTaskID("t-refid-3"); !errors.Is(err, gorm.ErrRecordNotFound) { + t.Errorf("application row exists after a failed generation, want none (got %v)", err) + } +} + +func TestCreateApplication_RefID_UnresolvableParam_Rejected(t *testing.T) { + stub := &stubRefIDRegistry{id: "unused"} + h := newServiceHarnessWithRefIDs(t, stub, func(root string) { + writeTaskConfigFile(t, root, "refid_task.json", refIDTaskConfig) + }) + + // Data carries no /nppo_office_location, so the declared param can't resolve. + err := h.service.CreateApplication(context.Background(), &InjectRequest{ + TaskID: "t-refid-4", + TaskCode: "refid_task", + ConsignmentID: "c-refid-4", + Data: map[string]any{"something_else": "x"}, + }) + if !errors.Is(err, ErrInvalidInjectRequest) { + t.Fatalf("CreateApplication returned %v, want ErrInvalidInjectRequest", err) + } + if len(stub.calls) != 0 { + t.Errorf("Generate called %d times despite an unresolvable param, want 0", len(stub.calls)) + } +} + +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) + if err := store.db.Exec(` + 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')) + )`).Error; err != nil { + t.Fatalf("failed to create refid_sequences: %v", err) + } + + 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) + } +} diff --git a/backend/internal/refidstore/refidstore.go b/backend/internal/refidstore/refidstore.go new file mode 100644 index 0000000..e5a5898 --- /dev/null +++ b/backend/internal/refidstore/refidstore.go @@ -0,0 +1,38 @@ +// 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 ( + "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 migration +// 000010, not by refid's own 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) + } +} diff --git a/backend/internal/refidstore/refidstore_test.go b/backend/internal/refidstore/refidstore_test.go new file mode 100644 index 0000000..bbfea9a --- /dev/null +++ b/backend/internal/refidstore/refidstore_test.go @@ -0,0 +1,148 @@ +package refidstore_test + +import ( + "context" + "errors" + "path/filepath" + "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 +// migrations/000010_create_refid_sequences.sql. 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) + } +} + +// TestRegistry_GeneratesFullID drives a real refid config end to end, so the +// padding, list validation and scope-key resolution are all exercised against +// this module's driver rather than just the raw counter. +func TestRegistry_GeneratesFullID(t *testing.T) { + cfg := 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"}}, + } + + reg, err := refid.NewRegistry(cfg, newTestStore(t)) + if err != nil { + t.Fatalf("NewRegistry: %v", err) + } + ctx := context.Background() + + got, err := reg.Generate(ctx, "NPQS", "application_id", map[string]string{"officeCode": "NPQS-KAT"}) + if err != nil { + t.Fatalf("Generate: %v", err) + } + if want := "NPQS/NPQS-KAT/000001"; got != want { + t.Fatalf("Generate returned %q, want %q", got, want) + } + + // A value outside the configured list must not reach the counter. + if _, err := reg.Generate(ctx, "NPQS", "application_id", map[string]string{"officeCode": "NOPE"}); !errors.Is(err, refid.ErrInvalidParam) { + t.Fatalf("Generate with unlisted office returned %v, want refid.ErrInvalidParam", err) + } + + // ... and the next valid call is 2, not 3 — the rejected call was side-effect free. + got, err = reg.Generate(ctx, "NPQS", "application_id", map[string]string{"officeCode": "NPQS-KAT"}) + if err != nil { + t.Fatalf("Generate: %v", err) + } + if want := "NPQS/NPQS-KAT/000002"; got != want { + t.Fatalf("Generate returned %q, want %q", got, want) + } +} 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). From e1dcfc4d4644430e234fbd053973fb69eb8e5969 Mon Sep 17 00:00:00 2001 From: Thanikan Date: Sun, 6 Sep 2026 11:55:01 +0530 Subject: [PATCH 2/6] chore(deps): point refid at the merged core module Drops the temporary replace directive now that OpenNSW/core#186 (the driver-registration fix refid/store/sqlite needs here) has merged, and repoints the require at that commit. Verified against the published module rather than the local checkout: build, vet and all tests pass, and the server boots without the "sql: Register called twice for driver sqlite" panic. Co-Authored-By: Claude Opus 5 (1M context) --- backend/go.mod | 6 +----- backend/go.sum | 2 ++ 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/backend/go.mod b/backend/go.mod index cb6e6d6..826a374 100644 --- a/backend/go.mod +++ b/backend/go.mod @@ -7,7 +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.0.0-00010101000000-000000000000 + 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 @@ -60,7 +60,3 @@ require ( modernc.org/mathutil v1.7.1 // indirect modernc.org/memory v1.12.1 // indirect ) - -// TODO: drop before merging — points at the local core checkout while the -// driver-import removal in refid/store/* is still unreleased. -replace github.com/OpenNSW/core/refid => ../../core/refid diff --git a/backend/go.sum b/backend/go.sum index 0370c48..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= From b6c975649b0567416fb960b02ed7affab100ffc7 Mon Sep 17 00:00:00 2001 From: Thanikan Date: Sun, 6 Sep 2026 21:29:58 +0530 Subject: [PATCH 3/6] fix(refid): address review feedback on reference ID generation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Honour the documented refid.params contract. Three places said params may be declared generously because refid ignores keys a format doesn't consume, but generateRefID resolved every declared param and rejected the inject if any pointer missed. Resolve what's present and let refid decide what it needs: it returns ErrInvalidParam for a param a segment requires and for a scope key left with an unresolved placeholder, and that already maps to a 400. Generate before creating the consignment, so a generation failure leaves nothing behind. CreateConsignment fetches NSW extras and inserts a row, which previously survived a later generation failure as an orphan. The cost is a slightly wider window in which a crash strands the counter value just claimed, which refid tolerates by design. Skip building the counter store and registry when no refIDGen section is configured, rather than building an empty registry and taking a database handle for a feature that is off. refidstore.Disabled fills the gap: a Registry whose Generate always fails, so a task declaring refid on such a deployment is still a loud misconfiguration rather than a silent no-op, and application.NewService keeps its non-nil-dependency invariant. Its error wraps ErrUnknownIssuer so the HTTP mapping is unchanged, but names the real cause instead of reading like a task-config typo. The startup log now says "not configured" rather than "configured issuers=0". Pick the counter-table DDL by dialect in the end-to-end test. newTestStore runs against PostgreSQL when AGENCY_DB_DRIVER=postgres, which has no datetime('now'), so the test failed during setup on that path. Drop the migration number from the docs and refidstore's comment — it goes stale if migrations are ever collapsed. --- backend/cmd/server/main.go | 32 +-- backend/docs/task-config-reference.md | 3 +- backend/internal/application/refid.go | 30 +-- backend/internal/application/service.go | 24 +- backend/internal/application/service_test.go | 213 +++++++++++++++--- backend/internal/refidstore/refidstore.go | 24 +- .../internal/refidstore/refidstore_test.go | 17 ++ 7 files changed, 273 insertions(+), 70 deletions(-) diff --git a/backend/cmd/server/main.go b/backend/cmd/server/main.go index ddf25c9..f5e782f 100644 --- a/backend/cmd/server/main.go +++ b/backend/cmd/server/main.go @@ -161,21 +161,25 @@ func main() { consignmentService := consignment.NewService(consignmentStore, nswClient, dataScopeResolver) consignmentHandler := consignment.NewHandler(consignmentService) - // Reference ID generation (optional per deployment). NewRegistry validates - // every configured format up front, so a malformed refIDGen section fails - // the boot rather than the first inject that needs it. With no section - // configured the registry holds zero formats and Generate returns - // ErrUnknownIssuer, which is what makes a task declaring refid against an - // unconfigured deployment fail loudly. - refIDSequences, err := refidstore.New(store.DB()) - if err != nil { - log.Fatalf("failed to create refid sequence store: %v", err) - } - refIDs, err := refid.NewRegistry(cfg.RefIDGen, refIDSequences) - if err != nil { - log.Fatalf("invalid refIDGen config: %v", err) + // Reference ID generation is optional per deployment: with no refIDGen + // section there is nothing to build, so skip the counter store and the + // registry entirely and hand the service a disabled one. NewRegistry + // validates every configured format up front, so a malformed section fails + // the boot rather than the first inject that needs it. + refIDs := refidstore.Disabled() + if n := len(cfg.RefIDGen.Issuers); n > 0 { + refIDSequences, err := refidstore.New(store.DB()) + if err != nil { + log.Fatalf("failed to create refid sequence store: %v", err) + } + refIDs, err = refid.NewRegistry(cfg.RefIDGen, refIDSequences) + if err != nil { + log.Fatalf("invalid refIDGen config: %v", err) + } + slog.Info("reference ID generation configured", "issuers", n) + } else { + slog.Info("reference ID generation not configured; tasks declaring a refid block will fail at inject") } - slog.Info("reference ID generation configured", "issuers", len(cfg.RefIDGen.Issuers)) // Initialize Agency service service := application.NewService(store, artifactRegistry, nswClient, roleService, consignmentService, dataScopeResolver, refIDs) diff --git a/backend/docs/task-config-reference.md b/backend/docs/task-config-reference.md index af271d7..f56010a 100644 --- a/backend/docs/task-config-reference.md +++ b/backend/docs/task-config-reference.md @@ -447,8 +447,7 @@ 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. Counter state lives in the `refid_sequences` -table (migration `000010`). +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 diff --git a/backend/internal/application/refid.go b/backend/internal/application/refid.go index 5f346a2..e1268fb 100644 --- a/backend/internal/application/refid.go +++ b/backend/internal/application/refid.go @@ -13,29 +13,29 @@ import ( // generateRefID mints this task's reference ID and returns the reviewer // response document to store it in, with the ID written at cfg.Path. // -// Unlike resolvePushedFields, an unresolved pointer here is an error rather -// than a silent skip: a params entry the configured format requires is the -// difference between a correct ID and none at all, and CreateApplication -// fails the whole inject rather than persisting an application without one. +// A params pointer that doesn't resolve to a string is skipped rather than +// rejected here, because refid ignores params the configured format doesn't +// consume — so a task may declare more than any one format needs. Whether an +// absent value actually matters is refid's call, not ours: it returns +// ErrInvalidParam for a param a segment requires, and for a scope key left +// with an unresolved placeholder. // -// Param resolution errors and refid.ErrInvalidParam wrap -// ErrInvalidInjectRequest (a 400 — the injected data couldn't supply a value -// the format needs). Everything else — an issuer/idType this deployment -// hasn't configured, counter overflow, a database failure — stays unwrapped -// and surfaces as a 500, since those are deployment or infrastructure faults -// rather than anything wrong with the request. +// refid.ErrInvalidParam then wraps ErrInvalidInjectRequest (a 400 — the +// injected data couldn't supply a value the format needs). Everything else — +// an issuer/idType this deployment hasn't configured, counter overflow, a +// database failure — stays unwrapped and surfaces as a 500, since those are +// deployment or infrastructure faults rather than anything wrong with the +// request. func generateRefID(ctx context.Context, reg refid.Registry, cfg *taskconfig.TaskRefID, data map[string]any) (JSONB, error) { params := make(map[string]string, len(cfg.Params)) for param, pointer := range cfg.Params { value, ok := jsonpointer.Get(data, pointer) if !ok { - return nil, fmt.Errorf("%w: refid param %q: injected data has no value at %q", ErrInvalidInjectRequest, param, pointer) + continue } - str, ok := value.(string) - if !ok { - return nil, fmt.Errorf("%w: refid param %q: value at %q must be a string, got %T", ErrInvalidInjectRequest, param, pointer, value) + if str, ok := value.(string); ok { + params[param] = str } - params[param] = str } id, err := reg.Generate(ctx, cfg.Issuer, cfg.IDType, params) diff --git a/backend/internal/application/service.go b/backend/internal/application/service.go index fb21f9f..ab97291 100644 --- a/backend/internal/application/service.go +++ b/backend/internal/application/service.go @@ -233,17 +233,11 @@ func (s *service) CreateApplication(ctx context.Context, req *InjectRequest) err appRecord.ReviewerResponse = existing.ReviewerResponse } - 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", - "consignmentID", req.ConsignmentID, "error", err) - } - } - - // Last thing before the write: Generate claims a counter value that a - // failed CreateOrUpdate can't give back, so keep the window small. Only - // for a brand-new application — a re-inject keeps the ID it already has. + // Only for a brand-new application — a re-inject keeps the ID it already + // has. Ahead of CreateConsignment so a generation failure doesn't leave a + // consignment with no application behind it; the cost is a slightly wider + // window in which a crash strands the counter value Generate just claimed, + // which refid tolerates by design (its formats are not gapless). if existing == nil && config.RefID != nil { reviewerResponse, err := generateRefID(ctx, s.refIDs, config.RefID, req.Data) if err != nil { @@ -252,6 +246,14 @@ func (s *service) CreateApplication(ctx context.Context, req *InjectRequest) err appRecord.ReviewerResponse = reviewerResponse } + 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", + "consignmentID", req.ConsignmentID, "error", err) + } + } + if err := s.store.CreateOrUpdate(appRecord, pushedFields); err != nil { return err } diff --git a/backend/internal/application/service_test.go b/backend/internal/application/service_test.go index ced09eb..2325b89 100644 --- a/backend/internal/application/service_test.go +++ b/backend/internal/application/service_test.go @@ -259,11 +259,12 @@ func (s *stubRefIDRegistry) Generate(_ context.Context, issuer, idType string, p return s.id, nil } -// unconfiguredRefIDs mirrors a deployment with no refIDGen section, where -// every Generate fails with ErrUnknownIssuer. This is what the existing -// helpers pass, since no task config in these tests declares a refid block. +// 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 &stubRefIDRegistry{err: refid.ErrUnknownIssuer} + return refidstore.Disabled() } // newWiredServiceWithRefIDs is newWiredService with an explicit reference ID @@ -2066,24 +2067,48 @@ func TestCreateApplication_RefID_UnconfiguredDeployment_FailsInject(t *testing.T } } -func TestCreateApplication_RefID_UnresolvableParam_Rejected(t *testing.T) { - stub := &stubRefIDRegistry{id: "unused"} - h := newServiceHarnessWithRefIDs(t, stub, func(root string) { - writeTaskConfigFile(t, root, "refid_task.json", refIDTaskConfig) - }) +// TestCreateApplication_RefID_GenerationFailure_LeavesNoConsignment pins the +// ordering: generation runs ahead of CreateConsignment, so a failure leaves +// nothing behind at all. +// +// It needs a mock NSW client whose consignment fetch succeeds. CreateConsignment +// fetches NSW extras before inserting the row, so with the default stub server +// (whose fetch fails) no consignment is ever created and the assertion below +// would hold regardless of ordering — i.e. be vacuous. +func TestCreateApplication_RefID_GenerationFailure_LeavesNoConsignment(t *testing.T) { + store := newTestStore(t) + root := t.TempDir() + mustMkdirTaskConfigsAndForms(t, root) + writeTaskConfigFile(t, root, "refid_task.json", refIDTaskConfig) - // Data carries no /nppo_office_location, so the declared param can't resolve. - err := h.service.CreateApplication(context.Background(), &InjectRequest{ - TaskID: "t-refid-4", + nswMock := &mockNSWClient{consignment: &nswclient.ConsignmentAgency{ + ConsignmentID: "c-refid-orphan", + TraderCompanyName: "CEYLON EXPORTS", + }} + // unconfiguredRefIDs makes generation fail the way a deployment missing + // the format would. + svc := newWiredServiceWithRefIDs(t, store, newTestRegistry(t, root), nswMock, unconfiguredRefIDs()) + + err := svc.CreateApplication(context.Background(), &InjectRequest{ + TaskID: "t-refid-orphan", TaskCode: "refid_task", - ConsignmentID: "c-refid-4", - Data: map[string]any{"something_else": "x"}, + ConsignmentID: "c-refid-orphan", + Data: map[string]any{"nppo_office_location": "NPQS-KAT"}, }) - if !errors.Is(err, ErrInvalidInjectRequest) { - t.Fatalf("CreateApplication returned %v, want ErrInvalidInjectRequest", err) + if err == nil { + t.Fatal("expected the inject to fail when no format is configured") + } + + var consignments int64 + if err := store.db.Model(&consignment.ConsignmentRecord{}). + Where("id = ?", "c-refid-orphan").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 len(stub.calls) != 0 { - t.Errorf("Generate called %d times despite an unresolvable param, want 0", len(stub.calls)) + if nswMock.fetchCount != 0 { + t.Errorf("NSW consignment fetch ran %d times despite generation failing, want 0", nswMock.fetchCount) } } @@ -2124,14 +2149,7 @@ func TestCreateApplication_NoRefIDBlock_LeavesReviewerResponseEmpty(t *testing.T // persistence; this one is the integration seam between them. func TestCreateApplication_RefID_RealRegistry_EndToEnd(t *testing.T) { store := newTestStore(t) - if err := store.db.Exec(` - 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')) - )`).Error; err != nil { - t.Fatalf("failed to create refid_sequences: %v", err) - } + mustCreateRefIDSequences(t, store) seq, err := refidstore.New(store.db) if err != nil { @@ -2209,3 +2227,146 @@ func TestCreateApplication_RefID_RealRegistry_EndToEnd(t *testing.T) { t.Errorf("application row exists after a rejected office code, 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"]) + } +} + +func TestCreateApplication_RefID_RequiredParamMissing_RejectedByFormat(t *testing.T) { + // A required param is now refid's call, not ours: with officeCode absent + // the list segment fails, and ErrInvalidParam still maps to a 400. + 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: "list", List: "office_location", Param: "officeCode"}, + {Type: "sequence", ScopeKey: "{issuer}:{idType}:{officeCode}", Padding: 6}, + }, + }}, + }}, + Lists: map[string][]string{"office_location": {"NPQS-KAT"}}, + }, 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) + + err = svc.CreateApplication(context.Background(), &InjectRequest{ + TaskID: "t-refid-required", + TaskCode: "refid_task", + ConsignmentID: "c-refid-required", + Data: map[string]any{"something_else": "x"}, + }) + if !errors.Is(err, ErrInvalidInjectRequest) { + t.Fatalf("CreateApplication returned %v, want ErrInvalidInjectRequest", err) + } + if _, err := store.GetByTaskID("t-refid-required"); !errors.Is(err, gorm.ErrRecordNotFound) { + t.Errorf("application row exists after a rejected required param, want none (got %v)", err) + } +} diff --git a/backend/internal/refidstore/refidstore.go b/backend/internal/refidstore/refidstore.go index e5a5898..957f6dc 100644 --- a/backend/internal/refidstore/refidstore.go +++ b/backend/internal/refidstore/refidstore.go @@ -4,6 +4,7 @@ package refidstore import ( + "context" "fmt" "github.com/OpenNSW/core/refid" @@ -17,8 +18,8 @@ import ( // 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 migration -// 000010, not by refid's own Migrate helpers. +// 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 { @@ -36,3 +37,22 @@ func New(db *gorm.DB) (refid.SequenceStore, error) { 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 and no counter table to reach for. Every +// Generate fails, so a task declaring a refid block against such a deployment +// is a loud misconfiguration rather than a silent no-op. +// +// Returned as a value rather than a nil Registry so callers keep their +// non-nil-dependency invariants (see application.NewService). +func Disabled() refid.Registry { return disabledRegistry{} } + +type disabledRegistry struct{} + +// Generate implements refid.Registry. It wraps ErrUnknownIssuer so callers +// classifying refid errors treat this like any other unknown format — the +// message just names the actual cause, which "unknown issuer" alone would not. +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 index bbfea9a..ca97ae6 100644 --- a/backend/internal/refidstore/refidstore_test.go +++ b/backend/internal/refidstore/refidstore_test.go @@ -4,6 +4,7 @@ import ( "context" "errors" "path/filepath" + "strings" "testing" "github.com/OpenNSW/agency/backend/internal/refidstore" @@ -146,3 +147,19 @@ func TestRegistry_GeneratesFullID(t *testing.T) { t.Fatalf("Generate returned %q, want %q", got, want) } } + +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) + } +} From d68389fa1daa9847f4ebc89a2b4586d7fa0f570c Mon Sep 17 00:00:00 2001 From: Thanikan Date: Sun, 6 Sep 2026 22:39:37 +0530 Subject: [PATCH 4/6] refactor(refid): trim redundant tests and commentary MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Self-review pass over the PR, no behaviour change. Drop three redundant tests. TestRegistry_GeneratesFullID duplicated the application end-to-end test, which covers strictly more — same real registry and store, plus persistence, and both dialects rather than SQLite only. The orphan-consignment test merged into the unconfigured-deployment one, which shares its setup and trigger. The missing-required-param test folded into the end-to-end test, which already had the registry built and an adjacent rejection case. Cut commentary that states what isn't done rather than what the code does: the task-config doc no longer carries a note about review-payload validation being future work, keeping only the caveat a form author acts on. generateRefID's doc comment was longer than the function; the counter-burn trade-off in CreateApplication belongs in a commit message, not beside the code. Stop naming the migration by number in refidstore's test comment, for the same reason it was dropped elsewhere — it goes stale if migrations are ever collapsed. Both regression checks still catch what they were written for: the old generation ordering still leaves an orphan consignment, and removing the ReviewerResponse carry-forward still loses the ID on re-inject. --- backend/docs/task-config-reference.md | 17 +-- backend/internal/application/refid.go | 22 ++- backend/internal/application/service.go | 6 +- backend/internal/application/service_test.go | 127 ++++++------------ backend/internal/refidstore/refidstore.go | 15 +-- .../internal/refidstore/refidstore_test.go | 56 +------- 6 files changed, 65 insertions(+), 178 deletions(-) diff --git a/backend/docs/task-config-reference.md b/backend/docs/task-config-reference.md index f56010a..781d1db 100644 --- a/backend/docs/task-config-reference.md +++ b/backend/docs/task-config-reference.md @@ -451,16 +451,13 @@ 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`. Every other field of that document stays absent -until the officer fills it in, which is expected: status and `reviewedAt` are -what mark an application reviewed, not a non-empty reviewer response. Note that -a review form whose schema sets `"additionalProperties": false` without -declaring the field would reject the officer's submission. - -> Making that control read-only is up to whoever authors the form, and is a -> client-side convention only: review submissions are not validated -> server-side, so an officer can still overwrite the number. Enforcing that -> properly needs backend validation of the review payload. +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 diff --git a/backend/internal/application/refid.go b/backend/internal/application/refid.go index e1268fb..1f04941 100644 --- a/backend/internal/application/refid.go +++ b/backend/internal/application/refid.go @@ -14,18 +14,13 @@ import ( // response document to store it in, with the ID written at cfg.Path. // // A params pointer that doesn't resolve to a string is skipped rather than -// rejected here, because refid ignores params the configured format doesn't -// consume — so a task may declare more than any one format needs. Whether an -// absent value actually matters is refid's call, not ours: it returns -// ErrInvalidParam for a param a segment requires, and for a scope key left -// with an unresolved placeholder. +// 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 then wraps ErrInvalidInjectRequest (a 400 — the -// injected data couldn't supply a value the format needs). Everything else — -// an issuer/idType this deployment hasn't configured, counter overflow, a -// database failure — stays unwrapped and surfaces as a 500, since those are -// deployment or infrastructure faults rather than anything wrong with the -// request. +// ErrInvalidParam maps to a 400; every other failure stays unwrapped and +// surfaces as a 500. func generateRefID(ctx context.Context, reg refid.Registry, cfg *taskconfig.TaskRefID, data map[string]any) (JSONB, error) { params := make(map[string]string, len(cfg.Params)) for param, pointer := range cfg.Params { @@ -48,9 +43,8 @@ func generateRefID(ctx context.Context, reg refid.Registry, cfg *taskconfig.Task reviewerResponse := JSONB{} if !jsonpointer.Set(reviewerResponse, cfg.Path, id) { - // Unreachable: the document is empty and Validate already checked - // Path is a well-formed pointer. Still an error rather than a - // discard — losing an already-issued ID would be silent corruption. + // Unreachable — the document is empty and Validate already checked + // Path. Still an error: dropping an issued ID would be silent loss. return nil, fmt.Errorf("failed to write reference ID to %q", cfg.Path) } return reviewerResponse, nil diff --git a/backend/internal/application/service.go b/backend/internal/application/service.go index ab97291..7af8552 100644 --- a/backend/internal/application/service.go +++ b/backend/internal/application/service.go @@ -234,10 +234,8 @@ func (s *service) CreateApplication(ctx context.Context, req *InjectRequest) err } // Only for a brand-new application — a re-inject keeps the ID it already - // has. Ahead of CreateConsignment so a generation failure doesn't leave a - // consignment with no application behind it; the cost is a slightly wider - // window in which a crash strands the counter value Generate just claimed, - // which refid tolerates by design (its formats are not gapless). + // has. Kept ahead of CreateConsignment so a generation failure leaves no + // consignment behind. if existing == nil && config.RefID != nil { reviewerResponse, err := generateRefID(ctx, s.refIDs, config.RefID, req.Data) if err != nil { diff --git a/backend/internal/application/service_test.go b/backend/internal/application/service_test.go index 2325b89..ddffd16 100644 --- a/backend/internal/application/service_test.go +++ b/backend/internal/application/service_test.go @@ -2042,66 +2042,49 @@ func TestCreateApplication_RefID_ReinjectKeepsOriginalID(t *testing.T) { } } -func TestCreateApplication_RefID_UnconfiguredDeployment_FailsInject(t *testing.T) { - // unconfiguredRefIDs mirrors a deployment with no refIDGen section. - h := newServiceHarnessWithRefIDs(t, unconfiguredRefIDs(), func(root string) { - writeTaskConfigFile(t, root, "refid_task.json", refIDTaskConfig) - }) - - err := h.service.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) - } - // Fail-closed: no application may exist without its reference ID. - if _, err := h.store.GetByTaskID("t-refid-3"); !errors.Is(err, gorm.ErrRecordNotFound) { - t.Errorf("application row exists after a failed generation, want none (got %v)", err) - } -} - -// TestCreateApplication_RefID_GenerationFailure_LeavesNoConsignment pins the -// ordering: generation runs ahead of CreateConsignment, so a failure leaves -// nothing behind at all. +// 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 the row, so with the default stub server -// (whose fetch fails) no consignment is ever created and the assertion below -// would hold regardless of ordering — i.e. be vacuous. -func TestCreateApplication_RefID_GenerationFailure_LeavesNoConsignment(t *testing.T) { +// 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-orphan", + ConsignmentID: "c-refid-3", TraderCompanyName: "CEYLON EXPORTS", }} - // unconfiguredRefIDs makes generation fail the way a deployment missing - // the format would. + // 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-orphan", + TaskID: "t-refid-3", TaskCode: "refid_task", - ConsignmentID: "c-refid-orphan", + ConsignmentID: "c-refid-3", Data: map[string]any{"nppo_office_location": "NPQS-KAT"}, }) if err == nil { - t.Fatal("expected the inject to fail when no format is configured") + 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-orphan").Count(&consignments).Error; err != nil { + Where("id = ?", "c-refid-3").Count(&consignments).Error; err != nil { t.Fatalf("counting consignments: %v", err) } if consignments != 0 { @@ -2226,6 +2209,22 @@ func TestCreateApplication_RefID_RealRegistry_EndToEnd(t *testing.T) { 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, @@ -2322,51 +2321,3 @@ func TestCreateApplication_RefID_UnusedParamNotResolved_StillGenerates(t *testin t.Errorf("officeCode = %q, want \"NPQS-KAT\"", stub.calls[0].params["officeCode"]) } } - -func TestCreateApplication_RefID_RequiredParamMissing_RejectedByFormat(t *testing.T) { - // A required param is now refid's call, not ours: with officeCode absent - // the list segment fails, and ErrInvalidParam still maps to a 400. - 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: "list", List: "office_location", Param: "officeCode"}, - {Type: "sequence", ScopeKey: "{issuer}:{idType}:{officeCode}", Padding: 6}, - }, - }}, - }}, - Lists: map[string][]string{"office_location": {"NPQS-KAT"}}, - }, 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) - - err = svc.CreateApplication(context.Background(), &InjectRequest{ - TaskID: "t-refid-required", - TaskCode: "refid_task", - ConsignmentID: "c-refid-required", - Data: map[string]any{"something_else": "x"}, - }) - if !errors.Is(err, ErrInvalidInjectRequest) { - t.Fatalf("CreateApplication returned %v, want ErrInvalidInjectRequest", err) - } - if _, err := store.GetByTaskID("t-refid-required"); !errors.Is(err, gorm.ErrRecordNotFound) { - t.Errorf("application row exists after a rejected required param, want none (got %v)", err) - } -} diff --git a/backend/internal/refidstore/refidstore.go b/backend/internal/refidstore/refidstore.go index 957f6dc..c5de646 100644 --- a/backend/internal/refidstore/refidstore.go +++ b/backend/internal/refidstore/refidstore.go @@ -39,19 +39,16 @@ func New(db *gorm.DB) (refid.SequenceStore, error) { } // Disabled returns a Registry for a deployment with no refIDGen section, where -// there is no format to generate from and no counter table to reach for. Every -// Generate fails, so a task declaring a refid block against such a deployment -// is a loud misconfiguration rather than a silent no-op. -// -// Returned as a value rather than a nil Registry so callers keep their -// non-nil-dependency invariants (see application.NewService). +// 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 implements refid.Registry. It wraps ErrUnknownIssuer so callers -// classifying refid errors treat this like any other unknown format — the -// message just names the actual cause, which "unknown issuer" alone would not. +// 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 index ca97ae6..538031c 100644 --- a/backend/internal/refidstore/refidstore_test.go +++ b/backend/internal/refidstore/refidstore_test.go @@ -14,9 +14,9 @@ import ( "gorm.io/gorm/logger" ) -// refidSequencesDDL mirrors the sqlite branch of -// migrations/000010_create_refid_sequences.sql. Unit tests don't replay the -// migrator, so the table is created here — keep the two in sync. +// 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, @@ -98,56 +98,6 @@ func TestNext_CounterOverflow(t *testing.T) { t.Fatalf("Next past max returned %v, want refid.ErrCounterOverflow", err) } } - -// TestRegistry_GeneratesFullID drives a real refid config end to end, so the -// padding, list validation and scope-key resolution are all exercised against -// this module's driver rather than just the raw counter. -func TestRegistry_GeneratesFullID(t *testing.T) { - cfg := 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"}}, - } - - reg, err := refid.NewRegistry(cfg, newTestStore(t)) - if err != nil { - t.Fatalf("NewRegistry: %v", err) - } - ctx := context.Background() - - got, err := reg.Generate(ctx, "NPQS", "application_id", map[string]string{"officeCode": "NPQS-KAT"}) - if err != nil { - t.Fatalf("Generate: %v", err) - } - if want := "NPQS/NPQS-KAT/000001"; got != want { - t.Fatalf("Generate returned %q, want %q", got, want) - } - - // A value outside the configured list must not reach the counter. - if _, err := reg.Generate(ctx, "NPQS", "application_id", map[string]string{"officeCode": "NOPE"}); !errors.Is(err, refid.ErrInvalidParam) { - t.Fatalf("Generate with unlisted office returned %v, want refid.ErrInvalidParam", err) - } - - // ... and the next valid call is 2, not 3 — the rejected call was side-effect free. - got, err = reg.Generate(ctx, "NPQS", "application_id", map[string]string{"officeCode": "NPQS-KAT"}) - if err != nil { - t.Fatalf("Generate: %v", err) - } - if want := "NPQS/NPQS-KAT/000002"; got != want { - t.Fatalf("Generate returned %q, want %q", got, want) - } -} - func TestDisabled_GenerateAlwaysFails(t *testing.T) { _, err := refidstore.Disabled().Generate(context.Background(), "NPQS", "application_id", nil) if err == nil { From d5e5d61fd0829e026f05b0650e9fd54bc3ebad75 Mon Sep 17 00:00:00 2001 From: Thanikan Date: Mon, 7 Sep 2026 22:44:27 +0530 Subject: [PATCH 5/6] refactor(refid): generateRefID returns the ID, not a document MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit generateRefID built a fresh JSONB and returned it for the caller to assign, which made two failures possible the moment anything changed: a caller running it against an existing reviewer response would silently discard that document, and the error path returned a nil map that nulls the field if the error is ever mishandled. It now returns a string, and CreateApplication owns the write. Fold the three consecutive `existing` checks into one if/else while here. They were mutually exclusive already, which is the only reason the reference ID write could not clobber a carried-forward reviewer response — as a single branch that safety is structural rather than incidental, and the new-application branch provably starts with no reviewer response, so no defensive nil check is needed. --- backend/internal/application/refid.go | 18 ++++--------- backend/internal/application/service.go | 34 ++++++++++++------------- 2 files changed, 21 insertions(+), 31 deletions(-) diff --git a/backend/internal/application/refid.go b/backend/internal/application/refid.go index 1f04941..bb212bd 100644 --- a/backend/internal/application/refid.go +++ b/backend/internal/application/refid.go @@ -10,8 +10,7 @@ import ( "github.com/OpenNSW/core/refid" ) -// generateRefID mints this task's reference ID and returns the reviewer -// response document to store it in, with the ID written at cfg.Path. +// 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 @@ -21,7 +20,7 @@ import ( // // ErrInvalidParam maps to a 400; every other failure stays unwrapped and // surfaces as a 500. -func generateRefID(ctx context.Context, reg refid.Registry, cfg *taskconfig.TaskRefID, data map[string]any) (JSONB, error) { +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) @@ -36,16 +35,9 @@ func generateRefID(ctx context.Context, reg refid.Registry, cfg *taskconfig.Task id, err := reg.Generate(ctx, cfg.Issuer, cfg.IDType, params) if err != nil { if errors.Is(err, refid.ErrInvalidParam) { - return nil, fmt.Errorf("%w: %v", ErrInvalidInjectRequest, err) + return "", fmt.Errorf("%w: %v", ErrInvalidInjectRequest, err) } - return nil, fmt.Errorf("failed to generate reference ID for issuer %q idType %q: %w", cfg.Issuer, cfg.IDType, err) + return "", fmt.Errorf("failed to generate reference ID for issuer %q idType %q: %w", cfg.Issuer, cfg.IDType, err) } - - reviewerResponse := JSONB{} - if !jsonpointer.Set(reviewerResponse, cfg.Path, id) { - // Unreachable — the document is empty and Validate already checked - // Path. Still an error: dropping an issued ID would be silent loss. - return nil, fmt.Errorf("failed to write reference ID to %q", cfg.Path) - } - return reviewerResponse, nil + return id, nil } diff --git a/backend/internal/application/service.go b/backend/internal/application/service.go index 7af8552..2727f41 100644 --- a/backend/internal/application/service.go +++ b/backend/internal/application/service.go @@ -226,25 +226,26 @@ func (s *service) CreateApplication(ctx context.Context, req *InjectRequest) err // 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, and - // the reviewer response so a re-inject doesn't destroy an - // already-issued reference ID (see generateRefID). + // the reviewer response so a re-inject keeps its reference ID. appRecord.ClaimedBy = existing.ClaimedBy appRecord.ClaimedAt = existing.ClaimedAt appRecord.ReviewerResponse = existing.ReviewerResponse - } - - // Only for a brand-new application — a re-inject keeps the ID it already - // has. Kept ahead of CreateConsignment so a generation failure leaves no - // consignment behind. - if existing == nil && config.RefID != nil { - reviewerResponse, err := generateRefID(ctx, s.refIDs, config.RefID, req.Data) - if err != nil { - return err + } 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) + } } - appRecord.ReviewerResponse = reviewerResponse - } - 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", @@ -252,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 From c692219c186981b0a0fddaec232e925fa62e4d8c Mon Sep 17 00:00:00 2001 From: Thanikan Date: Tue, 8 Sep 2026 14:07:17 +0530 Subject: [PATCH 6/6] refactor(refid): extract initRefIDs, drop HTTP codes from service docs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Move the reference ID wiring out of main() into initRefIDs, which returns an error rather than calling log.Fatalf so it is testable. Three tests cover it, including that a deployment with no refIDGen section never reaches the database — the nil *gorm.DB they pass is the assertion. generateRefID's doc comment described its errors as a 400 and a 500. It isn't an HTTP handler and has no business naming status codes; it now says which sentinel it wraps and leaves the mapping to the handler. --- backend/cmd/server/main.go | 48 ++++++++++------ backend/cmd/server/refid_test.go | 79 +++++++++++++++++++++++++++ backend/internal/application/refid.go | 5 +- 3 files changed, 112 insertions(+), 20 deletions(-) create mode 100644 backend/cmd/server/refid_test.go diff --git a/backend/cmd/server/main.go b/backend/cmd/server/main.go index f5e782f..7050022 100644 --- a/backend/cmd/server/main.go +++ b/backend/cmd/server/main.go @@ -32,6 +32,7 @@ import ( "github.com/OpenNSW/core/authz" "github.com/OpenNSW/core/refid" "github.com/OpenNSW/core/trace" + "gorm.io/gorm" ) func main() { @@ -161,24 +162,9 @@ func main() { consignmentService := consignment.NewService(consignmentStore, nswClient, dataScopeResolver) consignmentHandler := consignment.NewHandler(consignmentService) - // Reference ID generation is optional per deployment: with no refIDGen - // section there is nothing to build, so skip the counter store and the - // registry entirely and hand the service a disabled one. NewRegistry - // validates every configured format up front, so a malformed section fails - // the boot rather than the first inject that needs it. - refIDs := refidstore.Disabled() - if n := len(cfg.RefIDGen.Issuers); n > 0 { - refIDSequences, err := refidstore.New(store.DB()) - if err != nil { - log.Fatalf("failed to create refid sequence store: %v", err) - } - refIDs, err = refid.NewRegistry(cfg.RefIDGen, refIDSequences) - if err != nil { - log.Fatalf("invalid refIDGen config: %v", err) - } - slog.Info("reference ID generation configured", "issuers", n) - } else { - slog.Info("reference ID generation not configured; tasks declaring a refid block will fail at inject") + refIDs, err := initRefIDs(cfg.RefIDGen, store.DB()) + if err != nil { + log.Fatalf("failed to initialize reference ID generation: %v", err) } // Initialize Agency service @@ -347,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/internal/application/refid.go b/backend/internal/application/refid.go index bb212bd..0d229ae 100644 --- a/backend/internal/application/refid.go +++ b/backend/internal/application/refid.go @@ -18,8 +18,9 @@ import ( // matters is refid's call — it returns ErrInvalidParam for a param a segment // requires, and for an unresolved scope-key placeholder. // -// ErrInvalidParam maps to a 400; every other failure stays unwrapped and -// surfaces as a 500. +// 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 {