From 2555fe77ca7b010a24c23173b0cad28569a8ab06 Mon Sep 17 00:00:00 2001 From: Joshua Hawxwell Date: Thu, 4 Jan 2024 12:02:46 +0000 Subject: [PATCH] Refactor changes by introducing a parser --- .github/workflows/env-test.yml | 2 +- .github/workflows/workflow-pr.yml | 3 + docker-compose.yml | 2 +- go.mod | 2 +- go.sum | 3 + lambda/update/attorney_sign.go | 92 ++---- lambda/update/attorney_sign_test.go | 10 +- lambda/update/certificate_provider_sign.go | 97 ++---- .../update/certificate_provider_sign_test.go | 6 +- lambda/update/parse/changes.go | 236 +++++++++++++++ lambda/update/parse/changes_test.go | 282 ++++++++++++++++++ lambda/update/trust_corporation_sign.go | 160 +++------- lambda/update/trust_corporation_sign_test.go | 10 +- lambda/update/validate.go | 12 - 14 files changed, 629 insertions(+), 288 deletions(-) create mode 100644 lambda/update/parse/changes.go create mode 100644 lambda/update/parse/changes_test.go diff --git a/.github/workflows/env-test.yml b/.github/workflows/env-test.yml index 97f522c3..44a20dc4 100644 --- a/.github/workflows/env-test.yml +++ b/.github/workflows/env-test.yml @@ -35,7 +35,7 @@ jobs: fetch-depth: "0" - uses: actions/setup-go@v5 with: - go-version: "1.20" + go-version: "1.21" - uses: unfor19/install-aws-cli-action@v1 - name: Configure AWS uses: aws-actions/configure-aws-credentials@v4 diff --git a/.github/workflows/workflow-pr.yml b/.github/workflows/workflow-pr.yml index 5adbe933..b2852421 100644 --- a/.github/workflows/workflow-pr.yml +++ b/.github/workflows/workflow-pr.yml @@ -39,6 +39,9 @@ jobs: - uses: actions/checkout@v4 with: fetch-depth: "0" + - uses: actions/setup-go@v5 + with: + go-version: "1.21" - run: make check-code - name: Upload SARIF file uses: github/codeql-action/upload-sarif@v3 diff --git a/docker-compose.yml b/docker-compose.yml index dc284e44..be56f5fc 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -81,7 +81,7 @@ services: working_dir: /go/src/app volumes: - ./:/go/src/app - - ./.cache/golangci-lint/v1.53.3:/root/.cache + - ./.cache/golangci-lint/v1.55.2:/root/.cache command: golangci-lint run --timeout 5m --out-format=github-actions ./lambda/get/... ./lambda/create/... ./lambda/update/... gosec: diff --git a/go.mod b/go.mod index d0902eda..39504995 100644 --- a/go.mod +++ b/go.mod @@ -1,6 +1,6 @@ module github.com/ministryofjustice/opg-data-lpa-store -go 1.20 +go 1.21 require ( github.com/aws/aws-lambda-go v1.43.0 diff --git a/go.sum b/go.sum index 90905d11..6ba3536e 100644 --- a/go.sum +++ b/go.sum @@ -1,4 +1,5 @@ github.com/DATA-DOG/go-sqlmock v1.4.1 h1:ThlnYciV1iM/V0OSF/dtkqWb6xo5qITT1TJBG1MRDJM= +github.com/DATA-DOG/go-sqlmock v1.4.1/go.mod h1:f/Ixk793poVmq4qj/V1dPUg2JEAKC73Q5eFN3EC/SaM= github.com/andybalholm/brotli v1.0.6 h1:Yf9fFpf49Zrxb9NlQaluyE92/+X7UVHlhMNJN2sxfOI= github.com/andybalholm/brotli v1.0.6/go.mod h1:fO7iG3H7G2nSZ7m0zPUDn85XEX2GTukHGRSepvi9Eig= github.com/aws/aws-lambda-go v1.43.0 h1:Tdu7SnMB5bD+CbdnSq1Dg4sM68vEuGIDcQFZ+IjUfx0= @@ -18,9 +19,11 @@ github.com/golang/protobuf v1.5.3 h1:KhyjKVUg7Usr/dYsdSqoFveMYd5ko72D+zANwlG1mmg github.com/golang/protobuf v1.5.3/go.mod h1:XVQd3VNwM+JqD3oG2Ue2ip4fOMUkwXdXDdiuN0vRsmY= github.com/google/go-cmp v0.5.5/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE= github.com/google/go-cmp v0.5.9 h1:O2Tfq5qg4qc4AmwVlvv0oLiVAGB7enBSJ2x2DqQFi38= +github.com/google/go-cmp v0.5.9/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY= github.com/google/uuid v1.5.0 h1:1p67kYwdtXjb0gL0BPiP1Av9wiZPo5A8z2cWkTZ+eyU= github.com/google/uuid v1.5.0/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo= github.com/grpc-ecosystem/go-grpc-middleware v1.3.0 h1:+9834+KizmvFV7pXQGSXQTsaWhq2GjuNUt0aUU0YBYw= +github.com/grpc-ecosystem/go-grpc-middleware v1.3.0/go.mod h1:z0ButlSOZa5vEBq9m2m2hlwIgKw+rp3sdCBRoJY+30Y= github.com/jmespath/go-jmespath v0.4.0 h1:BEgLn5cpjn8UN1mAw4NjwDrS35OdebyEtFe+9YPoQUg= github.com/jmespath/go-jmespath v0.4.0/go.mod h1:T8mJZnbsbmF+m6zOOFylbeCJqk5+pHWvzYPziyZiYoo= github.com/jmespath/go-jmespath/internal/testify v1.5.1 h1:shLQSRRSCCPj3f2gpwzGwWFoC7ycTf1rcQZHOlsJ6N8= diff --git a/lambda/update/attorney_sign.go b/lambda/update/attorney_sign.go index 55f7c52a..1afaf5e7 100644 --- a/lambda/update/attorney_sign.go +++ b/lambda/update/attorney_sign.go @@ -1,15 +1,11 @@ package main import ( - "bytes" - "encoding/json" - "fmt" - "strconv" - "strings" "time" "github.com/ministryofjustice/opg-data-lpa-store/internal/shared" "github.com/ministryofjustice/opg-data-lpa-store/internal/validate" + "github.com/ministryofjustice/opg-data-lpa-store/lambda/update/parse" ) type AttorneySign struct { @@ -31,67 +27,31 @@ func (a AttorneySign) Apply(lpa *shared.Lpa) []shared.FieldError { return nil } -func validateAttorneySign(changes []shared.Change) (data AttorneySign, errors []shared.FieldError) { - for i, change := range changes { - after, ok := strings.CutPrefix(change.Key, "/attorneys/") - if !ok { - errors = append(errors, shared.FieldError{Source: fmt.Sprintf("/changes/%d", i), Detail: "change not allowed for type"}) - continue - } - - if !bytes.Equal(change.Old, []byte("null")) { - errors = append(errors, shared.FieldError{Source: fmt.Sprintf("/changes/%d/old", i), Detail: "field must be null"}) - } - - parts := strings.SplitN(after, "/", 2) - attorneyIndex, err := strconv.Atoi(parts[0]) - if err != nil { - errors = append(errors, shared.FieldError{Source: fmt.Sprintf("/changes/%d", i), Detail: "change not allowed for type"}) - continue - } - if data.Index != nil && *data.Index != attorneyIndex { - errors = append(errors, shared.FieldError{Source: fmt.Sprintf("/changes/%d/key", i), Detail: "must be for same attorney"}) - continue - } else { - data.Index = &attorneyIndex - } - - newKey := fmt.Sprintf("/changes/%d/new", i) - switch parts[1] { - case "mobile": - if err := json.Unmarshal(change.New, &data.Mobile); err != nil { - errors = errorMustBeString(errors, newKey) - } - case "signedAt": - if err := json.Unmarshal(change.New, &data.SignedAt); err != nil { - errors = errorMustBeDateTime(errors, newKey) - } - case "contactLanguagePreference": - if err := json.Unmarshal(change.New, &data.ContactLanguagePreference); err != nil { - errors = errorMustBeString(errors, newKey) - } else { - errors = append(errors, validate.IsValid(newKey, data.ContactLanguagePreference)...) - } - default: - errors = append(errors, shared.FieldError{Source: fmt.Sprintf("/changes/%d", i), Detail: "change not allowed for type"}) - } - } - - if data.Index == nil { - errors = append(errors, shared.FieldError{Source: "/changes", Detail: "must be specified"}) - } else { - if data.Mobile == "" { - errors = errorMissing(errors, fmt.Sprintf("/attorneys/%d/mobile", *data.Index)) - } - - if data.SignedAt.IsZero() { - errors = errorMissing(errors, fmt.Sprintf("/attorneys/%d/signedAt", *data.Index)) - } - - if data.ContactLanguagePreference == shared.Lang("") { - errors = errorMissing(errors, fmt.Sprintf("/attorneys/%d/contactLanguagePreference", *data.Index)) - } - } +func validateAttorneySign(changes []shared.Change) (AttorneySign, []shared.FieldError) { + var data AttorneySign + + errors := parse.Changes(changes). + Prefix("/attorneys", func(p *parse.Parser) []shared.FieldError { + return p. + Each(func(i int, p *parse.Parser) []shared.FieldError { + if data.Index != nil && *data.Index != i { + return p.OutOfRange() + } + + data.Index = &i + return p. + Field("/mobile", &data.Mobile). + Field("/signedAt", &data.SignedAt, parse.Validate(func() []shared.FieldError { + return validate.Time("", data.SignedAt) + })). + Field("/contactLanguagePreference", &data.ContactLanguagePreference, parse.Validate(func() []shared.FieldError { + return validate.IsValid("", data.ContactLanguagePreference) + })). + Consumed() + }). + Consumed() + }). + Consumed() return data, errors } diff --git a/lambda/update/attorney_sign_test.go b/lambda/update/attorney_sign_test.go index 2c385fb8..304408a9 100644 --- a/lambda/update/attorney_sign_test.go +++ b/lambda/update/attorney_sign_test.go @@ -73,7 +73,7 @@ func TestValidateUpdateAttorneySign(t *testing.T) { "missing all": { update: shared.Update{Type: "ATTORNEY_SIGN"}, errors: []shared.FieldError{ - {Source: "/changes", Detail: "must be specified"}, + {Source: "/changes", Detail: "missing /attorneys/..."}, }, }, "extra fields": { @@ -108,9 +108,9 @@ func TestValidateUpdateAttorneySign(t *testing.T) { }, }, errors: []shared.FieldError{ - {Source: "/changes/2/old", Detail: "field must be null"}, - {Source: "/changes/3", Detail: "change not allowed for type"}, - {Source: "/changes/4", Detail: "change not allowed for type"}, + {Source: "/changes/2/old", Detail: "must be null"}, + {Source: "/changes/3", Detail: "unexpected change provided"}, + {Source: "/changes/4", Detail: "unexpected change provided"}, }, }, "invalid contact language": { @@ -160,7 +160,7 @@ func TestValidateUpdateAttorneySign(t *testing.T) { }, }, errors: []shared.FieldError{ - {Source: "/changes/1/key", Detail: "must be for same attorney"}, + {Source: "/changes/1/key", Detail: "index out of range"}, {Source: "/changes", Detail: "missing /attorneys/0/signedAt"}, }, }, diff --git a/lambda/update/certificate_provider_sign.go b/lambda/update/certificate_provider_sign.go index b9369aec..d8b418cb 100644 --- a/lambda/update/certificate_provider_sign.go +++ b/lambda/update/certificate_provider_sign.go @@ -1,13 +1,11 @@ package main import ( - "bytes" - "encoding/json" - "fmt" "time" "github.com/ministryofjustice/opg-data-lpa-store/internal/shared" "github.com/ministryofjustice/opg-data-lpa-store/internal/validate" + "github.com/ministryofjustice/opg-data-lpa-store/lambda/update/parse" ) type CertificateProviderSign struct { @@ -28,76 +26,29 @@ func (c CertificateProviderSign) Apply(lpa *shared.Lpa) []shared.FieldError { return nil } -func validateCertificateProviderSign(changes []shared.Change) (data CertificateProviderSign, errors []shared.FieldError) { - for i, change := range changes { - if !bytes.Equal(change.Old, []byte("null")) { - errors = append(errors, shared.FieldError{Source: fmt.Sprintf("/changes/%d/old", i), Detail: "field must be null"}) - } - - newKey := fmt.Sprintf("/changes/%d/new", i) - switch change.Key { - case "/certificateProvider/address/line1": - if err := json.Unmarshal(change.New, &data.Address.Line1); err != nil { - errors = errorMustBeString(errors, newKey) - } - case "/certificateProvider/address/line2": - if err := json.Unmarshal(change.New, &data.Address.Line2); err != nil { - errors = errorMustBeString(errors, newKey) - } - case "/certificateProvider/address/line3": - if err := json.Unmarshal(change.New, &data.Address.Line3); err != nil { - errors = errorMustBeString(errors, newKey) - } - case "/certificateProvider/address/town": - if err := json.Unmarshal(change.New, &data.Address.Town); err != nil { - errors = errorMustBeString(errors, newKey) - } - case "/certificateProvider/address/postcode": - if err := json.Unmarshal(change.New, &data.Address.Postcode); err != nil { - errors = errorMustBeString(errors, newKey) - } - case "/certificateProvider/address/country": - if err := json.Unmarshal(change.New, &data.Address.Country); err != nil { - errors = errorMustBeString(errors, newKey) - } else { - errors = append(errors, validate.Country(newKey, data.Address.Country)...) - } - case "/certificateProvider/signedAt": - if err := json.Unmarshal(change.New, &data.SignedAt); err != nil { - errors = errorMustBeDateTime(errors, newKey) - } - case "/certificateProvider/contactLanguagePreference": - if err := json.Unmarshal(change.New, &data.ContactLanguagePreference); err != nil { - errors = errorMustBeString(errors, newKey) - } else { - errors = append(errors, validate.IsValid(newKey, data.ContactLanguagePreference)...) - } - default: - errors = append(errors, shared.FieldError{Source: fmt.Sprintf("/changes/%d", i), Detail: "change not allowed for type"}) - } - } - - if !data.Address.IsZero() { - if data.Address.Line1 == "" { - errors = errorMissing(errors, "/certificateProvider/address/line1") - } - - if data.Address.Town == "" { - errors = errorMissing(errors, "/certificateProvider/address/town") - } - - if data.Address.Country == "" { - errors = errorMissing(errors, "/certificateProvider/address/country") - } - } - - if data.SignedAt.IsZero() { - errors = errorMissing(errors, "/certificateProvider/signedAt") - } - - if data.ContactLanguagePreference == shared.Lang("") { - errors = errorMissing(errors, "/certificateProvider/contactLanguagePreference") - } +func validateCertificateProviderSign(changes []shared.Change) (CertificateProviderSign, []shared.FieldError) { + var data CertificateProviderSign + + errors := parse.Changes(changes). + Prefix("/certificateProvider/address", func(p *parse.Parser) []shared.FieldError { + return p. + Field("/line1", &data.Address.Line1). + Field("/line2", &data.Address.Line2, parse.Optional()). + Field("/line3", &data.Address.Line3, parse.Optional()). + Field("/town", &data.Address.Town). + Field("/postcode", &data.Address.Postcode, parse.Optional()). + Field("/country", &data.Address.Country, parse.Validate(func() []shared.FieldError { + return validate.Country("", data.Address.Country) + })). + Consumed() + }, parse.Optional()). + Field("/certificateProvider/signedAt", &data.SignedAt, parse.Validate(func() []shared.FieldError { + return validate.Time("", data.SignedAt) + })). + Field("/certificateProvider/contactLanguagePreference", &data.ContactLanguagePreference, parse.Validate(func() []shared.FieldError { + return validate.IsValid("", data.ContactLanguagePreference) + })). + Consumed() return data, errors } diff --git a/lambda/update/certificate_provider_sign_test.go b/lambda/update/certificate_provider_sign_test.go index b44af610..2d00eaf0 100644 --- a/lambda/update/certificate_provider_sign_test.go +++ b/lambda/update/certificate_provider_sign_test.go @@ -95,7 +95,7 @@ func TestValidateUpdateCertificateProviderSign(t *testing.T) { }, }, errors: []shared.FieldError{ - {Source: "/changes/0/new", Detail: "must be a string"}, + {Source: "/changes/0/new", Detail: "unexpected type"}, {Source: "/changes", Detail: "missing /certificateProvider/address/line1"}, {Source: "/changes", Detail: "missing /certificateProvider/address/town"}, {Source: "/changes/1/new", Detail: "must be a valid ISO-3166-1 country code"}, @@ -125,8 +125,8 @@ func TestValidateUpdateCertificateProviderSign(t *testing.T) { }, }, errors: []shared.FieldError{ - {Source: "/changes/1/old", Detail: "field must be null"}, - {Source: "/changes/2", Detail: "change not allowed for type"}, + {Source: "/changes/1/old", Detail: "must be null"}, + {Source: "/changes/2", Detail: "unexpected change provided"}, }, }, "invalid contact language": { diff --git a/lambda/update/parse/changes.go b/lambda/update/parse/changes.go new file mode 100644 index 00000000..2e0863bc --- /dev/null +++ b/lambda/update/parse/changes.go @@ -0,0 +1,236 @@ +package parse + +import ( + "bytes" + "encoding/json" + "fmt" + "slices" + "strconv" + "strings" + + "github.com/ministryofjustice/opg-data-lpa-store/internal/shared" +) + +type changeWithPosition struct { + shared.Change + pos int +} + +func (p changeWithPosition) Source(after string) string { + return fmt.Sprintf("/changes/%d%s", p.pos, after) +} + +type Parser struct { + root string + changes []changeWithPosition + errors []shared.FieldError +} + +// Changes constructs a new [Parser] for a set of changes. +func Changes(changes []shared.Change) *Parser { + cs := make([]changeWithPosition, len(changes)) + for i, change := range changes { + cs[i] = changeWithPosition{Change: change, pos: i} + } + + return &Parser{changes: cs} +} + +// Consumed checks the [Parser] has used all of the changes. It adds an error for any unparsed changes. +func (p *Parser) Consumed() []shared.FieldError { + for _, change := range p.changes { + p.errors = append(p.errors, shared.FieldError{Source: change.Source(""), Detail: "unexpected change provided"}) + } + + return p.errors +} + +// OutOfRange can be used with [Parser.Each] when the index is not in an expected range. It adds an out of range error for all changes. +func (p *Parser) OutOfRange() []shared.FieldError { + for _, change := range p.changes { + p.errors = append(p.errors, shared.FieldError{Source: change.Source("/key"), Detail: "index out of range"}) + } + + return p.errors +} + +// Errors returns the current error list for the Parser. +func (p *Parser) Errors() []shared.FieldError { + return p.errors +} + +type Option func(fieldOpts) fieldOpts + +type fieldOpts struct { + optional bool + validator func() []shared.FieldError +} + +// Optional stops [Parser.Field] or [Parser.Prefix] from adding an error when the expected key is missing. +func Optional() func(fieldOpts) fieldOpts { + return func(f fieldOpts) fieldOpts { + f.optional = true + return f + } +} + +// Validate runs fn on the [Parser.Field] after unmarshalling. It has no effect when passed to [Parser.Prefix]. +func Validate(fn func() []shared.FieldError) Option { + return func(f fieldOpts) fieldOpts { + f.validator = fn + return f + } +} + +// Field will JSON unmarshal the specified key into v. It will add an error if +// the key does not exist or if the old field was specified other than null. +// +// Consider the change: +// +// {"key": "/thing", "old": null, "new": "a string"} +// +// Then to parse to a string s do: +// +// parser.Field("/thing", &s) +func (p *Parser) Field(key string, v any, opts ...Option) *Parser { + options := fieldOpts{} + for _, opt := range opts { + options = opt(options) + } + + for i, change := range p.changes { + if change.Key == key { + if !bytes.Equal(change.Old, []byte("null")) { + p.errors = append(p.errors, shared.FieldError{Source: change.Source("/old"), Detail: "must be null"}) + } + + if err := json.Unmarshal(change.New, v); err != nil { + p.errors = append(p.errors, shared.FieldError{Source: change.Source("/new"), Detail: "unexpected type"}) + } else if options.validator != nil { + for _, error := range options.validator() { + p.errors = append(p.errors, shared.FieldError{Source: change.Source("/new"), Detail: error.Detail}) + } + } + + p.changes = slices.Delete(p.changes, i, i+1) + return p + } + } + + if !options.optional { + p.errors = append(p.errors, shared.FieldError{Source: "/changes", Detail: "missing " + p.root + key}) + } + return p +} + +// Each will run fn with a [Parser] for any indexed keys. If required is specified +// then those indexes must exist. +// +// Consider the changes: +// +// {"key": "/0/thing", "old": null, "new": "a string"} +// {"key": "/1/thing", "old": null, "new": "another string"} +// +// Then to parse to a list of strings s do: +// +// parser.Each(func(i int, p *Parser) { +// var v string +// p.Field("/thing", v) +// s = append(s, v) +// return p.Consumed() +// }) +func (p *Parser) Each(fn func(int, *Parser) []shared.FieldError, required ...int) *Parser { + indexedChanges := map[int][]changeWithPosition{} + + for _, idx := range required { + indexedChanges[idx] = []changeWithPosition{} + } + + for _, change := range p.changes { + parts := strings.SplitN(change.Key, "/", 3) + if len(parts) != 3 || parts[0] != "" { + p.errors = append(p.errors, shared.FieldError{Source: change.Source("/key"), Detail: "require index"}) + continue + } + + idx, err := strconv.Atoi(parts[1]) + if err != nil { + p.errors = append(p.errors, shared.FieldError{Source: change.Source("/key"), Detail: "require index"}) + continue + } + + indexedChanges[idx] = append(indexedChanges[idx], changeWithPosition{ + Change: shared.Change{Key: "/" + parts[2], Old: change.Old, New: change.New}, + pos: change.pos, + }) + } + + // because we should be going through all the changes, or they 'require index' so are not valid to use + p.changes = []changeWithPosition{} + + // so we always run through in a consistent order + indexes := make([]int, 0, len(indexedChanges)) + for k := range indexedChanges { + indexes = append(indexes, k) + } + slices.Sort(indexes) + + for _, idx := range indexes { + changes := indexedChanges[idx] + subParser := &Parser{root: p.root + "/" + strconv.Itoa(idx), changes: changes} + fn(idx, subParser) + p.errors = append(p.errors, subParser.errors...) + } + + return p +} + +// Prefix will run fn with a [Parser] of any changes with the specified prefix. It +// will add an error if the prefix does not exist. +// +// Consider the changes: +// +// {"key": "/thing/name", "old": null, "new": "a string"} +// {"key": "/thing/size", "old": null, "new": 5} +// +// Then to parse to a Thing t do: +// +// parser.Prefix("/thing", func(p *Parser) { +// return p. +// Field("/name", &t.Name). +// Field("/size", &t.Size). +// Consumed() +// }) +func (p *Parser) Prefix(prefix string, fn func(*Parser) []shared.FieldError, opts ...Option) *Parser { + var matching, remaining []changeWithPosition + + options := fieldOpts{} + for _, opt := range opts { + options = opt(options) + } + + for _, change := range p.changes { + if strings.HasPrefix(change.Key, prefix+"/") { + matching = append(matching, changeWithPosition{ + Change: shared.Change{Key: change.Key[len(prefix):], Old: change.Old, New: change.New}, + pos: change.pos, + }) + } else { + remaining = append(remaining, change) + } + } + + p.changes = remaining + + if len(matching) == 0 { + if !options.optional { + p.errors = append(p.errors, shared.FieldError{Source: "/changes", Detail: "missing " + p.root + prefix + "/..."}) + } + } else { + subParser := &Parser{root: p.root + prefix, changes: matching} + fn(subParser) + p.errors = append(p.errors, subParser.errors...) + } + + return p +} diff --git a/lambda/update/parse/changes_test.go b/lambda/update/parse/changes_test.go new file mode 100644 index 00000000..9d3043a4 --- /dev/null +++ b/lambda/update/parse/changes_test.go @@ -0,0 +1,282 @@ +package parse + +import ( + "encoding/json" + "testing" + + "github.com/ministryofjustice/opg-data-lpa-store/internal/shared" + "github.com/stretchr/testify/assert" +) + +var jsonNull = json.RawMessage("null") + +func TestField(t *testing.T) { + changes := []shared.Change{ + {Key: "/thing", New: json.RawMessage(`"val"`), Old: jsonNull}, + } + + var v string + Changes(changes).Field("/thing", &v) + + assert.Equal(t, "val", v) +} + +func TestFieldWhenMissing(t *testing.T) { + changes := []shared.Change{} + + var v string + errors := Changes(changes).Field("/thing", &v).Errors() + + assert.Equal(t, []shared.FieldError{{Source: "/changes", Detail: "missing /thing"}}, errors) +} + +func TestFieldWhenWrongType(t *testing.T) { + changes := []shared.Change{ + {Key: "/thing", New: json.RawMessage(`"val"`), Old: jsonNull}, + } + + var v int + errors := Changes(changes).Field("/thing", &v).Errors() + + assert.Equal(t, []shared.FieldError{{Source: "/changes/0/new", Detail: "unexpected type"}}, errors) +} + +func TestFieldOptional(t *testing.T) { + changes := []shared.Change{ + {Key: "/thing", New: json.RawMessage(`"val"`), Old: jsonNull}, + } + + var v string + Changes(changes).Field("/thing", &v, Optional()) + + assert.Equal(t, "val", v) +} + +func TestFieldOptionalWhenMissing(t *testing.T) { + changes := []shared.Change{} + + var v string + errors := Changes(changes).Field("/thing", &v, Optional()).Errors() + + assert.Empty(t, errors) +} + +func TestFieldValidate(t *testing.T) { + changes := []shared.Change{ + {Key: "/thing", New: json.RawMessage(`"val"`), Old: jsonNull}, + } + + var v string + Changes(changes).Field("/thing", &v, Validate(func() []shared.FieldError { + return nil + })) + + assert.Equal(t, "val", v) +} + +func TestFieldValidateWhenInvalid(t *testing.T) { + changes := []shared.Change{ + {Key: "/thing", New: json.RawMessage(`"what"`), Old: jsonNull}, + } + + var v string + errors := Changes(changes).Field("/thing", &v, Validate(func() []shared.FieldError { + return []shared.FieldError{{Source: "/rewritten", Detail: "invalid"}} + })).Errors() + + assert.Equal(t, []shared.FieldError{{Source: "/changes/0/new", Detail: "invalid"}}, errors) +} + +func TestConsumed(t *testing.T) { + changes := []shared.Change{} + errors := Changes(changes).Consumed() + + assert.Empty(t, errors) +} + +func TestConsumedWhenNot(t *testing.T) { + changes := []shared.Change{ + {Key: "/thing", New: json.RawMessage(`"val"`), Old: jsonNull}, + } + + errors := Changes(changes).Consumed() + + assert.Equal(t, []shared.FieldError{{Source: "/changes/0", Detail: "unexpected change provided"}}, errors) +} + +func TestConsumedWhenConsumed(t *testing.T) { + changes := []shared.Change{ + {Key: "/thing", New: json.RawMessage(`"val"`), Old: jsonNull}, + } + + var v string + errors := Changes(changes).Field("/thing", &v).Consumed() + + assert.Empty(t, errors) +} + +func TestEach(t *testing.T) { + changes := []shared.Change{ + {Key: "/0/thing", New: json.RawMessage(`"val"`), Old: jsonNull}, + {Key: "/1/other", New: json.RawMessage(`"other"`), Old: jsonNull}, + } + + var v, w string + errors := Changes(changes).Each(func(i int, p *Parser) []shared.FieldError { + if i == 0 { + p.Field("/thing", &v) + } else if i == 1 { + p.Field("/other", &w) + } + return p.Consumed() + }).Errors() + + assert.Equal(t, "val", v) + assert.Equal(t, "other", w) + assert.Empty(t, errors) +} + +func TestEachWhenNonIndexedKey(t *testing.T) { + changes := []shared.Change{ + {Key: "/thing", New: json.RawMessage(`"val"`), Old: jsonNull}, + {Key: "/-/other", New: json.RawMessage(`"other"`), Old: jsonNull}, + } + + errors := Changes(changes).Each(func(i int, p *Parser) []shared.FieldError { + var v any + p.Field("/thing", v) + return p.Errors() + }).Errors() + + assert.Equal(t, []shared.FieldError{ + {Source: "/changes/0/key", Detail: "require index"}, + {Source: "/changes/1/key", Detail: "require index"}, + }, errors) +} + +func TestEachWhenRequired(t *testing.T) { + changes := []shared.Change{} + + errors := Changes(changes).Each(func(i int, p *Parser) []shared.FieldError { + var v any + p.Field("/thing", v) + return p.Errors() + }, 0).Errors() + + assert.Equal(t, []shared.FieldError{ + {Source: "/changes", Detail: "missing /0/thing"}, + }, errors) +} + +func TestEachWhenOutOfRange(t *testing.T) { + changes := []shared.Change{ + {Key: "/0/thing", New: json.RawMessage(`"val"`), Old: jsonNull}, + {Key: "/1/thing", New: json.RawMessage(`"val"`), Old: jsonNull}, + {Key: "/2/thing", New: json.RawMessage(`"val"`), Old: jsonNull}, + } + + errors := Changes(changes).Each(func(i int, p *Parser) []shared.FieldError { + if i > 0 { + return p.OutOfRange() + } + + return p.Errors() + }).Errors() + + assert.ElementsMatch(t, []shared.FieldError{ + {Source: "/changes/1/key", Detail: "index out of range"}, + {Source: "/changes/2/key", Detail: "index out of range"}, + }, errors) +} + +func TestEachWhenNotConsumed(t *testing.T) { + changes := []shared.Change{ + {Key: "/0/thing", New: json.RawMessage(`"val"`), Old: jsonNull}, + {Key: "/1/thing", New: json.RawMessage(`"val"`), Old: jsonNull}, + {Key: "/2/thing", New: json.RawMessage(`"val"`), Old: jsonNull}, + } + + errors := Changes(changes).Each(func(i int, p *Parser) []shared.FieldError { + if i == 0 { + var v string + p.Field("/thing", &v) + } + + return p.Consumed() + }).Errors() + + assert.ElementsMatch(t, []shared.FieldError{ + {Source: "/changes/1", Detail: "unexpected change provided"}, + {Source: "/changes/2", Detail: "unexpected change provided"}, + }, errors) +} + +func TestPrefix(t *testing.T) { + changes := []shared.Change{ + {Key: "/a/thing", New: json.RawMessage(`"val"`), Old: jsonNull}, + } + + var v string + errors := Changes(changes).Prefix("/a", func(p *Parser) []shared.FieldError { + return p. + Field("/thing", &v). + Consumed() + }).Consumed() + + assert.Equal(t, "val", v) + assert.Empty(t, errors) +} + +func TestPrefixWhenNotConsumed(t *testing.T) { + changes := []shared.Change{ + {Key: "/a/thing", New: json.RawMessage(`"val"`), Old: jsonNull}, + {Key: "/a/what", New: json.RawMessage(`"val"`), Old: jsonNull}, + {Key: "/root", New: json.RawMessage(`"val"`), Old: jsonNull}, + } + + var v string + errors := Changes(changes).Prefix("/a", func(p *Parser) []shared.FieldError { + return p. + Field("/thing", &v). + Consumed() + }).Consumed() + + assert.Equal(t, "val", v) + assert.ElementsMatch(t, []shared.FieldError{ + {Source: "/changes/1", Detail: "unexpected change provided"}, + {Source: "/changes/2", Detail: "unexpected change provided"}, + }, errors) +} + +func TestPrefixWhenMissing(t *testing.T) { + changes := []shared.Change{} + + errors := Changes(changes).Prefix("/a", func(p *Parser) []shared.FieldError { return nil }).Consumed() + + assert.ElementsMatch(t, []shared.FieldError{ + {Source: "/changes", Detail: "missing /a/..."}, + }, errors) +} + +func TestPrefixOptional(t *testing.T) { + changes := []shared.Change{ + {Key: "/a/thing", New: json.RawMessage(`"val"`), Old: jsonNull}, + } + + var v string + errors := Changes(changes).Prefix("/a", func(p *Parser) []shared.FieldError { + return p. + Field("/thing", &v). + Consumed() + }, Optional()).Consumed() + + assert.Equal(t, "val", v) + assert.Empty(t, errors) +} + +func TestOptionalPrefixWhenMissing(t *testing.T) { + changes := []shared.Change{} + errors := Changes(changes).Prefix("/a", func(p *Parser) []shared.FieldError { return nil }, Optional()).Consumed() + + assert.Empty(t, errors) +} diff --git a/lambda/update/trust_corporation_sign.go b/lambda/update/trust_corporation_sign.go index 087f3652..6c1ccd8b 100644 --- a/lambda/update/trust_corporation_sign.go +++ b/lambda/update/trust_corporation_sign.go @@ -1,14 +1,9 @@ package main import ( - "bytes" - "encoding/json" - "fmt" - "strconv" - "strings" - "github.com/ministryofjustice/opg-data-lpa-store/internal/shared" "github.com/ministryofjustice/opg-data-lpa-store/internal/validate" + "github.com/ministryofjustice/opg-data-lpa-store/lambda/update/parse" ) type TrustCorporationSign struct { @@ -34,121 +29,44 @@ func (a TrustCorporationSign) Apply(lpa *shared.Lpa) []shared.FieldError { return nil } -func validateTrustCorporationSign(changes []shared.Change) (data TrustCorporationSign, errors []shared.FieldError) { - for i, change := range changes { - after, ok := strings.CutPrefix(change.Key, "/trustCorporations/") - if !ok { - errors = append(errors, shared.FieldError{Source: fmt.Sprintf("/changes/%d", i), Detail: "change not allowed for type"}) - continue - } - - if !bytes.Equal(change.Old, []byte("null")) { - errors = append(errors, shared.FieldError{Source: fmt.Sprintf("/changes/%d/old", i), Detail: "field must be null"}) - } - - parts := strings.SplitN(after, "/", 2) - trustCorporationIndex, err := strconv.Atoi(parts[0]) - if err != nil { - errors = append(errors, shared.FieldError{Source: fmt.Sprintf("/changes/%d", i), Detail: "change not allowed for type"}) - continue - } - if data.Index != nil && *data.Index != trustCorporationIndex { - errors = append(errors, shared.FieldError{Source: fmt.Sprintf("/changes/%d/key", i), Detail: "must be for same trust corporation"}) - continue - } else { - data.Index = &trustCorporationIndex - } - - newKey := fmt.Sprintf("/changes/%d/new", i) - switch parts[1] { - case "mobile": - if err := json.Unmarshal(change.New, &data.Mobile); err != nil { - errors = errorMustBeString(errors, newKey) - } - case "signatories/0/firstNames": - if err := json.Unmarshal(change.New, &data.Signatories[0].FirstNames); err != nil { - errors = errorMustBeString(errors, newKey) - } - case "signatories/0/lastName": - if err := json.Unmarshal(change.New, &data.Signatories[0].LastName); err != nil { - errors = errorMustBeString(errors, newKey) - } - case "signatories/0/professionalTitle": - if err := json.Unmarshal(change.New, &data.Signatories[0].ProfessionalTitle); err != nil { - errors = errorMustBeString(errors, newKey) - } - case "signatories/0/signedAt": - if err := json.Unmarshal(change.New, &data.Signatories[0].SignedAt); err != nil { - errors = errorMustBeDateTime(errors, newKey) - } - case "signatories/1/firstNames": - if err := json.Unmarshal(change.New, &data.Signatories[1].FirstNames); err != nil { - errors = errorMustBeString(errors, newKey) - } - case "signatories/1/lastName": - if err := json.Unmarshal(change.New, &data.Signatories[1].LastName); err != nil { - errors = errorMustBeString(errors, newKey) - } - case "signatories/1/professionalTitle": - if err := json.Unmarshal(change.New, &data.Signatories[1].ProfessionalTitle); err != nil { - errors = errorMustBeString(errors, newKey) - } - case "signatories/1/signedAt": - if err := json.Unmarshal(change.New, &data.Signatories[1].SignedAt); err != nil { - errors = errorMustBeDateTime(errors, newKey) - } - case "contactLanguagePreference": - if err := json.Unmarshal(change.New, &data.ContactLanguagePreference); err != nil { - errors = errorMustBeString(errors, newKey) - } else { - errors = append(errors, validate.IsValid(newKey, data.ContactLanguagePreference)...) - } - default: - errors = append(errors, shared.FieldError{Source: fmt.Sprintf("/changes/%d", i), Detail: "change not allowed for type"}) - } - } - - if data.Index == nil { - errors = append(errors, shared.FieldError{Source: "/changes", Detail: "must be specified"}) - } else { - if data.Mobile == "" { - errors = errorMissing(errors, fmt.Sprintf("/trustCorporations/%d/mobile", *data.Index)) - } - - if data.Signatories[0].IsZero() { - errors = errorMissing(errors, fmt.Sprintf("/trustCorporations/%d/signatories/0", *data.Index)) - } else { - errors = errorMissingSignatory(errors, fmt.Sprintf("/trustCorporations/%d/signatories/0", *data.Index), data.Signatories[0]) - } - - if !data.Signatories[1].IsZero() { - errors = errorMissingSignatory(errors, fmt.Sprintf("/trustCorporations/%d/signatories/1", *data.Index), data.Signatories[1]) - } - - if data.ContactLanguagePreference == shared.Lang("") { - errors = errorMissing(errors, fmt.Sprintf("/trustCorporations/%d/contactLanguagePreference", *data.Index)) - } - } +func validateTrustCorporationSign(changes []shared.Change) (TrustCorporationSign, []shared.FieldError) { + var data TrustCorporationSign + + errors := parse.Changes(changes). + Prefix("/trustCorporations", func(prefix *parse.Parser) []shared.FieldError { + return prefix. + Each(func(i int, each *parse.Parser) []shared.FieldError { + if data.Index != nil && *data.Index != i { + return each.OutOfRange() + } + + data.Index = &i + return each. + Field("/mobile", &data.Mobile). + Field("/contactLanguagePreference", &data.ContactLanguagePreference, parse.Validate(func() []shared.FieldError { + return validate.IsValid("", data.ContactLanguagePreference) + })). + Prefix("/signatories", func(prefix *parse.Parser) []shared.FieldError { + return prefix. + Each(func(i int, each *parse.Parser) []shared.FieldError { + if i > 1 { + return each.OutOfRange() + } + + return each. + Field("/firstNames", &data.Signatories[i].FirstNames). + Field("/lastName", &data.Signatories[i].LastName). + Field("/professionalTitle", &data.Signatories[i].ProfessionalTitle). + Field("/signedAt", &data.Signatories[i].SignedAt). + Consumed() + }, 0). + Consumed() + }). + Consumed() + }). + Consumed() + }). + Consumed() return data, errors } - -func errorMissingSignatory(errors []shared.FieldError, prefix string, signatory shared.Signatory) []shared.FieldError { - if signatory.FirstNames == "" { - errors = errorMissing(errors, fmt.Sprintf("%s/firstNames", prefix)) - } - - if signatory.LastName == "" { - errors = errorMissing(errors, fmt.Sprintf("%s/lastName", prefix)) - } - - if signatory.ProfessionalTitle == "" { - errors = errorMissing(errors, fmt.Sprintf("%s/professionalTitle", prefix)) - } - - if signatory.SignedAt.IsZero() { - errors = errorMissing(errors, fmt.Sprintf("%s/signedAt", prefix)) - } - - return errors -} diff --git a/lambda/update/trust_corporation_sign_test.go b/lambda/update/trust_corporation_sign_test.go index ea5e2808..4c3f4f8c 100644 --- a/lambda/update/trust_corporation_sign_test.go +++ b/lambda/update/trust_corporation_sign_test.go @@ -94,7 +94,7 @@ func TestValidateUpdateTrustCorporationSign(t *testing.T) { "missing all": { update: shared.Update{Type: "TRUST_CORPORATION_SIGN"}, errors: []shared.FieldError{ - {Source: "/changes", Detail: "must be specified"}, + {Source: "/changes", Detail: "missing /trustCorporations/..."}, }, }, "extra fields": { @@ -144,9 +144,9 @@ func TestValidateUpdateTrustCorporationSign(t *testing.T) { }, }, errors: []shared.FieldError{ - {Source: "/changes/5/old", Detail: "field must be null"}, - {Source: "/changes/6", Detail: "change not allowed for type"}, - {Source: "/changes/7", Detail: "change not allowed for type"}, + {Source: "/changes/5/old", Detail: "must be null"}, + {Source: "/changes/6", Detail: "unexpected change provided"}, + {Source: "/changes/7", Detail: "unexpected change provided"}, }, }, "invalid contact language": { @@ -226,7 +226,7 @@ func TestValidateUpdateTrustCorporationSign(t *testing.T) { }, }, errors: []shared.FieldError{ - {Source: "/changes/1/key", Detail: "must be for same trust corporation"}, + {Source: "/changes/1/key", Detail: "index out of range"}, {Source: "/changes", Detail: "missing /trustCorporations/0/signatories/0/firstNames"}, }, }, diff --git a/lambda/update/validate.go b/lambda/update/validate.go index be6c191d..05e07226 100644 --- a/lambda/update/validate.go +++ b/lambda/update/validate.go @@ -20,15 +20,3 @@ func validateUpdate(update shared.Update) (Applyable, []shared.FieldError) { return nil, []shared.FieldError{{Source: "/type", Detail: "invalid value"}} } } - -func errorMustBeString(errors []shared.FieldError, source string) []shared.FieldError { - return append(errors, shared.FieldError{Source: source, Detail: "must be a string"}) -} - -func errorMustBeDateTime(errors []shared.FieldError, source string) []shared.FieldError { - return append(errors, shared.FieldError{Source: source, Detail: "must be a datetime"}) -} - -func errorMissing(errors []shared.FieldError, key string) []shared.FieldError { - return append(errors, shared.FieldError{Source: "/changes", Detail: "missing " + key}) -}