Skip to content

Commit 058e6ca

Browse files
jhumpjhump
authored
Fix issue with missing imports for custom options in desc/builder (#592)
1 parent bf8a7d8 commit 058e6ca

File tree

5 files changed

+67
-30
lines changed

5 files changed

+67
-30
lines changed

‎desc/builder/builder_test.go‎

Lines changed: 39 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1155,18 +1155,35 @@ func TestCustomOptionsDiscoveredInDependencies(t *testing.T) {
11551155
fileDesc, err := file.Build()
11561156
testutil.Ok(t, err)
11571157

1158+
// Another file that imports the options. Since it's not a public import, presence
1159+
// of this file should not prevent builder from correctly adding options.proto dependency
1160+
// in the "auto" case below.
1161+
otherFileDesc, err := NewFile("other.proto").AddImportedDependency(fileDesc).Build()
1162+
testutil.Ok(t, err)
1163+
1164+
regWithOpts := &dynamic.ExtensionRegistry{}
1165+
regWithOpts.AddExtensionsFromFileRecursively(fileDesc)
1166+
11581167
// Now we can test referring to these and making sure they show up correctly
11591168
// in built descriptors
1160-
for name, useBuilder := range map[string]bool{"descriptor": false, "builder": true} {
1169+
for name, useBuilder := range map[string]*bool{"descriptor": proto.Bool(false), "builder": proto.Bool(true), "auto": nil} {
11611170
newFile := func() *FileBuilder {
1162-
fb := NewFile("foo.proto")
1163-
if useBuilder {
1164-
fb.AddDependency(file)
1165-
} else {
1166-
fb.AddImportedDependency(fileDesc)
1171+
fb := NewFile("foo.proto").AddImportedDependency(otherFileDesc)
1172+
if useBuilder != nil {
1173+
if *useBuilder {
1174+
fb.AddDependency(file)
1175+
} else {
1176+
fb.AddImportedDependency(fileDesc)
1177+
}
11671178
}
11681179
return fb
11691180
}
1181+
var extReg *dynamic.ExtensionRegistry
1182+
if useBuilder == nil {
1183+
// if providing neither builder nor descriptor, we need to provide
1184+
// a registry for resolving custom options
1185+
extReg = regWithOpts
1186+
}
11701187
t.Run(name, func(t *testing.T) {
11711188
t.Run("file options", func(t *testing.T) {
11721189
fb := newFile()
@@ -1175,7 +1192,7 @@ func TestCustomOptionsDiscoveredInDependencies(t *testing.T) {
11751192
testutil.Ok(t, err)
11761193
err = dynamic.SetExtension(fb.Options, ext, "fubar")
11771194
testutil.Ok(t, err)
1178-
checkBuildWithImportedExtensions(t, fb)
1195+
checkBuildWithImportedExtensions(t, fb, extReg)
11791196
})
11801197

11811198
t.Run("message options", func(t *testing.T) {
@@ -1188,7 +1205,7 @@ func TestCustomOptionsDiscoveredInDependencies(t *testing.T) {
11881205

11891206
fb := newFile()
11901207
fb.AddMessage(mb)
1191-
checkBuildWithImportedExtensions(t, mb)
1208+
checkBuildWithImportedExtensions(t, mb, extReg)
11921209
})
11931210

11941211
t.Run("field options", func(t *testing.T) {
@@ -1203,7 +1220,7 @@ func TestCustomOptionsDiscoveredInDependencies(t *testing.T) {
12031220

12041221
fb := newFile()
12051222
fb.AddMessage(mb)
1206-
checkBuildWithImportedExtensions(t, flb)
1223+
checkBuildWithImportedExtensions(t, flb, extReg)
12071224
})
12081225

12091226
t.Run("oneof options", func(t *testing.T) {
@@ -1219,7 +1236,7 @@ func TestCustomOptionsDiscoveredInDependencies(t *testing.T) {
12191236

12201237
fb := newFile()
12211238
fb.AddMessage(mb)
1222-
checkBuildWithImportedExtensions(t, oob)
1239+
checkBuildWithImportedExtensions(t, oob, extReg)
12231240
})
12241241

12251242
t.Run("extension range options", func(t *testing.T) {
@@ -1232,7 +1249,7 @@ func TestCustomOptionsDiscoveredInDependencies(t *testing.T) {
12321249

12331250
fb := newFile()
12341251
fb.AddMessage(mb)
1235-
checkBuildWithImportedExtensions(t, mb)
1252+
checkBuildWithImportedExtensions(t, mb, extReg)
12361253
})
12371254

12381255
t.Run("enum options", func(t *testing.T) {
@@ -1246,7 +1263,7 @@ func TestCustomOptionsDiscoveredInDependencies(t *testing.T) {
12461263

12471264
fb := newFile()
12481265
fb.AddEnum(eb)
1249-
checkBuildWithImportedExtensions(t, eb)
1266+
checkBuildWithImportedExtensions(t, eb, extReg)
12501267
})
12511268

12521269
t.Run("enum val options", func(t *testing.T) {
@@ -1261,7 +1278,7 @@ func TestCustomOptionsDiscoveredInDependencies(t *testing.T) {
12611278

12621279
fb := newFile()
12631280
fb.AddEnum(eb)
1264-
checkBuildWithImportedExtensions(t, evb)
1281+
checkBuildWithImportedExtensions(t, evb, extReg)
12651282
})
12661283

12671284
t.Run("service options", func(t *testing.T) {
@@ -1274,7 +1291,7 @@ func TestCustomOptionsDiscoveredInDependencies(t *testing.T) {
12741291

12751292
fb := newFile()
12761293
fb.AddService(sb)
1277-
checkBuildWithImportedExtensions(t, sb)
1294+
checkBuildWithImportedExtensions(t, sb, extReg)
12781295
})
12791296

12801297
t.Run("method options", func(t *testing.T) {
@@ -1293,20 +1310,22 @@ func TestCustomOptionsDiscoveredInDependencies(t *testing.T) {
12931310

12941311
fb := newFile()
12951312
fb.AddService(sb).AddMessage(req).AddMessage(resp)
1296-
checkBuildWithImportedExtensions(t, mtb)
1313+
checkBuildWithImportedExtensions(t, mtb, extReg)
12971314
})
12981315
})
12991316
}
13001317
}
13011318

1302-
func checkBuildWithImportedExtensions(t *testing.T, builder Builder) {
1319+
func checkBuildWithImportedExtensions(t *testing.T, builder Builder, extReg *dynamic.ExtensionRegistry) {
13031320
// requiring options and succeeding (since they are defined in explicit import)
1304-
var opts BuilderOptions
1305-
opts.RequireInterpretedOptions = true
1321+
opts := BuilderOptions{
1322+
RequireInterpretedOptions: true,
1323+
Extensions: extReg,
1324+
}
13061325
d, err := opts.Build(builder)
13071326
testutil.Ok(t, err)
1308-
// the only import is for the custom options
1309-
testutil.Eq(t, []string{"options.proto"}, d.GetFile().AsFileDescriptorProto().GetDependency())
1327+
// the only import is the explicitly added one and one added for the custom options
1328+
testutil.Eq(t, []string{"options.proto", "other.proto"}, d.GetFile().AsFileDescriptorProto().GetDependency())
13101329
}
13111330

13121331
func TestUseOfExtensionRegistry(t *testing.T) {

‎desc/builder/resolver.go‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ func (d *dependencies) add(fd *desc.FileDescriptor) {
3333
return
3434
}
3535
d.descs[fd] = struct{}{}
36-
internal.RegisterExtensionsForFile(&d.res, fd.UnwrapFile())
36+
internal.RegisterExtensionsFromImportedFile(&d.res, fd.UnwrapFile())
3737
}
3838

3939
// dependencyResolver is the work-horse for converting a tree of builders into a
@@ -150,7 +150,6 @@ func (r *dependencyResolver) resolveFile(fb *FileBuilder, root Builder, seen []B
150150
fileNames := map[string]struct{}{}
151151
for _, d := range depSlice {
152152
addFileNames(d, fileNames)
153-
fileNames[d.GetName()] = struct{}{}
154153
}
155154
unique := makeUnique(fp.GetName(), fileNames)
156155
if unique != fp.GetName() {
@@ -481,7 +480,7 @@ func (r *dependencyResolver) resolveTypesInOptions(root Builder, fileExts *dynam
481480
continue
482481
}
483482
// see if configured extension registry knows about it
484-
if extd := r.opts.Extensions.FindExtension(string(msgName), int32(tag)); extd != nil {
483+
if extd := r.opts.Extensions.FindExtension(msgName, tag); extd != nil {
485484
// extension registry recognized it!
486485
deps.add(extd.GetFile())
487486
continue

‎desc/internal/registry.go‎

Lines changed: 21 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,11 +6,30 @@ import (
66
"google.golang.org/protobuf/types/dynamicpb"
77
)
88

9-
func RegisterExtensionsForFile(reg *protoregistry.Types, fd protoreflect.FileDescriptor) {
9+
// RegisterExtensionsFromImportedFile registers extensions in the given file as well
10+
// as those in its public imports. So if another file imports the given fd, this adds
11+
// all extensions made visible to that importing file.
12+
//
13+
// All extensions in the given file are made visible to the importing file, and so are
14+
// extensions in any public imports in the given file.
15+
func RegisterExtensionsFromImportedFile(reg *protoregistry.Types, fd protoreflect.FileDescriptor) {
16+
registerTypesForFile(reg, fd, true, true)
17+
}
18+
19+
// RegisterExtensionsVisibleToFile registers all extensions visible to the given file.
20+
// This includes all extensions defined in fd and as well as extensions defined in the
21+
// files that it imports (and any public imports thereof, etc).
22+
//
23+
// This is effectively the same as registering the extensions in fd and then calling
24+
// RegisterExtensionsFromImportedFile for each file imported by fd.
25+
func RegisterExtensionsVisibleToFile(reg *protoregistry.Types, fd protoreflect.FileDescriptor) {
1026
registerTypesForFile(reg, fd, true, false)
1127
}
1228

13-
func RegisterTypesForFile(reg *protoregistry.Types, fd protoreflect.FileDescriptor) {
29+
// RegisterTypesVisibleToFile registers all types visible to the given file.
30+
// This is the same as RegisterExtensionsVisibleToFile but it also registers
31+
// message and enum types, not just extensions.
32+
func RegisterTypesVisibleToFile(reg *protoregistry.Types, fd protoreflect.FileDescriptor) {
1433
registerTypesForFile(reg, fd, false, false)
1534
}
1635

‎desc/protoparse/source_code_info_test.go‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -37,9 +37,9 @@ func TestSourceCodeInfo(t *testing.T) {
3737
// (human readable so diffs in source control are comprehensible)
3838
var buf bytes.Buffer
3939
for _, fd := range fds {
40-
printSourceCodeInfo(t, fd, &buf)
40+
printSourceCodeInfo(fd, &buf)
4141
}
42-
printSourceCodeInfo(t, importedFd, &buf)
42+
printSourceCodeInfo(importedFd, &buf)
4343
actual := buf.String()
4444

4545
if regenerateMode {
@@ -58,11 +58,11 @@ func TestSourceCodeInfo(t *testing.T) {
5858
// NB: this function can be used to manually inspect the source code info for a
5959
// descriptor, in a manner that is much easier to read and check than raw
6060
// descriptor form.
61-
func printSourceCodeInfo(t *testing.T, fd *desc.FileDescriptor, out io.Writer) {
61+
func printSourceCodeInfo(fd *desc.FileDescriptor, out io.Writer) {
6262
_, _ = fmt.Fprintf(out, "---- %s ----\n", fd.GetName())
6363
msg := fd.AsFileDescriptorProto().ProtoReflect()
6464
var reg protoregistry.Types
65-
internal.RegisterExtensionsForFile(&reg, fd.UnwrapFile())
65+
internal.RegisterExtensionsVisibleToFile(&reg, fd.UnwrapFile())
6666

6767
for _, loc := range fd.AsFileDescriptorProto().GetSourceCodeInfo().GetLocation() {
6868
var buf bytes.Buffer

‎desc/protoprint/print.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -319,7 +319,7 @@ func (p *Printer) printProto(dsc desc.Descriptor, out io.Writer) error {
319319
extendOptionLocations(sourceInfo, fdp.GetSourceCodeInfo().GetLocation())
320320

321321
var reg protoregistry.Types
322-
internal.RegisterTypesForFile(&reg, dsc.GetFile().UnwrapFile())
322+
internal.RegisterTypesVisibleToFile(&reg, dsc.GetFile().UnwrapFile())
323323
reparseUnknown(&reg, fdp.ProtoReflect())
324324

325325
path := findElement(dsc)

0 commit comments

Comments
 (0)