Skip to content

Commit f1cca03

Browse files
Edge case handling and tests for FindEntities (#1692)
The primary goal of this PR is to fix a panic when trying to access an element in an empty array. I've also taken the opportunity to add some tests for FindEntities and address another edge case (empty response from Maps API) that I found while adding those tests. The pattern for adding a facade in front of the Maps client is taken from here: https://github.com/googleapis/google-api-go-client/blob/main/testing.md --------- Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
1 parent 01bad1c commit f1cca03

File tree

10 files changed

+278
-50
lines changed

10 files changed

+278
-50
lines changed

cmd/main.go

Lines changed: 3 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ import (
2929

3030
"github.com/datacommonsorg/mixer/internal/featureflags"
3131
logger "github.com/datacommonsorg/mixer/internal/log"
32+
"github.com/datacommonsorg/mixer/internal/maps"
3233
"github.com/datacommonsorg/mixer/internal/metrics"
3334
pbs "github.com/datacommonsorg/mixer/internal/proto/service"
3435
"github.com/datacommonsorg/mixer/internal/server"
@@ -44,9 +45,7 @@ import (
4445
"github.com/datacommonsorg/mixer/internal/sqldb"
4546
"github.com/datacommonsorg/mixer/internal/store"
4647
"github.com/datacommonsorg/mixer/internal/store/bigtable"
47-
"github.com/datacommonsorg/mixer/internal/util"
4848
"golang.org/x/oauth2/google"
49-
"googlemaps.github.io/maps"
5049

5150
"cloud.google.com/go/bigquery"
5251
"cloud.google.com/go/profiler"
@@ -260,10 +259,6 @@ func main() {
260259
btClient,
261260
branchTableName,
262261
)
263-
if err != nil {
264-
slog.Error("Failed to create BigTable client", "error", err)
265-
os.Exit(1)
266-
}
267262
tables = append(tables, bigtable.NewTable(branchTableName, branchTable, false /*isCustom=*/))
268263
}
269264
slog.Info("After branch setup")
@@ -368,9 +363,9 @@ func main() {
368363
slog.Info("After cache creation")
369364

370365
// Maps client
371-
var mapsClient *maps.Client
366+
var mapsClient maps.MapsClient
372367
if *useMapsApi {
373-
mapsClient, err = util.MapsClient(ctx, metadata.HostProject)
368+
mapsClient, err = maps.NewMapsClient(ctx, metadata.HostProject)
374369
if err != nil {
375370
slog.Error("Failed to create Maps client", "error", err)
376371
os.Exit(1)

go.mod

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ require (
1818
github.com/golang/geo v0.0.0-20250723132703-4547674171cb
1919
github.com/google/go-cmp v0.7.0
2020
github.com/google/pprof v0.0.0-20250630185457-6e76a2b096b5
21+
github.com/google/uuid v1.6.0
2122
github.com/grpc-ecosystem/go-grpc-middleware v1.4.0
2223
github.com/jmoiron/sqlx v1.4.0
2324
github.com/prometheus/client_golang v1.22.0
@@ -73,7 +74,6 @@ require (
7374
github.com/google/btree v1.1.3 // indirect
7475
github.com/google/flatbuffers v23.5.26+incompatible // indirect
7576
github.com/google/s2a-go v0.1.9 // indirect
76-
github.com/google/uuid v1.6.0 // indirect
7777
github.com/googleapis/enterprise-certificate-proxy v0.3.6 // indirect
7878
github.com/googleapis/gax-go/v2 v2.15.0 // indirect
7979
github.com/grafana/regexp v0.0.0-20240518133315-a468a5bfb3bc // indirect

internal/maps/fake.go

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
// Copyright 2025 Google LLC
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// https://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
15+
package maps
16+
17+
import (
18+
"context"
19+
20+
"googlemaps.github.io/maps"
21+
)
22+
23+
// Implements MapsClient for testing.
24+
type FakeMapsClient struct{}
25+
26+
// Returns a result for query "Hawaii"; empty otherwise.
27+
func (*FakeMapsClient) FindPlaceFromText(ctx context.Context, r *maps.FindPlaceFromTextRequest) (maps.FindPlaceFromTextResponse, error) {
28+
if r.Input == "Hawaii" {
29+
return maps.FindPlaceFromTextResponse{
30+
Candidates: []maps.PlacesSearchResult{
31+
{PlaceID: "hawaii_place_id"},
32+
},
33+
}, nil
34+
}
35+
36+
return maps.FindPlaceFromTextResponse{}, nil
37+
}

internal/maps/maps.go

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
// Copyright 2025 Google LLC
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// https://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
15+
package maps
16+
17+
import (
18+
"context"
19+
20+
"github.com/datacommonsorg/mixer/internal/util"
21+
"googlemaps.github.io/maps"
22+
)
23+
24+
// MapsClient is a thin facade of googlemaps `maps.Client` for ease of testing.
25+
// If more methods are used, they can be added to the interface as needed.
26+
// See FakeMapsClient for an impl for use in tests.
27+
type MapsClient interface {
28+
FindPlaceFromText(ctx context.Context, r *maps.FindPlaceFromTextRequest) (maps.FindPlaceFromTextResponse, error)
29+
}
30+
31+
type mapsClient struct {
32+
client *maps.Client
33+
}
34+
35+
func (c *mapsClient) FindPlaceFromText(ctx context.Context, r *maps.FindPlaceFromTextRequest) (maps.FindPlaceFromTextResponse, error) {
36+
return c.client.FindPlaceFromText(ctx, r)
37+
}
38+
39+
func NewMapsClient(ctx context.Context, projectID string) (MapsClient, error) {
40+
apiKey, err := util.ReadLatestSecret(ctx, projectID, util.MapsAPIKeyID)
41+
if err != nil {
42+
return nil, err
43+
}
44+
client, err := maps.NewClient(maps.WithAPIKey(apiKey))
45+
if err != nil {
46+
return nil, err
47+
}
48+
return &mapsClient{client}, nil
49+
}

internal/server/recon/find_entities.go

Lines changed: 15 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ import (
2121
"sort"
2222
"strings"
2323

24+
internalmaps "github.com/datacommonsorg/mixer/internal/maps"
2425
pb "github.com/datacommonsorg/mixer/internal/proto"
2526
pbv1 "github.com/datacommonsorg/mixer/internal/proto/v1"
2627
"github.com/datacommonsorg/mixer/internal/server/v1/propertyvalues"
@@ -45,7 +46,7 @@ func FindEntities(
4546
ctx context.Context,
4647
in *pb.FindEntitiesRequest,
4748
store *store.Store,
48-
mapsClient *maps.Client,
49+
mapsClient internalmaps.MapsClient,
4950
) (*pb.FindEntitiesResponse, error) {
5051
bulkResp, err := BulkFindEntities(ctx,
5152
&pb.BulkFindEntitiesRequest{
@@ -62,6 +63,10 @@ func FindEntities(
6263
return nil, err
6364
}
6465

66+
if len(bulkResp.GetEntities()) == 0 {
67+
return &pb.FindEntitiesResponse{}, nil
68+
}
69+
6570
return &pb.FindEntitiesResponse{
6671
Dcids: bulkResp.GetEntities()[0].GetDcids(),
6772
}, nil
@@ -72,7 +77,7 @@ func BulkFindEntities(
7277
ctx context.Context,
7378
in *pb.BulkFindEntitiesRequest,
7479
store *store.Store,
75-
mapsClient *maps.Client,
80+
mapsClient internalmaps.MapsClient,
7681
) (*pb.BulkFindEntitiesResponse, error) {
7782
if l := len(in.GetEntities()); l == 0 {
7883
return nil, fmt.Errorf("empty input")
@@ -155,7 +160,7 @@ func BulkFindEntities(
155160
// but use Maps API as signal to reorder the results.
156161
func resolveDCIDs(
157162
ctx context.Context,
158-
mapsClient *maps.Client,
163+
mapsClient internalmaps.MapsClient,
159164
store *store.Store,
160165
entityInfoSet map[entityInfo]struct{},
161166
) (
@@ -269,7 +274,7 @@ func resolveWithRecognizePlaces(
269274

270275
func resolveWithMapsAPI(
271276
ctx context.Context,
272-
mapsClient *maps.Client,
277+
mapsClient internalmaps.MapsClient,
273278
store *store.Store,
274279
entityInfoSet map[entityInfo]struct{},
275280
) (
@@ -284,6 +289,10 @@ func resolveWithMapsAPI(
284289
return nil, nil, err
285290
}
286291

292+
if len(placeIDSet) == 0 {
293+
return map[entityInfo]map[string]struct{}{}, map[string]struct{}{}, nil
294+
}
295+
287296
// Resolve place IDs to get DCIDs.
288297
placeIDToDCIDs, dcidSet, err := resolveDCIDsFromPlaceIDs(ctx, placeIDSet, store)
289298
if err != nil {
@@ -309,7 +318,7 @@ func resolveWithMapsAPI(
309318

310319
func resolvePlaceIDsFromDescriptions(
311320
ctx context.Context,
312-
mapsClient *maps.Client,
321+
mapsClient internalmaps.MapsClient,
313322
entityInfoSet map[entityInfo]struct{},
314323
) (
315324
map[entityInfo][]string, /* entityInfo -> [place ID] */
@@ -382,7 +391,7 @@ func resolvePlaceIDsFromDescriptions(
382391

383392
func findPlaceIDsForEntity(
384393
ctx context.Context,
385-
mapsClient *maps.Client,
394+
mapsClient internalmaps.MapsClient,
386395
entityInfo *entityInfo,
387396
) ([]string, error) {
388397
// When type is supplied, we append it to the description to increase the accuracy.
Lines changed: 152 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,152 @@
1+
// Copyright 2025 Google LLC
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// https://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
15+
package recon
16+
17+
import (
18+
"context"
19+
"testing"
20+
21+
"github.com/datacommonsorg/mixer/internal/maps"
22+
"github.com/datacommonsorg/mixer/internal/store"
23+
"github.com/datacommonsorg/mixer/internal/store/bigtable"
24+
"github.com/datacommonsorg/mixer/internal/store/files"
25+
"github.com/datacommonsorg/mixer/internal/util"
26+
"github.com/google/go-cmp/cmp"
27+
"google.golang.org/protobuf/proto"
28+
"google.golang.org/protobuf/testing/protocmp"
29+
30+
pb "github.com/datacommonsorg/mixer/internal/proto"
31+
)
32+
33+
func TestFindEntities(t *testing.T) {
34+
for _, tc := range []struct {
35+
name string
36+
req *pb.FindEntitiesRequest
37+
want *pb.FindEntitiesResponse
38+
}{
39+
{
40+
"FromStore",
41+
&pb.FindEntitiesRequest{Description: "California"},
42+
&pb.FindEntitiesResponse{Dcids: []string{"geoId/06"}},
43+
},
44+
// Ensure graceful handling of empty results from both store and Maps API.
45+
{
46+
"EmptyResponse",
47+
&pb.FindEntitiesRequest{Description: "UnresolvableQuery"},
48+
&pb.FindEntitiesResponse{},
49+
},
50+
} {
51+
t.Run(tc.name, func(t *testing.T) {
52+
ctx := context.Background()
53+
s, err := newTestStore()
54+
if err != nil {
55+
t.Fatalf("newTestStore() = %v", err)
56+
}
57+
mc := &maps.FakeMapsClient{}
58+
59+
resp, err := FindEntities(ctx, tc.req, s, mc)
60+
if err != nil {
61+
t.Fatalf("FindEntities returned error: %v", err)
62+
}
63+
64+
if diff := cmp.Diff(resp, tc.want, protocmp.Transform()); diff != "" {
65+
t.Errorf("FindEntities() got diff: %s", diff)
66+
}
67+
})
68+
}
69+
}
70+
71+
func TestFindEntitiesFromMapsApi(t *testing.T) {
72+
ctx := context.Background()
73+
s := newTestStoreWithBt(t)
74+
75+
req := &pb.FindEntitiesRequest{Description: "Hawaii"}
76+
mc := &maps.FakeMapsClient{}
77+
resp, err := FindEntities(ctx, req, s, mc)
78+
if err != nil {
79+
t.Fatalf("FindEntities returned error: %v", err)
80+
}
81+
82+
want := &pb.FindEntitiesResponse{Dcids: []string{"geoId/15"}}
83+
if diff := cmp.Diff(resp, want, protocmp.Transform()); diff != "" {
84+
t.Errorf("FindEntities() got diff: %s", diff)
85+
}
86+
}
87+
88+
// Set up a RecogPlaceStore that has an entry for "California"
89+
func newTestStore() (*store.Store, error) {
90+
return &store.Store{
91+
RecogPlaceStore: &files.RecogPlaceStore{
92+
DcidToNames: map[string][]string{
93+
"geoId/06": {"California"},
94+
},
95+
RecogPlaceMap: map[string]*pb.RecogPlaces{
96+
"california": {
97+
Places: []*pb.RecogPlace{
98+
{
99+
Dcid: "geoId/06",
100+
Names: []*pb.RecogPlace_Name{
101+
{Parts: []string{"california"}},
102+
},
103+
},
104+
},
105+
},
106+
},
107+
},
108+
}, nil
109+
}
110+
111+
// Set up an empty RecogPlaceStore and a Bigtable ReconIDMap with an entry
112+
// for "hawaii_place_id", which is the value returned by the fake Maps client for "Hawaii".
113+
func newTestStoreWithBt(t *testing.T) *store.Store {
114+
reconEntities := &pb.ReconEntities{
115+
Entities: []*pb.ReconEntities_Entity{
116+
{
117+
Ids: []*pb.ReconEntities_Entity_ID{
118+
{
119+
Prop: "dcid",
120+
Val: "geoId/15",
121+
},
122+
},
123+
Types: []string{"State"},
124+
},
125+
},
126+
}
127+
raw, err := proto.Marshal(reconEntities)
128+
if err != nil {
129+
t.Fatalf("proto.Marshal failed: %v", err)
130+
}
131+
tableValue, err := util.ZipAndEncode(raw)
132+
if err != nil {
133+
t.Errorf("util.ZipAndEncode failed: %v", err)
134+
}
135+
btData := map[string]string{
136+
"d/5/placeId^hawaii_place_id^dcid": tableValue,
137+
}
138+
ctx := context.Background()
139+
tables, err := bigtable.SetupBigtable(ctx, btData)
140+
if err != nil {
141+
t.Fatalf("SetupBigtable got err: %v", err)
142+
}
143+
btGroup := bigtable.NewGroup(
144+
[]*bigtable.Table{bigtable.NewTable("test-table", tables, false)}, "", nil)
145+
return &store.Store{
146+
BtGroup: btGroup,
147+
RecogPlaceStore: &files.RecogPlaceStore{
148+
DcidToNames: map[string][]string{},
149+
RecogPlaceMap: map[string]*pb.RecogPlaces{},
150+
},
151+
}
152+
}

0 commit comments

Comments
 (0)