diff --git a/README.md b/README.md index acd0c28..0d77863 100644 --- a/README.md +++ b/README.md @@ -90,6 +90,40 @@ $request = $decoder->decode(/** protobuf buffer here */, CreateUserRequest::clas echo $request->name; ``` +### Required fields + +`Reflector::map()` maps missing fields using property defaults when they exist. +If a property is non-nullable and has no default value, it is treated as required. + +When one or more required properties are missing, decoding fails with `Thesis\Protobuf\Reflection\Exception\MappingError`. +The exception contains all reasons in `->reasons` (each reason is typically `PropertyRequired`). + +This behavior is aligned with other protobuf implementations/plugins: messages missing required fields are treated as invalid during decode. + +A field is considered required when either: +- it is defined as `required` in `proto2`; +- it is defined in editions with `features.field_presence = LEGACY_REQUIRED`. + +```php +use Thesis\Protobuf\Decoder; +use Thesis\Protobuf\Reflection; + +$decoder = Decoder\Builder::buildDefault(); + +try { + $message = $decoder->decode($buffer, CreateUserRequest::class); +} catch (Reflection\Exception\MappingError $e) { + foreach ($e->reasons as $reason) { + if ($reason instanceof Reflection\Exception\PropertyRequired) { + echo $reason->class . "::$" . $reason->property . PHP_EOL; + } + } +} +``` + +Both `Encoder::encode()` and `Decoder::decode()` throw `Thesis\Protobuf\ProtobufException`. +Domain protobuf exceptions are preserved and are not wrapped into generic runtime errors. + ### Unknown fields When a protobuf message is decoded, it may contain fields that are not defined in the target class. diff --git a/src/Decoder.php b/src/Decoder.php index 2ba269e..f14aee4 100644 --- a/src/Decoder.php +++ b/src/Decoder.php @@ -4,8 +4,6 @@ namespace Thesis\Protobuf; -use Thesis\Protobuf\Decoder\DecodingError; - /** * @api */ @@ -15,7 +13,7 @@ interface Decoder * @template T of object * @param class-string $classType * @return T - * @throws DecodingError + * @throws ProtobufException */ public function decode(string $buffer, string $classType): object; } diff --git a/src/Decoder/Internal/ReflectionDecoder.php b/src/Decoder/Internal/ReflectionDecoder.php index 0348353..f952bb9 100644 --- a/src/Decoder/Internal/ReflectionDecoder.php +++ b/src/Decoder/Internal/ReflectionDecoder.php @@ -5,6 +5,7 @@ namespace Thesis\Protobuf\Decoder\Internal; use Thesis\Protobuf\Decoder; +use Thesis\Protobuf\ProtobufException; use Thesis\Protobuf\Reflection\Reflector; use Thesis\Protobuf\Serializer; @@ -29,6 +30,8 @@ public function decode(string $buffer, string $classType): object ), $classType, ); + } catch (ProtobufException $e) { + throw $e; } catch (\Throwable $e) { throw new Decoder\DecodingError($e->getMessage(), (int) $e->getCode(), $e); } diff --git a/src/Encoder.php b/src/Encoder.php index b799921..4a45fd5 100644 --- a/src/Encoder.php +++ b/src/Encoder.php @@ -4,15 +4,13 @@ namespace Thesis\Protobuf; -use Thesis\Protobuf\Encoder\EncodingError; - /** * @api */ interface Encoder { /** - * @throws EncodingError + * @throws ProtobufException */ public function encode(object $message): string; } diff --git a/src/Encoder/Internal/ReflectionEncoder.php b/src/Encoder/Internal/ReflectionEncoder.php index 878744d..60d559d 100644 --- a/src/Encoder/Internal/ReflectionEncoder.php +++ b/src/Encoder/Internal/ReflectionEncoder.php @@ -5,6 +5,7 @@ namespace Thesis\Protobuf\Encoder\Internal; use Thesis\Protobuf\Encoder; +use Thesis\Protobuf\ProtobufException; use Thesis\Protobuf\Reflection\Reflector; use Thesis\Protobuf\Serializer; @@ -25,6 +26,8 @@ public function encode(object $message): string return $this->serializer->serialize( $this->reflector->message($message), ); + } catch (ProtobufException $e) { + throw $e; } catch (\Throwable $e) { throw new Encoder\EncodingError($e->getMessage(), (int) $e->getCode(), $e); } diff --git a/src/Reflection/Exception/MappingError.php b/src/Reflection/Exception/MappingError.php new file mode 100644 index 0000000..972df55 --- /dev/null +++ b/src/Reflection/Exception/MappingError.php @@ -0,0 +1,29 @@ + $reasons + */ + public function __construct( + public readonly array $reasons, + ) { + parent::__construct(\sprintf( + 'Multiple exceptions encountered (%d): %s', + \count($reasons), + implode('', array_map( + static fn(ReflectionException $reason) => \sprintf("\n\n%s: %s", $reason::class, $reason->getMessage()), + $this->reasons, + )), + )); + } +} diff --git a/src/Reflection/Exception/PropertyRequired.php b/src/Reflection/Exception/PropertyRequired.php new file mode 100644 index 0000000..d1bf41a --- /dev/null +++ b/src/Reflection/Exception/PropertyRequired.php @@ -0,0 +1,24 @@ + - */ -final class ToDefaultValueTypeVisitor extends DefaultTypeVisitor -{ - #[\Override] - public function bool(BoolT $type): mixed - { - return false; - } - - #[\Override] - public function float(FloatT $type): mixed - { - return 0; - } - - #[\Override] - public function double(DoubleT $type): mixed - { - return 0; - } - - #[\Override] - public function fixed32(Fixed32T $type): mixed - { - return 0; - } - - #[\Override] - public function sfixed32(SFixed32T $type): mixed - { - return 0; - } - - #[\Override] - public function string(StringT $type): mixed - { - return ''; - } - - #[\Override] - public function bytes(BytesT $type): mixed - { - return ''; - } - - #[\Override] - public function list(ListT $type): mixed - { - return []; - } - - #[\Override] - public function map(MapT $type): mixed - { - return new Map(); - } - - #[\Override] - public function enum(EnumT $type): mixed - { - $cases = $type->enum::cases(); - - foreach ($cases as $case) { - if ($case->value === 0) { - return $case; - } - } - - return null; - } - - #[\Override] - protected function default(Type $type): null - { - return null; - } -} diff --git a/src/Reflection/ReflectionException.php b/src/Reflection/ReflectionException.php index 2abaf28..5db431e 100644 --- a/src/Reflection/ReflectionException.php +++ b/src/Reflection/ReflectionException.php @@ -4,7 +4,9 @@ namespace Thesis\Protobuf\Reflection; +use Thesis\Protobuf\ProtobufException; + /** * @api */ -abstract class ReflectionException extends \Exception {} +abstract class ReflectionException extends ProtobufException {} diff --git a/src/Reflection/Reflector.php b/src/Reflection/Reflector.php index 9679ad2..5e8d8da 100644 --- a/src/Reflection/Reflector.php +++ b/src/Reflection/Reflector.php @@ -8,12 +8,10 @@ use Thesis\Protobuf; use Thesis\Protobuf\Message; use Thesis\Protobuf\Reflection\Internal\Api\ClassReflector; -use Thesis\Protobuf\Reflection\Internal\Api\PropertyReflection; use Thesis\Protobuf\Reflection\Internal\Cache\Cache; use Thesis\Protobuf\Reflection\Internal\Cache\InMemoryPsr16Cache; use Thesis\Protobuf\Reflection\Internal\Visitor\IsValueEmpty; use Thesis\Protobuf\Reflection\Internal\Visitor\RecursionBreakTypeVisitor; -use Thesis\Protobuf\Reflection\Internal\Visitor\ToDefaultValueTypeVisitor; use Thesis\Protobuf\Reflection\Internal\Visitor\ToProtobufTypeTypeVisitor; use Thesis\Protobuf\Reflection\Internal\Visitor\ToProtobufValueTypeVisitor; use Thesis\Protobuf\Reflection\Internal\Visitor\ToValueTypeVisitor; @@ -30,8 +28,6 @@ final class Reflector /** @var ToProtobufValueTypeVisitor<*> */ private readonly ToProtobufValueTypeVisitor $valueVisitor; - private readonly ToDefaultValueTypeVisitor $defaultValueTypeVisitor; - private readonly ClassReflector $classReflector; /** @var array */ @@ -55,7 +51,6 @@ private function __construct( ) { $this->typeVisitor = new ToProtobufTypeTypeVisitor($this); $this->valueVisitor = new ToProtobufValueTypeVisitor($this); - $this->defaultValueTypeVisitor = new ToDefaultValueTypeVisitor(); $this->classReflector = new ClassReflector(); } @@ -123,6 +118,8 @@ public function map(Message $message, string $class): object $classReflection = $this->classReflector->reflect($class); $object = $classReflection->class->newInstanceWithoutConstructor(); + $exceptions = []; + foreach ($classReflection->properties as $property) { $propertyType = $property->reflection->getType(); \assert($propertyType !== null); @@ -130,18 +127,27 @@ public function map(Message $message, string $class): object if ($property->attributes->has(Field::class)) { $field = $property->attributes->get(Field::class); - $descriptor = $message->fields[$field->num] ?? null; + if (isset($message->fields[$field->num])) { + $value = $field + ->type + ->accept(new ToValueTypeVisitor( + $this, + $message->fields[$field->num]->value->value, + )); + } elseif ($property->default !== null) { + $value = $property->default->value; + } else { + $exceptions[] = new Exception\PropertyRequired( + $property->reflection->getDeclaringClass()->getName(), + $property->reflection->getName(), + ); + + continue; + } + $property->reflection->setValue( $object, - match (true) { - $descriptor !== null => $field - ->type - ->accept(new ToValueTypeVisitor( - $this, - $descriptor->value->value, - )), - default => $this->defaultValuePropertyValue($property, $field), - }, + $value, ); } elseif ($property->attributes->has(OneOf::class)) { $oneof = $property->attributes->get(OneOf::class); @@ -163,11 +169,15 @@ public function map(Message $message, string $class): object $property->reflection->setValue( $object, - $this->defaultValuePropertyValue($property), + null, ); } } + if ($exceptions !== []) { + throw new Exception\MappingError($exceptions); + } + if ($message->unknowns !== []) { $this->unknowns?->handle($object, $message->unknowns); } @@ -220,19 +230,4 @@ private function doGetMessage(object $message, ?\Closure $presence = null): Mess return Protobuf\message(...$descriptors); } - - /** - * @throws Exception\PropertyUninitialized - */ - private function defaultValuePropertyValue(PropertyReflection $property, ?Field $field = null): mixed - { - if ($property->default !== null) { - return $property->default->value; - } - - return $field?->type->accept($this->defaultValueTypeVisitor) ?? throw new Exception\PropertyUninitialized( - $property->reflection->getDeclaringClass()->getName(), - $property->reflection->getName(), - ); - } } diff --git a/tests/Decoder/Internal/ReflectionDecoderTest.php b/tests/Decoder/Internal/ReflectionDecoderTest.php index 1c8e058..516f5fa 100644 --- a/tests/Decoder/Internal/ReflectionDecoderTest.php +++ b/tests/Decoder/Internal/ReflectionDecoderTest.php @@ -20,6 +20,18 @@ public function testDecode(): void self::assertIsString($buffer); self::assertEquals(new Request('John Doe', 50), $decoder->decode($buffer, Request::class)); } + + public function testRequiredPropertiesCannotBeInitializedImplicitly(): void + { + $decoder = Builder::buildDefault(); + + $this->expectExceptionObject(new Reflection\Exception\MappingError([ + new Reflection\Exception\PropertyRequired(Request::class, 'name'), + new Reflection\Exception\PropertyRequired(Request::class, 'id'), + ])); + + $decoder->decode('', Request::class); + } } final readonly class Request diff --git a/tests/testdata/reflection/oneof.php b/tests/testdata/reflection/oneof.php index 9b7e259..ed73b06 100644 --- a/tests/testdata/reflection/oneof.php +++ b/tests/testdata/reflection/oneof.php @@ -10,7 +10,7 @@ interface DeveloperContact {} { public function __construct( #[Reflection\Field(3, Reflection\StringT::T)] - public string $email, + public string $email = '', ) {} } @@ -18,7 +18,7 @@ public function __construct( { public function __construct( #[Reflection\Field(4, Reflection\StringT::T)] - public string $phone, + public string $phone = '', ) {} } @@ -26,7 +26,7 @@ public function __construct( { public function __construct( #[Reflection\Field(5, new Reflection\ObjectT(Chat::class))] - public Chat $chat, + public ?Chat $chat = null, ) {} } @@ -34,26 +34,27 @@ public function __construct( { public function __construct( #[Reflection\Field(1, Reflection\Int32T::T)] - public int $id, + public int $id = 0, #[Reflection\Field(2, Reflection\StringT::T)] - public string $username, + public string $username = '', ) {} } enum Role: int { - case ROLE_MEMBER = 0; - case ROLE_MAINTAINER = 1; - case ROLE_OWNER = 2; + case ROLE_UNSPECIFIED = 0; + case ROLE_MEMBER = 1; + case ROLE_MAINTAINER = 2; + case ROLE_OWNER = 3; } final readonly class Organization { public function __construct( #[Reflection\Field(1, Reflection\StringT::T)] - public string $name, + public string $name = '', #[Reflection\Field(2, new Reflection\EnumT(Role::class))] - public Role $role, + public Role $role = Role::ROLE_UNSPECIFIED, ) {} } @@ -64,22 +65,22 @@ public function __construct( */ public function __construct( #[Reflection\Field(1, Reflection\StringT::T)] - public string $name, + public string $name = '', #[Reflection\Field( 2, new Reflection\ListT( new Reflection\ObjectT(Organization::class), ), )] - public array $organizations, + public array $organizations = [], #[Reflection\Field(6, Reflection\StringT::T)] - public string $url, + public string $url = '', #[Reflection\OneOf([ EmailContact::class, PhoneContact::class, TelegramContact::class, ])] - public DeveloperContact $contact, + public ?DeveloperContact $contact = null, ) {} } diff --git a/tests/testdata/reflection/oneof.proto b/tests/testdata/reflection/oneof.proto index 41b53ca..4e52574 100644 --- a/tests/testdata/reflection/oneof.proto +++ b/tests/testdata/reflection/oneof.proto @@ -11,9 +11,10 @@ message Organization { string name = 1; enum Role { - ROLE_MEMBER = 0; - ROLE_MAINTAINER = 1; - ROLE_OWNER = 2; + ROLE_UNSPECIFIED = 0; + ROLE_MEMBER = 1; + ROLE_MAINTAINER = 2; + ROLE_OWNER = 3; }; Role role = 2; }