Skip to content

Commit 8fbbfc0

Browse files
coryancoryan
authored
cleanup(sidekick): no maps as nested messages (#5088)
Protobuf creates synthetic nested messages for `map<>` fields. These are annoying as they must be skipped from the mustache templates and from any tests that happen to use map fields. There was a long standing cleanup issue to remove them, and this PR finally implements it. I want to avoid the same annoying code in the Swift codec.
1 parent ce4cd02 commit 8fbbfc0

12 files changed

Lines changed: 103 additions & 18 deletions

File tree

‎internal/sidekick/dart/annotate_test.go‎

Lines changed: 18 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -576,8 +576,21 @@ func TestAnnotateMessage_OmitGeneration_Map(t *testing.T) {
576576
Package: "google.rpc",
577577
}
578578
message := &api.Message{
579+
Name: "HasMap",
580+
ID: ".some.package.HasMap",
581+
Package: "some.package",
582+
Fields: []*api.Field{
583+
{
584+
Name: "map_field",
585+
ID: ".some.package.HasMap.map_field",
586+
Typez: api.MESSAGE_TYPE,
587+
TypezID: ".some.package.HasMap.MapFieldEntry",
588+
},
589+
},
590+
}
591+
mapMessage := &api.Message{
579592
Name: "Entry",
580-
ID: ".some.package.Entry",
593+
ID: ".some.package.HasMap.MapFieldEntry",
581594
Package: "some.package",
582595
IsMap: true,
583596
Fields: []*api.Field{
@@ -594,6 +607,7 @@ func TestAnnotateMessage_OmitGeneration_Map(t *testing.T) {
594607
}
595608
model := api.NewTestAPI([]*api.Message{message}, []*api.Enum{}, []*api.Service{})
596609
model.State.MessageByID[status.ID] = status
610+
model.State.MessageByID[mapMessage.ID] = mapMessage
597611
annotate := newAnnotateModel(model)
598612

599613
annotate.annotateModel(map[string]string{
@@ -602,7 +616,7 @@ func TestAnnotateMessage_OmitGeneration_Map(t *testing.T) {
602616
annotate.annotateMessage(message)
603617

604618
codec := message.Codec.(*messageAnnotation)
605-
if !codec.OmitGeneration {
619+
if codec.OmitGeneration {
606620
t.Errorf("Expected OmitGeneration to be true for map entry")
607621
}
608622

@@ -1804,7 +1818,8 @@ func TestAnnotateField(t *testing.T) {
18041818
},
18051819
} {
18061820
t.Run(test.name, func(t *testing.T) {
1807-
model := api.NewTestAPI([]*api.Message{message, mapMessage}, []*api.Enum{enumState}, []*api.Service{})
1821+
model := api.NewTestAPI([]*api.Message{message}, []*api.Enum{enumState}, []*api.Service{})
1822+
model.State.MessageByID[mapMessage.ID] = mapMessage
18081823
annotate := newAnnotateModel(model)
18091824
registerMissingWkt(annotate.state)
18101825

‎internal/sidekick/dart/dart_test.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -368,7 +368,8 @@ func TestFieldType_Maps(t *testing.T) {
368368
Typez: api.MESSAGE_TYPE,
369369
TypezID: map1.ID,
370370
}
371-
model := api.NewTestAPI([]*api.Message{map1}, []*api.Enum{}, []*api.Service{})
371+
model := api.NewTestAPI([]*api.Message{}, []*api.Enum{}, []*api.Service{})
372+
model.State.MessageByID[map1.ID] = map1
372373
annotate := newAnnotateModel(model)
373374
annotate.annotateModel(map[string]string{})
374375

‎internal/sidekick/parser/protobuf.go‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -600,7 +600,9 @@ func processMessage(state *api.APIState, m *descriptorpb.DescriptorProto, mFQN,
600600
if err != nil {
601601
return nil, err
602602
}
603-
message.Messages = append(message.Messages, nmsg)
603+
if !nmsg.IsMap {
604+
message.Messages = append(message.Messages, nmsg)
605+
}
604606
}
605607
}
606608
for _, e := range m.GetEnumType() {

‎internal/sidekick/parser/protobuf_test.go‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -812,6 +812,10 @@ func TestProtobuf_MapFields(t *testing.T) {
812812
},
813813
})
814814

815+
if diff := cmp.Diff([]*api.Message(nil), message.Messages); diff != "" {
816+
t.Errorf("mismatch (-want +got):\n%s", diff)
817+
}
818+
815819
message, ok = test.State.MessageByID[".test.Fake.SingularMapEntry"]
816820
if !ok {
817821
t.Fatalf("Cannot find message %s in API State", ".test.Fake.SingularMapEntry")

‎internal/sidekick/rust/codec.go‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1461,6 +1461,9 @@ func findUsedPackagesMessage(message *api.Message, model *api.API, c *codec, vis
14611461
case api.MESSAGE_TYPE:
14621462
if fm, ok := model.State.MessageByID[f.TypezID]; ok {
14631463
usePackage(fm.Package, model, c)
1464+
if f.Map {
1465+
findUsedPackagesMessage(fm, model, c, visited)
1466+
}
14641467
}
14651468
case api.ENUM_TYPE:
14661469
if fe, ok := model.State.EnumByID[f.TypezID]; ok {

‎internal/sidekick/rust/codec_test.go‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -704,7 +704,9 @@ func rustFieldTypesCases() *api.API {
704704
},
705705
},
706706
}
707-
return api.NewTestAPI([]*api.Message{target, mapMessage, message}, []*api.Enum{}, []*api.Service{})
707+
model := api.NewTestAPI([]*api.Message{target, message}, []*api.Enum{}, []*api.Service{})
708+
model.State.MessageByID[mapMessage.ID] = mapMessage
709+
return model
708710

709711
}
710712

@@ -870,7 +872,8 @@ func TestFieldMapTypeValues(t *testing.T) {
870872
IsMap: true,
871873
Fields: []*api.Field{key, value},
872874
}
873-
model := api.NewTestAPI([]*api.Message{message, other_message, map_thing}, []*api.Enum{}, []*api.Service{})
875+
model := api.NewTestAPI([]*api.Message{message, other_message}, []*api.Enum{}, []*api.Service{})
876+
model.State.MessageByID[map_thing.ID] = map_thing
874877
api.LabelRecursiveFields(model)
875878
c := createRustCodec()
876879
got, err := c.fieldType(field, model.State, false, model.PackageName)
@@ -933,7 +936,8 @@ func TestFieldMapTypeKey(t *testing.T) {
933936
Name: "EnumType",
934937
ID: ".test.EnumType",
935938
}
936-
model := api.NewTestAPI([]*api.Message{message, map_thing}, []*api.Enum{enum}, []*api.Service{})
939+
model := api.NewTestAPI([]*api.Message{message}, []*api.Enum{enum}, []*api.Service{})
940+
model.State.MessageByID[map_thing.ID] = map_thing
937941
api.LabelRecursiveFields(model)
938942
c := createRustCodec()
939943
got, err := c.fieldType(field, model.State, false, model.PackageName)

‎internal/sidekick/rust/templates/common/message.mustache‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,6 @@ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
1313
See the License for the specific language governing permissions and
1414
limitations under the License.
1515
}}
16-
{{^IsMap}}
1716
{{^ServicePlaceholder}}
1817

1918
{{#Codec.DocLines}}
@@ -244,4 +243,3 @@ pub mod {{Codec.ModuleName}} {
244243
{{#Codec.HasNestedTypes}}
245244
}
246245
{{/Codec.HasNestedTypes}}
247-
{{/IsMap}}

‎internal/sidekick/rust/templates/common/model/debug/message.mustache‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,6 @@ See the License for the specific language governing permissions and
1414
limitations under the License.
1515
}}
1616

17-
{{^IsMap}}
1817
{{^ServicePlaceholder}}
1918
{{> /templates/common/feature_gate}}
2019
impl std::fmt::Debug for super::{{Codec.RelativeName}} {
@@ -33,7 +32,6 @@ impl std::fmt::Debug for super::{{Codec.RelativeName}} {
3332
}
3433
}
3534
{{/ServicePlaceholder}}
36-
{{/IsMap}}
3735
{{#Codec.HasNestedTypes}}
3836
{{#Messages}}
3937
{{> /templates/common/model/debug/message}}

‎internal/sidekick/rust/templates/common/model/deserialize/message.mustache‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,6 @@ See the License for the specific language governing permissions and
1414
limitations under the License.
1515
}}
1616

17-
{{^IsMap}}
1817
{{^ServicePlaceholder}}
1918
{{> /templates/common/feature_gate}}
2019
#[doc(hidden)]
@@ -76,4 +75,3 @@ impl<'de> serde::de::Deserialize<'de> for super::{{Codec.RelativeName}} {
7675
{{/Messages}}
7776
{{/Codec.HasNestedTypes}}
7877
{{/ServicePlaceholder}}
79-
{{/IsMap}}

‎internal/sidekick/rust/templates/common/model/serialize/message.mustache‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,6 @@ See the License for the specific language governing permissions and
1414
limitations under the License.
1515
}}
1616

17-
{{^IsMap}}
1817
{{^ServicePlaceholder}}
1918
{{> /templates/common/feature_gate}}
2019
#[doc(hidden)]
@@ -49,4 +48,3 @@ impl serde::ser::Serialize for super::{{Codec.RelativeName}} {
4948
{{/Messages}}
5049
{{/Codec.HasNestedTypes}}
5150
{{/ServicePlaceholder}}
52-
{{/IsMap}}

0 commit comments

Comments
 (0)