Fix nil references to sql DB. (#1485)

datacommonsorg · Jan 3, 2025 · c8d8fd7 · c8d8fd7
1 parent 2d046d8
commit c8d8fd7
Show file tree

Hide file tree

Showing 16 changed files with 33 additions and 31 deletions.
diff --git a/cmd/main.go b/cmd/main.go
@@ -202,23 +202,25 @@ func main() {
 	}
 
 	// SQL client
-	var sqlClient *sqldb.SQLClient
+	var sqlClient sqldb.SQLClient
 	if *useSQLite {
-		sqlClient, err = sqldb.NewSQLiteClient(*sqlitePath)
+		client, err := sqldb.NewSQLiteClient(*sqlitePath)
 		if err != nil {
 			log.Fatalf("Cannot open sqlite database from: %s: %v", *sqlitePath, err)
 		}
+		sqlClient.DB = client.DB
 		defer sqlClient.Close()
 	}
 
 	if *useCloudSQL {
-		if sqlClient != nil {
+		if sqlClient.DB != nil {
 			log.Printf("SQL client has already been created, will not use CloudSQL")
 		} else {
-			sqlClient, err = sqldb.NewCloudSQLClient(*cloudSQLInstance)
+			client, err := sqldb.NewCloudSQLClient(*cloudSQLInstance)
 			if err != nil {
 				log.Fatalf("Cannot open cloud sql database from %s: %v", *cloudSQLInstance, err)
 			}
+			sqlClient.DB = client.DB
 			defer sqlClient.Close()
 		}
 	}
@@ -236,7 +238,7 @@ func main() {
 	}
 
 	// Store
-	if len(tables) == 0 && *remoteMixerDomain == "" && sqlClient == nil {
+	if len(tables) == 0 && *remoteMixerDomain == "" && sqlClient.DB == nil {
 		log.Fatal("No bigtables or remote mixer domain or sql database are provided")
 	}
 	store, err := store.NewStore(
@@ -250,7 +252,7 @@ func main() {
 	cacheOptions := cache.CacheOptions{
 		FetchSVG:       *cacheSVG,
 		SearchSVG:      *cacheSVG,
-		CacheSQL:       store.SQLClient != nil,
+		CacheSQL:       store.SQLClient.DB != nil,
 		CacheSVFormula: *cacheSVFormula,
 	}
 	c, err := cache.NewCache(ctx, store, cacheOptions, metadata)

diff --git a/internal/server/count/count.go b/internal/server/count/count.go
@@ -76,7 +76,7 @@ func countInternal(
 			}
 		}
 	}
-	if st.SQLClient != nil {
+	if st.SQLClient.DB != nil {
 		// all SV contains the SV in the request and child SV in the request SVG.
 		allSV := []string{}
 		for _, svOrSvg := range svOrSvgs {

diff --git a/internal/server/handler_v1_test.go b/internal/server/handler_v1_test.go
@@ -22,7 +22,6 @@ import (
 	"github.com/datacommonsorg/mixer/internal/proto"
 	pbv1 "github.com/datacommonsorg/mixer/internal/proto/v1"
 	"github.com/datacommonsorg/mixer/internal/server/resource"
-	"github.com/datacommonsorg/mixer/internal/sqldb"
 	"github.com/datacommonsorg/mixer/internal/store"
 	"github.com/go-test/deep"
 )
@@ -32,9 +31,7 @@ func TestBulkVariableInfo(t *testing.T) {
 	ctx := context.Background()
 
 	s := Server{
-		store: &store.Store{
-			SQLClient: &sqldb.SQLClient{},
-		},
+		store:      &store.Store{},
 		metadata:   &resource.Metadata{},
 		httpClient: &http.Client{},
 	}

diff --git a/internal/server/node/property_label.go b/internal/server/node/property_label.go
@@ -76,7 +76,7 @@ func GetPropertiesHelper(
 		}
 	}
 	// Fetch data from SQLite
-	if store.SQLClient != nil {
+	if store.SQLClient.DB != nil {
 		var query string
 		if direction == util.DirectionOut {
 			query = fmt.Sprintf(

diff --git a/internal/server/placein/placein.go b/internal/server/placein/placein.go
@@ -76,7 +76,7 @@ func GetPlacesIn(
 			}
 		}
 	}
-	if store.SQLClient != nil {
+	if store.SQLClient.DB != nil {
 		var query string
 		var args []string
 		if len(parentPlaces) == 1 && parentPlaces[0] == childPlaceType {

diff --git a/internal/server/statvar/fetcher/entity_sv_fetcher.go b/internal/server/statvar/fetcher/entity_sv_fetcher.go
@@ -72,7 +72,7 @@ func FetchEntityVariables(
 		}
 	}
 	// Fetch from SQL database
-	if store.SQLClient != nil {
+	if store.SQLClient.DB != nil {
 		query := fmt.Sprintf(
 			`
 				SELECT entity, GROUP_CONCAT(DISTINCT variable) AS variables

diff --git a/internal/server/statvar/fetcher/svg_fetcher.go b/internal/server/statvar/fetcher/svg_fetcher.go
@@ -101,7 +101,7 @@ func FetchAllSVG(
 			}
 		}
 	}
-	if store.SQLClient != nil {
+	if store.SQLClient.DB != nil {
 		sqlResult, err := fetchSQLSVGs(store.SQLClient.DB)
 		if err != nil {
 			return nil, err

diff --git a/internal/server/statvar/statvar_summary.go b/internal/server/statvar/statvar_summary.go
@@ -33,7 +33,7 @@ import (
 func GetStatVarSummaryHelper(
 	ctx context.Context, entities []string, store *store.Store) (
 	map[string]*pb.StatVarSummary, error) {
-	if store.BtGroup == nil && store.SQLClient == nil {
+	if store.BtGroup == nil && store.SQLClient.DB == nil {
 		return nil, status.Error(codes.Internal, "No store found")
 	}
 
@@ -55,7 +55,7 @@ func GetStatVarSummaryHelper(
 		btChan <- map[string]*pb.StatVarSummary{}
 	}
 
-	if store.SQLClient != nil {
+	if store.SQLClient.DB != nil {
 		errGroup.Go(func() error {
 			sql, err := sqlGetStatVarSummary(entities, store.SQLClient.DB)
 			if err != nil {

diff --git a/internal/server/v0/propertylabel/property_label_test.go b/internal/server/v0/propertylabel/property_label_test.go
@@ -19,6 +19,7 @@ import (
 	"testing"
 
 	pb "github.com/datacommonsorg/mixer/internal/proto"
+	"github.com/datacommonsorg/mixer/internal/sqldb"
 	"github.com/datacommonsorg/mixer/internal/store"
 	"github.com/datacommonsorg/mixer/internal/store/bigtable"
 	"github.com/datacommonsorg/mixer/internal/util"
@@ -93,7 +94,7 @@ func TestMerge(t *testing.T) {
 
 		store, err := store.NewStore(
 			nil,
-			nil,
+			sqldb.SQLClient{},
 			[]*bigtable.Table{
 				bigtable.NewTable("borgcron_base", baseTable, false /*isCustom=*/),
 				bigtable.NewTable("borgcron_branch", branchTable, false /*isCustom=*/),

diff --git a/internal/server/v1/observationdates/observation_dates_linked.go b/internal/server/v1/observationdates/observation_dates_linked.go
@@ -128,7 +128,7 @@ func BulkObservationDatesLinked(
 	}
 
 	// Read data from SQL store.
-	if store.SQLClient != nil {
+	if store.SQLClient.DB != nil {
 		childPlaces, err := shared.FetchChildPlaces(
 			ctx, store, metadata, httpClient, metadata.RemoteMixerDomain, linkedEntity, entityType)
 		if err != nil {

diff --git a/internal/server/v1/propertyvalues/core.go b/internal/server/v1/propertyvalues/core.go
@@ -73,7 +73,7 @@ func Fetch(
 	}
 	// No pagination for sqlite query, so if there is a pagination token, meaning
 	// the data has already been queried and returned in previous query.
-	if store.SQLClient != nil && token == "" {
+	if store.SQLClient.DB != nil && token == "" {
 		sqlResp, err := fetchSQL(store.SQLClient.DB, nodes, properties, direction)
 		if err != nil {
 			return nil, nil, err

diff --git a/internal/server/v2/facet/series.go b/internal/server/v2/facet/series.go
@@ -125,7 +125,7 @@ func SeriesFacet(
 			}
 		}
 	}
-	if store.SQLClient != nil {
+	if store.SQLClient.DB != nil {
 		observationCount, err := sqlquery.CountObservation(store.SQLClient.DB, entities, variables)
 		if err != nil {
 			return nil, err

diff --git a/internal/server/v2/observation/contained_in.go b/internal/server/v2/observation/contained_in.go
@@ -183,7 +183,7 @@ func FetchContainedIn(
 
 	// Fetch Data from SQLite database.
 	var sqlResult *pbv2.ObservationResponse
-	if store.SQLClient != nil {
+	if store.SQLClient.DB != nil {
 		if ancestor == childType {
 			sqlResult = initObservationResult(variables)
 			variablesStr := "'" + strings.Join(variables, "', '") + "'"

diff --git a/internal/server/v2/shared/contained_in_test.go b/internal/server/v2/shared/contained_in_test.go
@@ -24,17 +24,14 @@ import (
 	pb "github.com/datacommonsorg/mixer/internal/proto"
 	pbv2 "github.com/datacommonsorg/mixer/internal/proto/v2"
 	"github.com/datacommonsorg/mixer/internal/server/resource"
-	"github.com/datacommonsorg/mixer/internal/sqldb"
 	"github.com/datacommonsorg/mixer/internal/store"
 )
 
 func TestFetchChildPlaces(t *testing.T) {
 	t.Parallel()
 	ctx := context.Background()
 
-	s := &store.Store{
-		SQLClient: &sqldb.SQLClient{},
-	}
+	s := &store.Store{}
 	metadata := &resource.Metadata{}
 	httpClient := &http.Client{}
 

diff --git a/internal/store/store.go b/internal/store/store.go
@@ -27,13 +27,17 @@ type Store struct {
 	BqClient        *bigquery.Client
 	BtGroup         *bigtable.Group
 	RecogPlaceStore *files.RecogPlaceStore
-	SQLClient       *sqldb.SQLClient
+	// TODO: Make SQLClient a pointer instead of a value once SQLClient.DB is made internal.
+	// Currently the direct DB connection is referenced at many places
+	// and a nil SQLClient pointer leads to NPEs.
+	// Using a value avoids those situations.
+	SQLClient sqldb.SQLClient
 }
 
 // NewStore creates a new store.
 func NewStore(
 	bqClient *bigquery.Client,
-	sqlClient *sqldb.SQLClient,
+	sqlClient sqldb.SQLClient,
 	tables []*bigtable.Table,
 	branchTableName string,
 	metadata *resource.Metadata,

diff --git a/test/setup.go b/test/setup.go
@@ -152,12 +152,13 @@ func setupInternal(
 		log.Fatalf("failed to create Bigquery client: %v", err)
 	}
 	// SQL client
-	var sqlClient *sqldb.SQLClient
+	var sqlClient sqldb.SQLClient
 	if useSQLite {
-		sqlClient, err = sqldb.NewSQLiteClient(filepath.Join(path.Dir(filename), "./datacommons.db"))
+		client, err := sqldb.NewSQLiteClient(filepath.Join(path.Dir(filename), "./datacommons.db"))
 		if err != nil {
 			log.Fatalf("Failed to read sqlite database: %v", err)
 		}
+		sqlClient.DB = client.DB
 		err = sqldb.CheckSchema(sqlClient.DB)
 		if err != nil {
 			log.Fatalf("SQL schema check failed: %v", err)
@@ -216,7 +217,7 @@ func SetupBqOnly() (pbs.MixerClient, error) {
 	if err != nil {
 		return nil, err
 	}
-	st, err := store.NewStore(bqClient, nil, nil, "", nil)
+	st, err := store.NewStore(bqClient, sqldb.SQLClient{}, nil, "", nil)
 	if err != nil {
 		return nil, err
 	}