From 720ecc7962f8770b4dc6fa2b4839d9ab76ed6e74 Mon Sep 17 00:00:00 2001 From: Joshua Hawxwell Date: Tue, 7 May 2024 13:13:49 +0100 Subject: [PATCH] MLPAB-2059 Index donor names and alter queries to work with that (#1219) --- internal/app/donor_store.go | 19 +++++- internal/app/donor_store_test.go | 19 ++++-- internal/app/uid_store.go | 9 ++- internal/app/uid_store_test.go | 6 +- internal/page/fixtures/supporter.go | 17 +++++- internal/search/client.go | 92 ++++++++++++++++------------- internal/search/client_test.go | 25 ++++---- terraform/environment/variables.tf | 2 +- 8 files changed, 120 insertions(+), 69 deletions(-) diff --git a/internal/app/donor_store.go b/internal/app/donor_store.go index 59eec56381..2f1ce2ccef 100644 --- a/internal/app/donor_store.go +++ b/internal/app/donor_store.go @@ -251,6 +251,16 @@ func (s *donorStore) GetByKeys(ctx context.Context, keys []dynamo.Keys) ([]actor var donors []actor.DonorProvidedDetails err = attributevalue.UnmarshalListOfMaps(items, &donors) + mappedDonors := map[string]actor.DonorProvidedDetails{} + for _, donor := range donors { + mappedDonors[donor.PK.PK()+"|"+donor.SK.SK()] = donor + } + + clear(donors) + for i, key := range keys { + donors[i] = mappedDonors[key.PK.PK()+"|"+key.SK.SK()] + } + return donors, err } @@ -268,9 +278,12 @@ func (s *donorStore) Put(ctx context.Context, donor *actor.DonorProvidedDetails) donor.UpdatedAt = s.now() if err := s.searchClient.Index(ctx, search.Lpa{ - PK: donor.PK.PK(), - SK: donor.SK.SK(), - DonorFullName: donor.Donor.FullName(), + PK: donor.PK.PK(), + SK: donor.SK.SK(), + Donor: search.LpaDonor{ + FirstNames: donor.Donor.FirstNames, + LastName: donor.Donor.LastName, + }, }); err != nil { return fmt.Errorf("donorStore index failed: %w", err) } diff --git a/internal/app/donor_store_test.go b/internal/app/donor_store_test.go index 0827fc9a3e..aa2eea7a99 100644 --- a/internal/app/donor_store_test.go +++ b/internal/app/donor_store_test.go @@ -291,14 +291,23 @@ func TestDonorStoreLatestWhenDataStoreError(t *testing.T) { } func TestDonorStoreGetByKeys(t *testing.T) { - keys := []dynamo.Keys{{}} - donors := []actor.DonorProvidedDetails{{LpaID: "1"}, {LpaID: "2"}} - av0, _ := attributevalue.MarshalMap(donors[0]) + keys := []dynamo.Keys{ + {PK: dynamo.LpaKey("1"), SK: dynamo.LpaOwnerKey(dynamo.DonorKey("a"))}, + {PK: dynamo.LpaKey("2"), SK: dynamo.LpaOwnerKey(dynamo.DonorKey("b"))}, + {PK: dynamo.LpaKey("3"), SK: dynamo.LpaOwnerKey(dynamo.DonorKey("c"))}, + } + donors := []actor.DonorProvidedDetails{ + {PK: dynamo.LpaKey("1"), SK: dynamo.LpaOwnerKey(dynamo.DonorKey("a")), LpaID: "1"}, + {PK: dynamo.LpaKey("2"), SK: dynamo.LpaOwnerKey(dynamo.DonorKey("b")), LpaID: "2"}, + {PK: dynamo.LpaKey("3"), SK: dynamo.LpaOwnerKey(dynamo.DonorKey("c")), LpaID: "3"}, + } + av0, _ := attributevalue.MarshalMap(donors[2]) av1, _ := attributevalue.MarshalMap(donors[1]) + av2, _ := attributevalue.MarshalMap(donors[0]) dynamoClient := newMockDynamoClient(t) dynamoClient.ExpectAllByKeys(ctx, keys, - []map[string]types.AttributeValue{av0, av1}, nil) + []map[string]types.AttributeValue{av0, av1, av2}, nil) donorStore := &donorStore{dynamoClient: dynamoClient} @@ -356,7 +365,7 @@ func TestDonorStorePutWhenUIDSet(t *testing.T) { searchClient := newMockSearchClient(t) searchClient.EXPECT(). - Index(ctx, search.Lpa{PK: dynamo.LpaKey("5").PK(), SK: dynamo.DonorKey("an-id").SK(), DonorFullName: "x y"}). + Index(ctx, search.Lpa{PK: dynamo.LpaKey("5").PK(), SK: dynamo.DonorKey("an-id").SK(), Donor: search.LpaDonor{FirstNames: "x", LastName: "y"}}). Return(nil) donorStore := &donorStore{dynamoClient: dynamoClient, searchClient: searchClient, now: testNowFn} diff --git a/internal/app/uid_store.go b/internal/app/uid_store.go index d6bc8e7adf..4e0d659c99 100644 --- a/internal/app/uid_store.go +++ b/internal/app/uid_store.go @@ -56,9 +56,12 @@ func (s *uidStore) Set(ctx context.Context, lpaID, sessionID, organisationID, ui } if err := s.searchClient.Index(ctx, search.Lpa{ - PK: dynamo.LpaKey(lpaID).PK(), - SK: sk.SK(), - DonorFullName: donor.Donor.FullName(), + PK: dynamo.LpaKey(lpaID).PK(), + SK: sk.SK(), + Donor: search.LpaDonor{ + FirstNames: donor.Donor.FirstNames, + LastName: donor.Donor.LastName, + }, }); err != nil { return fmt.Errorf("uidStore index failed: %w", err) } diff --git a/internal/app/uid_store_test.go b/internal/app/uid_store_test.go index 22f0ce9dbd..6150c3f58a 100644 --- a/internal/app/uid_store_test.go +++ b/internal/app/uid_store_test.go @@ -48,9 +48,9 @@ func TestUidStoreSet(t *testing.T) { searchClient := newMockSearchClient(t) searchClient.EXPECT(). Index(ctx, search.Lpa{ - PK: dynamo.LpaKey("lpa-id").PK(), - SK: tc.sk.SK(), - DonorFullName: "x y", + PK: dynamo.LpaKey("lpa-id").PK(), + SK: tc.sk.SK(), + Donor: search.LpaDonor{FirstNames: "x", LastName: "y"}, }). Return(nil) diff --git a/internal/page/fixtures/supporter.go b/internal/page/fixtures/supporter.go index fafaf4c161..1a470fc9b4 100644 --- a/internal/page/fixtures/supporter.go +++ b/internal/page/fixtures/supporter.go @@ -4,6 +4,7 @@ import ( "context" "encoding/base64" "fmt" + "log" "net/http" "strconv" "strings" @@ -303,11 +304,23 @@ func Supporter( } func waitForLPAIndex(searchClient *search.Client, organisationCtx context.Context) { + count := 0 + for range time.Tick(time.Second) { - if resp, _ := searchClient.Query(organisationCtx, search.QueryRequest{ + resp, err := searchClient.Query(organisationCtx, search.QueryRequest{ Page: 1, PageSize: 1, - }); resp != nil && len(resp.Keys) > 0 { + }) + if err != nil { + log.Println(err) + } + + if count > 10 { + return + } + count++ + + if resp != nil && len(resp.Keys) > 0 { break } } diff --git a/internal/search/client.go b/internal/search/client.go index 9882ad77b2..7964cede56 100644 --- a/internal/search/client.go +++ b/internal/search/client.go @@ -15,24 +15,22 @@ import ( requestsigner "github.com/opensearch-project/opensearch-go/v4/signer/awsv2" ) -const ( - indexDefinition = ` - { - "settings": { - "index": { - "number_of_shards": 1, - "number_of_replicas": 0 - } +var indexDefinition = map[string]any{ + "settings": map[string]any{ + "index": map[string]any{ + "number_of_shards": 1, + "number_of_replicas": 0, }, - "mappings": { - "properties": { - "DonorFullNameText": { "type": "text" }, - "DonorFullName": { "type": "keyword", "copy_to": "DonorFullNameText" }, - "SK": { "type": "keyword" } - } - } - }` -) + }, + "mappings": map[string]any{ + "properties": map[string]any{ + "PK": map[string]any{"type": "keyword"}, + "SK": map[string]any{"type": "keyword"}, + "Donor.FirstNames": map[string]any{"type": "keyword"}, + "Donor.LastName": map[string]any{"type": "keyword"}, + }, + }, +} type opensearchapiClient interface { Search(ctx context.Context, req *opensearchapi.SearchReq) (*opensearchapi.SearchResp, error) @@ -50,9 +48,14 @@ type QueryResponse struct { } type Lpa struct { - DonorFullName string - PK string - SK string + PK string + SK string + Donor LpaDonor +} + +type LpaDonor struct { + FirstNames string + LastName string } type QueryRequest struct { @@ -88,14 +91,16 @@ func NewClient(cfg aws.Config, endpoint, indexName string, indexingEnabled bool) } func (c *Client) CreateIndices(ctx context.Context) error { - _, err := c.indices.Exists(ctx, opensearchapi.IndicesExistsReq{Indices: []string{c.indexName}}) - if err == nil { - return nil + body, err := json.Marshal(indexDefinition) + if err != nil { + return err } - settings := strings.NewReader(indexDefinition) + if _, err := c.indices.Exists(ctx, opensearchapi.IndicesExistsReq{Indices: []string{c.indexName}}); err == nil { + return nil + } - if _, err := c.indices.Create(ctx, opensearchapi.IndicesCreateReq{Index: c.indexName, Body: settings}); err != nil { + if _, err := c.indices.Create(ctx, opensearchapi.IndicesCreateReq{Index: c.indexName, Body: bytes.NewReader(body)}); err != nil { return fmt.Errorf("search could not create index: %w", err) } @@ -128,11 +133,7 @@ func (c *Client) Query(ctx context.Context, req QueryRequest) (*QueryResponse, e } body, err := json.Marshal(map[string]any{ - "query": map[string]any{ - "match": map[string]any{ - "SK": sk, - }, - }, + "query": baseQuery(sk), }) if err != nil { return nil, err @@ -144,7 +145,7 @@ func (c *Client) Query(ctx context.Context, req QueryRequest) (*QueryResponse, e Params: opensearchapi.SearchParams{ From: aws.Int((req.Page - 1) * req.PageSize), Size: aws.Int(req.PageSize), - Sort: []string{"DonorFullName"}, + Sort: []string{"Donor.FirstNames", "Donor.LastName"}, }, }) if err != nil { @@ -181,15 +182,7 @@ func (c *Client) CountWithQuery(ctx context.Context, req CountWithQueryReq) (int "track_total_hits": true, } - query := map[string]map[string]any{ - "bool": { - "must": map[string]any{ - "match": map[string]string{ - "SK": sk, - }, - }, - }, - } + query := baseQuery(sk) if req.MustNotExist != "" { query["bool"]["must_not"] = map[string]any{ @@ -217,6 +210,25 @@ func (c *Client) CountWithQuery(ctx context.Context, req CountWithQueryReq) (int return resp.Hits.Total.Value, err } +func baseQuery(sk string) map[string]map[string]any { + return map[string]map[string]any{ + "bool": { + "must": []map[string]any{ + { + "match": map[string]string{ + "SK": sk, + }, + }, + { + "prefix": map[string]string{ + "PK": dynamo.LpaKey("").PK(), + }, + }, + }, + }, + } +} + func getSKFromContext(ctx context.Context) (string, error) { session, err := page.SessionDataFromContext(ctx) if err != nil { diff --git a/internal/search/client_test.go b/internal/search/client_test.go index aec43d7fe5..152cc576b3 100644 --- a/internal/search/client_test.go +++ b/internal/search/client_test.go @@ -8,7 +8,6 @@ import ( "fmt" "net/http" "net/http/httptest" - "strings" "testing" "github.com/aws/aws-sdk-go-v2/aws" @@ -53,12 +52,14 @@ func TestNewClient(t *testing.T) { } func TestClientCreateIndices(t *testing.T) { + data, _ := json.Marshal(indexDefinition) + indices := newMockIndicesClient(t) indices.EXPECT(). Exists(ctx, opensearchapi.IndicesExistsReq{Indices: []string{testIndexName}}). Return(nil, expectedError) indices.EXPECT(). - Create(ctx, opensearchapi.IndicesCreateReq{Index: testIndexName, Body: strings.NewReader(indexDefinition)}). + Create(ctx, opensearchapi.IndicesCreateReq{Index: testIndexName, Body: bytes.NewReader(data)}). Return(nil, nil) client := &Client{indices: indices, indexName: testIndexName, indexingEnabled: true} @@ -97,18 +98,18 @@ func TestClientIndex(t *testing.T) { Index(ctx, opensearchapi.IndexReq{ Index: testIndexName, DocumentID: "LPA--2020", - Body: bytes.NewReader([]byte(`{"DonorFullName":"x y","PK":"LPA#2020","SK":"abc#123"}`)), + Body: bytes.NewReader([]byte(`{"PK":"LPA#2020","SK":"abc#123","Donor":{"FirstNames":"x","LastName":"y"}}`)), }). Return(nil, nil) client := &Client{svc: svc, indexName: testIndexName, indexingEnabled: true} - err := client.Index(ctx, Lpa{DonorFullName: "x y", PK: dynamo.LpaKey("2020").PK(), SK: "abc#123"}) + err := client.Index(ctx, Lpa{Donor: LpaDonor{FirstNames: "x", LastName: "y"}, PK: dynamo.LpaKey("2020").PK(), SK: "abc#123"}) assert.Nil(t, err) } func TestClientIndexWhenNotEnabled(t *testing.T) { client := &Client{} - err := client.Index(ctx, Lpa{DonorFullName: "x y", PK: dynamo.LpaKey("2020").PK(), SK: "abc#123"}) + err := client.Index(ctx, Lpa{Donor: LpaDonor{FirstNames: "x", LastName: "y"}, PK: dynamo.LpaKey("2020").PK(), SK: "abc#123"}) assert.Nil(t, err) } @@ -119,7 +120,7 @@ func TestClientIndexWhenIndexErrors(t *testing.T) { Return(nil, expectedError) client := &Client{svc: svc, indexingEnabled: true} - err := client.Index(ctx, Lpa{DonorFullName: "x y", PK: dynamo.LpaKey("2020").PK(), SK: "abc#123"}) + err := client.Index(ctx, Lpa{Donor: LpaDonor{FirstNames: "x", LastName: "y"}, PK: dynamo.LpaKey("2020").PK(), SK: "abc#123"}) assert.Equal(t, expectedError, err) } @@ -171,11 +172,11 @@ func TestClientQuery(t *testing.T) { svc.EXPECT(). Search(ctx, &opensearchapi.SearchReq{ Indices: []string{testIndexName}, - Body: bytes.NewReader([]byte(fmt.Sprintf(`{"query":{"match":{"SK":"%s"}}}`, tc.sk.SK()))), + Body: bytes.NewReader([]byte(fmt.Sprintf(`{"query":{"bool":{"must":[{"match":{"SK":"%s"}},{"prefix":{"PK":"LPA#"}}]}}}`, tc.sk.SK()))), Params: opensearchapi.SearchParams{ From: aws.Int(tc.from), Size: aws.Int(10), - Sort: []string{"DonorFullName"}, + Sort: []string{"Donor.FirstNames", "Donor.LastName"}, }, }). Return(resp, nil) @@ -240,22 +241,22 @@ func TestClientCountWithQuery(t *testing.T) { }{ "no query - donor": { query: CountWithQueryReq{}, - body: []byte(`{"query":{"bool":{"must":{"match":{"SK":"DONOR#1"}}}},"size":0,"track_total_hits":true}`), + body: []byte(`{"query":{"bool":{"must":[{"match":{"SK":"DONOR#1"}},{"prefix":{"PK":"LPA#"}}]}},"size":0,"track_total_hits":true}`), session: &page.SessionData{SessionID: "1"}, }, "no query - organisation": { query: CountWithQueryReq{}, - body: []byte(`{"query":{"bool":{"must":{"match":{"SK":"ORGANISATION#1"}}}},"size":0,"track_total_hits":true}`), + body: []byte(`{"query":{"bool":{"must":[{"match":{"SK":"ORGANISATION#1"}},{"prefix":{"PK":"LPA#"}}]}},"size":0,"track_total_hits":true}`), session: &page.SessionData{OrganisationID: "1"}, }, "MustNotExist query - donor": { query: CountWithQueryReq{MustNotExist: "a-field"}, - body: []byte(`{"query":{"bool":{"must":{"match":{"SK":"DONOR#1"}},"must_not":{"exists":{"field":"a-field"}}}},"size":0,"track_total_hits":true}`), + body: []byte(`{"query":{"bool":{"must":[{"match":{"SK":"DONOR#1"}},{"prefix":{"PK":"LPA#"}}],"must_not":{"exists":{"field":"a-field"}}}},"size":0,"track_total_hits":true}`), session: &page.SessionData{SessionID: "1"}, }, "MustNotExist query - organisation": { query: CountWithQueryReq{MustNotExist: "a-field"}, - body: []byte(`{"query":{"bool":{"must":{"match":{"SK":"ORGANISATION#1"}},"must_not":{"exists":{"field":"a-field"}}}},"size":0,"track_total_hits":true}`), + body: []byte(`{"query":{"bool":{"must":[{"match":{"SK":"ORGANISATION#1"}},{"prefix":{"PK":"LPA#"}}],"must_not":{"exists":{"field":"a-field"}}}},"size":0,"track_total_hits":true}`), session: &page.SessionData{OrganisationID: "1"}, }, } diff --git a/terraform/environment/variables.tf b/terraform/environment/variables.tf index 5104a40014..4c7fb5f0f7 100644 --- a/terraform/environment/variables.tf +++ b/terraform/environment/variables.tf @@ -119,5 +119,5 @@ locals { mock_onelogin_version = "latest" - search_index_name = "lpas_${local.environment_name}" + search_index_name = "lpas_v2_${local.environment_name}" }