From 00882bf480c1517d73b49df6ac31b74a099b1d82 Mon Sep 17 00:00:00 2001 From: kafkiansky Date: Tue, 29 Sep 2026 15:17:25 +0300 Subject: [PATCH 1/2] fix: honour declared packing and accept both repeated encodings The reflection encoder dropped the `packed` flag of a repeated field, so a field declared `packed: false` was still written packed. It is now passed through to the wire. The decoder chose between the packed and the unpacked form by the field's declaration. The protobuf spec requires parsers to accept both encodings of a packable repeated field whatever it is declared as, so the form is now taken from the wire type of the record: a length-delimited record of a packable element type is the packed form, anything else is one element. Strings, bytes and messages are unaffected. OpenEnumTest no longer needs hand-written bytes for the unpacked case. Co-Authored-By: Claude Opus 5.5 --- .../Visitor/ToProtobufValueTypeVisitor.php | 3 + src/Type/Visitor/TypeDeserializerVisitor.php | 5 +- tests/OpenEnumTest.php | 14 +- tests/RepeatedPackingTest.php | 174 ++++++++++++++++++ 4 files changed, 183 insertions(+), 13 deletions(-) create mode 100644 tests/RepeatedPackingTest.php diff --git a/src/Reflection/Internal/Visitor/ToProtobufValueTypeVisitor.php b/src/Reflection/Internal/Visitor/ToProtobufValueTypeVisitor.php index 8f93481..db15eaa 100644 --- a/src/Reflection/Internal/Visitor/ToProtobufValueTypeVisitor.php +++ b/src/Reflection/Internal/Visitor/ToProtobufValueTypeVisitor.php @@ -162,11 +162,14 @@ public function list(ListT $type): \Closure $mapValue = static fn(array $value): array => array_map($mapper, $value); } + $packed = $type->packed; + /** @phpstan-ignore return.type */ return static fn(array $value) => Protobuf\listOf( $element, /** @phpstan-ignore argument.type */ $mapValue($value), + $packed, ); } diff --git a/src/Type/Visitor/TypeDeserializerVisitor.php b/src/Type/Visitor/TypeDeserializerVisitor.php index b30571f..d7d1ec7 100644 --- a/src/Type/Visitor/TypeDeserializerVisitor.php +++ b/src/Type/Visitor/TypeDeserializerVisitor.php @@ -43,6 +43,7 @@ use Thesis\Protobuf\Type\Uint32T; use Thesis\Protobuf\Type\Uint64T; use Thesis\Protobuf\Type\Visitor; +use Thesis\Protobuf\WireType; use function Thesis\Protobuf\fieldT; use function Thesis\Protobuf\messageT; @@ -142,11 +143,13 @@ public function string(StringT $type): DeserializeValue #[\Override] public function list(ListT $type): DeserializeValue { + // Parsers must accept both encodings of a packable repeated field whatever it is declared + // as: a length-delimited record of a packable element type is always the packed form. return new DeserializeList( /** @phpstan-ignore argument.type */ $type->element->accept($this), $this->tag, - $type->packed ?? $type->element->accept(new IsPacked()), + $this->tag->type === WireType::BYTES && $type->element->accept(new IsPacked()), ); } diff --git a/tests/OpenEnumTest.php b/tests/OpenEnumTest.php index 40350e0..057f5d0 100644 --- a/tests/OpenEnumTest.php +++ b/tests/OpenEnumTest.php @@ -51,9 +51,7 @@ public function testPackedRepeatedKeepsKnownValuesInOrder(): void public function testUnpackedRepeatedKeepsKnownValuesInOrder(): void { - // Field 4 as separate varints (tag 0x20): 1, 68, 2, 69. Written by hand, since the - // encoder currently packs repeated int32 even when declared `packed: false`. - $decoded = self::decodeBytes("\x20\x01\x20\x44\x20\x02\x20\x45", OpenEnumTestMessage::class); + $decoded = self::decode(new OpenEnumTestWire(unpacked: [1, 68, 2, 69]), OpenEnumTestMessage::class); self::assertSame([OpenEnumTestColor::RED, OpenEnumTestColor::BLUE], $decoded->unpacked); self::assertSame([[4, WireType::VARINT, '68'], [4, WireType::VARINT, '69']], self::unknowns($decoded)); @@ -98,16 +96,8 @@ public function testMapEntriesWithUnknownValuesAreDropped(): void */ private static function decode(object $wire, string $class): object { - return self::decodeBytes(Encoder\Builder::buildDefault()->encode($wire), $class); - } + $bytes = Encoder\Builder::buildDefault()->encode($wire); - /** - * @template T of object - * @param class-string $class - * @return T - */ - private static function decodeBytes(string $bytes, string $class): object - { return new Decoder\Builder()->withUnknownHandler(UnknownFields::handler())->build()->decode($bytes, $class); } diff --git a/tests/RepeatedPackingTest.php b/tests/RepeatedPackingTest.php new file mode 100644 index 0000000..997486f --- /dev/null +++ b/tests/RepeatedPackingTest.php @@ -0,0 +1,174 @@ +encode(new RepeatedPackingTestPacked([1, 2]))), 'packed: one length-delimited record'); + self::assertSame('08010802', bin2hex($encoder->encode(new RepeatedPackingTestUnpacked([1, 2]))), 'unpacked: a varint record per element'); + self::assertSame('0a020102', bin2hex($encoder->encode(new RepeatedPackingTestDefault([1, 2]))), 'proto3 packs scalars by default'); + } + + /** + * @param class-string $writer + * @param class-string $reader + */ + #[DataProvider('provideBothEncodingsAreAcceptedWhateverTheDeclarationCases')] + public function testBothEncodingsAreAcceptedWhateverTheDeclaration(string $writer, string $reader): void + { + $bytes = Encoder\Builder::buildDefault()->encode(new $writer( + [1, -2, 300], + [1.5, -2.25], + [7, 4_294_967_295], + [true, false, true], + [RepeatedPackingTestColor::RED, RepeatedPackingTestColor::BLUE], + [-1, 5], + )); + + $decoded = Decoder\Builder::buildDefault()->decode($bytes, $reader); + + self::assertSame([1, -2, 300], $decoded->ints); + self::assertSame([1.5, -2.25], $decoded->doubles); + self::assertSame([7, 4_294_967_295], $decoded->fixed); + self::assertSame([true, false, true], $decoded->bools); + self::assertSame([RepeatedPackingTestColor::RED, RepeatedPackingTestColor::BLUE], $decoded->colors); + self::assertSame([-1, 5], $decoded->zigzag); + } + + /** + * @return iterable + */ + public static function provideBothEncodingsAreAcceptedWhateverTheDeclarationCases(): iterable + { + yield 'packed bytes into an unpacked field' => [RepeatedPackingTestPacked::class, RepeatedPackingTestUnpacked::class]; + yield 'unpacked bytes into a packed field' => [RepeatedPackingTestUnpacked::class, RepeatedPackingTestPacked::class]; + yield 'unpacked bytes into a default field' => [RepeatedPackingTestUnpacked::class, RepeatedPackingTestDefault::class]; + } + + public function testDecoderFollowsTheWireTypeNotTheDeclaration(): void + { + $decoder = Decoder\Builder::buildDefault(); + + // Field 1 as two varint records, read into a field declared packed. + self::assertSame([1, 2], $decoder->decode("\x08\x01\x08\x02", RepeatedPackingTestPacked::class)->ints); + // Field 1 as one length-delimited record, read into a field declared unpacked. + self::assertSame([1, 2], $decoder->decode("\x0a\x02\x01\x02", RepeatedPackingTestUnpacked::class)->ints); + } + + public function testNonPackableElementsStayLengthDelimited(): void + { + $bytes = Encoder\Builder::buildDefault()->encode(new RepeatedPackingTestStrings(['a', 'bc'])); + + self::assertSame(['a', 'bc'], Decoder\Builder::buildDefault()->decode($bytes, RepeatedPackingTestStrings::class)->values); + } +} + +enum RepeatedPackingTestColor: int +{ + case UNSPECIFIED = 0; + case RED = 1; + case BLUE = 2; +} + +final readonly class RepeatedPackingTestPacked +{ + /** + * @param list $ints + * @param list $doubles + * @param list $fixed + * @param list $bools + * @param list $colors + * @param list $zigzag + */ + public function __construct( + #[Reflection\Field(1, new Reflection\ListT(Reflection\Int32T::T, packed: true))] + public array $ints = [], + #[Reflection\Field(2, new Reflection\ListT(Reflection\DoubleT::T, packed: true))] + public array $doubles = [], + #[Reflection\Field(3, new Reflection\ListT(Reflection\Fixed32T::T, packed: true))] + public array $fixed = [], + #[Reflection\Field(4, new Reflection\ListT(Reflection\BoolT::T, packed: true))] + public array $bools = [], + #[Reflection\Field(5, new Reflection\ListT(new Reflection\EnumT(RepeatedPackingTestColor::class), packed: true))] + public array $colors = [], + #[Reflection\Field(6, new Reflection\ListT(Reflection\SInt64T::T, packed: true))] + public array $zigzag = [], + ) {} +} + +final readonly class RepeatedPackingTestUnpacked +{ + /** + * @param list $ints + * @param list $doubles + * @param list $fixed + * @param list $bools + * @param list $colors + * @param list $zigzag + */ + public function __construct( + #[Reflection\Field(1, new Reflection\ListT(Reflection\Int32T::T, packed: false))] + public array $ints = [], + #[Reflection\Field(2, new Reflection\ListT(Reflection\DoubleT::T, packed: false))] + public array $doubles = [], + #[Reflection\Field(3, new Reflection\ListT(Reflection\Fixed32T::T, packed: false))] + public array $fixed = [], + #[Reflection\Field(4, new Reflection\ListT(Reflection\BoolT::T, packed: false))] + public array $bools = [], + #[Reflection\Field(5, new Reflection\ListT(new Reflection\EnumT(RepeatedPackingTestColor::class), packed: false))] + public array $colors = [], + #[Reflection\Field(6, new Reflection\ListT(Reflection\SInt64T::T, packed: false))] + public array $zigzag = [], + ) {} +} + +final readonly class RepeatedPackingTestDefault +{ + /** + * @param list $ints + * @param list $doubles + * @param list $fixed + * @param list $bools + * @param list $colors + * @param list $zigzag + */ + public function __construct( + #[Reflection\Field(1, new Reflection\ListT(Reflection\Int32T::T))] + public array $ints = [], + #[Reflection\Field(2, new Reflection\ListT(Reflection\DoubleT::T))] + public array $doubles = [], + #[Reflection\Field(3, new Reflection\ListT(Reflection\Fixed32T::T))] + public array $fixed = [], + #[Reflection\Field(4, new Reflection\ListT(Reflection\BoolT::T))] + public array $bools = [], + #[Reflection\Field(5, new Reflection\ListT(new Reflection\EnumT(RepeatedPackingTestColor::class)))] + public array $colors = [], + #[Reflection\Field(6, new Reflection\ListT(Reflection\SInt64T::T))] + public array $zigzag = [], + ) {} +} + +final readonly class RepeatedPackingTestStrings +{ + /** + * @param list $values + */ + public function __construct( + #[Reflection\Field(1, new Reflection\ListT(Reflection\StringT::T, packed: false))] + public array $values = [], + ) {} +} From af7ca256dc8b5ed66d3507f50a0b0997371b5864 Mon Sep 17 00:00:00 2001 From: kafkiansky Date: Tue, 29 Sep 2026 15:19:22 +0300 Subject: [PATCH 2/2] chore: fix cs --- .../Internal/Visitor/ToProtobufValueTypeVisitor.php | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/Reflection/Internal/Visitor/ToProtobufValueTypeVisitor.php b/src/Reflection/Internal/Visitor/ToProtobufValueTypeVisitor.php index db15eaa..6c668c7 100644 --- a/src/Reflection/Internal/Visitor/ToProtobufValueTypeVisitor.php +++ b/src/Reflection/Internal/Visitor/ToProtobufValueTypeVisitor.php @@ -162,14 +162,12 @@ public function list(ListT $type): \Closure $mapValue = static fn(array $value): array => array_map($mapper, $value); } - $packed = $type->packed; - /** @phpstan-ignore return.type */ return static fn(array $value) => Protobuf\listOf( $element, /** @phpstan-ignore argument.type */ $mapValue($value), - $packed, + $type->packed, ); }