Copilot commented on code in PR #3708: URL: https://github.com/apache/avro/pull/3708#discussion_r3403820642
########## lang/php/lib/Console/GenerateCommand.php: ########## @@ -0,0 +1,149 @@ +<?php + +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +declare(strict_types=1); + +namespace Apache\Avro\Console; + +use Apache\Avro\Generator\AvroCodeGenerator; +use Apache\Avro\Schema\AvroSchema; +use Symfony\Component\Console\Attribute\AsCommand; +use Symfony\Component\Console\Command\Command; +use Symfony\Component\Console\Input\InputInterface; +use Symfony\Component\Console\Input\InputOption; +use Symfony\Component\Console\Output\OutputInterface; +use Symfony\Component\Console\Style\SymfonyStyle; + +#[AsCommand( + name: 'generate', + description: 'Generate PHP classes from Avro schema files', +)] +class GenerateCommand extends Command +{ + protected function configure(): void + { + $this + ->addOption('file', 'f', InputOption::VALUE_OPTIONAL, 'One Avro schema file (.avsc)') + ->addOption('directory', 'd', InputOption::VALUE_OPTIONAL, 'A directory containing multiple Avro schema files (.avsc)') + ->addOption('output', 'o', InputOption::VALUE_REQUIRED, 'Output directory for generated PHP files') + ->addOption('namespace', 'ns', InputOption::VALUE_REQUIRED, 'PHP namespace for generated classes'); + } + + protected function execute(InputInterface $input, OutputInterface $output): int + { + $io = new SymfonyStyle($input, $output); + + /** @var string $outputDir */ + $outputDir = $input->getOption('output'); + /** @var string $namespace */ + $namespace = $input->getOption('namespace'); + + /** @var null|string $file */ + $file = $input->getOption('file'); + /** @var null|string $file */ + $directory = $input->getOption('directory'); Review Comment: The phpdoc for `$directory` is copy/pasted from `$file` (it currently says `@var ... $file`). This makes the local variable docs misleading for static analysis and future maintenance. ########## lang/php/lib/Console/GenerateCommand.php: ########## @@ -0,0 +1,149 @@ +<?php + +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +declare(strict_types=1); + +namespace Apache\Avro\Console; + +use Apache\Avro\Generator\AvroCodeGenerator; +use Apache\Avro\Schema\AvroSchema; +use Symfony\Component\Console\Attribute\AsCommand; +use Symfony\Component\Console\Command\Command; +use Symfony\Component\Console\Input\InputInterface; +use Symfony\Component\Console\Input\InputOption; +use Symfony\Component\Console\Output\OutputInterface; +use Symfony\Component\Console\Style\SymfonyStyle; + +#[AsCommand( + name: 'generate', + description: 'Generate PHP classes from Avro schema files', +)] +class GenerateCommand extends Command +{ + protected function configure(): void + { + $this + ->addOption('file', 'f', InputOption::VALUE_OPTIONAL, 'One Avro schema file (.avsc)') + ->addOption('directory', 'd', InputOption::VALUE_OPTIONAL, 'A directory containing multiple Avro schema files (.avsc)') + ->addOption('output', 'o', InputOption::VALUE_REQUIRED, 'Output directory for generated PHP files') + ->addOption('namespace', 'ns', InputOption::VALUE_REQUIRED, 'PHP namespace for generated classes'); + } + + protected function execute(InputInterface $input, OutputInterface $output): int + { + $io = new SymfonyStyle($input, $output); + + /** @var string $outputDir */ + $outputDir = $input->getOption('output'); + /** @var string $namespace */ + $namespace = $input->getOption('namespace'); + + /** @var null|string $file */ + $file = $input->getOption('file'); + /** @var null|string $file */ + $directory = $input->getOption('directory'); + + if ( + (null === $file && null === $directory) + || (null !== $file && null !== $directory) + ) { + $io->error('You must provide a file path or a directory'); + + return Command::FAILURE; + } + + if (null === $outputDir || '' === $outputDir) { + $io->error('Output directory is required (--output / -o).'); + + return Command::FAILURE; + } + + if (null === $namespace || '' === $namespace) { + $io->error('PHP namespace is required (--namespace / -n).'); + + return Command::FAILURE; + } Review Comment: The error message references the wrong short option (`-n`), but the command defines the short option as `-ns`. This is user-facing and can mislead users troubleshooting CLI usage. ########## lang/php/lib/Datum/AvroSpecificDatumWriter.php: ########## @@ -0,0 +1,236 @@ +<?php + +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +declare(strict_types=1); + +namespace Apache\Avro\Datum; + +use Apache\Avro\AvroException; +use Apache\Avro\Schema\AvroArraySchema; +use Apache\Avro\Schema\AvroEnumSchema; +use Apache\Avro\Schema\AvroMapSchema; +use Apache\Avro\Schema\AvroPrimitiveSchema; +use Apache\Avro\Schema\AvroRecordSchema; +use Apache\Avro\Schema\AvroSchema; +use Apache\Avro\Schema\AvroUnionSchema; + +/** + * Writes data from code-generated (specific) record classes to the encoder. + * + * It knows how to extract field values from generated PHP class instances (via getter methods) and backed enum + * instances, encoding them according to the Avro schema. + * + * Generated records expose each field through a getter method whose name matches the Avro field name. + * Generated enums are PHP 8.1+ backed string enums whose ->value holds the Avro symbol string. + * + * Usage: + * $schema = AvroSchema::parse($json); + * $writer = new AvroSpecificDatumWriter($schema); + * $io = new \Apache\Avro\IO\AvroStringIO(); + * $encoder = new AvroIOBinaryEncoder($io); + * $writer->write($myGeneratedObject, $encoder); + * $bytes = $io->string(); + */ +class AvroSpecificDatumWriter +{ + public function __construct( + private readonly AvroSchema $writersSchema + ) { + } + + /** + * Serializes the given datum (a generated record instance) to the encoder. + * + * @throws AvroException + */ + public function write(object $datum, AvroIOBinaryEncoder $encoder): void + { + $this->writeData($this->writersSchema, $datum, $encoder); + } + + /** + * @throws AvroException + */ + private function writeData(AvroSchema $schema, mixed $datum, AvroIOBinaryEncoder $encoder): void + { + match (true) { + $schema instanceof AvroRecordSchema => $this->writeRecord($schema, $datum, $encoder), + $schema instanceof AvroEnumSchema => $this->writeEnum($schema, $datum, $encoder), + $schema instanceof AvroArraySchema => $this->writeArray($schema, $datum, $encoder), + $schema instanceof AvroMapSchema => $this->writeMap($schema, $datum, $encoder), + $schema instanceof AvroUnionSchema => $this->writeUnion($schema, $datum, $encoder), + $schema instanceof AvroPrimitiveSchema => $this->writePrimitive($schema, $datum, $encoder), + default => throw new AvroException(sprintf('Unsupported schema type: %s', $schema->type())), + }; + } + + /** + * Writes a record by calling the getter for each field defined in the schema. + * + * @throws AvroException + */ + private function writeRecord(AvroRecordSchema $schema, object $datum, AvroIOBinaryEncoder $encoder): void + { + foreach ($schema->fields() as $field) { + $value = $datum->{$field->name()}(); + $this->writeData($field->type(), $value, $encoder); + } + } + + /** + * Writes a backed enum value by looking up its symbol index. + * + * @throws AvroException + */ + private function writeEnum(AvroEnumSchema $schema, \BackedEnum $datum, AvroIOBinaryEncoder $encoder): void + { + $symbolIndex = $schema->symbolIndex($datum->value); + $encoder->writeInt($symbolIndex); + } Review Comment: `writeData()` dispatches to `writeEnum()` whenever the schema is an enum, but `writeEnum()` currently type-hints `\BackedEnum`. If a non-enum value is passed, PHP will throw a `TypeError` instead of an Avro-specific exception. Make this method accept `mixed` and validate the datum, throwing `AvroIOTypeException` for mismatches. ########## lang/php/lib/Generator/AvroCodeGenerator.php: ########## @@ -0,0 +1,365 @@ +<?php + +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +declare(strict_types=1); + +namespace Apache\Avro\Generator; + +use Apache\Avro\Schema\AvroArraySchema; +use Apache\Avro\Schema\AvroEnumSchema; +use Apache\Avro\Schema\AvroMapSchema; +use Apache\Avro\Schema\AvroPrimitiveSchema; +use Apache\Avro\Schema\AvroRecordSchema; +use Apache\Avro\Schema\AvroSchema; +use Apache\Avro\Schema\AvroUnionSchema; +use PhpParser\BuilderFactory; +use PhpParser\Node; +use PhpParser\Node\Scalar\String_; +use PhpParser\Node\Stmt; +use PhpParser\PrettyPrinter\Standard; + +class AvroCodeGenerator +{ + private BuilderFactory $factory; + private Standard $printer; + + /** @var array<string, AvroSchema> */ + private array $registry = []; + + public function __construct() + { + $this->factory = new BuilderFactory(); + $this->printer = new Standard(['shortArraySyntax' => true]); + } + + /** + * @return array<string, string> Map of filename to file contents + */ + public function translate( + AvroSchema $schema, + string $path, + string $phpNamespace + ): array { + $this->buildRegistry($schema); + + $files = []; + + foreach ($this->registry as $name => $registeredSchema) { + $node = match (true) { + $registeredSchema instanceof AvroEnumSchema => $this->buildEnum( + $registeredSchema, + $phpNamespace, + $registeredSchema->symbols() + ), + $registeredSchema instanceof AvroRecordSchema => $this->buildRecord( + $registeredSchema, + $phpNamespace + ), + default => null + }; + + if (null !== $node) { + $filename = $path.'/'.ucwords($name).'.php'; + $files[$filename] = "<?php\n\ndeclare(strict_types=1);\n\n{$this->printer->prettyPrint([$node])}\n"; + } + } Review Comment: The registry key is the schema fullname (e.g. `com.example.User`). Using that key to build the output filename produces names like `Com.example.User.php`, which won't match the generated class name (`User`) and breaks typical autoloading expectations. Use the schema's short name for the filename (or otherwise normalize fullname consistently). ########## lang/php/lib/Console/GenerateCommand.php: ########## @@ -0,0 +1,149 @@ +<?php + +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +declare(strict_types=1); + +namespace Apache\Avro\Console; + +use Apache\Avro\Generator\AvroCodeGenerator; +use Apache\Avro\Schema\AvroSchema; +use Symfony\Component\Console\Attribute\AsCommand; +use Symfony\Component\Console\Command\Command; +use Symfony\Component\Console\Input\InputInterface; +use Symfony\Component\Console\Input\InputOption; +use Symfony\Component\Console\Output\OutputInterface; +use Symfony\Component\Console\Style\SymfonyStyle; + +#[AsCommand( + name: 'generate', + description: 'Generate PHP classes from Avro schema files', +)] +class GenerateCommand extends Command +{ + protected function configure(): void + { + $this + ->addOption('file', 'f', InputOption::VALUE_OPTIONAL, 'One Avro schema file (.avsc)') + ->addOption('directory', 'd', InputOption::VALUE_OPTIONAL, 'A directory containing multiple Avro schema files (.avsc)') + ->addOption('output', 'o', InputOption::VALUE_REQUIRED, 'Output directory for generated PHP files') + ->addOption('namespace', 'ns', InputOption::VALUE_REQUIRED, 'PHP namespace for generated classes'); + } + + protected function execute(InputInterface $input, OutputInterface $output): int + { + $io = new SymfonyStyle($input, $output); + + /** @var string $outputDir */ + $outputDir = $input->getOption('output'); + /** @var string $namespace */ + $namespace = $input->getOption('namespace'); + + /** @var null|string $file */ + $file = $input->getOption('file'); + /** @var null|string $file */ + $directory = $input->getOption('directory'); + + if ( + (null === $file && null === $directory) + || (null !== $file && null !== $directory) + ) { + $io->error('You must provide a file path or a directory'); + + return Command::FAILURE; + } + + if (null === $outputDir || '' === $outputDir) { + $io->error('Output directory is required (--output / -o).'); + + return Command::FAILURE; + } + + if (null === $namespace || '' === $namespace) { + $io->error('PHP namespace is required (--namespace / -n).'); + + return Command::FAILURE; + } + + if (!is_dir($outputDir) && !mkdir($outputDir, 0755, true) && !is_dir($outputDir)) { + $io->error(sprintf('Could not create output directory "%s".', $outputDir)); + + return Command::FAILURE; + } + + $outputDir = rtrim((string) realpath($outputDir), '/'); + $files = []; + if (null !== $file) { + $files[] = $file; + } elseif (null !== $directory) { + if (!is_dir($directory)) { + $io->error(sprintf('Directory not found: %s', $directory)); + + return Command::FAILURE; + } + $files = glob(rtrim($directory, '/').'/*.avsc'); + } Review Comment: `glob()` can return `false` on error; iterating over `$files` later will then raise a warning in PHP 8+. Coerce the result to an array to keep behavior predictable. ########## lang/php/lib/Datum/AvroSpecificDatumWriter.php: ########## @@ -0,0 +1,236 @@ +<?php + +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +declare(strict_types=1); + +namespace Apache\Avro\Datum; + +use Apache\Avro\AvroException; +use Apache\Avro\Schema\AvroArraySchema; +use Apache\Avro\Schema\AvroEnumSchema; +use Apache\Avro\Schema\AvroMapSchema; +use Apache\Avro\Schema\AvroPrimitiveSchema; +use Apache\Avro\Schema\AvroRecordSchema; +use Apache\Avro\Schema\AvroSchema; +use Apache\Avro\Schema\AvroUnionSchema; + +/** + * Writes data from code-generated (specific) record classes to the encoder. + * + * It knows how to extract field values from generated PHP class instances (via getter methods) and backed enum + * instances, encoding them according to the Avro schema. + * + * Generated records expose each field through a getter method whose name matches the Avro field name. + * Generated enums are PHP 8.1+ backed string enums whose ->value holds the Avro symbol string. + * + * Usage: + * $schema = AvroSchema::parse($json); + * $writer = new AvroSpecificDatumWriter($schema); + * $io = new \Apache\Avro\IO\AvroStringIO(); + * $encoder = new AvroIOBinaryEncoder($io); + * $writer->write($myGeneratedObject, $encoder); + * $bytes = $io->string(); + */ +class AvroSpecificDatumWriter +{ + public function __construct( + private readonly AvroSchema $writersSchema + ) { + } + + /** + * Serializes the given datum (a generated record instance) to the encoder. + * + * @throws AvroException + */ + public function write(object $datum, AvroIOBinaryEncoder $encoder): void + { + $this->writeData($this->writersSchema, $datum, $encoder); + } + + /** + * @throws AvroException + */ + private function writeData(AvroSchema $schema, mixed $datum, AvroIOBinaryEncoder $encoder): void + { + match (true) { + $schema instanceof AvroRecordSchema => $this->writeRecord($schema, $datum, $encoder), + $schema instanceof AvroEnumSchema => $this->writeEnum($schema, $datum, $encoder), + $schema instanceof AvroArraySchema => $this->writeArray($schema, $datum, $encoder), + $schema instanceof AvroMapSchema => $this->writeMap($schema, $datum, $encoder), + $schema instanceof AvroUnionSchema => $this->writeUnion($schema, $datum, $encoder), + $schema instanceof AvroPrimitiveSchema => $this->writePrimitive($schema, $datum, $encoder), + default => throw new AvroException(sprintf('Unsupported schema type: %s', $schema->type())), + }; + } + + /** + * Writes a record by calling the getter for each field defined in the schema. + * + * @throws AvroException + */ + private function writeRecord(AvroRecordSchema $schema, object $datum, AvroIOBinaryEncoder $encoder): void + { + foreach ($schema->fields() as $field) { + $value = $datum->{$field->name()}(); + $this->writeData($field->type(), $value, $encoder); + } Review Comment: Calling the getter dynamically (`$datum->{$field->name()}()`) will throw a fatal `Error` if the method doesn't exist, which bypasses the method's `@throws AvroException` contract and makes failures harder to diagnose. Validate the getter exists and throw `AvroIOTypeException` (or `AvroException`) instead. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
