Please allow types re-definition (custom mapping)

[Hubbitus]

Now most interesting for me to define optional (nullable fields), so, instead of just:

"type": "string",

Allow to set something like:

"type" : [ "null", "string" ],
"default": null

But there are also more interesting cases when e.g. instead of just int I would like to use logicalType date.

Please allow providing some functional interface to override fixed mechanism by providing an array of mappers, or just callable.

[nick-zh]

Thanks for the input, I'll look into it ✌️
Do you need this in the merger or generator context?

[nick-zh]

So the generator is still a bit experimental, so i think the following things are missing right now (you pointed out most of them):

• union types
• defaults
• logical type
• doc comment

IMHO it would make sense to have doc comments to be able to define them,
what do you think? I would propose:

• @avro-type (to not calculate the type but force one)
• @avro-default
• @avro-logical-type
• @avro-doc

In terms of types, i think unions are not properly supported yet, so i will enhance that.
I think i will do it with #26, since it will make some of these things easier

Additionally, i agree that it would be nice if one could use an own Parser that complies with a newly introduced ParserInterface

[Hubbitus]

Do you need this in the merger or generator context?

I primarily use generators.

And honestly, I need generation schema by a particular object, not for all classes.

[Hubbitus]

IMHO it would make sense to have doc comments to be able to define them,
what do you think? I would propose:

Good proposal. That may be useful. Meantime my intention was to provide some schema generation configuration, instead of changing classes.

In my use case, I need to generate AVRO schema for PHP generated classes (PIMCore). So, generally, I have no control over classes themselves but want to change generated schema. E.g. take PHP doc field comment into AVRO doc.

[nick-zh]

Thx for the insight, i will factor this in ✌️

[nick-zh]

So i've created a pre-release for this new functionality. I still need to do some more testing, but if you want you can already check it out.
Basically you are able to write your own ClassPropertyParserInterface which i think is what you want.
Additionally you need your own AppContainer and console file, so you can register your own property parser.
If you do it directly in PHP, you can check the readme or the example folder ✌️

[Hubbitus]

@nick-zh, thank you very much! It looks very promising!

[Hubbitus]

The concept itself looks good. Thank you!
But could you please make some enhancements:

1. In ClassPropertyParser::parseProperty($property) provide some context. E.g. class file, or just pass the reference to ClassParser caller?
2. Allow skipping such property in the scheme (some hidden, implementation details, transient). For example by throwing something like SkipPropertyException, or simply return null or false?

[Hubbitus]

3. Why PhpClassPropertyInterface::NO_DEFAULT = 'there-was-no-default-set' has so strange string value, used then in PhpClassProperty constructor by default?

I would like to suggest making the default value null by default.

[Hubbitus]

4. Why PhpClassConverter::getConvertedProperties silently skips properties which type is not string??
   What if I already return there array like ['null', 'string'] from an one of the ClassPropertyParser::parseProperty($property) descendant implementation?
   Instead of skipping I would like to just return it as is. What you think?

[Hubbitus]

5. PhpClassConverter is one or the converter implementation and intended to be sub-classed, is not? Why most methods has private modifier and not at least protected?

[Hubbitus]

6. PhpClassConverter::getConvertedUnionType logic looks strange:

        if (0 !== count($convertedUnionType) && [] !== $arrayType) {
            $convertedUnionType[] = $arrayType;
        } elseif (0 === count($convertedUnionType) && [] !== $arrayType) {
            return $arrayType;
        }

Why if $arrayType is empty array we return it? Looks like a mistake. And all else branch must be ommited.

[Hubbitus]

7. You hardcode types to skip in PhpClassConverter::typesToSkip and include there also type null. But allow provide null as default value. That will lead to generating in AVRO scheme fields like:

{"name":"fullName","type":["string"],"default":null}

said by @var null|string or ?string.
But that is totally incorrect by AVRO specification, citing:

Union types are denoted as union { typeA, typeB, typeC, ... }. For example, this record contains a string field that is optional (unioned with null).

Also there in spec note what if there present default value, that must match the first type:

(Note that when a default value is specified for a record field whose type is a union, the type of the default value must match the first element of the union. Thus, for unions containing "null", the "null" is usually listed first, since the default value of such unions is typically null.)

So, that is logically incorrect to exclude null-type as that is legitime in AVRO.

I would like to propose:

1. Exclude null type from skipped. Or at least allow overriding that array (e.g. parameter in constructor and/or setter)
2. On generation phase check that on manner:

diff --git a/src/Generator/SchemaGenerator.php b/src/Generator/SchemaGenerator.php
index 9e99bc6..81fbaee 100644
--- a/src/Generator/SchemaGenerator.php
+++ b/src/Generator/SchemaGenerator.php
@@ -5,6 +5,7 @@ declare(strict_types=1);
 namespace PhpKafka\PhpAvroSchemaGenerator\Generator;
 
 use PhpKafka\PhpAvroSchemaGenerator\Avro\Avro;
+use PhpKafka\PhpAvroSchemaGenerator\Exception\SchemaGeneratorException;
 use PhpKafka\PhpAvroSchemaGenerator\PhpClass\PhpClassInterface;
 use PhpKafka\PhpAvroSchemaGenerator\PhpClass\PhpClassPropertyInterface;
 use PhpKafka\PhpAvroSchemaGenerator\Registry\ClassRegistryInterface;
@@ -106,6 +107,9 @@ final class SchemaGenerator implements SchemaGeneratorInterface
 
         if (PhpClassPropertyInterface::NO_DEFAULT !== $property->getPropertyDefault()) {
             $field['default'] = $property->getPropertyDefault();
+            if (null === $field['default'] && (!is_array($field['type']) or empty($field['type']) or 'null' != @$field['type'][0])){
+                throw new SchemaGeneratorException('Provided default value "null", but that type is not provided as first possible in union (see https://avro.apache.org/docs/current/spec.html#Unions)!');
+            }
         }
 
         if (null !== $property->getPropertyDoc() && '' !== $property->getPropertyDoc()) {

[nick-zh]

Hey @Hubbitus
Thanks for your answers and requests, i'll try to answer:

1. This was left open on purpose and only my implementation checks for PhpParser\Node\Stmt\Property so if you want to create your own class and stick to the interface, you are not forced to use nikic/php-parser like i do in my implementation
2. I am not sure if we need to add this in my base implementation, since you can also easily create your own ClassParser that can skip properties. Could you elaborate maybe more what kind of properties and why you would want to skip?
3. The problem is also a bit with Improve @avro-default annotation #43 it is hard to represent an empty string in annotations and since i also need to support null itself at somepoint, that probably won't do
4. Due to the fact that i am offering interfaces to give the user to override certain parts i also need some safeguards for my classes that for cases that i did not implement. While i myself return always string for PhpClassPropertyInterface::getPropertyType other people might want to maybe return different things. I could make the the interface more strict, to only allow string return, then i would not need to skip other types than string.
5. I was a bit torn, normally maintainers often make classes final to prevent extending, so you don't get issues that have nothing to do with implementation. I agree, i should either make the class final or add protected, i am still a bit undecided.
6. This logic is a bit more complex, agreed, the first if would add an array type { type: array, items: string } to other basic types (int, string, etc) if present. If the type is only an array union type { type: array, items: int,string } we would return it in the elseif. I hope that makes sense
7. Thank you very much, total oversight on my end, null should indeed not be skipped and needs to be first type in that case if we have a union type 😄

Many thanks for having a look at the new implementation and challenging it 🙏 i appreciate this a lot