From 2e67ea692c9bbd17f8dff0240d6debff440fb901 Mon Sep 17 00:00:00 2001 From: Joshua Hawxwell Date: Thu, 13 Jun 2024 15:18:15 +0100 Subject: [PATCH] Always use MustMatchExisting behaviour --- lambda/update/attorney_sign.go | 10 ++--- lambda/update/certificate_provider_sign.go | 20 ++++----- lambda/update/parse/changes.go | 49 ++++++++++------------ lambda/update/parse/changes_test.go | 18 ++++---- lambda/update/trust_corporation_sign.go | 4 +- 5 files changed, 47 insertions(+), 54 deletions(-) diff --git a/lambda/update/attorney_sign.go b/lambda/update/attorney_sign.go index e4db15a6..46fbdfc3 100644 --- a/lambda/update/attorney_sign.go +++ b/lambda/update/attorney_sign.go @@ -53,17 +53,17 @@ func validateAttorneySign(changes []shared.Change, lpa *shared.Lpa) (AttorneySig } return p. - Field("/mobile", &data.Mobile, parse.MustMatchExisting()). + Field("/mobile", &data.Mobile). Field("/signedAt", &data.SignedAt, parse.Validate(func() []shared.FieldError { return validate.Time("", data.SignedAt) - }), parse.MustMatchExisting()). + })). Field("/contactLanguagePreference", &data.ContactLanguagePreference, parse.Validate(func() []shared.FieldError { return validate.IsValid("", data.ContactLanguagePreference) - }), parse.MustMatchExisting()). + })). Field("/channel", &data.Channel, parse.Validate(func() []shared.FieldError { return validate.IsValid("", data.Channel) - }), parse.MustMatchExisting(), parse.Optional()). - Field("/email", &data.Email, parse.MustMatchExisting(), parse.Optional()). + }), parse.Optional()). + Field("/email", &data.Email, parse.Optional()). Consumed() }). Consumed() diff --git a/lambda/update/certificate_provider_sign.go b/lambda/update/certificate_provider_sign.go index f163c9ef..b10df5d2 100644 --- a/lambda/update/certificate_provider_sign.go +++ b/lambda/update/certificate_provider_sign.go @@ -45,28 +45,28 @@ func validateCertificateProviderSign(changes []shared.Change, lpa *shared.Lpa) ( errors := parse.Changes(changes). Prefix("/certificateProvider/address", func(p *parse.Parser) []shared.FieldError { return p. - Field("/line1", &data.Address.Line1, parse.MustMatchExisting()). - Field("/line2", &data.Address.Line2, parse.Optional(), parse.MustMatchExisting()). - Field("/line3", &data.Address.Line3, parse.Optional(), parse.MustMatchExisting()). - Field("/town", &data.Address.Town, parse.MustMatchExisting()). - Field("/postcode", &data.Address.Postcode, parse.Optional(), parse.MustMatchExisting()). + 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) - }), parse.MustMatchExisting()). + })). Consumed() }, parse.Optional()). Field("/certificateProvider/signedAt", &data.SignedAt, parse.Validate(func() []shared.FieldError { return validate.Time("", data.SignedAt) - }), parse.MustMatchExisting()). + })). Field("/certificateProvider/contactLanguagePreference", &data.ContactLanguagePreference, parse.Validate(func() []shared.FieldError { return validate.IsValid("", data.ContactLanguagePreference) - }), parse.MustMatchExisting()). + })). Field("/certificateProvider/email", &data.Email, parse.Validate(func() []shared.FieldError { return validate.Required("", data.Email) - }), parse.Optional(), parse.MustMatchExisting()). + }), parse.Optional()). Field("/certificateProvider/channel", &data.Channel, parse.Validate(func() []shared.FieldError { return validate.IsValid("", data.Channel) - }), parse.Optional(), parse.MustMatchExisting()). + }), parse.Optional()). Consumed() return data, errors diff --git a/lambda/update/parse/changes.go b/lambda/update/parse/changes.go index fed78a81..bfa9fc78 100644 --- a/lambda/update/parse/changes.go +++ b/lambda/update/parse/changes.go @@ -62,9 +62,8 @@ func (p *Parser) Errors() []shared.FieldError { type Option func(fieldOpts) fieldOpts type fieldOpts struct { - optional bool - mustMatchExisting bool - validator func() []shared.FieldError + optional bool + validator func() []shared.FieldError } // Optional stops [Parser.Field] or [Parser.Prefix] from adding an error when the expected key is missing. @@ -75,14 +74,6 @@ func Optional() func(fieldOpts) fieldOpts { } } -// MustMatchExisting [Parser.Field] will add an error when the old value does not match the existing value. -func MustMatchExisting() func(fieldOpts) fieldOpts { - return func(f fieldOpts) fieldOpts { - f.mustMatchExisting = 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 { @@ -109,25 +100,20 @@ func (p *Parser) Field(key string, existing any, opts ...Option) *Parser { for i, change := range p.changes { if change.Key == key { - // Adding for expand phase - will be removed once consumers are updated - if options.mustMatchExisting { - var old any - if err := json.Unmarshal(change.Old, &old); err != nil { - p.errors = append(p.errors, shared.FieldError{Source: change.Source("/old"), Detail: "error marshalling old value"}) - } - - if !oldEqualsExisting(old, existing) { - p.errors = append(p.errors, shared.FieldError{Source: change.Source("/old"), Detail: "does not match existing value"}) - - return p - } + var old any + if err := json.Unmarshal(change.Old, &old); err != nil { + p.errors = append(p.errors, shared.FieldError{Source: change.Source("/old"), Detail: "error marshalling old value"}) } - if err := json.Unmarshal(change.New, existing); 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}) + if !oldEqualsExisting(old, existing) { + p.errors = append(p.errors, shared.FieldError{Source: change.Source("/old"), Detail: "does not match existing value"}) + } else { + if err := json.Unmarshal(change.New, existing); 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}) + } } } @@ -177,6 +163,13 @@ func oldEqualsExisting(old any, existing any) bool { return old.(string) == *v + case *int: + if old == nil { + return *v == 0 + } + + return old.(int) == *v + default: return false } diff --git a/lambda/update/parse/changes_test.go b/lambda/update/parse/changes_test.go index d38273fd..8ddb7865 100644 --- a/lambda/update/parse/changes_test.go +++ b/lambda/update/parse/changes_test.go @@ -88,18 +88,18 @@ func TestFieldValidateWhenInvalid(t *testing.T) { assert.Equal(t, []shared.FieldError{{Source: "/changes/0/new", Detail: "invalid"}}, errors) } -func TestFieldMustMatchExistingString(t *testing.T) { +func TestFieldOldString(t *testing.T) { changes := []shared.Change{ {Key: "/thing", New: json.RawMessage(`"new val"`), Old: json.RawMessage(`"old val"`)}, } v := "old val" - Changes(changes).Field("/thing", &v, MustMatchExisting()) + Changes(changes).Field("/thing", &v) assert.Equal(t, "new val", v) } -func TestFieldMustMatchExistingTime(t *testing.T) { +func TestFieldOldTime(t *testing.T) { now := time.Now() yesterday := time.Now().Add(-24 * time.Hour) @@ -107,29 +107,29 @@ func TestFieldMustMatchExistingTime(t *testing.T) { {Key: "/thing", New: json.RawMessage(`"` + now.Format(time.RFC3339Nano) + `"`), Old: json.RawMessage(`"` + yesterday.Format(time.RFC3339Nano) + `"`)}, } - Changes(changes).Field("/thing", &yesterday, MustMatchExisting()) + Changes(changes).Field("/thing", &yesterday) assert.WithinDuration(t, now, yesterday, time.Second) } -func TestFieldMustMatchExistingLang(t *testing.T) { +func TestFieldOldLang(t *testing.T) { changes := []shared.Change{ {Key: "/thing", New: json.RawMessage(`"cy"`), Old: json.RawMessage(`"en"`)}, } v := shared.LangEn - Changes(changes).Field("/thing", &v, MustMatchExisting()) + Changes(changes).Field("/thing", &v) assert.Equal(t, shared.LangCy, v) } -func TestFieldMustMatchExistingChannel(t *testing.T) { +func TestFieldOldChannel(t *testing.T) { changes := []shared.Change{ {Key: "/thing", New: json.RawMessage(`"online"`), Old: json.RawMessage(`"paper"`)}, } v := shared.ChannelPaper - Changes(changes).Field("/thing", &v, MustMatchExisting()) + Changes(changes).Field("/thing", &v) assert.Equal(t, shared.ChannelOnline, v) } @@ -147,7 +147,7 @@ func TestFieldWhenOldDoesNotMatchExisting(t *testing.T) { } v := "existing" - errors := Changes(changes).Field("/thing", &v, MustMatchExisting()).Errors() + errors := Changes(changes).Field("/thing", &v).Errors() assert.Equal(t, []shared.FieldError{{Source: "/changes/0/old", Detail: "does not match existing value"}}, errors) }) diff --git a/lambda/update/trust_corporation_sign.go b/lambda/update/trust_corporation_sign.go index e1d6689a..8e552f4b 100644 --- a/lambda/update/trust_corporation_sign.go +++ b/lambda/update/trust_corporation_sign.go @@ -54,10 +54,10 @@ func validateTrustCorporationSign(changes []shared.Change, lpa *shared.Lpa) (Tru Field("/contactLanguagePreference", &data.ContactLanguagePreference, parse.Validate(func() []shared.FieldError { return validate.IsValid("", data.ContactLanguagePreference) })). - Field("/email", &data.Email, parse.Optional(), parse.MustMatchExisting()). + Field("/email", &data.Email, parse.Optional()). Field("/channel", &data.Channel, parse.Validate(func() []shared.FieldError { return validate.IsValid("", data.Channel) - }), parse.Optional(), parse.MustMatchExisting()). + }), parse.Optional()). Prefix("/signatories", func(prefix *parse.Parser) []shared.FieldError { return prefix. Each(func(i int, each *parse.Parser) []shared.FieldError {