satellite/overlaycache: fix typo in UpdateCheckIn request

- fix a malformed SQL query
- add test to be sure we don't have this problem again.

Change-Id: I3fde8c59ba01335411e51d964bec95bc26cfc961

satellite/overlay/service.go

@@ -123,6 +123,8 @@ type DB interface {
 	TestSuspendNodeOffline(ctx context.Context, nodeID storj.NodeID, suspendedAt time.Time) (err error)
 	// TestNodeCountryCode sets node country code.
 	TestNodeCountryCode(ctx context.Context, nodeID storj.NodeID, countryCode string) (err error)
+	// TestUpdateCheckInDirectUpdate tries to update a node info directly. Returns true if it succeeded, false if there were no node with the provided (used for testing).
+	TestUpdateCheckInDirectUpdate(ctx context.Context, node NodeCheckInInfo, timestamp time.Time, semVer version.SemVer, walletFeatures string) (updated bool, err error)
 
 	// IterateAllContactedNodes will call cb on all known nodes (used in restore trash contexts).
 	IterateAllContactedNodes(context.Context, func(context.Context, *SelectedNode) error) error

satellite/satellitedb/overlaycache.go

@@ -1270,24 +1270,7 @@ func (cache *overlaycache) getNodesForDQLastSeenBefore(ctx context.Context, cuto
 	return nodeIDs, rows.Err()
 }
 
-// UpdateCheckIn updates a single storagenode with info from when the the node last checked in.
-func (cache *overlaycache) UpdateCheckIn(ctx context.Context, node overlay.NodeCheckInInfo, timestamp time.Time, config overlay.NodeSelectionConfig) (err error) {
-	defer mon.Task()(&ctx)(&err)
-
-	if node.Address.GetAddress() == "" {
-		return Error.New("error UpdateCheckIn: missing the storage node address")
-	}
-
-	semVer, err := version.NewSemVer(node.Version.GetVersion())
-	if err != nil {
-		return Error.New("unable to convert version to semVer")
-	}
-
-	walletFeatures, err := encodeWalletFeatures(node.Operator.GetWalletFeatures())
-	if err != nil {
-		return Error.Wrap(err)
-	}
-
+func (cache *overlaycache) updateCheckInDirectUpdate(ctx context.Context, node overlay.NodeCheckInInfo, timestamp time.Time, semVer version.SemVer, walletFeatures string) (updated bool, err error) {
 	// First try the fast path.
 	var res sql.Result
 	res, err = cache.db.ExecContext(ctx, `
@@ -1310,7 +1293,7 @@ func (cache *overlaycache) UpdateCheckIn(ctx context.Context, node overlay.NodeC
 			END,
 			last_ip_port=$16,
 			wallet_features=$17,
-			country_code=$18
+			country_code=$18,
 			last_software_update_email = CASE
 				WHEN $19::bool IS TRUE THEN $15::timestamptz
 				WHEN $20::bool IS FALSE THEN NULL
@@ -1340,11 +1323,40 @@ func (cache *overlaycache) UpdateCheckIn(ctx context.Context, node overlay.NodeC
 		node.SoftwareUpdateEmailSent, node.VersionBelowMin,
 	)
 
-	if err == nil {
-		affected, affectedErr := res.RowsAffected()
-		if affectedErr == nil && affected > 0 {
-			return nil
-		}
+	if err != nil {
+		return false, Error.Wrap(err)
+	}
+	affected, affectedErr := res.RowsAffected()
+	if affectedErr != nil {
+		return false, Error.Wrap(err)
+	}
+	return affected > 0, nil
+}
+
+// UpdateCheckIn updates a single storagenode with info from when the the node last checked in.
+func (cache *overlaycache) UpdateCheckIn(ctx context.Context, node overlay.NodeCheckInInfo, timestamp time.Time, config overlay.NodeSelectionConfig) (err error) {
+	defer mon.Task()(&ctx)(&err)
+
+	if node.Address.GetAddress() == "" {
+		return Error.New("error UpdateCheckIn: missing the storage node address")
+	}
+
+	semVer, err := version.NewSemVer(node.Version.GetVersion())
+	if err != nil {
+		return Error.New("unable to convert version to semVer")
+	}
+
+	walletFeatures, err := encodeWalletFeatures(node.Operator.GetWalletFeatures())
+	if err != nil {
+		return Error.Wrap(err)
+	}
+
+	updated, err := cache.updateCheckInDirectUpdate(ctx, node, timestamp, semVer, walletFeatures)
+	if err != nil {
+		return Error.Wrap(err)
+	}
+	if updated {
+		return nil
 	}
 
 	_, err = cache.db.ExecContext(ctx, `
@@ -1580,3 +1592,7 @@ func (cache *overlaycache) IterateAllNodeDossiers(ctx context.Context, cb func(c
 		}
 	}
 }
+
+func (cache *overlaycache) TestUpdateCheckInDirectUpdate(ctx context.Context, node overlay.NodeCheckInInfo, timestamp time.Time, semVer version.SemVer, walletFeatures string) (updated bool, err error) {
+	return cache.updateCheckInDirectUpdate(ctx, node, timestamp, semVer, walletFeatures)
+}

satellite/satellitedb/overlaycache_test.go

@@ -13,6 +13,7 @@ import (
 	"storj.io/common/storj"
 	"storj.io/common/testcontext"
 	"storj.io/common/testrand"
+	"storj.io/private/version"
 	"storj.io/storj/private/teststorj"
 	"storj.io/storj/satellite"
 	"storj.io/storj/satellite/overlay"
@@ -184,3 +185,41 @@ func TestSetNodeContained(t *testing.T) {
 		require.False(t, cacheInfo.Contained)
 	})
 }
+
+func TestUpdateCheckInDirectUpdate(t *testing.T) {
+	satellitedbtest.Run(t, func(ctx *testcontext.Context, t *testing.T, db satellite.DB) {
+		cache := db.OverlayCache()
+		db.OverlayCache()
+		selectionCfg := overlay.NodeSelectionConfig{
+			OnlineWindow: 4 * time.Hour,
+		}
+		nodeID := teststorj.NodeIDFromString("testnode0")
+		checkInInfo := overlay.NodeCheckInInfo{
+			IsUp: true,
+			Address: &pb.NodeAddress{
+				Address: "1.2.3.4",
+			},
+			Version: &pb.NodeVersion{
+				Version:    "v0.0.0",
+				CommitHash: "",
+				Timestamp:  time.Time{},
+				Release:    false,
+			},
+			Operator: &pb.NodeOperator{
+				Email: "test@storj.test",
+			},
+		}
+		now := time.Now().UTC()
+		checkInInfo.NodeID = nodeID
+		semVer, err := version.NewSemVer(checkInInfo.Version.Version)
+		require.NoError(t, err)
+		// node unknown - should not be updated by updateCheckInDirectUpdate
+		updated, err := cache.TestUpdateCheckInDirectUpdate(ctx, checkInInfo, now, semVer, "encodedwalletfeature")
+		require.NoError(t, err)
+		require.False(t, updated)
+		require.NoError(t, cache.UpdateCheckIn(ctx, checkInInfo, now, selectionCfg))
+		updated, err = cache.TestUpdateCheckInDirectUpdate(ctx, checkInInfo, now.Add(6*time.Hour), semVer, "encodedwalletfeature")
+		require.NoError(t, err)
+		require.True(t, updated)
+	})
+}