From 3f32185a2e70878aef6574ebde78ebe79fd75c6c Mon Sep 17 00:00:00 2001 From: kafkiansky Date: Tue, 29 Sep 2026 15:12:56 +0300 Subject: [PATCH] fix: decode unknown enum values instead of failing Proto3 enums are open, but PHP enums are closed: an enum number without a matching case made BackedEnum::from() throw, and the whole message failed to decode. Such numbers are now handled the way protobuf handles closed enums: a singular field (or a oneof variant) is left unset, a repeated field keeps only the known values in order, and every unknown number is kept among the message's unknown fields as a varint with the field's number. A map entry with an unknown enum value is dropped, as the map decoder already drops incomplete entries. Enum numbers are also read as int32 now: negative values, which arrive sign-extended to 64 bits, used to be cast to int directly and broke. Co-Authored-By: Claude Opus 5.5 --- src/Internal/Serde/DeserializeEnum.php | 26 ++- src/Internal/Serde/DeserializeMessage.php | 9 + src/Internal/Serde/UnknownEnumValue.php | 64 +++++++ src/Serializer.php | 10 + tests/OpenEnumTest.php | 224 ++++++++++++++++++++++ 5 files changed, 329 insertions(+), 4 deletions(-) create mode 100644 src/Internal/Serde/UnknownEnumValue.php create mode 100644 tests/OpenEnumTest.php diff --git a/src/Internal/Serde/DeserializeEnum.php b/src/Internal/Serde/DeserializeEnum.php index f765bc4..1dec734 100644 --- a/src/Internal/Serde/DeserializeEnum.php +++ b/src/Internal/Serde/DeserializeEnum.php @@ -4,12 +4,13 @@ namespace Thesis\Protobuf\Internal\Serde; +use BcMath\Number; use Thesis\Protobuf\Internal\Buffer\ReadBuffer; /** * @internal * @template T of \BackedEnum - * @template-implements DeserializeValue + * @template-implements DeserializeValue */ final readonly class DeserializeEnum implements DeserializeValue { @@ -20,11 +21,28 @@ public function __construct( private string $enum, ) {} + /** + * @return T|UnknownEnumValue + */ #[\Override] - public function deserialize(ReadBuffer $buffer): \BackedEnum + public function deserialize(ReadBuffer $buffer): \BackedEnum|UnknownEnumValue { - $num = SerdeVarint::T->deserialize($buffer); + /** @var ?Number $p31 */ + static $p31; + $p31 ??= new Number(2)->pow(31); + + /** @var ?Number $p32 */ + static $p32; + $p32 ??= new Number(2)->pow(32); + + $raw = SerdeVarint::T->deserialize($buffer); + + // Enum numbers are int32: negative ones arrive sign-extended to 64 bits. + $num = $raw->mod($p32); + if ($num->compare($p31) >= 0) { + $num -= $p32; + } - return $this->enum::from((int) $num->value); + return $this->enum::tryFrom((int) $num->value) ?? new UnknownEnumValue($raw); } } diff --git a/src/Internal/Serde/DeserializeMessage.php b/src/Internal/Serde/DeserializeMessage.php index 6703fd9..ddd17e6 100644 --- a/src/Internal/Serde/DeserializeMessage.php +++ b/src/Internal/Serde/DeserializeMessage.php @@ -47,6 +47,15 @@ public function deserialize(ReadBuffer $buffer): Message ->accept(new TypeDeserializerVisitor($tag)) ->deserialize($buffer); + [$value, $unknownEnums] = UnknownEnumValue::extract($value, $field->num); + if ($unknownEnums !== []) { + $unknowns = [...$unknowns, ...$unknownEnums]; + + if ($value === null) { + continue; + } + } + $descriptors[] = new FieldDescriptor( $field->num, new Value( diff --git a/src/Internal/Serde/UnknownEnumValue.php b/src/Internal/Serde/UnknownEnumValue.php new file mode 100644 index 0000000..ab166f1 --- /dev/null +++ b/src/Internal/Serde/UnknownEnumValue.php @@ -0,0 +1,64 @@ +} the value without unknown enum numbers + * (null for a singular unknown one) and those numbers as unknown fields + */ + public static function extract(mixed $value, int $num): array + { + if ($value instanceof self) { + return [null, [$value->toUnknownField($num)]]; + } + + if (!\is_array($value) || !array_any($value, static fn(mixed $item): bool => $item instanceof self)) { + return [$value, []]; + } + + $known = []; + $unknowns = []; + + foreach ($value as $item) { + if ($item instanceof self) { + $unknowns[] = $item->toUnknownField($num); + } else { + $known[] = $item; + } + } + + return [$known, $unknowns]; + } + + /** + * @param positive-int $num + */ + private function toUnknownField(int $num): UnknownField + { + return new UnknownField(new Tag($num, WireType::VARINT), $this->raw); + } +} diff --git a/src/Serializer.php b/src/Serializer.php index 6b0d6fb..3455027 100644 --- a/src/Serializer.php +++ b/src/Serializer.php @@ -6,6 +6,7 @@ use Thesis\Protobuf\Exception\BufferUnderflow; use Thesis\Protobuf\Internal\Buffer\ByteBuffer; +use Thesis\Protobuf\Internal\Serde\UnknownEnumValue; use Thesis\Protobuf\Internal\Wire; use Thesis\Protobuf\Type\MessageT; use Thesis\Protobuf\Type\Visitor\DetermineWireType; @@ -62,6 +63,15 @@ public function deserialize(MessageT $type, string $bytes): Message ->accept(new TypeDeserializerVisitor($tag)) ->deserialize($buffer); + [$value, $unknownEnums] = UnknownEnumValue::extract($value, $field->num); + if ($unknownEnums !== []) { + $unknowns = [...$unknowns, ...$unknownEnums]; + + if ($value === null) { + continue; + } + } + $descriptors[] = new FieldDescriptor( $field->num, new Value( diff --git a/tests/OpenEnumTest.php b/tests/OpenEnumTest.php new file mode 100644 index 0000000..40350e0 --- /dev/null +++ b/tests/OpenEnumTest.php @@ -0,0 +1,224 @@ +color); + self::assertSame([], UnknownFields::of($decoded)); + } + + public function testUnknownValueLeavesTheFieldUnsetAndKeepsTheNumber(): void + { + $decoded = self::decode(new OpenEnumTestWire(name: 'x', color: 68), OpenEnumTestMessage::class); + + self::assertSame('x', $decoded->name); + self::assertSame(OpenEnumTestColor::UNSPECIFIED, $decoded->color); + self::assertSame([[2, WireType::VARINT, '68']], self::unknowns($decoded)); + } + + public function testNegativeValues(): void + { + $known = self::decode(new OpenEnumTestWire(color: -1), OpenEnumTestMessage::class); + self::assertSame(OpenEnumTestColor::NEGATIVE, $known->color); + + $unknown = self::decode(new OpenEnumTestWire(color: -5), OpenEnumTestMessage::class); + self::assertSame(OpenEnumTestColor::UNSPECIFIED, $unknown->color); + self::assertSame([[2, WireType::VARINT, '18446744073709551611']], self::unknowns($unknown), 'The raw 64-bit varint is kept.'); + } + + public function testPackedRepeatedKeepsKnownValuesInOrder(): void + { + $decoded = self::decode(new OpenEnumTestWire(packed: [1, 68, 2, 69]), OpenEnumTestMessage::class); + + self::assertSame([OpenEnumTestColor::RED, OpenEnumTestColor::BLUE], $decoded->packed); + self::assertSame([[3, WireType::VARINT, '68'], [3, WireType::VARINT, '69']], self::unknowns($decoded)); + } + + 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); + + self::assertSame([OpenEnumTestColor::RED, OpenEnumTestColor::BLUE], $decoded->unpacked); + self::assertSame([[4, WireType::VARINT, '68'], [4, WireType::VARINT, '69']], self::unknowns($decoded)); + } + + public function testOneofWithAnUnknownEnumVariantIsUnset(): void + { + $decoded = self::decode(new OpenEnumTestWire(choice: 68), OpenEnumTestMessage::class); + + self::assertNull($decoded->choice); + self::assertSame([[5, WireType::VARINT, '68']], self::unknowns($decoded)); + } + + public function testNestedMessages(): void + { + $decoded = self::decode( + new OpenEnumTestWireParent([new OpenEnumTestWire(color: 1), new OpenEnumTestWire(color: 68)]), + OpenEnumTestParent::class, + ); + + self::assertSame( + [OpenEnumTestColor::RED, OpenEnumTestColor::UNSPECIFIED], + array_map(static fn(OpenEnumTestMessage $child): OpenEnumTestColor => $child->color, $decoded->children), + ); + self::assertSame([[], [[2, WireType::VARINT, '68']]], array_map(self::unknowns(...), $decoded->children)); + } + + public function testMapEntriesWithUnknownValuesAreDropped(): void + { + $decoded = self::decode( + new OpenEnumTestWire(map: new Map(new KVPair('known', 1), new KVPair('unknown', 68))), + OpenEnumTestMessage::class, + ); + + self::assertEquals(new Map(new KVPair('known', OpenEnumTestColor::RED)), $decoded->map); + } + + /** + * @template T of object + * @param class-string $class + * @return T + */ + private static function decode(object $wire, string $class): object + { + return self::decodeBytes(Encoder\Builder::buildDefault()->encode($wire), $class); + } + + /** + * @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); + } + + /** + * @return list + */ + private static function unknowns(object $message): array + { + return array_map( + static fn(UnknownFields\UnknownField $field): array => [$field->tag->num, $field->tag->type, (string) $field->value], + UnknownFields::of($message), + ); + } +} + +enum OpenEnumTestColor: int +{ + case NEGATIVE = -1; + case UNSPECIFIED = 0; + case RED = 1; + case BLUE = 2; +} + +/** + * The decoding side: enum fields. + */ +final readonly class OpenEnumTestMessage +{ + /** + * @param list $packed + * @param list $unpacked + * @param Map $map + */ + public function __construct( + #[Reflection\Field(1, Reflection\StringT::T)] + public string $name = '', + #[Reflection\Field(2, new Reflection\EnumT(OpenEnumTestColor::class))] + public OpenEnumTestColor $color = OpenEnumTestColor::UNSPECIFIED, + #[Reflection\Field(3, new Reflection\ListT(new Reflection\EnumT(OpenEnumTestColor::class), packed: true))] + public array $packed = [], + #[Reflection\Field(4, new Reflection\ListT(new Reflection\EnumT(OpenEnumTestColor::class), packed: false))] + public array $unpacked = [], + #[Reflection\OneOf([OpenEnumTestChoiceColor::class, OpenEnumTestChoiceName::class])] + public ?OpenEnumTestChoice $choice = null, + #[Reflection\Field(7, new Reflection\MapT(Reflection\StringT::T, new Reflection\EnumT(OpenEnumTestColor::class)))] + public Map $map = new Map(), + ) {} +} + +interface OpenEnumTestChoice {} + +final readonly class OpenEnumTestChoiceColor implements OpenEnumTestChoice +{ + public function __construct( + #[Reflection\Field(5, new Reflection\EnumT(OpenEnumTestColor::class))] + public OpenEnumTestColor $color = OpenEnumTestColor::UNSPECIFIED, + ) {} +} + +final readonly class OpenEnumTestChoiceName implements OpenEnumTestChoice +{ + public function __construct( + #[Reflection\Field(6, Reflection\StringT::T)] + public string $name = '', + ) {} +} + +/** + * The encoding side: the same field numbers as plain int32, so any number can be written. + */ +final readonly class OpenEnumTestWire +{ + /** + * @param list $packed + * @param list $unpacked + * @param Map $map + */ + public function __construct( + #[Reflection\Field(1, Reflection\StringT::T)] + public string $name = '', + #[Reflection\Field(2, Reflection\Int32T::T)] + public int $color = 0, + #[Reflection\Field(3, new Reflection\ListT(Reflection\Int32T::T, packed: true))] + public array $packed = [], + #[Reflection\Field(4, new Reflection\ListT(Reflection\Int32T::T, packed: false))] + public array $unpacked = [], + #[Reflection\Field(5, Reflection\Int32T::T)] + public int $choice = 0, + #[Reflection\Field(7, new Reflection\MapT(Reflection\StringT::T, Reflection\Int32T::T))] + public Map $map = new Map(), + ) {} +} + +final readonly class OpenEnumTestParent +{ + /** + * @param list $children + */ + public function __construct( + #[Reflection\Field(1, new Reflection\ListT(new Reflection\ObjectT(OpenEnumTestMessage::class)))] + public array $children = [], + ) {} +} + +final readonly class OpenEnumTestWireParent +{ + /** + * @param list $children + */ + public function __construct( + #[Reflection\Field(1, new Reflection\ListT(new Reflection\ObjectT(OpenEnumTestWire::class)))] + public array $children = [], + ) {} +}