Do not use packed repeating fields for copies numbers #30

New issue

Closed

opened 2023-06-28 08:29:35 +00:00 by alexvanin · 1 comment

alexvanin commented

2023-06-28 08:29:35 +00:00

Owner

#17 introduces copies number as a repeated field in the protocol. There is a compatibility issue with it which can be mitigated.

Proto3 uses packed repeated fields by default for scalar types, e.g. numbers. It means that there is a difference in wire code for one repeated number and just one number.

// message Before {
//     uint32 copies_numbers = 1;
// }
wireBefore, _ := proto.Marshal(&Before{CopiesNumbers: 1}) // 0801

// message After {
//     repeated uint32 copies_numbers = 1;
// }
wireAfter, _ := proto.Marshal(&After{CopiesNumbers: []uint32{1}}) // 0a0101

This is incompatible change and it breaks signature checks.

Describe the solution you'd like

Disable packing for the copies numbers.

// message After {
//     repeated uint32 copies_numbers = 1 [packed=false];
// }
wireAfter, _ := proto.Marshal(&After{CopiesNumbers: []uint32{1}}) // 0801

Describe alternatives you've considered

Ignore this because protocol compatibility is already broken with the #25.

/cc @realloc @fyrchik

## Is your feature request related to a problem? Please describe. #17 introduces copies number as a `repeated` field in the protocol. There is a compatibility issue with it which can be mitigated. Proto3 uses [packed repeated fields](https://protobuf.dev/programming-guides/encoding/#packed) by default for scalar types, e.g. numbers. It means that there is a difference in wire code for one repeated number and just one number. ```go // message Before { // uint32 copies_numbers = 1; // } wireBefore, _ := proto.Marshal(&Before{CopiesNumbers: 1}) // 0801 // message After { // repeated uint32 copies_numbers = 1; // } wireAfter, _ := proto.Marshal(&After{CopiesNumbers: []uint32{1}}) // 0a0101 ``` This is incompatible change and it breaks signature checks. ## Describe the solution you'd like Disable packing for the copies numbers. ```go // message After { // repeated uint32 copies_numbers = 1 [packed=false]; // } wireAfter, _ := proto.Marshal(&After{CopiesNumbers: []uint32{1}}) // 0801 ``` ## Describe alternatives you've considered Ignore this because protocol compatibility is already broken with the #25. /cc @realloc @fyrchik

alexvanin added the

discussion

triage

labels 2023-06-28 08:29:35 +00:00

alexvanin changed title from ~~Do not use packed repeating fields for copies numbers.~~ to Do not use packed repeating fields for copies numbers

2023-06-28 08:30:35 +00:00