Repository: jhump/protoreflect
Pull request #331 (2020-06-25 15:58:38)

Propagate errors within linkFiles
This issue came up in buf. Buf has it's own `ErrorReporter` scheme, where `nil` is always returned. After debugging, we found out that `handleErrorWithPos` was being called after the last call to `getError`, which meant an error was being reported that was not being propagated to the return value of `ParseFiles`. This meant that the `ErrorReporter` had been called, but `ParseFiles` returned no error, when it should have returned `ErrInvalidSource`.

This fixes the issue by doing another call to `getError` at the end of `linkFiles`, so that `linkFiles` returns all errors, as `ParseFiles` expects (`ParseFiles` does not call `getError` again). This also adds a specific test for this that reproduces the issue we saw - without the fix, this test fails, but with this fix, it passes.


Patch:
diff --git a/desc/protoparse/linker.go b/desc/protoparse/linker.go
index 1b42e02..9d49e56 100644
--- a/desc/protoparse/linker.go
+++ b/desc/protoparse/linker.go
@@ -73,6 +73,12 @@ func (l *linker) linkFiles() (map[string]*desc.FileDescriptor, error) {
 		}
 	}
 
+	// When Parser calls linkFiles, it does not check errs again, and it expects that linkFiles
+	// will return all errors it should process. If the ErrorReporter handles all errors itself
+	// and always returns nil, we should get ErrInvalidSource here, and need to propagate this
+	if err := l.errs.getError(); err != nil {
+		return nil, err
+	}
 	return linked, nil
 }
 


Test patch:
diff --git a/desc/protoparse/linker_test.go b/desc/protoparse/linker_test.go
index 9c643aa..26c2384 100644
--- a/desc/protoparse/linker_test.go
+++ b/desc/protoparse/linker_test.go
@@ -393,3 +393,55 @@ func TestProto3Enums(t *testing.T) {
 		}
 	}
 }
+
+func TestCustomErrorReporterWithLinker(t *testing.T) {
+	input := map[string]string{
+		"a/b/b.proto": `package a.b;
+
+import "google/protobuf/descriptor.proto";
+
+extend google.protobuf.FieldOptions {
+  optional Foo foo = 50001;
+}
+
+message Foo {
+  optional string bar = 1;
+}`,
+		"a/c/c.proto": `import "a/b/b.proto";
+
+message ReferencesFooOption {
+  optional string baz = 1 [(a.b.foo).bat = "hello"];
+}`,
+	}
+	errMsg := "a/c/c.proto:4:38: field ReferencesFooOption.baz: option (a.b.foo).bat: field bat of a.b.Foo does not exist"
+
+	acc := func(filename string) (io.ReadCloser, error) {
+		f, ok := input[filename]
+		if !ok {
+			return nil, fmt.Errorf("file not found: %s", filename)
+		}
+		return ioutil.NopCloser(strings.NewReader(f)), nil
+	}
+	names := make([]string, 0, len(input))
+	for k := range input {
+		names = append(names, k)
+	}
+	var errs []error
+	_, err := Parser{
+		Accessor: acc,
+		ErrorReporter: func(errorWithPos ErrorWithPos) error {
+			errs = append(errs, errorWithPos)
+			// need to return nil to make sure this test case works
+			// this will result in us only getting an error from errorHandler.getError()
+			// we need to make sure this is called correctly in the linker so that all
+			// errors are properly propagated from the return value of linkFiles(), and
+			// therefor Parse returns ErrInvalidSource
+			return nil
+		},
+	}.ParseFiles(names...)
+	if err != ErrInvalidSource {
+		t.Errorf("expecting validation error %v; instead got: %v", ErrInvalidSource, err)
+	} else if len(errs) != 1 || errs[0].Error() != errMsg {
+		t.Errorf("expecting validation error %q; instead got: %q", errs[0].Error(), errMsg)
+	}
+}