Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

protoparse: enforce unique JSON names #523

Merged
merged 2 commits into from Aug 17, 2022
Merged

protoparse: enforce unique JSON names #523

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9ca2033
enforce unique JSON names, case-insensitive
jhump Aug 16, 2022
69c0288
remove superfluous comment
jhump Aug 17, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion desc/protoparse/descriptor_protos.go
View file
Open in desktop
Expand Up @@ -33,7 +33,7 @@ func (r *parseResult) createFileDescriptor(filename string, file *ast.FileNode)
fd.Syntax = proto.String(file.Syntax.Syntax.AsString())
}
} else {
r.errs.warn(file.Start(), ErrNoSyntax)
r.errs.warnWithPos(file.Start(), ErrNoSyntax)
}

for _, decl := range file.Decls {
Expand Down
10 changes: 8 additions & 2 deletions desc/protoparse/errors.go
View file
Open in desktop
Expand Up @@ -76,12 +76,18 @@ func (h *errorHandler) handleError(err error) error {
return err
}

func (h *errorHandler) warn(pos *SourcePos, err error) {
func (h *errorHandler) warnWithPos(pos *SourcePos, err error) {
if h.warnReporter != nil {
h.warnReporter(ErrorWithSourcePos{Pos: pos, Underlying: err})
}
}

func (h *errorHandler) warn(err ErrorWithSourcePos) {
if h.warnReporter != nil {
h.warnReporter(err)
}
}

func (h *errorHandler) getError() error {
if h.errsReported > 0 && h.err == nil {
return ErrInvalidSource
Expand Down Expand Up @@ -138,7 +144,7 @@ func (e ErrorWithSourcePos) Unwrap() error {

var _ ErrorWithPos = ErrorWithSourcePos{}

func errorWithPos(pos *SourcePos, format string, args ...interface{}) ErrorWithPos {
func errorWithPos(pos *SourcePos, format string, args ...interface{}) ErrorWithSourcePos {
return ErrorWithSourcePos{Pos: pos, Underlying: fmt.Errorf(format, args...)}
}

Expand Down
103 changes: 102 additions & 1 deletion desc/protoparse/linker.go
View file
Open in desktop
Expand Up @@ -75,6 +75,10 @@ func (l *linker) linkFiles() (map[string]*desc.FileDescriptor, error) {
if err := l.checkExtensionsInFile(fd, r); err != nil {
return nil, err
}
// and final check for json name configuration
if err := l.checkJsonNamesInFile(fd, r); err != nil {
return nil, err
}
}

// When Parser calls linkFiles, it does not check errs again, and it expects that linkFiles
Expand Down Expand Up @@ -1051,7 +1055,7 @@ func (l *linker) checkForUnusedImports(filename string) {
if pos == nil {
pos = ast.UnknownPos(r.fd.GetName())
}
l.errs.warn(pos, errUnusedImport(dep))
l.errs.warnWithPos(pos, errUnusedImport(dep))
}
}
}
Expand Down Expand Up @@ -1111,3 +1115,100 @@ func (l *linker) checkExtension(fld *desc.FieldDescriptor, res *parseResult) err

return nil
}

func (l *linker) checkJsonNamesInFile(fd *desc.FileDescriptor, res *parseResult) error {
for _, md := range fd.GetMessageTypes() {
if err := l.checkJsonNamesInMessage(md, res); err != nil {
return err
}
}
return nil
}

func (l *linker) checkJsonNamesInMessage(md *desc.MessageDescriptor, res *parseResult) error {
if err := checkJsonNames(md, res, false); err != nil {
return err
}
if err := checkJsonNames(md, res, true); err != nil {
return err
}
return nil
}

func checkJsonNames(md *desc.MessageDescriptor, res *parseResult, useCustom bool) error {
type seenName struct {
source *dpb.FieldDescriptorProto
// field's original JSON nane (which can differ in case from map key)
orig string
// true if orig is a custom JSON name (vs. the field's default JSON name)
custom bool
}
seen := map[string]seenName{}

for _, fd := range md.GetFields() {
scope := "field " + md.GetName() + "." + fd.GetName()
defaultName := internal.JsonName(fd.GetName())
name := defaultName
custom := false
if useCustom {
n := fd.GetJSONName()
if n != defaultName || hasCustomJsonName(res, fd) {
name = n
custom = true
}
}
lcaseName := strings.ToLower(name)
if existing, ok := seen[lcaseName]; ok {
// When useCustom is true, we'll only report an issue when a conflict is
// due to a custom name. That way, we don't double report conflicts on
// non-custom names.
if !useCustom || custom || existing.custom {
fldNode := res.getFieldNode(fd.AsFieldDescriptorProto())
customStr, srcCustomStr := "custom", "custom"
if !custom {
customStr = "default"
}
if !existing.custom {
srcCustomStr = "default"
}
otherName := ""
if name != existing.orig {
otherName = fmt.Sprintf(" %q", existing.orig)
}
conflictErr := errorWithPos(fldNode.Start(), "%s: %s JSON name %q conflicts with %s JSON name%s of field %s, defined at %v",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
conflictErr := errorWithPos(fldNode.Start(), "%s: %s JSON name %q conflicts with %s JSON name%s of field %s, defined at %v",
conflictErr := errorWithPos(fldNode.Start(), "%s: %s JSON name %q conflicts with %s JSON name %s of field %s, defined at %v",

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would have thought we'd need a space here, but is there always a space in the value we're including here? By the error messages included in your test, everything looks correct, so I'm a little surprised.

This would be the existing.source.GetName() value.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We only print a name here when the two names that conflict differ (which can happen if they vary only by case). So if the previously defined JSON name is the same as the one causing the conflict, we omit it entirely. See how the name var is actually constructed above: if it's not empty, it includes a leading space.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah ha! Thanks for the clarification - makes sense.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, this is actually for the origValue value. The existing.source.GetName() is the next %s in the format string.

scope, customStr, name, srcCustomStr, otherName, existing.source.GetName(), res.getFieldNode(existing.source).Start())

// Since proto2 did not originally have default JSON names, we report conflicts involving
// default names as just warnings.
if !md.IsProto3() && (!custom || !existing.custom) {
res.errs.warn(conflictErr)
} else if err := res.errs.handleError(conflictErr); err != nil {
return err
}
}
} else {
seen[lcaseName] = seenName{orig: name, source: fd.AsFieldDescriptorProto(), custom: custom}
}
}
return nil
}

func hasCustomJsonName(res *parseResult, fd *desc.FieldDescriptor) bool {
// if we have the AST, we can more precisely determine if there was a custom
// JSON named defined, even if it is explicitly configured to tbe the same
// as the default JSON name for the field.
fdProto := fd.AsFieldDescriptorProto()
opts := res.getFieldNode(fdProto).GetOptions()
if opts == nil {
return false
}
for _, opt := range opts.Options {
if len(opt.Name.Parts) == 1 &&
opt.Name.Parts[0].Name.AsIdentifier() == "json_name" &&
!opt.Name.Parts[0].IsExtension() {
// found it
jhump marked this conversation as resolved.
Show resolved Hide resolved
return true
}
}
return false
}
171 changes: 171 additions & 0 deletions desc/protoparse/linker_test.go
View file
Open in desktop
Expand Up @@ -1006,6 +1006,115 @@ func TestLinkerValidation(t *testing.T) {
},
"foo.proto:4:3: field Foo.e: google.protobuf.Struct.FieldsEntry is a synthetic map entry and may not be referenced explicitly",
},
{
map[string]string{
"foo.proto": "syntax = \"proto3\";\n" +
"message Foo {\n" +
" string foo = 1;\n" +
" string bar = 2 [json_name=\"foo\"];\n" +
"}\n",
},
"foo.proto:4:3: field Foo.bar: custom JSON name \"foo\" conflicts with default JSON name of field foo, defined at foo.proto:3:3",
},
{
map[string]string{
"foo.proto": "syntax = \"proto3\";\n" +
"message Foo {\n" +
" string foo = 1 [json_name=\"foo_bar\"];\n" +
" string bar = 2 [json_name=\"Foo_Bar\"];\n" +
"}\n",
},
"foo.proto:4:3: field Foo.bar: custom JSON name \"Foo_Bar\" conflicts with custom JSON name \"foo_bar\" of field foo, defined at foo.proto:3:3",
},
{
map[string]string{
"foo.proto": "syntax = \"proto3\";\n" +
"message Foo {\n" +
" string fooBar = 1;\n" +
" string foo_bar = 2;\n" +
"}\n",
},
"foo.proto:4:3: field Foo.foo_bar: default JSON name \"fooBar\" conflicts with default JSON name of field fooBar, defined at foo.proto:3:3",
},
{
map[string]string{
"foo.proto": "syntax = \"proto3\";\n" +
"message Foo {\n" +
" string fooBar = 1;\n" +
" string foo_bar = 2 [json_name=\"fuber\"];\n" +
"}\n",
},
"foo.proto:4:3: field Foo.foo_bar: default JSON name \"fooBar\" conflicts with default JSON name of field fooBar, defined at foo.proto:3:3",
}, {
map[string]string{
"foo.proto": "syntax = \"proto3\";\n" +
"message Foo {\n" +
" string fooBar = 1;\n" +
" string FOO_BAR = 2;\n" +
"}\n",
},
"foo.proto:4:3: field Foo.FOO_BAR: default JSON name \"FOOBAR\" conflicts with default JSON name \"fooBar\" of field fooBar, defined at foo.proto:3:3",
},
{
map[string]string{
"foo.proto": "syntax = \"proto3\";\n" +
"message Foo {\n" +
" string fooBar = 1;\n" +
" string __foo_bar = 2;\n" +
"}\n",
},
"foo.proto:4:3: field Foo.__foo_bar: default JSON name \"FooBar\" conflicts with default JSON name \"fooBar\" of field fooBar, defined at foo.proto:3:3",
},
{
map[string]string{
"foo.proto": "syntax = \"proto2\";\n" +
"message Foo {\n" +
" optional string foo = 1 [json_name=\"foo_bar\"];\n" +
" optional string bar = 2 [json_name=\"Foo_Bar\"];\n" +
"}\n",
},
"foo.proto:4:3: field Foo.bar: custom JSON name \"Foo_Bar\" conflicts with custom JSON name \"foo_bar\" of field foo, defined at foo.proto:3:3",
},
{
map[string]string{
"foo.proto": "syntax = \"proto2\";\n" +
"message Foo {\n" +
" optional string fooBar = 1;\n" +
" optional string foo_bar = 2;\n" +
"}\n",
},
"", // should succeed: only check default JSON names in proto3
},
{
map[string]string{
"foo.proto": "syntax = \"proto2\";\n" +
"message Foo {\n" +
" optional string fooBar = 1 [json_name=\"fooBar\"];\n" +
" optional string foo_bar = 2 [json_name=\"fooBar\"];\n" +
"}\n",
},
"foo.proto:4:3: field Foo.foo_bar: custom JSON name \"fooBar\" conflicts with custom JSON name of field fooBar, defined at foo.proto:3:3",
},
{
map[string]string{
"foo.proto": "syntax = \"proto2\";\n" +
"message Foo {\n" +
" optional string fooBar = 1;\n" +
" optional string FOO_BAR = 2;\n" +
"}\n",
},
"", // should succeed: only check default JSON names in proto3
},
{
map[string]string{
"foo.proto": "syntax = \"proto2\";\n" +
"message Foo {\n" +
" optional string fooBar = 1;\n" +
" optional string __foo_bar = 2;\n" +
"}\n",
},
"", // should succeed: only check default JSON names in proto3
},
}
for i, tc := range testCases {
acc := func(filename string) (io.ReadCloser, error) {
Expand Down Expand Up @@ -1180,3 +1289,65 @@ func TestSyntheticOneOfCollisions(t *testing.T) {
}
testutil.Eq(t, actual, expected)
}

func TestCustomJSONNameWarnings(t *testing.T) {
testCases := []struct {
source string
warning string
}{
{
source: "syntax = \"proto2\";\n" +
"message Foo {\n" +
" optional string foo = 1;\n" +
" optional string bar = 2 [json_name=\"foo\"];\n" +
"}\n",
warning: "test.proto:4:3: field Foo.bar: custom JSON name \"foo\" conflicts with default JSON name of field foo, defined at test.proto:3:3",
},
{
source: "syntax = \"proto2\";\n" +
"message Foo {\n" +
" optional string foo_bar = 1;\n" +
" optional string fooBar = 2;\n" +
"}\n",
warning: "test.proto:4:3: field Foo.fooBar: default JSON name \"fooBar\" conflicts with default JSON name of field foo_bar, defined at test.proto:3:3",
},
{
source: "syntax = \"proto2\";\n" +
"message Foo {\n" +
" optional string foo_bar = 1;\n" +
" optional string fooBar = 2;\n" +
"}\n",
warning: "test.proto:4:3: field Foo.fooBar: default JSON name \"fooBar\" conflicts with default JSON name of field foo_bar, defined at test.proto:3:3",
},
}
for i, tc := range testCases {
acc := func(filename string) (io.ReadCloser, error) {
if filename == "test.proto" {
return ioutil.NopCloser(strings.NewReader(tc.source)), nil
}
return nil, fmt.Errorf("file not found: %s", filename)
}
var warnings []string
warnFunc := func(err ErrorWithPos) {
warnings = append(warnings, err.Error())
}
_, err := Parser{Accessor: acc, WarningReporter: warnFunc}.ParseFiles("test.proto")
if err != nil {
t.Errorf("case %d: expecting no error; instead got error %q", i, err)
}
if tc.warning == "" && len(warnings) > 0 {
t.Errorf("case %d: expecting no warnings; instead got: %v", i, warnings)
} else {
found := false
for _, w := range warnings {
if w == tc.warning {
found = true
break
}
}
if !found {
t.Errorf("case %d: expecting warning %q; instead got: %v", i, tc.warning, warnings)
}
}
}
}