Repository: cloudspannerecosystem/harbourbridge
Pull request #45 (2020-02-14 10:36:14)

fix: update to exit early when the file is not parsable.
This is a revert for #44. The new added test is broken due to the last change, which has been fixed. 

hengfengli: @nevinheintze We should configure running unit tests in PR checks. PR must pass unit tests before merging. 

Sorry for the inconvenience. I saw a `green` "squash and merge" button soI thought the unit tests are passed. 
lgruen: Good idea, but we'd need to find a way to trigger the "ci/circleci:
build_and_test" check before the merge, but after approval. We don't want
to run it for any PR as that would risk leaking credentials.

On Fri, Feb 14, 2020 at 9:40 PM Hengfeng Li <notifications@github.com>
wrote:

> @nevinheintze <https://github.com/nevinheintze> We should configure
> running unit tests in PR checks. PR must pass unit tests before merging.
>
> Sorry for the inconvenience. I saw a green "squash and merge" button soI
> thought the unit tests are passed.
>
> —
> You are receiving this because you are subscribed to this thread.
> Reply to this email directly, view it on GitHub
> <https://github.com/cloudspannerecosystem/harbourbridge/pull/45?email_source=notifications&email_token=AAV6E3ETPAZB7MBGM2IQ2QDRCZYJVA5CNFSM4KVFMOQKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOELYMLHA#issuecomment-586204572>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAV6E3DHV27QOZ5IRTR7GJLRCZYJVANCNFSM4KVFMOQA>
> .
>

hengfengli: @lgruen I think we can have two CircleCi jobs, i.e., unit tests and integration tests (or builds). We only run unit tests for forked PRs (a PR from a forked repo). When you enable CI for forked PRs, it should provide an option “Pass secrets to builds from forked pull requests”, which is disabled by default. So the environment variables are not visible in the unit tests in forked PRs. 

When the forked PR is approved and merged with the master, this becomes a push to the master repo (not a forked PR) so it will trigger both unit tests and integration tests and the secrets will be visible to them. 

Please see the example in the reference. 

### Reference
* https://circleci.com/blog/triggering-trusted-ci-jobs-on-untrusted-forks/
hengfengli: I set up a small demo: https://github.com/hengfengli/test-circleci. It works well. 

Patch:
diff --git a/internal/postgres/process.go b/internal/postgres/process.go
index b776b59..586e230 100644
--- a/internal/postgres/process.go
+++ b/internal/postgres/process.go
@@ -31,11 +31,14 @@ import (
 // In schema mode, ProcessPgDump incrementally builds a schema (updating conv).
 // In data mode, ProcessPgDump uses this schema to convert PostgreSQL data
 // and writes it to Spanner, using the data sink specified in conv.
-func ProcessPgDump(conv *Conv, r *internal.Reader) {
+func ProcessPgDump(conv *Conv, r *internal.Reader) error {
 	for {
 		startLine := r.LineNumber
 		startOffset := r.Offset
-		b, stmts := readAndParseChunk(conv, r)
+		b, stmts, err := readAndParseChunk(conv, r)
+		if err != nil {
+			return err
+		}
 		ci := processStatements(conv, stmts)
 		internal.VerbosePrintf("Parsed SQL command at line=%d/fpos=%d: %d stmts (%d lines, %d bytes) ci=%v\n", startLine, startOffset, len(stmts), r.LineNumber-startLine, len(b), ci != nil)
 		if ci != nil {
@@ -53,6 +56,8 @@ func ProcessPgDump(conv *Conv, r *internal.Reader) {
 	if conv.schemaMode() {
 		conv.AddPrimaryKeys()
 	}
+
+	return nil
 }
 
 // ProcessRow converts a row of data and writes it out to Spanner.
@@ -98,7 +103,7 @@ func byteSize(r *row) int64 {
 
 // readAndParseChunk parses a chunk of pg_dump data, returning the bytes read,
 // the parsed AST (nil if nothing read), and whether we've hit end-of-file.
-func readAndParseChunk(conv *Conv, r *internal.Reader) ([]byte, []nodes.Node) {
+func readAndParseChunk(conv *Conv, r *internal.Reader) ([]byte, []nodes.Node, error) {
 	var l [][]byte
 	for {
 		b := r.ReadLine()
@@ -117,7 +122,7 @@ func readAndParseChunk(conv *Conv, r *internal.Reader) ([]byte, []nodes.Node) {
 			}
 			tree, err := pg_query.Parse(string(s))
 			if err == nil {
-				return s, tree.Statements
+				return s, tree.Statements, nil
 			}
 			// Likely causes of failing to parse:
 			// a) complex statements with embedded semicolons e.g. 'CREATE FUNCTION'
@@ -127,10 +132,7 @@ func readAndParseChunk(conv *Conv, r *internal.Reader) ([]byte, []nodes.Node) {
 			conv.stats.reparsed++
 		}
 		if r.EOF {
-			if len(l) != 0 {
-				fmt.Printf("Error parsing last %d line(s) of input\n", len(l))
-			}
-			return nil, nil
+			return nil, nil, fmt.Errorf("Error parsing last %d line(s) of input", len(l))
 		}
 	}
 }
diff --git a/main.go b/main.go
index 8104d66..e838a70 100644
--- a/main.go
+++ b/main.go
@@ -94,7 +94,10 @@ func main() {
 	if filePrefix == "" {
 		filePrefix = dbName + "."
 	}
-	firstPass(f, n, conv)
+	err = firstPass(f, n, conv)
+	if err != nil {
+		panic(fmt.Errorf("Failed to parse the data file: %v", err))
+	}
 	writeSchemaFile(conv, now, filePrefix+schemaFile)
 	db, err := createDatabase(project, instance, dbName, now, conv)
 	if err != nil {
@@ -143,13 +146,17 @@ func report(bw *spanner.BatchWriter, bytesRead int64, now time.Time, db string,
 	writeBadData(bw, conv, banner, filePrefix+badDataFile)
 }
 
-func firstPass(f *os.File, fileSize int64, conv *postgres.Conv) {
+func firstPass(f *os.File, fileSize int64, conv *postgres.Conv) error {
 	p := internal.NewProgress(fileSize, "Generating schema", internal.Verbose())
 	r := internal.NewReader(bufio.NewReader(f), p)
 	conv.SetSchemaMode() // Build schema and ignore data in pg_dump.
 	conv.SetDataSink(nil)
-	postgres.ProcessPgDump(conv, r)
+	err := postgres.ProcessPgDump(conv, r)
+	if err != nil {
+		return err
+	}
 	p.Done()
+	return nil
 }
 
 func secondPass(f *os.File, client *sp.Client, conv *postgres.Conv, totalRows int64) *spanner.BatchWriter {


Test patch:
diff --git a/internal/postgres/process_test.go b/internal/postgres/process_test.go
index 6a6d327..905c318 100644
--- a/internal/postgres/process_test.go
+++ b/internal/postgres/process_test.go
@@ -530,6 +530,20 @@ func TestProcessPgDump_AddPrimaryKeys(t *testing.T) {
 	}
 }
 
+func TestProcessPgDump_WithUnparsableContent(t *testing.T) {
+	s := "This is unparsable content"
+	conv := MakeConv()
+	conv.SetLocation(time.UTC)
+	conv.SetSchemaMode()
+	err := ProcessPgDump(conv, internal.NewReader(bufio.NewReader(strings.NewReader(s)), nil))
+	if err == nil {
+		t.Fatalf("Expect an error, but got nil")
+	}
+	if !strings.Contains(err.Error(), "Error parsing") {
+		t.Fatalf("Expect a parsing error, but got %q", err)
+	}
+}
+
 func runProcessPgDump(s string) (*Conv, []spannerData) {
 	conv := MakeConv()
 	conv.SetLocation(time.UTC)