diff --git a/src/Reflection/Internal/Visitor/ToProtobufValueTypeVisitor.php b/src/Reflection/Internal/Visitor/ToProtobufValueTypeVisitor.php index 8f93481..6c668c7 100644 --- a/src/Reflection/Internal/Visitor/ToProtobufValueTypeVisitor.php +++ b/src/Reflection/Internal/Visitor/ToProtobufValueTypeVisitor.php @@ -167,6 +167,7 @@ public function list(ListT $type): \Closure $element, /** @phpstan-ignore argument.type */ $mapValue($value), + $type->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 = [], + ) {} +}