Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unions of complex types are not handled correctly #77

Open
ulrikestampa opened this issue Jun 14, 2023 · 4 comments
Open

Unions of complex types are not handled correctly #77

ulrikestampa opened this issue Jun 14, 2023 · 4 comments

Comments

@ulrikestampa
Copy link

Since the supposedly bugfix #66 in avro-php 4.3.0 the handling of unions with complex types does not work correctly any more: No matter which of the union members is provided, always the first member is written (with value NULL).
For illustration please see the attached file containing tests for a sample schema.
UnionsWithoutDefaultsTest.txt

In my opinion, there are multiple aspects causing this problem:

  • The function default_value() returns NULL if no default value exists for a field.
  • The function is_valid_datum in schema.php replaces field->name() by field->default_value() and thus gets a NULL, if no default value has been defined. Therefore, always the first union member is evaluated as "valid datum".
  • The same happens in function write_record in datum.php: Always the first union member is written with (supposed) default NULL.

I think a solution would include the following fixes:
=> A record that is missing an expected field must not pass as a valid record if the field is not nullable.
=> If an expected field is missing, this field must not be written at all (instead of writing it with value NULL). If the field is not nullable, this must raise an error.

BTW according to Avro spec, default values are not to be used when writing. They only provide a means for the reader to replace missing fields by default values, see https://avro.apache.org/docs/1.10.2/spec.html:
default: A default value for this field, only used when reading instances that lack the field for schema evolution purposes. The presence of a default value does not make the field optional at encoding time.

@tPl0ch
Copy link
Collaborator

tPl0ch commented Jun 14, 2023

Big thanks for the thorough and detailed report, always helps a lot. I acknowledge this, but we need to prioritize this first. Please give us some time and expect an answer sometime next week.

@ulrikestampa
Copy link
Author

Did you already have time to do a first evaluation and estimation? It would be great if you could let us know if/when this probably gets fixed. This would make it easier for us to decide on an interim-solution on our side. Thanks.

@tPl0ch
Copy link
Collaborator

tPl0ch commented Jan 26, 2024

@ulrikestampa I am sorry to disappoint, but most of the teams at our organisation are moving away from PHP as primary programming languages, so this library is currently in a limbo state with no real ownership. I am also not sure if and when any work on this will be done. I wish I would have better news - even though it comes half a year late.

@ulrikestampa
Copy link
Author

@tPl0ch Thanks for letting us know. Then we will go in search for other solutions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants