Repository: google/exposure-notifications-server
Pull request #614 (2020-06-11 18:00:40)

Reduce function arity
/fixes 312

google-oss-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/google/exposure-notifications-server/pull/614#" title="Author self-approved">mikehelmick</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=google%2Fexposure-notifications-server).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/google/exposure-notifications-server/blob/master/OWNERS)~~ [mikehelmick]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->

Patch:
diff --git a/builders/migrate.yaml b/builders/migrate.yaml
index daf21aa..ea87255 100644
--- a/builders/migrate.yaml
+++ b/builders/migrate.yaml
@@ -48,6 +48,6 @@ steps:
 
     echo "==> Running migrations"
     /usr/local/bin/migrate \
-      -database postgres://${_DB_USER}:$$(cat ./dbpass)@localhost:${_DB_PORT}/${_DB_NAME}?sslmode=${_DB_SSLMODE} \
+      -database postgres://${_DB_USER}:$$(cat ./dbpass | urlencode)@localhost:${_DB_PORT}/${_DB_NAME}?sslmode=${_DB_SSLMODE} \
       -path migrations \
       ${_COMMAND}
diff --git a/docker/migrate.dockerfile b/docker/migrate.dockerfile
index 9865839..3d06dfe 100644
--- a/docker/migrate.dockerfile
+++ b/docker/migrate.dockerfile
@@ -27,10 +27,13 @@ RUN chmod +x /bin/cloud_sql_proxy
 RUN go get -tags 'postgres' -u github.com/golang-migrate/migrate/cmd/migrate
 
 FROM alpine
-RUN apk add --no-cache ca-certificates
+RUN apk add --no-cache bash ca-certificates
 
 COPY --from=builder /bin/cloud_sql_proxy /bin/cloud_sql_proxy
 COPY --from=builder /go/bin/migrate /usr/local/bin/migrate
 
+# Install urlencode - see https://github.com/golang-migrate/migrate/issues/396.
+ADD urlencode /bin/urlencode
+RUN chmod +x /bin/urlencode
 
 ENTRYPOINT ["/usr/local/bin/migrate"]
diff --git a/docker/urlencode b/docker/urlencode
new file mode 100755
index 0000000..3468a4d
--- /dev/null
+++ b/docker/urlencode
@@ -0,0 +1,28 @@
+#!/usr/bin/env bash
+
+# Copyright 2020 Google LLC
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+set -eEuo pipefail
+
+read IN
+LENGTH="${#IN}"
+for (( i = 0; i < $LENGTH; i++ )); do
+  c="${IN:i:1}"
+  case $c in
+    [a-zA-Z0-9.~_-]) printf "$c" ;;
+    *) printf '%s' "$c" | xxd -p -c1 |
+      while read c; do printf '%%%s' "$c"; done ;;
+  esac
+done
diff --git a/docs/server_functional_requirements.md b/docs/server_functional_requirements.md
index 614fad9..91db401 100644
--- a/docs/server_functional_requirements.md
+++ b/docs/server_functional_requirements.md
@@ -99,11 +99,11 @@ Minimum required fields, followed by a JSON example:
   * Description: Name of the application being used to send the request. This
     is used to determine what app is uploading keys and if it is an allowed
     region for that app. (BundleID on iOS)
-* `verificationPayload` (**REQUIRED**)
+* `verificationPayload` (**REQUIRED FOR VERIFICATION PROTOCOL**)
   * Type: String
   * Description: verificationPayload is a signed certificate from a public
     health authority, indicating a confirmed diagnosis
-* `hmackey` (**REQUIRED**)
+* `hmackey` (**REQUIRED FOR VERIFICATION PROTOCOL**)
   * Type: String
   * Description: The device generated, random key that was used to create the
   HMAC for the data sent to the diagnosis verification server. The actual
diff --git a/internal/federationin/federationin.go b/internal/federationin/federationin.go
index ad6b798..18a03ae 100644
--- a/internal/federationin/federationin.go
+++ b/internal/federationin/federationin.go
@@ -189,13 +189,17 @@ func (h *handler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 	timeoutContext, cancel := context.WithTimeout(ctx, h.config.Timeout)
 	defer cancel()
 
-	deps := pullDependencies{
-		fetch:               client.Fetch,
-		insertExposures:     h.publishdb.InsertExposures,
-		startFederationSync: h.db.StartFederationInSync,
+	opts := pullOptions{
+		deps: pullDependencies{
+			fetch:               client.Fetch,
+			insertExposures:     h.publishdb.InsertExposures,
+			startFederationSync: h.db.StartFederationInSync,
+		},
+		query:          query,
+		batchStart:     time.Now(),
+		truncateWindow: h.config.TruncateWindow,
 	}
-	batchStart := time.Now()
-	if err := pull(timeoutContext, metrics, deps, query, batchStart, h.config.TruncateWindow); err != nil {
+	if err := pull(timeoutContext, metrics, &opts); err != nil {
 		internalErrorf(ctx, w, "Federation query %q failed: %v", queryID, err)
 		return
 	}
@@ -205,7 +209,14 @@ func (h *handler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 	}
 }
 
-func pull(ctx context.Context, metrics metrics.Exporter, deps pullDependencies, q *model.FederationInQuery, batchStart time.Time, truncateWindow time.Duration) (err error) {
+type pullOptions struct {
+	deps           pullDependencies
+	query          *model.FederationInQuery
+	batchStart     time.Time
+	truncateWindow time.Duration
+}
+
+func pull(ctx context.Context, metrics metrics.Exporter, opts *pullOptions) (err error) {
 	ctx, span := trace.StartSpan(ctx, "federationin.pull")
 	defer func() {
 		if err != nil {
@@ -215,17 +226,17 @@ func pull(ctx context.Context, metrics metrics.Exporter, deps pullDependencies,
 	}()
 
 	logger := logging.FromContext(ctx)
-	logger.Infof("Processing query %q", q.QueryID)
+	logger.Infof("Processing query %q", opts.query.QueryID)
 
 	request := &pb.FederationFetchRequest{
-		RegionIdentifiers:             q.IncludeRegions,
-		ExcludeRegionIdentifiers:      q.ExcludeRegions,
-		LastFetchResponseKeyTimestamp: q.LastTimestamp.Unix(),
+		RegionIdentifiers:             opts.query.IncludeRegions,
+		ExcludeRegionIdentifiers:      opts.query.ExcludeRegions,
+		LastFetchResponseKeyTimestamp: opts.query.LastTimestamp.Unix(),
 	}
 
-	syncID, finalizeFn, err := deps.startFederationSync(ctx, q, batchStart)
+	syncID, finalizeFn, err := opts.deps.startFederationSync(ctx, opts.query, opts.batchStart)
 	if err != nil {
-		return fmt.Errorf("starting federation sync for query %s: %w", q.QueryID, err)
+		return fmt.Errorf("starting federation sync for query %s: %w", opts.query.QueryID, err)
 	}
 
 	var maxTimestamp time.Time
@@ -234,7 +245,7 @@ func pull(ctx context.Context, metrics metrics.Exporter, deps pullDependencies,
 		logger.Infof("Inserted %d keys", total)
 	}()
 
-	createdAt := publishmodel.TruncateWindow(batchStart, truncateWindow)
+	createdAt := publishmodel.TruncateWindow(opts.batchStart, opts.truncateWindow)
 	partial := true
 	nPartials := int64(0)
 	for partial {
@@ -243,9 +254,9 @@ func pull(ctx context.Context, metrics metrics.Exporter, deps pullDependencies,
 
 		// TODO(squee1945): react to the context timeout and complete a chunk of work so next invocation can pick up where left off.
 
-		response, err := deps.fetch(ctx, request)
+		response, err := opts.deps.fetch(ctx, request)
 		if err != nil {
-			return fmt.Errorf("fetching query %s: %w", q.QueryID, err)
+			return fmt.Errorf("fetching query %s: %w", opts.query.QueryID, err)
 		}
 
 		responseTimestamp := time.Unix(response.FetchResponseKeyTimestamp, 0)
@@ -281,7 +292,7 @@ func pull(ctx context.Context, metrics metrics.Exporter, deps pullDependencies,
 					})
 
 					if len(exposures) == fetchBatchSize {
-						if err := deps.insertExposures(ctx, exposures); err != nil {
+						if err := opts.deps.insertExposures(ctx, exposures); err != nil {
 							metrics.WriteInt("federation-pull-inserts", false, len(exposures))
 							return fmt.Errorf("inserting %d exposures: %w", len(exposures), err)
 						}
@@ -292,7 +303,7 @@ func pull(ctx context.Context, metrics metrics.Exporter, deps pullDependencies,
 			}
 		}
 		if len(exposures) > 0 {
-			if err := deps.insertExposures(ctx, exposures); err != nil {
+			if err := opts.deps.insertExposures(ctx, exposures); err != nil {
 				metrics.WriteInt("federation-pull-inserts", false, len(exposures))
 				return fmt.Errorf("inserting %d exposures: %w", len(exposures), err)
 			}
@@ -305,7 +316,7 @@ func pull(ctx context.Context, metrics metrics.Exporter, deps pullDependencies,
 
 	if err := finalizeFn(maxTimestamp, total); err != nil {
 		// TODO(squee1945): how do we clean up here? Just leave the records in and have the exporter eliminate them? Other?
-		return fmt.Errorf("finalizing federation sync for query %s: %w", q.QueryID, err)
+		return fmt.Errorf("finalizing federation sync for query %s: %w", opts.query.QueryID, err)
 	}
 
 	return nil
diff --git a/tools/export-generate/main.go b/tools/export-generate/main.go
index 50ca5ab..444c334 100644
--- a/tools/export-generate/main.go
+++ b/tools/export-generate/main.go
@@ -160,30 +160,55 @@ func main() {
 		currentBatch = append(currentBatch, &exposureKeys[i])
 		if len(currentBatch) == *batchSize {
 			b++
-			writeFile(eb, currentBatch, b, numBatches, actualNumKeys, privateKey)
+			w := exportFileWriter{
+				exportBatch: eb,
+				exposures:   currentBatch,
+				curBatch:    b,
+				numBatches:  numBatches,
+				totalKeys:   actualNumKeys,
+				privateKey:  privateKey,
+			}
+			w.writeFile()
 			currentBatch = []*publishmodel.Exposure{}
 		}
 	}
 	if len(currentBatch) > 0 {
 		b++
-		writeFile(eb, currentBatch, b, numBatches, actualNumKeys, privateKey)
+		w := exportFileWriter{
+			exportBatch: eb,
+			exposures:   currentBatch,
+			curBatch:    b,
+			numBatches:  numBatches,
+			totalKeys:   actualNumKeys,
+			privateKey:  privateKey,
+		}
+		w.writeFile()
 	}
 }
 
-func writeFile(eb *model.ExportBatch, currentBatch []*publishmodel.Exposure, b, numBatches, numRecords int, privateKey *ecdsa.PrivateKey) {
+type exportFileWriter struct {
+	exportBatch *model.ExportBatch
+	exposures   []*publishmodel.Exposure
+	curBatch    int
+	numBatches  int
+	totalKeys   int
+	privateKey  *ecdsa.PrivateKey
+}
+
+func (e *exportFileWriter) writeFile() {
 	signatureInfo := &model.SignatureInfo{
 		SigningKeyID:      *keyID,
 		SigningKeyVersion: *keyVersion,
 	}
 	signer := &export.Signer{
 		SignatureInfo: signatureInfo,
-		Signer:        privateKey,
+		Signer:        e.privateKey,
 	}
-	data, err := export.MarshalExportFile(eb, currentBatch, b, numBatches, []*export.Signer{signer})
+	data, err := export.MarshalExportFile(e.exportBatch, e.exposures, e.curBatch, e.numBatches, []*export.Signer{signer})
 	if err != nil {
 		log.Fatalf("error marshaling export file: %v", err)
 	}
-	fileName := fmt.Sprintf(eb.FilenameRoot+"%d-records-%d-of-%d"+filenameSuffix, numRecords, b, numBatches)
+	fileName := fmt.Sprintf(e.exportBatch.FilenameRoot+"%d-records-%d-of-%d"+filenameSuffix, e.totalKeys, e.curBatch, e.numBatches)
 	log.Printf("Creating %v", fileName)
 	err = ioutil.WriteFile(fileName, data, 0666)
 	if err != nil {


Test patch:
diff --git a/internal/federationin/federationin_test.go b/internal/federationin/federationin_test.go
index 5cd7146..ece2fc3 100644
--- a/internal/federationin/federationin_test.go
+++ b/internal/federationin/federationin_test.go
@@ -276,13 +276,19 @@ func TestFederationPull(t *testing.T) {
 				fetchBatchSize = tc.batchSize
 				defer func() { fetchBatchSize = oldBatchSize }()
 			}
-			deps := pullDependencies{
-				fetch:               remote.fetch,
-				insertExposures:     idb.insertExposures,
-				startFederationSync: sdb.startFederationSync,
+
+			opts := pullOptions{
+				deps: pullDependencies{
+					fetch:               remote.fetch,
+					insertExposures:     idb.insertExposures,
+					startFederationSync: sdb.startFederationSync,
+				},
+				query:          query,
+				batchStart:     batchStart,
+				truncateWindow: time.Hour,
 			}
 
-			err := pull(ctx, metrics.NewLogsBasedFromContext(ctx), deps, query, batchStart, time.Hour)
+			err := pull(ctx, metrics.NewLogsBasedFromContext(ctx), &opts)
 			if err != nil {
 				t.Fatalf("pull returned err=%v, want err=nil", err)
 			}